Skip to content

Remove more attributes from metadata - #98450

Merged
bors merged 5 commits into
rust-lang:masterfrom
lqd:doc-metadata
Oct 21, 2022
Merged

Remove more attributes from metadata#98450
bors merged 5 commits into
rust-lang:masterfrom
lqd:doc-metadata

Conversation

@lqd

@lqdlqd commented Jun 24, 2022

Copy link
Copy Markdown
Member

A lot of the attributes that are currently stored in the metadata aren't used at all. The biggest metadata usage comes from the doc attributes currently but they are needed by rustdoc so we only removed the ones that cannot be used in downstream crates (doc comments on private items).

r? @ghost

@rustbotrustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 24, 2022
@lqd

lqd commented Jun 24, 2022

Copy link
Copy Markdown
MemberAuthor

@bors try @rust-timer queue

@rust-timer

Copy link
Copy Markdown
Collaborator

Awaiting bors try build completion.

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

@rustbotrustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 24, 2022
@bors

bors commented Jun 24, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 27b259ccc15c4aba9396c486a2980575d1b8e120 with merge be1cdcd82f77e6d9756195ddabd239691e85bfc1...

@bors

bors commented Jun 24, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

Copy link
Copy Markdown
Collaborator

Queued be1cdcd82f77e6d9756195ddabd239691e85bfc1 with parent d017d59, future comparison URL.

@jyn514

Copy link
Copy Markdown
Member

Unsure of what this would break in rustdoc

Maybe just inlining? Inlining is where rustdoc takes items that are re-exported from one crate to another and shows the full documentation in the new crate (rather than a pub use xxx; item, which is the default across crates).

Unfortunately there's no way to predict that ahead of time because the inline annotation is in the downstream crate, not the dependency ...

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (be1cdcd82f77e6d9756195ddabd239691e85bfc1): comparison url.

Instruction count

  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1maxcount2
Regressions 😿
(primary)
N/AN/A0
Regressions 😿
(secondary)
N/AN/A0
Improvements 🎉
(primary)
-3.9%-66.3%64
Improvements 🎉
(secondary)
-7.5%-19.5%39
All 😿🎉 (primary)-3.9%-66.3%64

Max RSS (memory usage)

Results
  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1maxcount2
Regressions 😿
(primary)
N/AN/A0
Regressions 😿
(secondary)
N/AN/A0
Improvements 🎉
(primary)
-3.7%-14.1%140
Improvements 🎉
(secondary)
-5.0%-14.9%108
All 😿🎉 (primary)-3.7%-14.1%140

Cycles

Results
  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1maxcount2
Regressions 😿
(primary)
2.7%2.7%1
Regressions 😿
(secondary)
N/AN/A0
Improvements 🎉
(primary)
-12.4%-52.3%18
Improvements 🎉
(secondary)
-14.7%-20.9%22
All 😿🎉 (primary)-11.6%-52.3%19

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 change23

  2. number of relevant changes23

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jun 24, 2022
@lqd

lqd commented Jun 24, 2022

Copy link
Copy Markdown
MemberAuthor

Maybe just inlining? Inlining is where rustdoc takes items that are re-exported from one crate to another and shows the full documentation in the new crate (rather than a pub use xxx; item, which is the default across crates).

Yes, it seems some of the rustdoc inline-cross and intra-doc across crates tests fail indeed

@lqd

lqd commented Jun 24, 2022

Copy link
Copy Markdown
MemberAuthor

Ignoring the doc benchmarks which I'm not even sure are correctly handled by this PR (even though I initially stumbled upon this by seeing all of the libcore doc comments being hashed in a check build of stm32f4-0.14.0), there seems to be some small wins in the regular profiles.

@lqd

lqd commented Jun 24, 2022

Copy link
Copy Markdown
MemberAuthor

I was mostly interested in looking at metadata size, for libcore (-10%)

54M libcore-d017d59ed013a4bc2431d023077eb7209fe9c60d.rmeta
49M libcore-be1cdcd82f77e6d9756195ddabd239691e85bfc1.rmeta

and libstd (-15%)

8.7M libstd-d017d59ed013a4bc2431d023077eb7209fe9c60d.rmeta
7.4M libstd-be1cdcd82f77e6d9756195ddabd239691e85bfc1.rmeta

btw @jyn514 can I reproduce the rustdoc benchmarks simply by running cargo doc (optionally with flags defined in perf-config.json) ?

(if that's the case, then the only difference in the stm32f4-0.14.0 generated docs is in the search-index, which is 8% smaller. It's not straightforward to diff, it's very big for that benchmark, 3.6MB)

@camelid

Copy link
Copy Markdown
Member

(if that's the case, then the only difference in the stm32f4-0.14.0 generated docs is in the search-index, which is 8% smaller. It's not straightforward to diff, it's very big for that benchmark, 3.6MB)

Perhaps the search-index is shrinking because rustdoc sees that there are no docs for the external items and is deciding not to inline them? Otherwise I can't think of why the search-index would shrink with this change.

@Mark-SimulacrumMark-Simulacrum 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 Jul 24, 2022
@lqd

lqd commented Sep 5, 2022

Copy link
Copy Markdown
MemberAuthor

@bors try @rust-timer queue

@rust-timer

Copy link
Copy Markdown
Collaborator

Awaiting bors try build completion.

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

@rustbotrustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 5, 2022
@bors

bors commented Sep 5, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 8da1bc8c2db72836f75891e8de593e03aa26fdfa with merge ec24c7b6173ddb76e1308214ea2b4bd1520ab4fb...

@GuillaumeGomez

Copy link
Copy Markdown
Member

Here are the diff when we only remove unreachable items' documentation:

rmeta libBeforeAfterDiff
libaddr2line-92bc8313d5711253.rlib4972884964880.2%
libadler-ce8b9a1966280505.rlib64734647340%
liballoc-5d73a6291bdcf84d.rlib718681471623740.4%
libcfg_if-6ce0c15b4b6b1a04.rlib995899580%
libcompiler_builtins-d130db2e7ffa5e06.rlib158880815770960.7%
libcore-95c594ea0242ac01.rlib61091692609987800.2%
libgetopts-9b299cdaedbd7503.rlib6892626880540.2%
libgimli-eec949a0a15fca68.rlib714221671354400.1%
libhashbrown-f6d925553742e1b3.rlib160620016033200.2%
liblibc-6cf37d22eaa1f096.rlib326461632636240.3%
libLLVM-15-rust-1.65.0-nightly.so1096975281096975280%
libmemchr-a8101b21344f81eb.rlib143146813893163%
libminiz_oxide-44bf32af694d4f3a.rlib10016309922541%
libobject-af2efcb345913075.rlib863097686304080%
libpanic_abort-6da9002fe89b2e2a.rlib11180111800%
libpanic_unwind-c2d7eb851cafcedd.rlib39498371865.9%
libproc_macro-4b89ad880cf901ef.rlib419949641890400.3%
librustc_demangle-e1681d60af5e6ee1.rlib5919665867420.9%
librustc_std_workspace_alloc-091abb8a2f3ef39a.rlib518051800%
librustc_std_workspace_core-6f37b8376277b571.rlib672267220%
librustc_std_workspace_std-879df989f3ba3bc5.rlib838483840%
libstd-b6dbfa4842ad1467.rlib15548624153240241.5%
libstd-b6dbfa4842ad1467.so*720564070717842%
libstd_detect-4062402ee65c0247.rlib5077605041680.7%
libtest-c7b87c602ca2d4e9.rlib484081648352800.1%
libtest-c7b87c602ca2d4e9.so*149191214889840.2%
libunicode_width-b08619fcd1265a63.rlib1628441628440%
libunwind-58a66e84df914c1c.rlib46256462560%

An idea we discussed with @lqd was to split the doc comments into their own metadata file (so we would have .rlib and .rdoc or something like that) which would allow to greatly benefit "normal" compilation. To be discussed I guess.

@bors

bors commented Sep 5, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

Copy link
Copy Markdown
Collaborator

Queued ec24c7b6173ddb76e1308214ea2b4bd1520ab4fb with parent 6e4a9ab, future comparison URL.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (ec24c7b6173ddb76e1308214ea2b4bd1520ab4fb): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1rangecount2
Regressions ❌
(primary)
0.5%[0.2%, 1.1%]48
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.5%[0.2%, 1.1%]48

Max RSS (memory usage)

Results

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.

mean1rangecount2
Regressions ❌
(primary)
1.2%[0.5%, 1.8%]6
Regressions ❌
(secondary)
4.6%[2.4%, 8.4%]3
Improvements ✅
(primary)
-1.3%[-1.3%, -1.3%]1
Improvements ✅
(secondary)
--0
All ❌✅ (primary)0.8%[-1.3%, 1.8%]7

Cycles

Results

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.

mean1rangecount2
Regressions ❌
(primary)
2.9%[2.9%, 2.9%]1
Regressions ❌
(secondary)
3.2%[3.2%, 3.2%]1
Improvements ✅
(primary)
--0
Improvements ✅
(secondary)
-2.7%[-2.7%, -2.7%]1
All ❌✅ (primary)2.9%[2.9%, 2.9%]1

Footnotes

  1. the arithmetic mean of the percent change23

  2. number of relevant changes23

@rustbotrustbot added perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 6, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez

Copy link
Copy Markdown
Member

When keeping link_name, results are less good, but it's an improvement:

namebefore PRwithout link namewith link nametotal diff
std.so7205640686763269433763.7%
std.rlib1554862414885520152513062%
core6109169258468300598101902.1%

@bors try @rust-timer queue

@rust-timer

Copy link
Copy Markdown
Collaborator

Awaiting bors try build completion.

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

@bors

bors commented Oct 11, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 2ffbff52a7ecb62e1c9194b80ccda75858b65eb8 with merge d2f9957aff987901ad233b79296112545de1c6f8...

@bors

bors commented Oct 11, 2022

Copy link
Copy Markdown
Collaborator

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

@rust-timer

Copy link
Copy Markdown
Collaborator

Queued d2f9957aff987901ad233b79296112545de1c6f8 with parent cde693c, future comparison URL.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (d2f9957aff987901ad233b79296112545de1c6f8): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1rangecount2
Regressions ❌
(primary)
--0
Regressions ❌
(secondary)
2.2%[1.1%, 3.3%]2
Improvements ✅
(primary)
-2.4%[-8.7%, -0.6%]23
Improvements ✅
(secondary)
-5.7%[-8.6%, -0.7%]23
All ❌✅ (primary)-2.4%[-8.7%, -0.6%]23

Max RSS (memory usage)

Results

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.

mean1rangecount2
Regressions ❌
(primary)
--0
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
-3.8%[-6.5%, -0.8%]33
Improvements ✅
(secondary)
-3.5%[-8.5%, -0.7%]89
All ❌✅ (primary)-3.8%[-6.5%, -0.8%]33

Cycles

Results

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.

mean1rangecount2
Regressions ❌
(primary)
2.4%[2.4%, 2.4%]1
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
-4.0%[-8.5%, -2.0%]10
Improvements ✅
(secondary)
-6.0%[-8.0%, -2.9%]20
All ❌✅ (primary)-3.4%[-8.5%, 2.4%]11

Footnotes

  1. the arithmetic mean of the percent change23

  2. number of relevant changes23

@oli-obk

Copy link
Copy Markdown
Contributor

The previous version of the PR did break some miri tests, but they seemed to pass locally, and failed on builders after r+ -- so it's hard to reproduce.

That's odd, how did you run those tests?

@lqd

lqd commented Oct 20, 2022

Copy link
Copy Markdown
MemberAuthor

Locally benchmarking the 2 incremental regressions in the last perf run didn't yield much of interest: they don't seem to be regressions to cachegrind.

Rebased, so let's re-run perf again to see if it could have been noise.

@bors try @rust-timer queue

@rust-timer

Copy link
Copy Markdown
Collaborator

Awaiting bors try build completion.

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

@bors

bors commented Oct 20, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 41263d2 with merge 9f7d8a6c2c3311e9e7231f533709680c98854e8b...

@bors

bors commented Oct 21, 2022

Copy link
Copy Markdown
Collaborator

☀️ Try build successful - checks-actions
Build commit: 9f7d8a6c2c3311e9e7231f533709680c98854e8b (9f7d8a6c2c3311e9e7231f533709680c98854e8b)

@rust-timer

Copy link
Copy Markdown
Collaborator

Queued 9f7d8a6c2c3311e9e7231f533709680c98854e8b with parent 5ffa67d, future comparison URL.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (9f7d8a6c2c3311e9e7231f533709680c98854e8b): comparison URL.

Overall result: ✅ improvements - no action needed

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1rangecount2
Regressions ❌
(primary)
--0
Regressions ❌
(secondary)
0.7%[0.4%, 0.9%]2
Improvements ✅
(primary)
-2.7%[-8.2%, -0.2%]18
Improvements ✅
(secondary)
-5.5%[-8.3%, -0.8%]23
All ❌✅ (primary)-2.7%[-8.2%, -0.2%]18

Max RSS (memory usage)

Results

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.

mean1rangecount2
Regressions ❌
(primary)
--0
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
-2.4%[-6.4%, -0.7%]58
Improvements ✅
(secondary)
-5.5%[-6.8%, -1.5%]24
All ❌✅ (primary)-2.4%[-6.4%, -0.7%]58

Cycles

Results

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.

mean1rangecount2
Regressions ❌
(primary)
--0
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
-3.3%[-6.7%, -2.0%]9
Improvements ✅
(secondary)
-5.3%[-7.0%, -2.9%]18
All ❌✅ (primary)-3.3%[-6.7%, -2.0%]9

Footnotes

  1. the arithmetic mean of the percent change23

  2. number of relevant changes23

@GuillaumeGomez

Copy link
Copy Markdown
Member

After discussion with @lqd, seems like we're ready here. We will very likely follow-up this up with a RFC about storing attributes in another separate file to reduce this even further.

@bors: r=lqd,GuillaumeGomez

@bors

bors commented Oct 21, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit 41263d2 has been approved by lqd,GuillaumeGomez

It is now in the queue for this repository.

@bors

bors commented Oct 21, 2022

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 41263d2 with merge ba9d01b...

@bors

bors commented Oct 21, 2022

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: lqd,GuillaumeGomez
Pushing ba9d01b to master...

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (ba9d01b): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1rangecount2
Regressions ❌
(primary)
--0
Regressions ❌
(secondary)
0.3%[0.3%, 0.3%]1
Improvements ✅
(primary)
-2.7%[-8.2%, -0.2%]18
Improvements ✅
(secondary)
-5.5%[-8.4%, -0.8%]23
All ❌✅ (primary)-2.7%[-8.2%, -0.2%]18

Max RSS (memory usage)

Results

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.

mean1rangecount2
Regressions ❌
(primary)
--0
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
-3.5%[-7.0%, -0.6%]30
Improvements ✅
(secondary)
-5.9%[-7.3%, -2.5%]23
All ❌✅ (primary)-3.5%[-7.0%, -0.6%]30

Cycles

Results

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.

mean1rangecount2
Regressions ❌
(primary)
--0
Regressions ❌
(secondary)
--0
Improvements ✅
(primary)
-3.6%[-7.2%, -2.0%]11
Improvements ✅
(secondary)
-5.8%[-7.5%, -3.2%]19
All ❌✅ (primary)-3.6%[-7.2%, -2.0%]11

Footnotes

  1. the arithmetic mean of the percent change23

  2. number of relevant changes23

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-testsuiteArea: The testsuite used to check the correctness of rustcmerged-by-borsThis PR was explicitly merged by bors.perf-regression-triagedThe performance regression has been triaged.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

16 participants

@lqd@rust-timer@bors@jyn514@camelid@GuillaumeGomez@notriddle@craterbot@bjorn3@rust-log-analyzer@emilyalbini@rustbot@oli-obk@RalfJung@cjgillot@Mark-Simulacrum