Skip to content

Regression of applier build times in Rust 1.57 #746

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nightkr opened this issue Dec 6, 2021 · 5 comments · Fixed by #916
Closed

Regression of applier build times in Rust 1.57 #746

nightkr opened this issue Dec 6, 2021 · 5 comments · Fixed by #916
Labels
bug Something isn't working msrv related to minimum supported rust version runtime controller runtime related

Comments

@nightkr
Copy link
Member

nightkr commented Dec 6, 2021

Current and expected behavior

1.57 behaviour

  1. Check out https://gitlab.com/teozkr/repros/-/tree/kube/rs1.57-compile-regression.
  2. rustup update 1.57.0
  3. cargo +1.57.0 build
  4. Note that the full build gets stuck for several minutes on rs157-compile-regression(bin)
  5. touch src/main.rs
  6. cargo +1.57.0 build
  7. Note that the incremental build takes ~40s (on my machine, scale to yours)

1.56 behaviour

  1. rustup update 1.56.1
  2. cargo +1.56.1 build
  3. Note that the full build completes in ~30s (on my machine, scale to yours)
  4. touch src/main.rs
  5. cargo +1.56.1 build
  6. Note that the incremental build takes ~3s (on my machine, scale to yours)

Possible solution

More of a workaround, but boxing queue:

diff --git a/src/main.rs b/src/main.rs
index a306bba..6e08d03 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -25,7 +25,8 @@ async fn main() {
                 stream::empty::<Result<watcher::Event<ConfigMap>, watcher::Error>>(),
             )),
             (),
-        ),
+        )
+        .boxed(),
     )
     .boxed()
     .for_each(|_| async {})

brings the incremental rebuild time down to ~4s.

Additional context

No response

Environment

Arch Linux
Rustc 1.57 (bad), 1.56.1 (good)
K8s cluster is irrelevant since this is a build issue

Configuration and features

kube = { version = "0.64.0", features = ["runtime"] }
k8s-openapi = { version = "0.13.1", features = ["v1_22"] }

Affected crates

kube-runtime

Would you like to work on fixing this bug?

yes

@nightkr nightkr added bug Something isn't working runtime controller runtime related labels Dec 6, 2021
@nightkr
Copy link
Member Author

nightkr commented Dec 6, 2021

Curiously, this only seems to happen for the pattern applier(..., trigger_self(try_flatten_applied(reflector(...)))). Removing any of those steps, or erasing to a trait object (with StreamExt::boxed) seems to step around the issue.

@clux
Copy link
Member

clux commented Dec 6, 2021

Good find. Confirming it happens on mine as well.

$ touch src/main.rs 
$ time cargo +1.57.0 build
   Compiling rs157-compile-regression v0.1.0 (/home/clux/repos/repros/repros)
    Finished dev [unoptimized + debuginfo] target(s) in 1m 18s

real	1m18.798s
user	1m12.865s
sys	0m5.807s

then clean and after building once:

$ touch src/main.rs 
$ time cargo +1.56.1 build
   Compiling rs157-compile-regression v0.1.0 (/home/clux/repos/repros/repros)
    Finished dev [unoptimized + debuginfo] target(s) in 3.12s

real	0m3.158s
user	0m2.700s
sys	0m0.458s

Dramatic difference.

@nightkr
Copy link
Member Author

nightkr commented Dec 6, 2021

I can reproduce this with just futures, so this seems more like a rustc issue than kube-rs in particular. Filed rust-lang/rust#91598.

@nightkr nightkr mentioned this issue Feb 14, 2022
@clux clux added the msrv related to minimum supported rust version label Feb 14, 2022
@clux clux added the blocked awaiting upstream work label Feb 23, 2022
@clux clux removed the blocked awaiting upstream work label Apr 28, 2022
@clux
Copy link
Member

clux commented May 1, 2022

This is is presumably no longer an issue with the fix getting into stable. Are you OK with bumping the msrv from this point on?

@nightkr
Copy link
Member Author

nightkr commented May 1, 2022

Happy to close this and unblock the MSRV, I'm back on stable since 1.59.

@nightkr nightkr closed this as completed May 1, 2022
clux added a commit that referenced this issue May 23, 2022
First bump since #746

Signed-off-by: clux <[email protected]>
clux added a commit that referenced this issue May 23, 2022
…916)

* Bump k8s-openapi to 0.15

Signed-off-by: clux <[email protected]>

* just bump-k8s to v1_24

Signed-off-by: clux <[email protected]>

* better msrv automation

Signed-off-by: clux <[email protected]>

* bump msrv to 1.60

First bump since #746

Signed-off-by: clux <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working msrv related to minimum supported rust version runtime controller runtime related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants