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.

Sign up for GitHub

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

Jump to bottom

Remove remaining uses of box syntax from librustdoc #99577

Merged
merged 6 commits into from
Jul 29, 2022
Merged

Remove remaining uses of box syntax from librustdoc #99577

merged 6 commits into from
Jul 29, 2022

Conversation

est31
Copy link
Member

@est31 est31 commented Jul 21, 2022 •
edited
Loading

Remove the remaining uses of box syntax from librustdoc. Followup of #99066 where these changes were split out because they were responsible for a small but noticeable regression. This PR avoids the regression by boxing some large variants of ItemKind to reduce the enum's size by half from 224 bytes to 112 bytes (on x86-64). This should also help with reducing memory usage.

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jul 21, 2022
@rust-highfive
Copy link
Collaborator

r? @jsha

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 21, 2022
@est31
Copy link
Member Author

est31 commented Jul 21, 2022

@compiler-errors or someone else, can this get a perf run? Thanks!

@compiler-errors
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 21, 2022
@bors
Copy link
Contributor

bors commented Jul 21, 2022

⌛ Trying commit 0d2249c6cda35aba6cb90bd71ab5e3d46a4c4c42 with merge e4d4199ad31d4d6bdac9a4468e32c2738164fdb0...

@est31 est31 force-pushed the remove_box_librustdoc branch from 0d2249c to 8b057c8 Compare July 21, 2022 22:17
@est31
Copy link
Member Author

est31 commented Jul 22, 2022

@bors try

@bors
Copy link
Contributor

bors commented Jul 22, 2022

⌛ Trying commit 8b057c808bcb640077f948f3bd4773ce6e04f51c with merge 5290cb7cfb03f6f9a07b09bdac69135e2aaa4a05...

@bors
Copy link
Contributor

bors commented Jul 22, 2022

☀️ Try build successful - checks-actions
Build commit: 5290cb7cfb03f6f9a07b09bdac69135e2aaa4a05 (5290cb7cfb03f6f9a07b09bdac69135e2aaa4a05)

@rust-timer
Copy link
Collaborator

Queued 5290cb7cfb03f6f9a07b09bdac69135e2aaa4a05 with parent aa01891, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5290cb7cfb03f6f9a07b09bdac69135e2aaa4a05): comparison url.

Instruction count

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 -0.3% -0.4% 2
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) -0.3% -0.4% 2

Max RSS (memory usage)

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)		 1.6% 1.6% 1
Regressions 😿
(secondary)		 2.7% 2.7% 1
Improvements 🎉
(primary)		 -3.4% -3.4% 1
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) -0.9% -3.4% 2

Cycles

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)		 3.5% 3.9% 2
Regressions 😿
(secondary)		 5.3% 5.3% 1
Improvements 🎉
(primary)		 -2.2% -2.2% 1
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 1.6% 3.9% 3

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. 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-review -S-waiting-on-perf -perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 22, 2022
@est31
Copy link
Member Author

est31 commented Jul 22, 2022 •
edited
Loading

I'm only looking at the performance changes related to doc builds as this PR only changes librustdoc. There are up to 0.43% improvements in instruction count and up to 2.71% regression in max-rss (memory usage). The latter is weird because it's supposed to reduce memory overhead. Of course the extra indirection and extra number of allocations do have a footprint that needs to be earned back by the halving of the enum size, aka if sufficiently large percentage of uses of the enum were bigger variants, then obviously more memory is going to be used due to the indirection. But I don't think that this case is ever hit, as "sufficiently large" requires almost all uses of it to be, because a pointer and an extra allocation don't take up that much extra space. It might just be noise, idk.

What's really important is that the characteristic 0.4% regression that originally plagued #99066 and its preceeding attempts is gone (picture from this perf run):

Screenshot_20220722_163018

Now it's like:

Screenshot_20220722_163053

Which is way nicer :).

I think this means that the performance issues of the box removal are resolved. I'll soon make the PR ready for merging and will ask for a review.

@est31 est31 force-pushed the remove_box_librustdoc branch 2 times, most recently from bc93d59 to 7b8aa7f Compare July 22, 2022 18:05
@est31 est31 marked this pull request as ready for review July 22, 2022 18:05
@rustbot
Copy link
Collaborator

rustbot commented Jul 22, 2022

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

@est31
Copy link
Member Author

est31 commented Jul 22, 2022

r? @jsha it's ready for review now.

@bors
Copy link
Contributor

bors commented Jul 24, 2022

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

@jsha
Copy link
Contributor

jsha commented Jul 24, 2022

Thanks! Can you resolve the merge conflicts?

@est31 est31 force-pushed the remove_box_librustdoc branch from 7b8aa7f to 46311b7 Compare July 24, 2022 20:00
@est31
Copy link
Member Author

est31 commented Jul 24, 2022

@jsha rebased.

@bors
Copy link
Contributor

bors commented Jul 29, 2022

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

@est31
Remove box syntax from Box<rustdoc::clean::types::ItemKind> construction
e62f6a0 
The type has 240 bytes according to compiler internal rustdoc.
@est31 est31 force-pushed the remove_box_librustdoc branch from 46311b7 to fabb4b0 Compare July 29, 2022 17:30
@est31
Copy link
Member Author

est31 commented Jul 29, 2022

@jsha rebased again.

@jsha
Copy link
Contributor

jsha commented Jul 29, 2022

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Jul 29, 2022

📌 Commit fabb4b0 has been approved by jsha

It is now in the queue for this repository.

@bors 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 Jul 29, 2022
@bors
Copy link
Contributor

bors commented Jul 29, 2022

⌛ Testing commit fabb4b0 with merge 3924dac...

@bors
Copy link
Contributor

bors commented Jul 29, 2022

☀️ Test successful - checks-actions
Approved by: jsha
Pushing 3924dac to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 29, 2022
@bors bors merged commit 3924dac into rust-lang:master Jul 29, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 29, 2022
This was referenced Jul 29, 2022
Closed
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3924dac): comparison url.

Instruction count

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 0.3% 0.3% 4
Improvements 🎉
(primary)		 -0.3% -0.3% 2
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) -0.3% -0.3% 2

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)		 1.6% 1.7% 3
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -2.6% -2.6% 1
All 😿🎉 (primary) 1.6% 1.7% 3

Cycles

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 3.0% 3.0% 1
Regressions 😿
(secondary)		 2.5% 2.5% 2
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -4.2% -4.2% 1
All 😿🎉 (primary) 3.0% 3.0% 1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@est31 est31 mentioned this pull request Feb 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jsha jsha

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.64.0
Development

Successfully merging this pull request may close these issues.
7 participants
@est31 @rust-highfive @compiler-errors @rust-timer @bors @rustbot @jsha