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

Investigate increased CI time #1262

Closed
wants to merge 1 commit into from
Closed

Investigate increased CI time #1262

wants to merge 1 commit into from

Conversation

alexytsu
Copy link
Contributor

@alexytsu alexytsu commented Mar 28, 2023

The change in #1259 has caused a significant increase in CI times for this repo.

https://github.com/filecoin-project/builtin-actors/actions/runs/4527273009/jobs/7972963894 took around 18 minutes and has doubled (34 minutes) both when it landed on master: https://github.com/filecoin-project/builtin-actors/actions/runs/4538508299/jobs/7997478197 and when it ran on the PR branch (35 minutes) https://github.com/filecoin-project/builtin-actors/actions/runs/4528315067/jobs/7974981200

This PR reverts commit 424669b to see if that commit caused the increase in CI times. (It does)

By switching to a generic trait, the compiler will monomorphize the TestVM code. This should not in fact affect test execution times but running this PR to make sure. Locally I see that the time taken to build the TestVM tests has more than doubled ~2:30 -> 6 minutes whilst time taken to run the tests remains roughly the same ~4 minutes.

@codecov-commenter
Copy link

Codecov Report

Merging #1262 (308b6b7) into master (424669b) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1262      +/-   ##
==========================================
- Coverage   90.90%   90.88%   -0.03%     
==========================================
  Files         133      133              
  Lines       26754    26721      -33     
==========================================
- Hits        24322    24285      -37     
- Misses       2432     2436       +4     
Impacted Files Coverage Δ
actors/market/src/testing.rs 96.03% <100.00%> (-0.05%) ⬇️
state/src/check.rs 86.35% <100.00%> (+0.26%) ⬆️
test_vm/src/lib.rs 82.66% <100.00%> (-0.50%) ⬇️
test_vm/src/util.rs 99.91% <100.00%> (-0.01%) ⬇️

... and 2 files with indirect coverage changes

@alexytsu
Copy link
Contributor Author

From my local timings, almost all the increase in time can be attributed to building the tests in the test_vm package

  master nv19 delta
cargo test —workspace —no-run cargo test --workspace --no-run 2716.88s user 116.09s system 830% cpu 5:40.96 total cargo test --workspace --no-run 2948.45s user 141.17s system 774% cpu 6:38.89 total cargo test --workspace --no-run 1002.52s user 87.83s system 718% cpu 2:31.72 total cargo test --workspace --no-run 995.24s user 89.34s system 705% cpu 2:33.75 total ~4 minutes increase
with prebuilt tests cargo test --workspace cargo test --workspace 336.27s user 6.37s system 153% cpu 3:43.14 total cargo test --workspace 339.78s user 7.77s system 123% cpu 4:40.51 total cargo test --workspace 338.90s user 6.96s system 125% cpu 4:35.64 total cargo test --workspace 337.49s user 6.47s system 154% cpu 3:43.32 total ~no increase
only building tests in test_vm package cargo test --no-run 2148.08s user 60.56s system 909% cpu 4:02.95 total cargo test --no-run 411.78s user 32.10s system 662% cpu 1:06.96 total ~3 minutes increase

@Stebalien
Copy link
Member

Try running cargo bloat.

@Stebalien
Copy link
Member

Stebalien commented Mar 28, 2023

Hm. And twiggy monos. (wasm only, unfortunately)

@Stebalien
Copy link
Member

It looks like removing the opt-level=2 on the test vm (

opt-level = 2
) reduces the build time in half, at least for me. However, that'll probably slow down the tests.

@alexytsu
Copy link
Contributor Author

alexytsu commented Mar 29, 2023

removing the opt-level=2

Significantly decreases build time for me too but it is far outweighed by increase in test running time

(for both opt-level 0 and 1 neither seems net faster)

@alexytsu alexytsu closed this Apr 5, 2023
@alexytsu alexytsu deleted the alex/ci-test-times branch May 30, 2023 00:44
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.

3 participants