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

Disable LTO builds in tests (and bump deps to -O2) #1497

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

@TheBlueMatt TheBlueMatt commented May 25, 2022

For some reason rustc, at some point, decided that our optimization
of dependencies implies we want to also build with LTO. This causes
our test builds to take substantially longer to compile, even with
only a trivial change. By hard-disabling this (even keeping the
optimization of the test and in-tree libraries enabled) the time
required to build with only a trivial change to
`functional_tests.rs` goes from 0m25.635s wall clock/1m14.220s CPU
time to 0m17.841s wall clock/0m17.828s CPU time on my i7-13700K on
rustc 1.63.

While we're at it, we also bump dependencies to build with -O2
because their build time is now substantially reduced cost.

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2022

Codecov Report

Base: 87.30% // Head: 90.89% // Increases project coverage by +3.58% 🎉

Coverage data is based on head (90372f3) compared to base (22d1bab).
Patch has no changes to coverable lines.

❗ Current head 90372f3 differs from pull request most recent head 8d35d2c. Consider uploading reports for the commit 8d35d2c to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1497      +/-   ##
==========================================
+ Coverage   87.30%   90.89%   +3.58%     
==========================================
  Files         101       77      -24     
  Lines       44364    42393    -1971     
  Branches    44364    42393    -1971     
==========================================
- Hits        38734    38533     -201     
+ Misses       5630     3860    -1770     
Impacted Files Coverage Δ
lightning/src/chain/chaininterface.rs 0.00% <0.00%> (-41.18%) ⬇️
lightning-block-sync/src/rest.rs 65.45% <0.00%> (-27.41%) ⬇️
lightning/src/chain/mod.rs 63.63% <0.00%> (-20.58%) ⬇️
lightning-block-sync/src/rpc.rs 78.51% <0.00%> (-11.31%) ⬇️
lightning/src/util/macro_logger.rs 85.48% <0.00%> (-6.01%) ⬇️
lightning/src/util/ser_macros.rs 89.13% <0.00%> (-3.56%) ⬇️
lightning-invoice/src/de.rs 82.38% <0.00%> (-2.33%) ⬇️
lightning/src/ln/channelmanager.rs 84.58% <0.00%> (-1.96%) ⬇️
lightning/src/ln/peer_handler.rs 57.15% <0.00%> (-1.63%) ⬇️
lightning/src/ln/wire.rs 62.92% <0.00%> (-1.23%) ⬇️
... and 96 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Cargo.toml Outdated
# Ideally we would only do this in profile.test, but profile.test only applies to
# the test binary, not dependencies, which means most of the critical code still
# gets compiled as -O0. See
# Our tests do actual crypo and lots of work, the tradeoff for -O1 is well
Copy link
Contributor

Choose a reason for hiding this comment

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

"[...] the tradeoff for -O1 is well worth it."

Wait, I now thought the test and dev profiles are disjunct, and here we're only enabling -O1 for the latter?

Also: s/crypo/crypto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, so I didn't realize, but test applies for just the crate being tested, dev applies for all the dependencies thereof. I think, but I'm not sure, that we're building the lightning crate in O1 as a dependency for other workspace crates. The change here should make it all-workspace-crates are O0.

tnull
tnull previously approved these changes May 26, 2022
@TheBlueMatt
Copy link
Collaborator Author

@wpaulino had some tests that may have indicated this wasn't working as intended (slowed down post-compile cargo test runs). Gonna assign him until he can investigate further.

@wpaulino
Copy link
Contributor

See my results below on a M1 MacBook Pro.

Steps before each measurement:

> cargo clean
> cargo test --no-run

I confirmed that cargo test --no-run results in rustc using the proper opt-level based on the commit checked out.

At 90372f3 (PR head):

> time cargo test
       94.71 real       476.56 user         2.64 sys

At 75ca50f (PR base):

> time cargo test
       17.92 real        71.15 user         2.22 sys

@tnull
Copy link
Contributor

tnull commented May 27, 2022

I can confirm this on my development VM (Ubuntu aarch64, 6 cores, on MacBook Pro 2021):

HEAD:
real    1m43.808s    user    7m27.421s    sys     0m3.066s

BASE:
real    0m18.657s    user    1m10.569s    sys     0m3.191s

On older server hardware (Ubuntu, Intel(R) Xeon(R) CPU E3-1226 v3 @ 3.30GHz, 4 cores), the slowdown seems not as drastic, but still present:

HEAD:
real    0m46.174s    user    1m55.931s    sys     0m3.548s

BASE:
real    0m28.448s    user    1m12.130s    sys     0m3.266s

@TheBlueMatt
Copy link
Collaborator Author

Blocked on rust-lang/rust#97460

@TheBlueMatt TheBlueMatt changed the title Clarify optimization comment in Cargo.toml (and bump deps to -O2) Fix optimization settings for tests in Cargo.toml (and bump deps to -O2) Feb 24, 2023
@TheBlueMatt TheBlueMatt changed the title Fix optimization settings for tests in Cargo.toml (and bump deps to -O2) Disable LTO builds in tests (and bump deps to -O2) Feb 24, 2023
For some reason rustc, at some point, decided that our optimization
of dependencies implies we want to also build with LTO. This causes
our test builds to take substantially longer to compile, even with
only a trivial change. By hard-disabling this (even keeping the
optimization of the test and in-tree libraries enabled) the time
required to build with only a trivial change to
`functional_tests.rs` goes from 0m25.635s wall clock/1m14.220s CPU
time to 0m17.841s wall clock/0m17.828s CPU time on my i7-13700K on
rustc 1.63.

The changes in test execution time appear to be within noise.

While we're at it, we also bump dependencies to build with -O2
because their build time is now substantially reduced cost.
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

Without this branch (HEAD^):

  • Clean build: cargo test --no-run 623.52s user 244.74s system 612% cpu 2:21.75 total
  • Trivial test change build: cargo test --no-run 189.47s user 135.20s system 500% cpu 1:04.82 total

With this branch:

  • Clean build: cargo test --no-run 551.26s user 87.84s system 610% cpu 1:44.70 total
  • Trivial test change build: cargo test --no-run 30.06s user 6.60s system 186% cpu 19.665 total

@TheBlueMatt TheBlueMatt merged commit d355ce1 into lightningdevkit:main Mar 4, 2023
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.

None yet

5 participants