Fix macro parser quadratic complexity in small repeating groups #52145
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
For a fuller description of the performance issue fixed by this: rust-lang#51754 (comment)
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
It would be cool to see the results of a perf run :) |
After re-reading this a couple times, I think autocorrect got you there =P |
|
Oh, yeah. Autoincorrect rather... I edited, but I forgot it doesn't send emails. |
|
@bors try I don't know that we'll see much from it, but we can try. |
bors
added a commit
that referenced
this pull request
Jul 8, 2018
Fix macro parser quadratic complexity in small repeating groups Observed in #51754, and more easily demonstrated with the following: ```rust macro_rules! stress { ($($t:tt)+) => { }; } fn main() { stress!{ a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a // ... 65536 copies of "a" total ... a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a } } ``` which takes 50 seconds to compile prior to the fix and <1s after. I hope this has a visible impact on the compile times for real code. (I think it is most likely to affect incremental TT munchers that deal with large inputs, though it depends on how they are written) For a fuller description of the performance issue: #51754 (comment) --- There is no test (yet) because I'm not sure how easily to measure this for regressions.
|
☀️ Test successful - status-travis |
|
@rust-timer build 257d854 |
|
Success: Queued 257d854 with parent 0e6b713, comparison URL. |
|
Performance... regressed? This is just dropping something earlier, so all I can imagine is:
|
|
Fairly certain that the measurements are just noise. |
|
+/- a few percent seems to "just happen" sometimes, yeah |
|
@ExpHP I think adding a regression test to the perf benchmark suite would be appropriate, no @Mark-Simulacrum ? |
nikomatsakis
approved these changes
Jul 16, 2018
nikomatsakis
approved these changes
Jul 16, 2018
src/libsyntax/ext/tt/macro_parser.rs
Outdated
| @@ -696,10 +696,17 @@ pub fn parse( | |||
| } else { | |||
| return Failure(parser.span, token::Eof); | |||
| } | |||
| } else { | |||
There was a problem hiding this comment.
Just curious, why put this in an else?
There was a problem hiding this comment.
Short answer:
To satisfy (oops, see next comment)dropck.
Long answer:
I primarily saw two options here:
Drop eof_items at the beginning of the one branch that is currently affected by the drop. -- This had the disadvantage that future refactorings could easily introduce similar performance regressions (possibly under different conditions).
Drop eof_items as soon as possible. -- This required splitting the large if {} else if {} else if {} ... into two. I saw this as the lesser of two evils, despite the following disadvantages:
eof_itemsmust be awkwardly dropped inside anelse, since it was moved in theifbranch.- It takes a bit of careful reading to realize that the
ifbranch unconditionally diverges, especially now that it has anelsebranch. For this reason, I added the assertion two lines below.
There was a problem hiding this comment.
Oops. Right after posting that I decided to double-check on the playground, and it turns out that move analysis is in fact smart enough that the else is not necessary.
That, and now that I look more closely, the if branch here doesn't actually even move eof_items.
|
@bors r+ |
|
📌 Commit 191e76c has been approved by |
bors
added
S-waiting-on-bors
|
Agreed that a regression test would be good. |
|
After rerunning tests locally I'm going to push another commit to get rid of the (I will also remove the assertion, which was originally added because I thought the |
|
local test failed on some |
|
@bors r=nikomatsakis |
|
📌 Commit 0467ae0 has been approved by |
|
⌛ Testing commit 0467ae0 with merge dd6e0358baae473ce9d0b10c6dd729780bf8a10c... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
1 similar comment
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jul 17, 2018
Fix macro parser quadratic complexity in small repeating groups Observed in #51754, and more easily demonstrated with the following: ```rust macro_rules! stress { ($($t:tt)+) => { }; } fn main() { stress!{ a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a // ... 65536 copies of "a" total ... a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a } } ``` which takes 50 seconds to compile prior to the fix and <1s after. I hope this has a visible impact on the compile times for real code. (I think it is most likely to affect incremental TT munchers that deal with large inputs, though it depends on how they are written) For a fuller description of the performance issue: #51754 (comment) --- There is no test (yet) because I'm not sure how easily to measure this for regressions.
|
☀️ Test successful - status-appveyor, status-travis |
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.
Observed in #51754, and more easily demonstrated with the following:
which takes 50 seconds to compile prior to the fix and <1s after.
I hope this has a visible impact on the compile times for real code. (I think it is most likely to affect incremental TT munchers that deal with large inputs, though it depends on how they are written)
For a fuller description of the performance issue: #51754 (comment)
There is no test (yet) because I'm not sure how easily to measure this for regressions.