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

Less TokenTree cloning #114115

Merged
merged 8 commits into from Jul 28, 2023
Merged

Less TokenTree cloning #114115

merged 8 commits into from Jul 28, 2023

Conversation

nnethercote
Copy link
Contributor

TokenTreeCursor has this comment on it:

// FIXME: Many uses of this can be replaced with by-reference iterator to avoid clones.

This PR completes that FIXME. It doesn't have much perf effect, but at least we now know that.

r? @petrochenkov

@nnethercote
Simplify the ttdelim_span test.
53379a7 
The existing code is a very complex and inefficient way to the get the
span of the last token.
@nnethercote
Replace into_trees with trees in a test.
a9d8459 
There's no need for token tree cloning here.
@nnethercote
Use TokenStream::trees instead of into_trees for attributes.
103bd4a 
This avoids cloning some token trees. A couple of `clone` calls were
inserted, but only on some paths, and the next commit will remove them.
@nnethercote
Make TokenTree::uninterpolate take &self and return a Cow.
55a7324 
Making it similar to `Token::uninterpolate`. This avoids some more token
tree cloning.
@nnethercote
Avoid some token cloning in filter_tokens_from_list.
853f453 
Now the cloning only happens on some paths, instead of all paths.
@nnethercote
Avoid some token tree cloning in decl macro parsing.
d2f7f67 
By changing `into_trees` into `trees`. Some of the subsequent paths
require explicit clones, but not all.
@nnethercote
Avoid into_trees usage in rustfmt.
ee6ed60 
Token tree cloning is only needed in one place.
@nnethercote
Remove Iterator impl for TokenTreeCursor.
4ebf2be 
This is surprising, but the new comment explains why. It's a logical
conclusion in the drive to avoid `TokenTree` clones.

`TokenTreeCursor` is now only used within `Parser`. It's still needed
due to `replace_prev_and_rewind`.
@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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 27, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 27, 2023

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@nnethercote
Copy link
Contributor Author

Best reviewed one commit at a time.

The last commit is quite aggressive, and I could be talked out of it. I was seeing what was the limit of the possibilities here.

Local measurements suggest no significant perf effect, let's confirm that here:

@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 27, 2023
@bors
Copy link
Contributor

bors commented Jul 27, 2023

⌛ Trying commit 4ebf2be with merge 028e606494c3e231dcbd3f8fb891d1709ea275cd...

@bors
Copy link
Contributor

bors commented Jul 27, 2023

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

1 similar comment
@bors
Copy link
Contributor

bors commented Jul 27, 2023

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

@rust-timer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member

Nice improvement for rustdoc. 👍

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 27, 2023

📌 Commit 4ebf2be 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2023
@petrochenkov petrochenkov removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 27, 2023
@bors
Copy link
Contributor

bors commented Jul 28, 2023

⌛ Testing commit 4ebf2be with merge 0699d99...

@bors
Copy link
Contributor

bors commented Jul 28, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 28, 2023
@bors bors merged commit 0699d99 into rust-lang:master Jul 28, 2023
12 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Jul 28, 2023
@bors bors mentioned this pull request Jul 28, 2023
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0699d99): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)		 2.5% [2.0%, 2.9%] 2
Improvements ✅
(primary)		 -0.4% [-0.4%, -0.4%] 2
Improvements ✅
(secondary)		 -2.9% [-2.9%, -2.9%] 1
All ❌✅ (primary) -0.4% [-0.4%, -0.4%] 2

Cycles

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

Binary size

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

Bootstrap: 652.994s -> 653.092s (0.02%)

calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Oct 23, 2023
@bors
Auto merge of rust-lang#114115 - nnethercote:less-token-tree-cloning,…
1f369f8 
… r=petrochenkov

Less `TokenTree` cloning

`TokenTreeCursor` has this comment on it:
```
// FIXME: Many uses of this can be replaced with by-reference iterator to avoid clones.
```
This PR completes that FIXME. It doesn't have much perf effect, but at least we now know that.

r? `@petrochenkov`
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

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. T-rustdoc Relevant to the rustdoc 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.

None yet
6 participants
@nnethercote @rustbot @rust-timer @bors @GuillaumeGomez @petrochenkov