-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Use u64 for incr comp allocation offsets #113562
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 |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit c157ed5ebb2c7a774fe4e33032261f4ec5335dad with merge 95e855e355817d2ab0983eae1b0c1c2bc192b552... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (95e855e355817d2ab0983eae1b0c1c2bc192b552): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking 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 may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 656.708s -> 657.823s (0.17%) |
c157ed5
to
752f285
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit 752f2857bf8e77932ff8f05d8194f36494714949 with merge 895ebafae6f9467924efe5ea4721c573424631c0... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (895ebafae6f9467924efe5ea4721c573424631c0): comparison URL. Overall result: no relevant changes - no action neededBenchmarking 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 may lead to changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 658.555s -> 658.567s (0.00%) |
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
Yea that seems not great. But I also don't think it's too useful a test, as it is hard to regress again |
Should there be a comment somewhere explaining why u64 is needed?
|
Indeed, there are no comments in the commit and no explanation in the PR. What's the motivation here? |
752f285
to
4e117a9
Compare
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (6f65ef5): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 657.644s -> 656.437s (-0.18%) |
Fixes #76037
Fixes #95780
Fixes #111613
These issues are all reporting ICEs caused by using
u32
to store offsets to allocations in the incremental compilation cache. This PR aims to lift that limitation by changing the offset type in question tou64
.There are two perf runs in this PR. The first reports a regression, and the second does not. The changes are the same in both. I rebased the PR then did the second perf run because I noticed that the primary regression in it was very commonly seen in spurious regression reports.
I do not know what the perf run will report when this is merged. I would not be surprised to see regression or neutral, but the cachegrind diffs for the regression point at
try_mark_previous_green
which is a common source of inexplicable regressions and I don't think should be perturbed by this PR.I'm not opposed to adding a regression test such as
But that program takes 1 minute to compile and consumes 4.6 GB of memory then writes that much to disk. Is that a concerning amount of resource use for a test?
r? @nnethercote