-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Convert some iter macros to normal functions #124393
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
With all the MIR optimization changes that have happened since these were written, let's see if they still actually matter.
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
… r=<try> Convert some iter macros to normal functions With all the MIR optimization changes that have happened since these were written, let's see if they still actually matter. r? ghost
☀️ Try build successful - checks-actions |
1 similar comment
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (2c1caf5): 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResultsThis 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.
Bootstrap: 670.073s -> 670.802s (0.11%) |
r? libs |
Fantastic! These macros add so much confusion (at least for me), so it's great that we can remove them. @bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (5ff8fbb): 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResultsThis 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.
Bootstrap: 670.073s -> 670.891s (0.12%) |
With all the MIR optimization changes that have happened since these were written, let's see if they still actually matter.
*perf comes back*
Well, it looks like it's not longer relevant for instruction, cycle, nor wall-time perf. Looks like a bunch of things are maybe 10kb bigger in debug, but one is also 50k smaller in debug.
So I think they should switch to being normal functions as the "greatly improves performance" justification for them being macros seems to no longer be true -- probably thanks to us always building
core
with-Z inline-mir
so the difference is negligible.