Use an enum for keywords and intern them to improve parser performance #6737
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, keywords are stored in hashsets that are recreated for every Parser instance, which is quite expensive since macro expansion creates lots of them. Additionally, the parser functions that look for a keyword currently accept a string and have a runtime check to validate that they actually received a keyword. By creating an enum for the keywords and inserting them into the ident interner, we can avoid the creation of the hashsets and get static checks for the keywords. For libstd, this cuts the parse+expansion part from ~2.6s to ~1.6s.
bors
added a commit
that referenced
this pull request
May 25, 2013
Currently, keywords are stored in hashsets that are recreated for every Parser instance, which is quite expensive since macro expansion creates lots of them. Additionally, the parser functions that look for a keyword currently accept a string and have a runtime check to validate that they actually received a keyword. By creating an enum for the keywords and inserting them into the ident interner, we can avoid the creation of the hashsets and get static checks for the keywords. For libstd, this cuts the parse+expansion part from ~2.6s to ~1.6s.
flip1995
pushed a commit
to flip1995/rust
that referenced
this pull request
Sep 3, 2021
Downgrade option_if_let_else to nursery I believe that this lint's loose understanding of ownership (rust-lang#5822, rust-lang#6737) makes it unsuitable to be enabled by default in its current state, even as a pedantic lint. Additionally the lint has known problems with type inference (rust-lang#6137), though I may be willing to consider this a non-blocker in isolation if it weren't for the ownership false positives. A fourth false positive involving const fn: rust-lang#7567. But on top of these, for me the biggest issue is I basically fully agree with rust-lang/rust-clippy#6137 (comment). In my experience this lint universally makes code worse even when the resulting code does compile. --- changelog: remove [`option_if_let_else`] from default set of enabled lints
flip1995
pushed a commit
to flip1995/rust
that referenced
this pull request
Sep 3, 2021
Fix `option_if_let_else` fixes: rust-lang#5822 fixes: rust-lang#6737 fixes: rust-lang#7567 The inference from rust-lang#6137 still exists so I'm not sure if this should be moved from the nursery. Before doing that though I'd almost want to see this split into two lints. One suggesting `map_or` and the other suggesting `map_or_else`. `map_or_else` tends to have longer expressions for both branches so it doesn't end up much shorter than a match expression in practice. It also seems most people find it harder to read. `map_or` at least has the terseness benefit of being on one line most of the time, especially when the `None` branch is just a literal or path expression. changelog: `break` and `continue` statments local to the would-be closure are allowed in `option_if_let_else` changelog: don't lint in const contexts in `option_if_let_else` changelog: don't lint when yield expressions are used in `option_if_let_else` changelog: don't lint when the captures made by the would-be closure conflict with the other branch in `option_if_let_else` changelog: don't lint when a field of a local is used when the type could be pontentially moved from in `option_if_let_else` changelog: in some cases, don't lint when scrutinee expression conflicts with the captures of the would-be closure in `option_if_let_else`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, keywords are stored in hashsets that are recreated for every
Parser instance, which is quite expensive since macro expansion creates
lots of them. Additionally, the parser functions that look for a keyword
currently accept a string and have a runtime check to validate that they
actually received a keyword.
By creating an enum for the keywords and inserting them into the
ident interner, we can avoid the creation of the hashsets and get static
checks for the keywords.
For libstd, this cuts the parse+expansion part from ~2.6s to ~1.6s.