Add two inline annotations for hot functions #89495
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These two functions are essentially no-ops (and compile to just a load and return), but show up in process_obligations profiles with a high call count -- so worthwhile to try and inline them away.
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 1c2ad79 with merge fe7c7f009e2813c80fc7fd1b9cc982956d072f32... |
|
☀️ Try build successful - checks-actions |
|
Queued fe7c7f009e2813c80fc7fd1b9cc982956d072f32 with parent 5051904, future comparison URL. |
|
Finished benchmarking commit (fe7c7f009e2813c80fc7fd1b9cc982956d072f32): comparison url. Summary: This change led to very large relevant improvements 🎉 in compiler performance.
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. @bors rollup=never |
rustbot
added
S-waiting-on-review
|
r? rust-lang/compiler-team |
|
Nice find! @bors r+ |
|
📌 Commit 1c2ad79 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 1c2ad79 with merge d30ccf1f1786718f8425f73b740d9bedc4f4260f... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
A job failed! Check out the build log: (web) (plain) Click to see the possible cause of the failure (guessed by this bot) |
|
@bors treeclosed=100 |
bors
added
S-waiting-on-bors
This was referenced Oct 6, 2021
Merged
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (ca8078d): comparison url. Summary: This change led to very large relevant mixed results 🤷 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression |
|
@Mark-Simulacrum after merge this led to a few small regressions (in addition to the large improvements seen in the try run). Given that this change was just in inlining, I could imagine that performance losses seen in some test cases could be do to differences in program layout which might be specific to the circumstances of the perf machine (and not super generalizable). Given this I would recommend we mark this as triaged. Thoughts? |
|
I think specific to perf machine is probably not quite right -- but it does seem likely they are not worth the investigation given the large improvements here. |
Mark-Simulacrum
added
the
perf-regression-triaged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These two functions are essentially no-ops (and compile to just a load and
return), but show up in process_obligations profiles with a high call count --
so worthwhile to try and inline them. This is not normally possible as they're
non-generic, so they don't get offered for inlining by our current algorithm.