Skip to content

Move doc comment desugaring out of TokenCursor. #114273

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
Merged
merged 8 commits into from
Aug 1, 2023
Merged

Move doc comment desugaring out of TokenCursor. #114273

merged 8 commits into from
Aug 1, 2023

Conversation

nnethercote
Copy link
Contributor

It's awkward that TokenCursor sometimes desugars doc comments on the fly, but usually doesn't.

r? @petrochenkov

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 31, 2023
@nnethercote
Copy link
Contributor Author

Best reviewed one commit at a time.

Local measurements showed some very small wins and losses that balanced out. Let's check on CI:

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 31, 2023
@bors
Copy link
Collaborator

bors commented Jul 31, 2023

⌛ Trying commit 207e91f1f1bb8b7f30b81df3e7ee25e5b2bd7194 with merge c1da2ac8b4d0792d44e141afd78ed4de1889044f...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@nnethercote
Move doc comment desugaring out of TokenCursor.
ff7d5ba 
`TokenCursor` currently does doc comment desugaring on the fly, if the
`desugar_doc_comment` field is set. This requires also modifying the
token stream on the fly with `replace_prev_and_rewind`.

This commit moves the doc comment desugaring out of `TokenCursor`, by
introducing a new `TokenStream::desugar_doc_comment` method. This
separation of desugaring and iterating makes the code nicer.
@nnethercote
Remove TokenTreeCursor::replace_prev_and_rewind.
d72fc5c 
It's no longer used.
@nnethercote
Remove desugar_doc_comments arg from Parser::new().
2e6ce68 
It's only true at one call site; do the desugaring there instead.
@nnethercote nnethercote force-pushed the move-doc-comment-desugaring branch from 207e91f to 2e6ce68 Compare July 31, 2023 04:47
@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Jul 31, 2023

⌛ Trying commit 2e6ce68 with merge 1c99872827b1b98e6ddd5920b7c0dfb26790528f...

@petrochenkov
Copy link
Contributor

So, the performance assumption here is that if we create a parser for macro RHS, then it will go through all the input token stream without returning early (at least on a good path without errors).
That means we can eagerly desugar all doc comments once at the start, instead of desugaring them lazily as we go.

This assumption seems true, unless I'm missing something, so we can merge this.
r=me if perf is good.
@rustbot author

@rustbot rustbot 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 Jul 31, 2023
@bors
Copy link
Collaborator

bors commented Jul 31, 2023

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

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1c99872827b1b98e6ddd5920b7c0dfb26790528f): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking 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 @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 0.4% [0.1%, 0.6%] 6
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -1.1% [-1.2%, -1.1%] 4
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

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.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -3.5% [-3.5%, -3.5%] 1
Improvements ✅
(secondary)		 -2.4% [-2.4%, -2.4%] 1
All ❌✅ (primary) -3.5% [-3.5%, -3.5%] 1

Cycles

Results

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.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.8% [2.8%, 2.8%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Binary size

Results

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.

mean range count
Regressions ❌
(primary)		 0.1% [0.0%, 0.1%] 7
Regressions ❌
(secondary)		 0.0% [0.0%, 0.0%] 6
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.1% [0.0%, 0.1%] 7

Bootstrap: 652.068s -> 652.15s (0.01%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 31, 2023
@nnethercote
Copy link
Contributor Author

nnethercote commented Jul 31, 2023 •
edited
Loading

Perf is as expected: very small icount regressions on tt-muncher, which is the worst-case scenario for this change, because it heavily stresses decl. macro expansion. Otherwise, very small changes across the board, with tiny improvements more common than tiny regressions.

@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented Jul 31, 2023

📌 Commit 2e6ce68 has been approved by petrochenkov

It is now in the queue for this repository.

@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 Jul 31, 2023
@bors
Copy link
Collaborator

bors commented Aug 1, 2023

⌛ Testing commit 2e6ce68 with merge d12c6e9...

@bors
Copy link
Collaborator

bors commented Aug 1, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 1, 2023
@bors bors merged commit d12c6e9 into rust-lang:master Aug 1, 2023
@rustbot rustbot added this to the 1.73.0 milestone Aug 1, 2023
@nnethercote nnethercote deleted the move-doc-comment-desugaring branch August 1, 2023 23:16
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d12c6e9): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 0.5% [0.4%, 0.6%] 6
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

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.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -1.8% [-1.8%, -1.8%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -1.8% [-1.8%, -1.8%] 1

Binary size

Results

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.

mean range count
Regressions ❌
(primary)		 0.1% [0.0%, 0.1%] 7
Regressions ❌
(secondary)		 0.0% [0.0%, 0.0%] 6
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.1% [0.0%, 0.1%] 7

Bootstrap: missing data

@rustbot rustbot removed the perf-regression Performance regression. label Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.
6 participants
@nnethercote @rust-timer @bors @rust-log-analyzer @petrochenkov @rustbot