Add config.toml options for enabling overflow checks in rustc and std by hkratz...

Copy link

Contributor

failures:
    [ui] ui/parser/item-free-const-no-body-semantic-fail.rs
    [ui] ui/parser/item-free-static-no-body-semantic-fail.rs

rust-highfive commented 11 days ago

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

hkratz commented 10 days ago

Performance impact for running the full stage 1 testsuite locally is less than I expected:

# With fix from #87761 for the failing tests.
./x.py clean; time ./x.py test --stage 1

# no checks:
real	23m19.492s
user	176m22.251s
sys	20m28.837s

# w/ overflow checks in everything but std:
real	23m26.973s
user	176m50.005s
sys	20m32.393s

# w/ overflow checks in everything:
real	23m57.156s
user	181m20.771s
sys	21m25.673s

Could you change them to default to true in this PR temporarily, so we can run perf on this PR? I think if we can get away with not exposing this as an option (and instead always enabling them) we should.

hkratz commented 10 days ago

Could you change them to default to true in this PR temporarily, so we can run perf on this PR? I think if we can get away with not exposing this as an option (and instead always enabling them) we should.

@Mark-Simulacrum Done. This should be ready for a perf test.

rust-timer commented 10 days ago

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors commented 10 days ago

Trying commit 72ada17 with merge f9bbcb1...

bors commented 10 days ago

Try build successful - checks-actions
Build commit: f9bbcb1 (f9bbcb152f7bd5dba5923c3c42999ee3c78aae98)

rust-timer commented 10 days ago

rust-timer commented 10 days ago

Finished benchmarking try commit (f9bbcb1): comparison url.

Summary: This change led to significant regressions in compiler performance.

• Very large regression in instruction counts (up to 23.8% on incr-unchanged builds of ctfe-stress-4-check)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

hkratz commented 10 days ago

Hmm, at least now we know.

(Sorry the Close and Reopen was an accident on my mobile)

Could you drop the enablement for std and we can rerun benchmarks? I'd be interested in just rustc enablement impact.

I expect it'd be interesting to work out if there's just a few adds or whatever we can replace with explicit wrapping adds (with comments about performance or so). But that's a larger project, I just want numbers for now on the rustc-only overflow checks and we can likely merge after that.

hkratz commented 10 days ago

@Mark-Simulacrum Ready for perf testing.

rust-timer commented 10 days ago

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors commented 10 days ago

Trying commit 1ee9083 with merge d570e77cc2384b466568c2ebc072def445db0f1b...

bors commented 10 days ago

Try build successful - checks-actions
Build commit: d570e77 (d570e77cc2384b466568c2ebc072def445db0f1b)

rust-timer commented 10 days ago

rust-timer commented 10 days ago

Finished benchmarking try commit (d570e77): comparison url.

Summary: This change led to significant regressions in compiler performance.

• Very large regression in instruction counts (up to 21.4% on incr-unchanged builds of ctfe-stress-4-check)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Alright, thanks! Looks like neither is an option for now as default, so if you can drop the testing commits we can probably merge (I'll give another glance over the code but don't think there's likely to be more work before merging).

r=me after last nit is fixed

@bors r+

Thanks!

bors commented 9 days ago

Commit 2f70953 has been approved by Mark-Simulacrum

bors commented 9 days ago

Testing commit 2f70953 with merge 24380a6...

bors commented 9 days ago

Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 24380a6 to master...