rustdoc: Box GenericArgs::Parenthesized.output
#88522
Conversation
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
Box `GenericArgs::Parenthesized.output` Split out from rust-lang#88379. This reduces the size of `GenericArgs` from 104 bytes to 56 bytes, essentially reducing it by half. `GenericArgs` is one of the fields of `PathSegment`, so this should reduce the amount of memory allocated for `PathSegment`s in the cases where the generics are not for a `Fn`, `FnMut`, or `FnOnce` trait. r? `@jyn514`
|
|
|
Queued 51d7bb9 with parent 6f388bb, future comparison URL. |
|
Finished benchmarking try commit (51d7bb9): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. @bors rollup=never |
|
max-rss looks to have improved by between 0.8% and 1.6% for several benchmarks, with no significant regressions. instruction count is unchanged. |
|
The |
| output: if output != Type::Tuple(Vec::new()) { Some(output) } else { None }, | ||
| } | ||
| let output = | ||
| if output != Type::Tuple(Vec::new()) { Some(Box::new(output)) } else { None }; |
jyn514
Sep 1, 2021
Member
Personally I would use box output here 🤷 but it won't affect performance in practice, output is already on the stack.
Oh, this is the bit that breaks #83718 (comment) ! I've been looking for it.
Personally I would use box output here output is already on the stack.
Oh, this is the bit that breaks #83718 (comment) ! I've been looking for it.
camelid
Sep 1, 2021
Author
Member
Personally I would use box output here 🤷
Yeah, I thought about that, but someone recently removed all the uses of box_syntax from rustdoc and other tools; I think the goal is to get rid of box_syntax eventually. IIRC from the discussion there, the newest version of LLVM has an optimization that gets rid of the perf difference, but I may be misremembering.
Personally I would use
box outputhere🤷
Yeah, I thought about that, but someone recently removed all the uses of box_syntax from rustdoc and other tools; I think the goal is to get rid of box_syntax eventually. IIRC from the discussion there, the newest version of LLVM has an optimization that gets rid of the perf difference, but I may be misremembering.
camelid
Sep 1, 2021
Author
Member
Oh, this is the bit that breaks #83718 (comment) ! I've been looking for it.
Hooray! 🎉
Oh, this is the bit that breaks #83718 (comment) ! I've been looking for it.
Hooray!
jyn514
changed the title
GenericArgs::Parenthesized.outputGenericArgs::Parenthesized.output
|
r=me unless you want to try boxing GenericArgs |
| #[derive(Clone, PartialEq, Eq, Debug, Hash)] | ||
| crate struct PathSegment { | ||
| crate name: Symbol, | ||
| crate args: GenericArgs, | ||
| } | ||
|
|
||
| // `PathSegment` occurs multiple times in every `Path`, so its size can | ||
| // significantly affect rustdoc's memory usage. | ||
| rustc_data_structures::static_assert_size!(PathSegment, 64); | ||
|
|
jyn514
Sep 1, 2021
Member
Hmm, this is a good point - I wonder whether it hurts or helps to box GenericArgs instead of output? Are you interested in making another perf run with that change? (no problem if not, this is still a good perf improvement as-is)
Hmm, this is a good point - I wonder whether it hurts or helps to box GenericArgs instead of output? Are you interested in making another perf run with that change? (no problem if not, this is still a good perf improvement as-is)
camelid
Sep 1, 2021
•
Author
Member
Huh, I just saw your comment now; I wonder if there was a glitch in GitHub.
The thing is that GenericArgs is constructed for every PathSegment, so I think rustdoc would still be allocating the same amount of memory as without the Box (in fact, a bit more, because of the usize for the Box). Whereas with the change I made, the Box is only constructed if (a) the GenericArgs is for Fn sugar and (b) the Fn sugar has an output type.
However, making PathSegment.args an Option<Box<GenericArgs>> that would be None if there were no GenericArgs (the common case) could give us a nice perf win. One problem with that is that we would then have multiple ways to represent no GenericArgs.
Huh, I just saw your comment now; I wonder if there was a glitch in GitHub.
The thing is that GenericArgs is constructed for every PathSegment, so I think rustdoc would still be allocating the same amount of memory as without the Box (in fact, a bit more, because of the usize for the Box). Whereas with the change I made, the Box is only constructed if (a) the GenericArgs is for Fn sugar and (b) the Fn sugar has an output type.
However, making PathSegment.args an Option<Box<GenericArgs>> that would be None if there were no GenericArgs (the common case) could give us a nice perf win. One problem with that is that we would then have multiple ways to represent no GenericArgs.
camelid
Sep 1, 2021
•
Author
Member
Actually, writing up that comment just gave me an idea for another way to improve perf :D
Here it is: #88574
Actually, writing up that comment just gave me an idea for another way to improve perf :D
Here it is: #88574
This comment has been hidden.
This comment has been hidden.
This reduces the size of `GenericArgs` from 104 bytes to 56 bytes, essentially reducing it by half. `GenericArgs` is one of the fields of `PathSegment`, so this should reduce the amount of memory allocated for `PathSegment`s in the cases where the generics are not for a `Fn`, `FnMut`, or `FnOnce` trait. I also added `static_assert_size!`s to `GenericArgs` and `PathSegment` to ensure they don't increase in size unexpectedly.
|
@bors r+ |
|
|
|
|
rustdoc: Box `GenericArg::Const` to reduce enum size This is blocked on rust-lang#88522 and should be rebased over it once merged before perf is run. r? `@ghost`
Split out from #88379.
This reduces the size of
GenericArgsfrom 104 bytes to 56 bytes,essentially reducing it by half.
GenericArgsis one of the fields ofPathSegment, so this shouldreduce the amount of memory allocated for
PathSegments in the caseswhere the generics are not for a
Fn,FnMut, orFnOncetrait.r? @jyn514