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.

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

Convert some macros from LegacyBang to Bang. #125094

Closed
wants to merge 1 commit into from

Conversation

nnethercote
Copy link
Contributor

Specifically: cfg!, column!, file!, line!, module_path!.

This requires changing BangProcMacro for F to use the ecx and span arguments, instead of ignoring them.

r? @petrochenkov

Specifically: `cfg!`, `column!`, `file!`, `line!`, `module_path!`.

This requires changing `BangProcMacro for F` to use the `ecx` and `span`
arguments, instead of ignoring them.
@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 May 13, 2024
@nnethercote
Copy link
Contributor Author

I don't know if this is a good idea or not, but I was curious and I converted a few of the easiest ones. There are several more than look easy. There are also harder ones like expand_env that return ExpandResult::Retry(()) if their arguments haven't been fully expanded. I don't know how to handle those.

Comment on lines -293 to +301
F: Fn(TokenStream) -> TokenStream,
F: Fn(&mut ExtCtxt<'_>, Span, TokenStream) -> TokenStream,
{
fn expand<'cx>(
&self,
_ecx: &'cx mut ExtCtxt<'_>,
_span: Span,
ecx: &'cx mut ExtCtxt<'_>,
span: Span,
ts: TokenStream,
) -> Result<TokenStream, ErrorGuaranteed> {
// FIXME setup implicit context in TLS before calling self.
Ok(self(ts))
Ok(self(ecx, span, ts))
Copy link
Member

Choose a reason for hiding this comment

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

I think we can improve the return types here to handle ExpandResult::Retry(()) . e.g.

pub enum BangRetry {
    Yes,
    No,
}

impl<F> BangProcMacro for F
where
    F: Fn(&mut ExtCtxt<'_>, Span, TokenStream) -> Result<TokenStream, BangRetry>,
{
    fn expand<'cx>(
        &self,
        ecx: &'cx mut ExtCtxt<'_>,
        span: Span,
        ts: TokenStream,
    ) -> Result<TokenStream, (BangRetry, ErrorGuaranteed)> {
        self(ecx, span, ts).map_err(|retry| (retry, ErrorGuaranteed(())))
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good suggestion. But I'll wait for feedback from @petrochenkov about whether converting LegacyBang to Bang is a good idea in general before putting more effort in.

@petrochenkov, what do you think?

@petrochenkov
Copy link
Contributor

petrochenkov commented May 23, 2024

Right now built-in bang macros go through the next pipeline when expanding:

"Legacy" in eager positions in built-in macros: produced | AST | consumed
"Modern" in eager positions in built-in macros: produced | Tokens -> Parser -> AST | consumed
"Legacy" in normal positions: produced | AST | consumed
"Modern" in normal positions: produced | Tokens -> Parser -> AST | consumed

So currently we do not have any token-based eager expansion, we only ever consume AST, so we never need the token representation of builtin macros' outputs.
Therefore, the LegacyBang to Bang conversion only adds a performance cost of the unnecessary Tokens -> Parser step right now. (And also makes the macro implementations a bit more wordy, at least until we have some kind of quote! for writing them.)

When/if we have token-based eager expansions we will indeed need the built-in macro outputs in their token forms.
Eager expansions like this probably won't be common compared to regular macro calls, so we'll likely need to continue generate AST directly in most cases just for performance. Tokens will then be either produced from AST somehow (hopefully not by pretty-printing and reparsing) or just generated by an alternative token-based implementation supplied for each built-in macro.

Basically, I don't think we need to do this now.

@petrochenkov
Copy link
Contributor

Very few macros are converted here, so I don't expect much perf difference, but let's try anyway.
@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 23, 2024
@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2024
Convert some macros from `LegacyBang` to `Bang`.

Specifically: `cfg!`, `column!`, `file!`, `line!`, `module_path!`.

This requires changing `BangProcMacro for F` to use the `ecx` and `span` arguments, instead of ignoring them.

r? `@petrochenkov`
@bors
Copy link
Contributor

bors commented May 23, 2024

⌛ Trying commit ca590a9 with merge 38deb65...

@bors
Copy link
Contributor

bors commented May 23, 2024

☀️ Try build successful - checks-actions
Build commit: 38deb65 (38deb65e0d96b976877c54da863fbd9006c2f016)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (38deb65): comparison URL.

Overall result: ❌ regressions - no 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.

@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.3% [0.3%, 0.3%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 2.5%)

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)
2.5% [2.5%, 2.5%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.5% [2.5%, 2.5%] 1

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: 673.262s -> 673.191s (-0.01%)
Artifact size: 315.60 MiB -> 315.71 MiB (0.03%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 23, 2024
@petrochenkov petrochenkov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 24, 2024
@nnethercote
Copy link
Contributor Author

Basically, I don't think we need to do this now.

Ok. My motivation was simple: I saw two ways of doing something, so I tried to take a step towards removing the one described as "legacy", to simplify things. Sounds like that's not appropriate.

@petrochenkov
Copy link
Contributor

Sometimes things in the compiler are named "legacy" too soon.
At some point all macro_rules were referred as "legacy macros" internally (as opposed to "modern" macros 2.0).

@nnethercote nnethercote deleted the LegacyBang-to-Bang branch May 28, 2024 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

None yet

6 participants