Skip to content
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

Fix lookahead with None-delimited group #84130

Merged
merged 2 commits into from
Apr 14, 2021

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Apr 12, 2021

Fixes #84162, a regression introduced by #82608.

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 12, 2021
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 12, 2021
@bors
Copy link
Contributor

bors commented Apr 12, 2021

⌛ Trying commit eb7b1a1 with merge ce7729406a858c09fb39886aa783593f2f8d2d62...

Comment on lines +933 to 944
let mut i = 0;
let mut token = Token::dummy();
while i < dist {
token = cursor.next().0;
if matches!(
token.kind,
token::OpenDelim(token::NoDelim) | token::CloseDelim(token::NoDelim)
) {
continue;
}
i += 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let mut i = 0;
let mut token = Token::dummy();
while i < dist {
token = cursor.next().0;
if matches!(
token.kind,
token::OpenDelim(token::NoDelim) | token::CloseDelim(token::NoDelim)
) {
continue;
}
i += 1;
}
let mut token = Token::dummy();
while dist > 0 {
token = cursor.next().0;
if !matches!(
token.kind,
token::OpenDelim(token::NoDelim) | token::CloseDelim(token::NoDelim)
) {
dist -= 1;
}
}

Will this be better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks less intuitive to me.

@bors
Copy link
Contributor

bors commented Apr 12, 2021

☀️ Try build successful - checks-actions
Build commit: ce7729406a858c09fb39886aa783593f2f8d2d62 (ce7729406a858c09fb39886aa783593f2f8d2d62)

@rust-timer
Copy link
Collaborator

Queued ce7729406a858c09fb39886aa783593f2f8d2d62 with parent c18c0ad, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (ce7729406a858c09fb39886aa783593f2f8d2d62): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 12, 2021
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 12, 2021
@bors
Copy link
Contributor

bors commented Apr 12, 2021

⌛ Trying commit c6d67f8 with merge 2cf9ebe7a46b277e9440ebc9a642932dc0a3f08c...

@bors
Copy link
Contributor

bors commented Apr 12, 2021

☀️ Try build successful - checks-actions
Build commit: 2cf9ebe7a46b277e9440ebc9a642932dc0a3f08c (2cf9ebe7a46b277e9440ebc9a642932dc0a3f08c)

@rust-timer
Copy link
Collaborator

Queued 2cf9ebe7a46b277e9440ebc9a642932dc0a3f08c with parent d0695c9, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (2cf9ebe7a46b277e9440ebc9a642932dc0a3f08c): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 13, 2021
@Aaron1011
Copy link
Member Author

Aaron1011 commented Apr 13, 2021

Apparently, this is a small performance win on a few benchmarks.

r? @petrochenkov

@Mark-Simulacrum
Copy link
Member

rustc_parse compilation has regressed 10%, which is above the usual noise on that measurement. Is that expected?

@Aaron1011
Copy link
Member Author

@virtualritz: It should be possible for affected crate authors to request rebuilds by opening issues on https://github.com/rust-lang/docs.rs

@jethrogb
Copy link
Contributor

@rustbot modify labels: +regression-from-stable-to-nightly

@rustbot rustbot added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Apr 14, 2021
@jyn514
Copy link
Member

jyn514 commented Apr 14, 2021

@jethrogb this is tracked by #84162, we don't prioritize PRs usually.

@jyn514 jyn514 removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 14, 2021
@jethrogb
Copy link
Contributor

Whoops, my bad, it's right there in the PR description.

@jyn514
Copy link
Member

jyn514 commented Apr 14, 2021

Whoops, my bad, it's right there in the PR description.

Only since a few minutes ago 😉 I edited it

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 14, 2021

📌 Commit c6d67f8 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 14, 2021
@jyn514
Copy link
Member

jyn514 commented Apr 14, 2021

@bors p=1 this is breaking a lot of crates

@bors
Copy link
Contributor

bors commented Apr 14, 2021

⌛ Testing commit c6d67f8 with merge 16bf626...

@bors
Copy link
Contributor

bors commented Apr 14, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 16bf626 to master...

@rylev
Copy link
Member

rylev commented Apr 21, 2021

@Aaron1011 @petrochenkov @Mark-Simulacrum Looks like the perf run performed after this was merged showed the same regressions in compilation of the same benchmarks as well as compilation of rustc_parse crate.

The compilation regressions in the benchmarks might be expected since this is strictly doing more work, but I wonder if we have an idea why this would regress compilation of the rustc_parse crate itself so much.

chfast added a commit to ethereum/evmc that referenced this pull request Apr 26, 2021
This reverts commit 29dcf23
because the Rust nightly regression has been fixed.
rust-lang/rust#84130
@Mark-Simulacrum
Copy link
Member

I think I actually have a pretty good sense: that function likely gets duplicated a bunch of times, as it's generic over the looker closure passed in. This PR added a non-trivial chunk of code to it, which is now getting monomorphized a bunch more. I've added to my todo list to investigate whether changing the function to take the looker without monomorphization or outlining a chunk of the code makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clang-sys 0.29.3 fails to build on nightly