Re-add call site inlining attributes#156849
Conversation
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…<try> Re-add call site inlining attributes
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
(for when this leaves draft status) It'd probably be good to also add a comment here giving some context :)
There was a problem hiding this comment.
Added a little comment. Wasn't sure if I wanted to say "this is a regression test for X"
|
Finished benchmarking commit (fe957f6): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.1%, secondary 0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeResults (primary 0.0%, secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 510.282s -> 511.641s (0.27%) |
985004c to
adbf6f5
Compare
|
r? dianqk |
|
r? @JohnTitor rustbot has assigned @JohnTitor. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Also I think it is very odd that we are blindly setting the call site attributes to the same as the definition, because I think the point of the call site attributes is to differentiate between calls. Probably worth looking into later, but for now I think we should just be fixing the accidental change. |
|
@bors r+ |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing a06c1ca (parent) -> 1d59f66 (this PR) Test differencesShow 5 test diffsStage 1
Stage 2
Additionally, 3 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 1d59f669f9c699b982f112ebbc81e8b1eb550d68 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (1d59f66): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Our benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -3.1%, secondary 0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 0.2%, secondary 2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 511.447s -> 510.511s (-0.18%) |
|
@rustbot label: +perf-regression-triaged Perf impact is the inverse of the PR linked in the description, within typical jitter. The net effect of the two PRs is nothing. |
It is very odd that we are blindly setting the call site attributes to the same as the definition, because I think the point of the call site attributes is to differentiate between calls. Probably worth looking into later, but for now I think we should just be undoing the change we made by mistake.
This fixes the accidental regression from #156242