-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Check RUSTC_CTFE_BACKTRACE
much less by generating fewer errors
#69290
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
Conversation
@bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit e9d7e4c5637d1584551727e34e6fc7ffb98bdad8 with merge 86fcc9b22b448b73a89589ccc348771cdd426a09... |
Looks good, r=me when perf agrees |
(But I think there's a typo in the PR description where it fails to link to where @nnethercote discovered the problem.) |
Heh. Vim word-wrapped the line as I was typing the issue number and since git-commit ignores lines that start with |
Also, adding the flag to the session as you proposed would still probably be better than the current env var scheme. Even if this solves the main perf problem, there is no good reason to make error creation that expensive. |
☀️ Try build successful - checks-azure |
Queued 86fcc9b22b448b73a89589ccc348771cdd426a09 with parent ae54678, future comparison URL. |
Finished benchmarking try commit 86fcc9b22b448b73a89589ccc348771cdd426a09, comparison URL. |
8% speedup, nice :) @bors r+ rollup=never |
📌 Commit e9d7e4c5637d1584551727e34e6fc7ffb98bdad8 has been approved by |
@RalfJung Yeah, that's correct. PRs that impact performance shouldn't be rolled up. |
Nice perf results! I didn't realize this would help benchmarks other than |
BTW, before this lands can you reword the commit message so that the |
@bors r- |
e9d7e4c
to
eb6a44f
Compare
Before this change, `get_size_and_align()` calls `get_fn_alloc()` *a lot* in CTFE heavy code. This previously returned an `Error` which would check if `RUSTC_CTFE_BACKTRACE` was set on construction. Doing this turned out to be a performance hotspot as @nnethercote discovered in rust-lang#68792. This is an alternate take on that PR which resolves the performance issue by generating *many* fewer errors. Previously, `ctfe-stress-4` would generate over 5,000,000 errors each of which would check for the presence of the environment variable. With these changes, that number is reduced to 30.
eb6a44f
to
9f3bc82
Compare
@bors r=RalfJung |
📌 Commit 9f3bc82 has been approved by |
☀️ Test successful - checks-azure |
Before this change,
get_size_and_align()
callsget_fn_alloc()
alot in CTFE heavy code. This previously returned an
Error
which wouldcheck if
RUSTC_CTFE_BACKTRACE
was set on construction. Doing thisturned out to be a performance hotspot as @nnethercote discovered in
#68792.
This is an alternate take on that PR which resolves the performance
issue by generating many fewer errors. Previously,
ctfe-stress-4
would generate over 5,000,000 errors each of which would check for the
presence of the environment variable. With these changes, that number is
reduced to 30.
r? @RalfJung