rustdoc: Box `GenericArgs::Parenthesized.output` #88522

camelid · 2021-08-31T01:43:06Z

Split out from #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 PathSegments in the cases
where the generics are not for a Fn, FnMut, or FnOnce trait.

r? @jyn514

camelid · 2021-08-31T01:58:03Z

@bors try @rust-timer queue

rust-timer · 2021-08-31T01:58:05Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-08-31T01:58:11Z

⌛ Trying commit 074d79f0e66ed08d6bf514523314a6e27f6a48f0 with merge 51d7bb9752da9a4aa758f2dab28c1b56245687ed...

bors · 2021-08-31T03:19:12Z

☀️ Try build successful - checks-actions
Build commit: 51d7bb9752da9a4aa758f2dab28c1b56245687ed (51d7bb9752da9a4aa758f2dab28c1b56245687ed)

rust-timer · 2021-08-31T03:19:14Z

Queued 51d7bb9752da9a4aa758f2dab28c1b56245687ed with parent 6f388bb, future comparison URL.

src/librustdoc/clean/auto_trait.rs

rust-timer · 2021-08-31T05:31:38Z

Finished benchmarking try commit (51d7bb9752da9a4aa758f2dab28c1b56245687ed): 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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

camelid · 2021-08-31T23:58:19Z

max-rss looks to have improved by between 0.8% and 1.6% for several benchmarks, with no significant regressions. instruction count is unchanged.

jyn514

The rustc_assert_size is the only change I feel strongly about, the rest you can choose to add or not. Good catch! I wouldn't have found this :)

src/librustdoc/clean/auto_trait.rs

jyn514 · 2021-09-01T00:01:24Z

src/librustdoc/clean/mod.rs

-                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 };


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 🤷

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.

Oh, this is the bit that breaks #83718 (comment) ! I've been looking for it.

Hooray! 🎉

src/librustdoc/json/conversions.rs

src/librustdoc/clean/types.rs

jyn514

r=me unless you want to try boxing GenericArgs

jyn514 · 2021-09-01T02:33:29Z

src/librustdoc/clean/types.rs

 #[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);
+


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)

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.

Actually, writing up that comment just gave me an idea for another way to improve perf :D

Here it is: #88574

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.

jyn514 · 2021-09-01T21:59:55Z

@bors r+

bors · 2021-09-01T21:59:56Z

📌 Commit 280e167 has been approved by jyn514

bors · 2021-09-02T00:12:25Z

⌛ Testing commit 280e167 with merge e3c71f1...

bors · 2021-09-02T02:39:58Z

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing e3c71f1 to master...

camelid added I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. I-compiletime Issue: Problems and improvements with respect to compile times. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Aug 31, 2021

rust-highfive assigned jyn514 Aug 31, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 31, 2021

camelid force-pushed the box-paren-output branch from 5d3e08a to 074d79f Compare August 31, 2021 01:55

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 31, 2021

camelid commented Aug 31, 2021

View reviewed changes

src/librustdoc/clean/auto_trait.rs Outdated Show resolved Hide resolved

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 31, 2021

jyn514 removed the I-compiletime Issue: Problems and improvements with respect to compile times. label Aug 31, 2021

jyn514 mentioned this pull request Aug 31, 2021

Perf summaries ignore max-rss rust-lang/rustc-perf#988

Closed

jyn514 approved these changes Sep 1, 2021

View reviewed changes

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2021

jyn514 changed the title ~~Box GenericArgs::Parenthesized.output~~ rustdoc: Box GenericArgs::Parenthesized.output Sep 1, 2021

camelid force-pushed the box-paren-output branch from 074d79f to 3a0b073 Compare September 1, 2021 02:32

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 1, 2021

jyn514 approved these changes Sep 1, 2021

View reviewed changes

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2021

camelid force-pushed the box-paren-output branch from 3a0b073 to c3ceee1 Compare September 1, 2021 02:34

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 1, 2021

This comment has been minimized.

Sign in to view

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2021

camelid added 2 commits August 31, 2021 20:32

Remove unhelpful expect() message

280e167

camelid force-pushed the box-paren-output branch from c3ceee1 to 280e167 Compare September 1, 2021 03:32

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 1, 2021

camelid mentioned this pull request Sep 1, 2021

rustdoc: Cleanup various clean types #88379

Merged

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2021

camelid mentioned this pull request Sep 1, 2021

rustdoc: Box GenericArg::Const to reduce enum size #88574

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 2, 2021

bors merged commit e3c71f1 into rust-lang:master Sep 2, 2021

camelid deleted the box-paren-output branch September 2, 2021 02:40

cuviper added this to the 1.56.0 milestone Nov 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: Box `GenericArgs::Parenthesized.output` #88522

rustdoc: Box `GenericArgs::Parenthesized.output` #88522

camelid commented Aug 31, 2021 •

edited

Loading

camelid commented Aug 31, 2021

rust-timer commented Aug 31, 2021

bors commented Aug 31, 2021

bors commented Aug 31, 2021

rust-timer commented Aug 31, 2021

rust-timer commented Aug 31, 2021

camelid commented Aug 31, 2021 •

edited

Loading

jyn514 left a comment

jyn514 Sep 1, 2021

camelid Sep 1, 2021

camelid Sep 1, 2021

jyn514 left a comment

jyn514 Sep 1, 2021

camelid Sep 1, 2021 •

edited

Loading

camelid Sep 1, 2021 •

edited

Loading

This comment has been minimized.

jyn514 commented Sep 1, 2021

bors commented Sep 1, 2021

bors commented Sep 2, 2021

bors commented Sep 2, 2021

rustdoc: Box GenericArgs::Parenthesized.output #88522

rustdoc: Box GenericArgs::Parenthesized.output #88522

Conversation

camelid commented Aug 31, 2021 • edited Loading

camelid commented Aug 31, 2021

rust-timer commented Aug 31, 2021

bors commented Aug 31, 2021

bors commented Aug 31, 2021

rust-timer commented Aug 31, 2021

rust-timer commented Aug 31, 2021

camelid commented Aug 31, 2021 • edited Loading

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 Sep 1, 2021

Choose a reason for hiding this comment

camelid Sep 1, 2021

Choose a reason for hiding this comment

camelid Sep 1, 2021

Choose a reason for hiding this comment

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 Sep 1, 2021

Choose a reason for hiding this comment

camelid Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

camelid Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

jyn514 commented Sep 1, 2021

bors commented Sep 1, 2021

bors commented Sep 2, 2021

bors commented Sep 2, 2021

rustdoc: Box `GenericArgs::Parenthesized.output` #88522

rustdoc: Box `GenericArgs::Parenthesized.output` #88522

camelid commented Aug 31, 2021 •

edited

Loading

camelid commented Aug 31, 2021 •

edited

Loading

camelid Sep 1, 2021 •

edited

Loading

camelid Sep 1, 2021 •

edited

Loading