-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
#124141 preliminaries #132629
#124141 preliminaries #132629
Conversation
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
…<try> rust-lang#124141 preliminaries Preliminary changes required to start removing `Nonterminal`. r? `@petrochenkov`
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (feea15e): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking 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 the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (secondary 4.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.
CyclesResults (secondary 9.7%)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 sizeResults (primary 0.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.
Bootstrap: 781.474s -> 782.28s (0.10%) |
Quite a few perf regressions, do you know what is the reason? Are they avoidable? |
The regressions are smaller when I benchmark locally, so it's hard to tell exactly what's happening. But I suspect there are two possible causes:
The regressions are mostly in @rustbot label: +perf-regression-triaged |
@rustbot ready |
3266ef9
to
38efad3
Compare
r=me after removing the remaining glob imports (#132629 (comment)). |
It's not used meaningfully yet, but will be needed to get rid of interpolated tokens.
Pasted metavariables are wrapped in invisible delimiters, which pretty-print as empty strings, and changing that can break some proc macros. But error messages saying "expected identifer, found ``" are bad. So this commit adds support for metavariables in `TokenDescription` so they print as "metavariable" in error messages, instead of "``". It's not used meaningfully yet, but will be needed to get rid of interpolated tokens.
Current places where `Interpolated` is used are going to change to instead use invisible delimiters. This prepares for that. - It adds invisible delimiter cases to the `can_begin_*`/`may_be_*` methods and the `failed_to_match_macro` that are equivalent to the existing `Interpolated` cases. - It adds panics/asserts in some places where invisible delimiters should never occur. - In `Parser::parse_struct_fields` it excludes an ident + invisible delimiter from special consideration in an error message, because that's quite different to an ident + paren/brace/bracket.
It was added in rust-lang#130349, but it's not used meaningfully, and causes difficulties for Nonterminal removal in rust-lang#124141.
38efad3
to
03159d4
Compare
I addressed all the comments. @bors r=petrochenkov |
⌛ Testing commit 03159d4 with merge 31e990b61226b63aeab7a1a9318ceb1e05d498a4... |
…petrochenkov rust-lang#124141 preliminaries Preliminary changes required to start removing `Nonterminal` (rust-lang#124141). r? `@petrochenkov`
💔 Test failed - checks-actions |
@bors retry Failed to connect to github.com port 443: Connection timed out |
…petrochenkov rust-lang#124141 preliminaries Preliminary changes required to start removing `Nonterminal` (rust-lang#124141). r? `@petrochenkov`
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@bors retry because it smells like a random linker crash |
⌛ Testing commit 03159d4 with merge 717f5df2c308dfb4b7b8e6c002c11fe8269c4011... |
☀️ Test successful - checks-actions |
Finished benchmarking commit (717f5df): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResults (primary 0.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.
Bootstrap: 796.195s -> 795.947s (-0.03%) |
Preliminary changes required to start removing
Nonterminal
(#124141).r? @petrochenkov