Skip to content

Refactor HygieneEncodeContext - #162520

Open
aerooneqq wants to merge 9 commits into
rust-lang:mainfrom
aerooneqq:hygiene-perf
Open

Refactor HygieneEncodeContext#162520
aerooneqq wants to merge 9 commits into
rust-lang:mainfrom
aerooneqq:hygiene-perf

Conversation

@aerooneqq

@aerooneqq aerooneqq commented Sep 9, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

View all comments

Some refactorings around HygieneEncodeContext with minor perf improvements, the most notable thing is that Locks were removed.

First part for #161450.

r? @petrochenkov

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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 Sep 9, 2026
@petrochenkov

petrochenkov commented Sep 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

This is missing some high level description.
What is the logic behind the changes in HygieneEncodeContext::encode?
Is the &'a -> Rc change necessary to get rid of locks, or is just to avoid lifetimes in signatures?
Some fields like latest_ctxts -> queued_ctxts apparently change their meaning but the descriptions are not updated.

petrochenkov
petrochenkov reviewed Sep 9, 2026
View reviewed changes
Comment thread compiler/rustc_span/src/hygiene.rs Outdated
Comment thread compiler/rustc_span/src/hygiene.rs Outdated
Comment thread compiler/rustc_span/src/hygiene.rs
petrochenkov
petrochenkov reviewed Sep 9, 2026
View reviewed changes
Comment thread compiler/rustc_span/src/hygiene.rs Outdated
Comment thread compiler/rustc_span/src/hygiene.rs
Comment thread compiler/rustc_span/src/hygiene.rs Outdated
Comment thread compiler/rustc_span/src/hygiene.rs Outdated
Comment thread compiler/rustc_span/src/hygiene.rs
Comment thread compiler/rustc_span/src/hygiene.rs Outdated
Comment thread compiler/rustc_span/src/hygiene.rs Outdated
@petrochenkov

petrochenkov commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

What is the logic behind the changes in HygieneEncodeContext::encode?

So the changes are to 1) reuse the capacity between the loop iterations (#162520 (comment)) and 2) move some map searches under the HygieneData lock (#162520 (comment)).

@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 Sep 9, 2026
@aerooneqq

aerooneqq commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Some fields like latest_ctxts -> queued_ctxts apparently change their meaning but the descriptions are not updated.

Renamed them back.

Is the &'a -> Rc change necessary to get rid of locks, or is just to avoid lifetimes in signatures?

There were borrowing problems because hygiene context is contained inside the encoder.

@rustbot ready

@rustbot rustbot 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 Sep 10, 2026
petrochenkov
petrochenkov reviewed Sep 10, 2026
View reviewed changes
Comment thread compiler/rustc_span/src/hygiene.rs
// Consume the current round of syntax contexts.
// Drop the lock() temporary early.
// It's fine to iterate over a HashMap, because the serialization of the table
// that we insert data into doesn't depend on insertion order.

@petrochenkov petrochenkov Sep 10, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you keep these comments? They look useful.

View changes since the review

Comment thread compiler/rustc_span/src/hygiene.rs
pub fn raw_encode_syntax_context(
ctxt: SyntaxContext,
context: &HygieneEncodeContext,
context: Rc<RefCell<HygieneEncodeContext>>,

@petrochenkov petrochenkov Sep 10, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
context: Rc<RefCell<HygieneEncodeContext>>,
context: &RefCell<HygieneEncodeContext>,

This doesn't work here due to encode below?

View changes since the review

@petrochenkov

petrochenkov commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

r=me after addressing the remaining comments and squashing commits.
@bors delegate+
@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 10, 2026
@rust-bors

rust-bors Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

✌️ @aerooneqq, you can now approve this pull request!

If @petrochenkov told you to "r=me" after making some further change, then please make that change and post @bors r=petrochenkov.

View changes since this delegation.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 10, 2026
@rustbot

rustbot commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

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

At least 1 approving review is required to merge this pull request.

Assignees

@petrochenkov petrochenkov

Labels

A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aerooneqq @petrochenkov @rustbot