Skip to content

build: disable incremental build in CI #7838

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

Merged
merged 5 commits into from
Feb 11, 2023
Merged

Conversation

xxchan
Copy link
Member

@xxchan xxchan commented Feb 10, 2023

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

significantly reduced build time

@github-actions github-actions bot added Invalid PR Title type/build Type: Building system & package manager integration. labels Feb 10, 2023
@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #7838 (90833f3) into main (18b71a2) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #7838      +/-   ##
==========================================
- Coverage   71.73%   71.73%   -0.01%     
==========================================
  Files        1109     1109              
  Lines      176825   176825              
==========================================
- Hits       126850   126839      -11     
- Misses      49975    49986      +11     
Flag Coverage Δ
rust 71.73% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/batch/src/executor/group_top_n.rs 68.23% <0.00%> (-6.48%) ⬇️
src/meta/src/hummock/mock_hummock_meta_client.rs 63.95% <0.00%> (-0.51%) ⬇️
src/storage/src/hummock/compactor/mod.rs 80.46% <0.00%> (-0.20%) ⬇️
src/meta/src/hummock/manager/mod.rs 78.14% <0.00%> (-0.06%) ⬇️
src/common/src/types/ordered_float.rs 31.06% <0.00%> (+0.19%) ⬆️
src/batch/src/task/task_execution.rs 52.13% <0.00%> (+0.50%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@xxchan xxchan changed the title build build: try to reduce compile time for build-simulation Feb 10, 2023
@xxchan
Copy link
Member Author

xxchan commented Feb 10, 2023

opt-level=2
image

opt-level=3 (Why it's slower?? 😅)
image

@xxchan
Copy link
Member Author

xxchan commented Feb 10, 2023

image

Why build-deterministic-simulation only takes 7 min now? (-5min) 😅😅😅

Is it because opt-level = 3 or incremental = false? 😅

@xxchan xxchan changed the title build: try to reduce compile time for build-simulation build: disable incremental build in CI Feb 10, 2023
@xxchan
Copy link
Member Author

xxchan commented Feb 10, 2023

sccache shows Non-cacheable calls 215 -> 188

@@ -36,3 +36,4 @@ RUN cargo install cargo-llvm-cov cargo-nextest cargo-udeps cargo-hakari cargo-so

ENV RUSTC_WRAPPER=sccache
ENV SCCACHE_BUCKET=ci-sccache-bucket
ENV CARGO_INCREMENTAL=0
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this override incremental build for non sim build? Did we show a speedup or lack of regression there?

Copy link
Member Author

@xxchan xxchan Feb 10, 2023

Choose a reason for hiding this comment

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

We already have that set for ci-dev profile previously.

risingwave/Cargo.toml

Lines 95 to 97 in 90833f3

[profile.ci-dev]
inherits = "dev"
incremental = false

I think generally it makes not sense to have incremental build in CI (besides affecting sccache, IIRC it also has a little bit overheads), so I added the envvar as an one-for-all approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Disable incremental compilation. CI builds often are closer to from-scratch builds, as changes are typically much bigger than from a local edit-compile cycle. For from-scratch builds, incremental adds an extra dependency-tracking overhead. It also significantly increases the amount of IO and the size of ./target, which make caching less effective.

https://matklad.github.io/2021/09/04/fast-rust-builds.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Great. I wonder why we did not make the deterministic build non-incremental in the first place then :)

Anw, if it is faster then we should definitely do it. Btw, do we test the CI on a release build?

Copy link
Contributor

Choose a reason for hiding this comment

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

opt-level=2

Perhaps we should be testing at the same opt-level as our release build?

Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

How can I miss that in ci-sim! 🥵🚀

@@ -36,3 +36,4 @@ RUN cargo install cargo-llvm-cov cargo-nextest cargo-udeps cargo-hakari cargo-so

ENV RUSTC_WRAPPER=sccache
ENV SCCACHE_BUCKET=ci-sccache-bucket
ENV CARGO_INCREMENTAL=0
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this extra env? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Well as @xxchan mentioned we are building the docker image from scratch

Copy link
Contributor

@jon-chuang jon-chuang Feb 11, 2023

Choose a reason for hiding this comment

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

Although, I'm not sure if we use this dockerfile to build release binaries/performance pipeline. If we do it is even better since the docker build is always from scratch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do we need this extra env? 🤔

So that we don’t need to set every ci-* profile…🤪 I just didn’t delete the profile settings

Copy link
Member Author

Choose a reason for hiding this comment

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

Although, I'm not sure if we use this dockerfile to build release binaries/performance pipeline. If we do it is even better since the docker build is always from scratch.

Didn’t thought about that, but yes it can be enabled. It’s a free optimization 🤔

Copy link
Contributor

@jon-chuang jon-chuang Feb 11, 2023

Choose a reason for hiding this comment

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

Yes, I'm saying it's in fact this env var which would set the incremental build to true for those, as the benchmarking build relies on the docker image built from this file.

@Xuanwo
Copy link
Contributor

Xuanwo commented Feb 11, 2023

Hi, is there any comparison before/after using sccache? I want to shout it out (as a sccache contrituor)~

@jon-chuang
Copy link
Contributor

@Xuanwo #7799

Here, seems about 10%?

@mergify mergify bot merged commit 001ee16 into main Feb 11, 2023
@mergify mergify bot deleted the xxchan/fluttering-chicken branch February 11, 2023 06:46
@xxchan
Copy link
Member Author

xxchan commented Feb 11, 2023

Hi, is there any comparison before/after using sccache? I want to shout it out (as a sccache contrituor)~

You can look at the buildkite pipeline page 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/build Type: Building system & package manager integration.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants