-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Simplify LazyAttrTokenStream
#127516
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
Simplify LazyAttrTokenStream
#127516
Conversation
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
…ream, r=<try> Simplify `LazyAttrTokenStream` `LazyAttrTokenStream` is an unpleasant type: `Lrc<Box<dyn ToAttrTokenStream>>`. Why does it look like that? - There are two `ToAttrTokenStream` impls, one for the lazy case, and one for the case where we already have an `AttrTokenStream`. - The lazy case (`LazyAttrTokenStreamImpl`) is implemented in `rustc_parse`, but `LazyAttrTokenStream` is defined in `rustc_ast`, which does not depend on `rustc_parse`. The use of the trait lets `rustc_ast` implicitly depend on `rustc_parse`. This explains the `dyn`. - `LazyAttrTokenStream` must have a `size_of` as small as possible, because it's used in many AST nodes. This explains the `Lrc<Box<_>>`, which keeps it to one word. (It's required `Lrc<dyn _>` would be a fat pointer.) This PR moves `LazyAttrTokenStreamImpl` (and a few other token stream things) from `rustc_parse` to `rustc_ast`. This lets us replace the `ToAttrTokenStream` trait with a two-variant enum and also remove the `Box`, changing `LazyAttrTokenStream` to `Lrc<LazyAttrTokenStreamInner>`. r? `@petrochenkov`
I tried a simpler approach in #127478: just removing |
This comment has been minimized.
This comment has been minimized.
ad11b0b
to
54c1d40
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
…ream, r=<try> Simplify `LazyAttrTokenStream` `LazyAttrTokenStream` is an unpleasant type: `Lrc<Box<dyn ToAttrTokenStream>>`. Why does it look like that? - There are two `ToAttrTokenStream` impls, one for the lazy case, and one for the case where we already have an `AttrTokenStream`. - The lazy case (`LazyAttrTokenStreamImpl`) is implemented in `rustc_parse`, but `LazyAttrTokenStream` is defined in `rustc_ast`, which does not depend on `rustc_parse`. The use of the trait lets `rustc_ast` implicitly depend on `rustc_parse`. This explains the `dyn`. - `LazyAttrTokenStream` must have a `size_of` as small as possible, because it's used in many AST nodes. This explains the `Lrc<Box<_>>`, which keeps it to one word. (It's required `Lrc<dyn _>` would be a fat pointer.) This PR moves `LazyAttrTokenStreamImpl` (and a few other token stream things) from `rustc_parse` to `rustc_ast`. This lets us replace the `ToAttrTokenStream` trait with a two-variant enum and also remove the `Box`, changing `LazyAttrTokenStream` to `Lrc<LazyAttrTokenStreamInner>`. Plus it does a few cleanups. r? `@petrochenkov`
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (c0bb39c): comparison URL. Overall result: ❌✅ regressions and improvements - no action neededBenchmarking 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 Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 1.4%, secondary 0.1%)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.
CyclesResults (primary 3.7%)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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 703.117s -> 703.335s (0.03%) |
I'm not sure yet if I want this to be merged. I've marked this as a draft for now. |
What are you concerns? Some parts of the moved code definitely feel like a parser logic - the last token breaking, or the flat token cursor (to a least extent), but there's not too much of it. |
Those are the concerns, pretty much. |
54c1d40
to
c71202b
Compare
…ream, r=<try> Simplify `LazyAttrTokenStream` `LazyAttrTokenStream` is an unpleasant type: `Lrc<Box<dyn ToAttrTokenStream>>`. Why does it look like that? - There are two `ToAttrTokenStream` impls, one for the lazy case, and one for the case where we already have an `AttrTokenStream`. - The lazy case (`LazyAttrTokenStreamImpl`) is implemented in `rustc_parse`, but `LazyAttrTokenStream` is defined in `rustc_ast`, which does not depend on `rustc_parse`. The use of the trait lets `rustc_ast` implicitly depend on `rustc_parse`. This explains the `dyn`. - `LazyAttrTokenStream` must have a `size_of` as small as possible, because it's used in many AST nodes. This explains the `Lrc<Box<_>>`, which keeps it to one word. (It's required `Lrc<dyn _>` would be a fat pointer.) This PR moves `LazyAttrTokenStreamImpl` (and a few other token stream things) from `rustc_parse` to `rustc_ast`. This lets us replace the `ToAttrTokenStream` trait with a two-variant enum and also remove the `Box`, changing `LazyAttrTokenStream` to `Lrc<LazyAttrTokenStreamInner>`. Plus it does a few cleanups. r? `@petrochenkov`
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (f0cba53): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis 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.
Max RSS (memory usage)Results (primary 0.2%, secondary -0.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.
CyclesResults (primary 0.8%, secondary -2.7%)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.
Binary sizeResults (primary 1.1%)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.
Bootstrap: 764.09s -> 765.668s (0.21%) |
r=me with #127516 (comment) addressed. |
Reminder, once the PR becomes ready for a review, use |
This commit does the following. - Changes it from `Lrc<Box<dyn ToAttrTokenStream>>` to `Lrc<LazyAttrTokenStreamInner>`. - Reworks `LazyAttrTokenStreamImpl` as `LazyAttrTokenStreamInner`, which is a two-variant enum. - Removes the `ToAttrTokenStream` trait and the two impls of it. The recursion limit must be increased in some crates otherwise rustdoc aborts.
0f03af0
to
880e6f7
Compare
I removed @bors r=petrochenkov |
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 0fbb922 (parent) -> f242d6c (this PR) Test differencesShow 8 test diffs8 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard f242d6c26cc6fc187257bd1be9590b4b39632425 --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (f242d6c): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countThis 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.
Max RSS (memory usage)Results (primary -0.3%, secondary -1.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.
CyclesResults (primary -0.2%, secondary 2.3%)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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 769.061s -> 770.064s (0.13%) |
Improvements outweigh regressions, and all the movement is in secondary benchmarks. @rustbot label: +perf-regression-triaged |
LazyAttrTokenStream
is an unpleasant type:Lrc<Box<dyn ToAttrTokenStream>>
. Why does it look like that?ToAttrTokenStream
impls, one for the lazy case, and one for the case where we already have anAttrTokenStream
.LazyAttrTokenStreamImpl
) is implemented inrustc_parse
, butLazyAttrTokenStream
is defined inrustc_ast
, which does not depend onrustc_parse
. The use of the trait letsrustc_ast
implicitly depend onrustc_parse
. This explains thedyn
.LazyAttrTokenStream
must have asize_of
as small as possible, because it's used in many AST nodes. This explains theLrc<Box<_>>
, which keeps it to one word. (It's requiredLrc<dyn _>
would be a fat pointer.)This PR moves
LazyAttrTokenStreamImpl
(and a few other token stream things) fromrustc_parse
torustc_ast
. This lets us replace theToAttrTokenStream
trait with a two-variant enum and also remove theBox
, changingLazyAttrTokenStream
toLrc<LazyAttrTokenStreamInner>
. Plus it does a few cleanups.r? @petrochenkov