Skip to content
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

ci: consolidate tasks and use sccache and nextest #1015

Merged
merged 1 commit into from
Feb 26, 2024
Merged

ci: consolidate tasks and use sccache and nextest #1015

merged 1 commit into from
Feb 26, 2024

Conversation

shadaj
Copy link
Member

@shadaj shadaj commented Jan 4, 2024

ci: consolidate tasks and use sccache and nextest

Copy link

cloudflare-workers-and-pages bot commented Jan 4, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3542f85
Status: ✅  Deploy successful!
Preview URL: https://93890caa.hydroflow.pages.dev
Branch Preview URL: https://pr1015.hydroflow.pages.dev

View logs

@shadaj shadaj requested a review from MingweiSamuel January 4, 2024 02:42
@shadaj
Copy link
Member Author

shadaj commented Jan 4, 2024

Should help speed up builds a lot by avoiding redundant compilation.

@shadaj
Copy link
Member Author

shadaj commented Jan 4, 2024

Two main optimizations:

  • Run clippy and check in the same job, since clippy requires check
  • Run all tests in a single build, so we can share compiler artifacts and also cache across CI runs with sccache

@shadaj shadaj marked this pull request as ready for review January 4, 2024 03:01
@MingweiSamuel
Copy link
Member

We split up CI testing in order to speed it up. Although some code gets compiled multiple times, the parallelism benefits the overall runtime - #944

What does sccache do?

Combining clippy and check (and fmt?) is fine

https://github.com/hydro-project/hydroflow/actions/runs/7404907733/job/20147104671?pr=1015

@shadaj
Copy link
Member Author

shadaj commented Jan 5, 2024

sccache caches compiled crates into GitHub's action cache, so that future builds can reuse those. Also locally this eliminates redundant builds by using more fine-grained cache keys than cargo.

Looking at https://github.com/hydro-project/hydroflow/actions/runs/7404907733/job/20147104671, it seems that after consolidation the total time is only a little longer than the previous longest run 11m -> 14m. My thinking was that this will make it possible to enable Windows builds in PR since this makes it much harder to hit the concurrency limit.

@shadaj shadaj mentioned this pull request Jan 5, 2024
@shadaj
Copy link
Member Author

shadaj commented Jan 5, 2024

With nextest and some caching fixes (cidev breaks caching because crates build as part of tests are built with dev, so we use an environment variable), down to 9m 43s for all tests.

@shadaj shadaj changed the title ci: consolidate tasks and use sccache ci: consolidate tasks and use sccache and nextest Jan 5, 2024
@shadaj shadaj force-pushed the pr1015 branch 5 times, most recently from c5630e7 to 12b3475 Compare January 6, 2024 01:14
@@ -47,18 +47,14 @@ pub fn test_tick_loop() {
async fn test_persist_stratum_run_available() -> Result<(), Box<dyn Error>> {
let (out_send, out_recv) = hydroflow::util::unbounded_channel();

let handle = tokio::task::spawn_blocking(|| {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was flaky but doesn't actually need any concurrency?

@shadaj
Copy link
Member Author

shadaj commented Feb 26, 2024

@MingweiSamuel bump on considering this? although it increases the max job time by ~1-2 minutes, I've been seeing a lot of builds blocked on a full queue which this would help a lot with

Copy link
Member

@MingweiSamuel MingweiSamuel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure sccache actually does anything for us here? But fine with just merging it if you prefer this way

@shadaj
Copy link
Member Author

shadaj commented Feb 26, 2024

I checked the stats being printed out and it seems to definitely be helping with shared compilations of dependencies. Might not help much with the in-tree sources though.

@shadaj shadaj merged commit e9639f6 into main Feb 26, 2024
13 checks passed
@shadaj shadaj deleted the pr1015 branch February 26, 2024 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants