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.

Sign up for GitHub

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

Jump to bottom

Store tokens inside ast::Expr #72287

Merged
merged 1 commit into from
May 25, 2020
Merged

Store tokens inside ast::Expr #72287

merged 1 commit into from
May 25, 2020

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented May 17, 2020 •
edited
Loading

This is a smaller version of #70091.

We now store captured tokens inside ast::Expr, which allows us to avoid some reparsing in nt_to_tokenstream. To try to mitigate the performance impact, we only collect tokens when we've seen an outer attribute.

This makes progress towards solving #43081. There are still many things left to do:

  • Collect tokens for other AST items.
  • Come up with a way to handle inner attributes (we need to be collecting tokens by the time we encounter them)
  • Avoid re-parsing when a #[cfg] attr is used.

However, this is enough to fix spans for a simple example, which I've included as a test case.

@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 17, 2020
@Aaron1011
Copy link
Member Author

r? @petrochenkov

@Aaron1011

This comment has been minimized.

Sign in to view
@rust-timer

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 2728275cf62b7f7bfb2bbc99674bd8476722477a, comparison URL.

petrochenkov
petrochenkov reviewed May 17, 2020
View reviewed changes
src/test/ui/proc-macro/auxiliary/noop-collect.rs Outdated Show resolved Hide resolved
src/librustc_parse/parser/expr.rs Outdated Show resolved Hide resolved
src/librustc_parse/parser/expr.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

Not sure why there's such slowdown, absolute majority of expressions doesn't have attributes.
Perhaps it happens due to #72287 (comment)?
(We know that simply increasing the ast::Expr size doesn't result in a slowdown, see #70200.)

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2020
@Aaron1011

This comment has been minimized.

Sign in to view
@rust-timer

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 11e2f20befcdd9b10073fe96ba2e880ac822352d, comparison URL.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 18, 2020
@Aaron1011 Aaron1011 force-pushed the feature/min-token-collect branch from 72b44ee to 4855363 Compare May 19, 2020 20:56
@Aaron1011
Copy link
Member Author

Let's make sure that my new collect_tokens implementation isn't slower.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 7bc7cfa40f58523836318ebc43f17436ce067963, comparison URL.

@estebank
Copy link
Contributor

FWIW, the code looks mergeable and I'm really excited about the UX improvement.

@Aaron1011 Aaron1011 force-pushed the feature/min-token-collect branch from 4855363 to d5a2dbe Compare May 20, 2020 03:32
@petrochenkov
Copy link
Contributor

I'll review this in detail ~tomorrow, but from a cursory view the collecting mechanism change in librustc_parse/parser/mod.rs looks entirely orthogonal to collecting tokens for expressions specifically.
Could you move it to a separate PR?

@Aaron1011 Aaron1011 mentioned this pull request May 20, 2020
Merged
@Aaron1011
Copy link
Member Author

Aaron1011 commented May 20, 2020 •
edited
Loading

@petrochenkov: I opened #72393 with just the collect_tokens changes

@petrochenkov
Copy link
Contributor

Blocked on #72393.

@petrochenkov petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2020
RalfJung added a commit to RalfJung/rust that referenced this pull request May 23, 2020
@RalfJung
Rollup merge of rust-lang#72393 - Aaron1011:feature/rewrite-collect-t…
8e98d8d 
…okens, r=petrochenkov

Rewrite `Parser::collect_tokens`

The previous implementation did not work when called on an opening
delimiter, or when called re-entrantly from the same `TokenCursor` stack
depth.

I'm not sure how to test this apart from rust-lang#72287
RalfJung added a commit to RalfJung/rust that referenced this pull request May 24, 2020
@RalfJung
Rollup merge of rust-lang#72393 - Aaron1011:feature/rewrite-collect-t…
8692c45 
…okens, r=petrochenkov

Rewrite `Parser::collect_tokens`

The previous implementation did not work when called on an opening
delimiter, or when called re-entrantly from the same `TokenCursor` stack
depth.

I'm not sure how to test this apart from rust-lang#72287
@bors

This comment has been minimized.

Sign in to view
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels May 24, 2020
@Aaron1011 Aaron1011 force-pushed the feature/min-token-collect branch from d5a2dbe to 14382c6 Compare May 24, 2020 20:11
@Aaron1011
Copy link
Member Author

@petrochenkov: Rebased

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 24, 2020

📌 Commit 14382c6 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 24, 2020
@bors
Copy link
Contributor

bors commented May 24, 2020

⌛ Testing commit 14382c6 with merge 62da38d...

@bors
Copy link
Contributor

bors commented May 25, 2020

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing 62da38d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 25, 2020
@bors bors merged commit 62da38d into rust-lang:master May 25, 2020
@Aaron1011 Aaron1011 mentioned this pull request May 25, 2020
Closed
@taiki-e taiki-e mentioned this pull request Jun 25, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

@estebank estebank estebank left review comments

Assignees

@petrochenkov petrochenkov

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Aaron1011 @rust-highfive @rust-timer @bors @petrochenkov @estebank