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

Speed up Parser::expected_tokens #133793

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Dec 3, 2024

The constant pushing/clearing of Parser::expected_tokens during parsing is slow. This PR speeds it up greatly.

r? @estebank

@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 Dec 3, 2024
@nnethercote
Copy link
Contributor Author

@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 Dec 3, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 3, 2024
…, r=<try>

Speed up `Parser::expected_tokens`

r? `@ghost`
@bors
Copy link
Contributor

bors commented Dec 3, 2024

⌛ Trying commit 0133601 with merge 4e6952e...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 3, 2024

☀️ Try build successful - checks-actions
Build commit: 4e6952e (4e6952e2fa4367d9a5ef87505fa18f0dd3fedcc4)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4e6952e): comparison URL.

Overall result: ✅ improvements - 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 the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.9% [-2.4%, -0.2%] 211
Improvements ✅
(secondary)
-0.8% [-2.6%, -0.1%] 101
All ❌✅ (primary) -0.9% [-2.4%, -0.2%] 211

Max RSS (memory usage)

Results (primary -1.2%, secondary 1.6%)

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.5% [0.5%, 0.5%] 1
Regressions ❌
(secondary)
2.9% [0.9%, 5.4%] 3
Improvements ✅
(primary)
-2.1% [-2.3%, -1.8%] 2
Improvements ✅
(secondary)
-2.2% [-2.2%, -2.2%] 1
All ❌✅ (primary) -1.2% [-2.3%, 0.5%] 3

Cycles

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

Binary size

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

Bootstrap: 767.333s -> 766.554s (-0.10%)
Artifact size: 332.08 MiB -> 332.14 MiB (0.02%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 3, 2024
@nnethercote nnethercote force-pushed the speed-up-expected_tokens branch from 0133601 to f5482df Compare December 4, 2024 05:40
@nnethercote nnethercote marked this pull request as ready for review December 4, 2024 05:40
@rustbot
Copy link
Collaborator

rustbot commented Dec 4, 2024

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@nnethercote
Copy link
Contributor Author

Best reviewed one commit at a time.

Let's re-run perf just to be sure: @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 Dec 4, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 4, 2024
…, r=<try>

Speed up `Parser::expected_tokens`

The constant pushing/clearing of `Parser::expected_tokens` during parsing is slow. This PR speeds it up greatly.

r? `@estebank`
@bors
Copy link
Contributor

bors commented Dec 4, 2024

⌛ Trying commit f5482df with merge 26060e6...

Comment on lines 16 to 17
/// We really want to keep the number of variants to 128 or fewer, sot that
/// `TokenTypeSet` can be implemented with a `u128`.
Copy link
Contributor

Choose a reason for hiding this comment

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

On the one hand, we should be able to do so. On the other, I can see this becoming a point of contention with t-lang in the medium future if we push back on a feature for this reason :)

Copy link
Contributor Author

@nnethercote nnethercote Dec 4, 2024

Choose a reason for hiding this comment

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

The 17 asm symbols would be a good place to cut things down if necessary. I'm a bit annoyed that they are even in there; so many of them for such a rare use case.

Comment on lines 264 to 277
// This assertion will detect if this method and the type definition get out of sync.
assert_eq!(token_type as u32, val);
token_type
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't the function just be the as cast with a <=104 check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. You can convert a C-style enum to an integer with as, but you can't convert in the other direction, e.g. as per this StackOverflow answer. There are proc macros to do it, but that answer pointed out an alternative that is suitable here: transmute is fine so long as the enum is repr(uN) for some value of N. So I will do that with repr(u8), which will cut over 100 lines of code, yay.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a static assertion that all of them roundtrip? I'm always concerned about a careless future reformat breaking the bidirectional mapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be happy for extra protection, but I'm having trouble imagining what such a static assertion would look like. Can you explain more?

Copy link
Contributor Author

@nnethercote nnethercote Dec 5, 2024

Choose a reason for hiding this comment

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

Alternatively, I can go back to an explicit match. The StackOverflow answer mentioned this style:

match v {
    x if x == MyEnum::A as i32 => Ok(MyEnum::A),
    x if x == MyEnum::B as i32 => Ok(MyEnum::B),
    x if x == MyEnum::C as i32 => Ok(MyEnum::C),
    _ => Err(()),
}

It requires a line for every variant, but avoids having to write a number on each line.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be happy for extra protection, but I'm having trouble imagining what such a static assertion would look like. Can you explain more?

I'd forgotten that the range operation doesn't work today in const, but I was picturing something like:

const __CHECK: () = const {
    for i in 0..2 {
        assert_eq!(E::to_i32(&E::from_i32(i)), i);
    }
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've gone with the guard-based version, using a macro to avoid excessive boilerplate. It doesn't rely on unsafe, and also doesn't rely on matching up the right integer with the right variant.

@estebank
Copy link
Contributor

estebank commented Dec 4, 2024

I'll finish reviewing tomorrow

@bors
Copy link
Contributor

bors commented Dec 4, 2024

☀️ Try build successful - checks-actions
Build commit: 26060e6 (26060e63f06a4dcd55fc0757eb5b0bdc8136ed3b)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (26060e6): comparison URL.

Overall result: ✅ improvements - 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 the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.9% [-2.4%, -0.2%] 213
Improvements ✅
(secondary)
-0.8% [-2.7%, -0.1%] 98
All ❌✅ (primary) -0.9% [-2.4%, -0.2%] 213

Max RSS (memory usage)

Results (primary -0.9%, secondary 0.8%)

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.4% [0.4%, 0.4%] 1
Regressions ❌
(secondary)
1.7% [0.7%, 2.3%] 3
Improvements ✅
(primary)
-2.2% [-2.2%, -2.2%] 1
Improvements ✅
(secondary)
-1.8% [-1.8%, -1.8%] 1
All ❌✅ (primary) -0.9% [-2.2%, 0.4%] 2

Cycles

Results (primary 1.4%, secondary 2.4%)

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)
1.4% [0.9%, 2.5%] 22
Regressions ❌
(secondary)
2.4% [1.3%, 3.2%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.4% [0.9%, 2.5%] 22

Binary size

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

Bootstrap: 767.635s -> 770.294s (0.35%)
Artifact size: 330.89 MiB -> 330.89 MiB (0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 4, 2024
@nnethercote nnethercote force-pushed the speed-up-expected_tokens branch from f5482df to a353d56 Compare December 4, 2024 09:57
@nnethercote
Copy link
Contributor Author

I updated, adding a new commit that uses transmute for the int-to-TokenType conversion.

@nnethercote nnethercote force-pushed the speed-up-expected_tokens branch 4 times, most recently from a9b457b to 4fef25f Compare December 6, 2024 03:59
@bors
Copy link
Contributor

bors commented Dec 9, 2024

☔ The latest upstream changes (presumably #134039) made this pull request unmergeable. Please resolve the merge conflicts.

@nnethercote nnethercote force-pushed the speed-up-expected_tokens branch from 4fef25f to f2df88c Compare December 9, 2024 09:10
@nnethercote
Copy link
Contributor Author

I rebased.

@estebank, are you happy with the updated final commit?

@bors
Copy link
Contributor

bors commented Dec 10, 2024

☔ The latest upstream changes (presumably #129514) made this pull request unmergeable. Please resolve the merge conflicts.

Because the `Token` type is similar to but different to the `TokenType`
type, and the difference is important, so we want to avoid confusion.
The most significant is `check_keyword`: it now only pushes to
`expected_token_types` if the keyword check fails, which matches how all
the other `check` methods work.

The remainder are just tweaks to make these methods more consistent with
each other.
This is a naming convention used in a handful of spots in the parser for
delimiters. It confused me when I first saw it a long time ago, and I've
never liked it. A web search says "Bra-ket notation" exists in linear
algebra but the terminology has zero prior use in a programming context,
as far as I can tell.

This commit changes it to `open`/`close`, which is consistent with the
rest of the compiler.
The parser pushes a `TokenType` to `Parser::expected_token_types` on
every call to the various `check`/`eat` methods, and clears it on every
call to `bump`. Some of those `TokenType` values are full tokens that
require cloning and dropping. This is a *lot* of work for something
that is only used in error messages and it accounts for a significant
fraction of parsing execution time.

This commit overhauls `TokenType` so that `Parser::expected_token_types`
can be implemented as a bitset. This requires changing `TokenType` to a
C-style parameterless enum, and adding `TokenTypeSet` which uses a
`u128` for the bits. (The new `TokenType` has 105 variants.)

The new types `ExpTokenPair` and `ExpKeywordPair` are now arguments to
the `check`/`eat` methods. This is for maximum speed. The elements in
the pairs are always statically known; e.g. a
`token::BinOp(token::Star)` is always paired with a `TokenType::Star`.
So we now compute `TokenType`s in advance and pass them in to
`check`/`eat` rather than the current approach of constructing them on
insertion into `expected_token_types`.

Values of these pair types can be produced by the new `exp!` macro,
which is used at every `check`/`eat` call site. The macro is for
convenience, allowing any pair to be generated from a single identifier.

The ident/keyword filtering in `expected_one_of_not_found` is no longer
necessary. It was there to account for some sloppiness in
`TokenKind`/`TokenType` comparisons.

The existing `TokenType` is moved to a new file `token_type.rs`, and all
its new infrastructure is added to that file. There is more boilerplate
code than I would like, but I can't see how to make it shorter.
Currently it relies on having the right integer for every variant, and
if you add a variant you need to adjust the integers for all subsequent
variants, which is a pain.

This commit introduces a match guard formulation that takes advantage of
the enum-to-integer conversion to avoid specifying the integer for each
variant. And it does this via a macro to avoid lots of boilerplate.
@nnethercote nnethercote force-pushed the speed-up-expected_tokens branch from f2df88c to d124dcd Compare December 10, 2024 04:06
@nnethercote
Copy link
Contributor Author

I rebased again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants