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

Rework the available Cargo profiles #479

Merged
merged 3 commits into from
Jan 28, 2025

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Jan 25, 2025

Currently the default release profile enables LTO and single CGU builds, which is very slow. Most tests are better run with optimizations enabled since it allows testing a much larger number of inputs, so it is inconvenient that building can sometimes take significantly longer than the tests.

Remedy this by doing the following:

  • Move the existing release profile to release-opt.
  • With the above, the default release profile is untouched (16 CGUs and thin local LTO).
  • release-checked inherits release, so no LTO or single CGU.

This means that the simple cargo test --release becomes much faster for local development. We are able to enable the other profiles as needed in CI.

Tests should ideally still be run with --profile release-checked to ensure there are no debug assetions or unexpected wrapping math hit.

no-panic still needs a single CGU, so must be run with --profile release-opt. Since it is not possible to detect CGU or profilel configuration from within build scripts, the ENSURE_NO_PANIC environment variable must now always be set.

ci: allow-regressions

The Cargo feature `checked` was added in d166a30 ("Overhaul tests")
and later removed in 5e0eca7 ("swap stable to be unstable, checked
is now debug_assertions"). However, there are a few remaining uses of
`feature = "checked"` that did not get removed. Clean these up here.
@tgross35 tgross35 force-pushed the cargo-profile-updates branch 2 times, most recently from d3274f9 to e5b4ce2 Compare January 25, 2025 07:30
Currently the default release profile enables LTO and single CGU builds,
which is very slow to build. Most tests are better run with
optimizations enabled since it allows testing a much larger number of
inputs, so it is inconvenient that building can sometimes take
significantly longer than the tests.

Remedy this by doing the following:

* Move the existing `release` profile to `release-opt`.
* With the above, the default `release` profile is untouched (16 CGUs
  and thin local LTO).
* `release-checked` inherits `release`, so no LTO or single CGU.

This means that the simple `cargo test --release` becomes much faster
for local development. We are able to enable the other profiles as
needed in CI.

Tests should ideally still be run with `--profile release-checked` to
ensure there are no debug assetions or unexpected wrapping math hit.

`no-panic` still needs a single CGU, so must be run with `--profile
release-opt`. Since it is not possible to detect CGU or profilel
configuration from within build scripts, the `ENSURE_NO_PANIC`
environment variable must now always be set.
@tgross35 tgross35 force-pushed the cargo-profile-updates branch from e5b4ce2 to b1e7ea0 Compare January 25, 2025 07:38
@tgross35
Copy link
Contributor Author

CI time is consistent, the wins from not LTOing every test probably washes out with whatever performance gain there could have been.

@tgross35 tgross35 enabled auto-merge January 25, 2025 10:41
@beetrees
Copy link
Contributor

CI is failing on i586:

 thread 'math::atan2::sanity_check' panicked at src/math/atan2.rs:123:5:
assertion `left == right` failed
  left: 2.0344439357957027
 right: 2.0344439357957027

It looks like another bug caused by rust-lang/rust#114479 (the x87 extended precision results are not equal, but the rounded f64 results are). Given the bug is in LLVM, not libm or the test itself, the solution is probably just to #[cfg_attr(all(target_arch = "x86", not(target_feature = "sse2")), ignore)] the test.

@tgross35
Copy link
Contributor Author

tgross35 commented Jan 25, 2025

Intereesting that the sanity check failed here, I will update with your suggestion when I get the chance. The random tests occasionally fail for atan2 and the bessel functions on i586, but I never checked to see if it was reproducible with the same inputs.

Restarted CI just to verify.

@tgross35 tgross35 force-pushed the cargo-profile-updates branch from d599300 to a9e8dfb Compare January 27, 2025 11:43
There seems to be a case of unsoundness with the `i586` version of
`atan2`. For the following test:

    assert_eq!(atan2(2.0, -1.0), atan(2.0 / -1.0) + PI);atan2(2.0, -1.0)

The output is optimization-dependent. The new `release-checked` profile
produces the following failure:

    thread 'math::atan2::sanity_check' panicked at src/math/atan2.rs:123:5:
    assertion `left == right` failed
      left: 2.0344439357957027
     right: 2.0344439357957027

Similarly, `sin::test_near_pi` fails with the following:

    thread 'math::sin::test_near_pi' panicked at src/math/sin.rs:91:5:
    assertion `left == right` failed
      left: 6.273720864039203e-7
     right: 6.273720864039205e-7

Mark the tests ignored on `i586` for now.
@tgross35 tgross35 force-pushed the cargo-profile-updates branch from a9e8dfb to f5f789a Compare January 27, 2025 12:35
@tgross35
Copy link
Contributor Author

tgross35 commented Jan 27, 2025

CI on some other platforms is now stopping at rust-lang/rust#136096. I will just wait for rust-lang/rust#136098 to land.

I also had to ignore a similar test for sin.

@tgross35 tgross35 merged commit 8660ace into rust-lang:master Jan 28, 2025
35 checks passed
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