Skip to content

mbe: Refactor transcription #142713

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 2 commits into from
Jun 21, 2025
Merged

mbe: Refactor transcription #142713

merged 2 commits into from
Jun 21, 2025

Conversation

tgross35
Copy link
Contributor

Introduce MacroTcbCtx that holds everything relevant to transcription. This allows for the following changes:

  • Split transcribe_sequence and transcribe_metavar out of the heavily nested transcribe
  • Split metavar_expr_concat out of transcribe_metavar_expr

This is a nonfunctional change.

@rustbot
Copy link
Collaborator

rustbot commented Jun 19, 2025

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Jun 19, 2025
@tgross35 tgross35 force-pushed the mbe-transcribe-refactor branch 2 times, most recently from 290b8be to 620e379 Compare June 19, 2025 09:50
@petrochenkov petrochenkov self-assigned this Jun 19, 2025
fbstj
fbstj reviewed Jun 19, 2025
View reviewed changes
@tgross35 tgross35 force-pushed the mbe-transcribe-refactor branch from 620e379 to 0c434db Compare June 19, 2025 15:55
petrochenkov
petrochenkov reviewed Jun 19, 2025
View reviewed changes
@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 Jun 19, 2025
tgross35 added 2 commits June 19, 2025 16:27
@tgross35
mbe: Move transcribe_metavar_expr directly after transcribe
496cbe1 
Be more consistent with the otherwise top-down organization of this
file.
@tgross35
mbe: Refactor transcription
67a9fb2 
Introduce `MacroTcbCtx` that holds everything relevant to transcription.
This allows for the following changes:

* Split `transcribe_sequence` and `transcribe_metavar` out of the
  heavily nested `transcribe`
* Split `metavar_expr_concat` out of `transcribe_metavar_expr`

This is a nonfunctional change.
@tgross35 tgross35 force-pushed the mbe-transcribe-refactor branch from 0c434db to 67a9fb2 Compare June 19, 2025 16:29
@tgross35
Copy link
Contributor Author

@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 Jun 19, 2025
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 19, 2025

📌 Commit 67a9fb2 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 Jun 19, 2025
@tgross35 tgross35 mentioned this pull request Jun 20, 2025
Merged
bors added a commit that referenced this pull request Jun 20, 2025
@bors
Auto merge of #142794 - tgross35:rollup-iae7okj, r=tgross35
710a45d 
Rollup of 9 pull requests

Successful merges:

 - #142331 (Add `trim_prefix` and `trim_suffix` methods for both `slice` and `str` types.)
 - #142491 (Rework #[cold] attribute parser)
 - #142494 (Fix missing docs in `rustc_attr_parsing`)
 - #142495 (Better template for `#[repr]` attributes)
 - #142497 (Fix random failure when JS code is executed when the whole file was not read yet)
 - #142575 (Ensure copy* intrinsics also perform the static self-init checks)
 - #142650 (Refactor Translator)
 - #142713 (mbe: Refactor transcription)
 - #142755 (rustdoc: Remove `FormatRenderer::cache`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request Jun 20, 2025
@bors
Auto merge of #142794 - tgross35:rollup-iae7okj, r=tgross35
15c701f 
Rollup of 9 pull requests

Successful merges:

 - #142331 (Add `trim_prefix` and `trim_suffix` methods for both `slice` and `str` types.)
 - #142491 (Rework #[cold] attribute parser)
 - #142494 (Fix missing docs in `rustc_attr_parsing`)
 - #142495 (Better template for `#[repr]` attributes)
 - #142497 (Fix random failure when JS code is executed when the whole file was not read yet)
 - #142575 (Ensure copy* intrinsics also perform the static self-init checks)
 - #142650 (Refactor Translator)
 - #142713 (mbe: Refactor transcription)
 - #142755 (rustdoc: Remove `FormatRenderer::cache`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e7a2c40 into rust-lang:master Jun 21, 2025
10 checks passed
@rustbot rustbot added this to the 1.89.0 milestone Jun 21, 2025
rust-timer added a commit that referenced this pull request Jun 21, 2025
@rust-timer
Unrolled build for #142713
7c92850 
Rollup merge of #142713 - tgross35:mbe-transcribe-refactor, r=petrochenkov

mbe: Refactor transcription

Introduce `MacroTcbCtx` that holds everything relevant to transcription. This allows for the following changes:

* Split `transcribe_sequence` and `transcribe_metavar` out of the heavily nested `transcribe`
* Split `metavar_expr_concat` out of `transcribe_metavar_expr`

This is a nonfunctional change.
@tgross35 tgross35 deleted the mbe-transcribe-refactor branch June 21, 2025 02:06
@tgross35
Copy link
Contributor Author

@rust-timer build 7c92850

For #142794 (comment)

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot mentioned this pull request Jun 21, 2025
Draft
rust-bors bot added a commit that referenced this pull request Jun 21, 2025
@rust-bors
Auto merge of #142815 - tgross35:transcribe-perf, r=<try>
ae84552 
mbe: Inline functions in `transcribe` that are only called once

[#142713] did some refactoring of macro expansion, which seems to have had an effect on performance. The only change was to split large functions into smaller ones, so mark some functions that are only called once `#[inline]` to make codegen more similar.

[#142713]: https://www.github.com/rust-lang/rust/pull/142713
@tgross35
Copy link
Contributor Author

Trying to get some perf back at #142815

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7c92850): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)		 2.9% [2.9%, 2.9%] 1
Regressions ❌
(secondary)		 0.8% [0.2%, 1.2%] 10
Improvements ✅
(primary)		 -0.2% [-0.2%, -0.2%] 2
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.8% [-0.2%, 2.9%] 3

Max RSS (memory usage)

Results (primary -2.5%, secondary -1.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 4.4% [3.8%, 5.1%] 2
Improvements ✅
(primary)		 -2.5% [-3.2%, -1.7%] 4
Improvements ✅
(secondary)		 -2.3% [-3.0%, -1.3%] 11
All ❌✅ (primary) -2.5% [-3.2%, -1.7%] 4

Cycles

Results (primary 2.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Binary size

Results (primary 1.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Bootstrap: 692.994s -> 693.507s (0.07%)
Artifact size: 371.93 MiB -> 372.01 MiB (0.02%)

@rustbot rustbot added the perf-regression Performance regression. label Jun 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fbstj fbstj fbstj left review comments

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
perf-regression Performance regression. 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.89.0
Development

Successfully merging this pull request may close these issues.
7 participants
@tgross35 @rustbot @petrochenkov @bors @rust-timer @fbstj @oli-obk