Avoid checking the edition as much as possible #125828
Conversation
rustbot
added
S-waiting-on-review
|
@bors try @rust-timer queue You can follow the discussion on zulip |
This comment has been minimized.
This comment has been minimized.
|
@vincenzopalazzo: 🔑 Insufficient privileges: not in try users |
While working on macros ofter is good to ask "we are introducing a regression in terms of performance?" So this commit is allowing the macros wg to use the perf bot. Link: rust-lang/rust#125828 Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo
changed the title
While working on macros ofter is good to ask "we are introducing a regression in terms of performance?" So this commit is allowing the macros wg to use the perf bot. Link: rust-lang/rust#125828 Co-authored-by: Michael Goulet <[email protected]> Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo
force-pushed
the
macros/performance-regression
branch
from
ae963e8
to
252f83a
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
…egression, r=<try> [perf-only] rustc_parser: avoid checking the edition as much as possible CC rust-lang#123865 r? ghost
|
🤔 I would assume that PGO already does such kind of optimization on its own. |
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (d693eba): comparison URL. Overall result: ✅ improvements - 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 is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 3.9%)This 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.
CyclesResults (primary -1.1%, secondary 2.0%)This 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: 672.771s -> 666.635s (-0.91%) |
rustbot
removed
the
S-waiting-on-perf
|
Comparing these results with the results from the last PR, it looks like this more than makes up for the regressions in html5ever, but only gets back some of the performance for ttmuncher. |
It is incorrect to reorder |
|
And yes, the optimization may be permissible in this case, but LLVM got slightly more skittish about reordering those after it got burned by doing the optimization unsoundly. |
|
Sorry for the delay! Well, we've already made some improvements with @eholk's suggestion, right? Why don't we try to merge this PR and track the performance issue with a tracking issue? I think we already have several PRs for expr2024, so maybe it's worth looking at this later to see if we need some other kind of optimization that allows us cheap access to the edition? r? fmease |
vincenzopalazzo
changed the title
vincenzopalazzo
force-pushed
the
macros/performance-regression
branch
from
252f83a
to
1d9f0ee
Compare
vincenzopalazzo
force-pushed
the
macros/performance-regression
branch
from
1d9f0ee
to
5c164fa
Compare
Inside rust-lang#123865, we are adding support for the new semantics for expr2024, but we have noted a performance issue. We realized there is a redundant check for each token regarding an edition. This commit moves the edition check to the end, avoiding some extra checks that can slow down compilation time. Link: rust-lang#123865 Co-Developed-by: @eholk Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo
force-pushed
the
macros/performance-regression
branch
from
5c164fa
to
36d5fc9
Compare
|
@bors r |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (a6416d8): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression 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)This benchmark run did not return any relevant results for this metric. CyclesResults (primary 2.2%, secondary 2.5%)This 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: 669.121s -> 667.68s (-0.22%) |
Inside #123865, we are adding support for the new semantics for expr2024, but we have noted a performance issue.
While talking with @eholk, we realized there is a redundant check for each token regarding an edition. This commit moves the edition check to the end, avoiding some extra checks that can slow down compilation time.
However, we should keep this issue under observation because we may want to improve the edition check if we are unable to significantly improve compiler performance.
r? ghost