Remove more attributes from metadata by lqd · Pull Request #98450 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation134 Commits5 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Member
• Loading
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
Relevant to the compiler team, which will review and decide on the PR/issue.
label
Awaiting bors try build completion.
@rustbot label: +S-waiting-on-perf
⌛ Trying commit 27b259ccc15c4aba9396c486a2980575d1b8e120 with merge be1cdcd82f77e6d9756195ddabd239691e85bfc1...
☀️ Try build successful - checks-actions
Build commit: be1cdcd82f77e6d9756195ddabd239691e85bfc1 (be1cdcd82f77e6d9756195ddabd239691e85bfc1)
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 ...
Finished benchmarking commit (be1cdcd82f77e6d9756195ddabd239691e85bfc1): comparison url.
Instruction count
- Primary benchmarks: 🎉 relevant improvements found
- Secondary benchmarks: 🎉 relevant improvements found
| mean1 | max | count2 | |
|---|---|---|---|
| Regressions 😿 (primary) | N/A | N/A | 0 |
| Regressions 😿 (secondary) | N/A | N/A | 0 |
| 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
| mean1 | max | count2 | |
|---|---|---|---|
| Regressions 😿 (primary) | N/A | N/A | 0 |
| Regressions 😿 (secondary) | N/A | N/A | 0 |
| 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
| mean1 | max | count2 | |
|---|---|---|---|
| Regressions 😿 (primary) | 2.7% | 2.7% | 1 |
| Regressions 😿 (secondary) | N/A | N/A | 0 |
| 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
Member Author
lqd commented
• Loading
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
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.
Member Author
lqd commented
• Loading
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)
(if that's the case, then the only difference in the
stm32f4-0.14.0generated 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.
Awaiting bors try build completion.
@rustbot label: +S-waiting-on-perf
⌛ Trying commit 8da1bc8c2db72836f75891e8de593e03aa26fdfa with merge ec24c7b6173ddb76e1308214ea2b4bd1520ab4fb...
Here are the diff when we only remove unreachable items' documentation:
| rmeta lib | Before | After | Diff |
|---|---|---|---|
| libaddr2line-92bc8313d5711253.rlib | 497288 | 496488 | 0.2% |
| libadler-ce8b9a1966280505.rlib | 64734 | 64734 | 0% |
| liballoc-5d73a6291bdcf84d.rlib | 7186814 | 7162374 | 0.4% |
| libcfg_if-6ce0c15b4b6b1a04.rlib | 9958 | 9958 | 0% |
| libcompiler_builtins-d130db2e7ffa5e06.rlib | 1588808 | 1577096 | 0.7% |
| libcore-95c594ea0242ac01.rlib | 61091692 | 60998780 | 0.2% |
| libgetopts-9b299cdaedbd7503.rlib | 689262 | 688054 | 0.2% |
| libgimli-eec949a0a15fca68.rlib | 7142216 | 7135440 | 0.1% |
| libhashbrown-f6d925553742e1b3.rlib | 1606200 | 1603320 | 0.2% |
| liblibc-6cf37d22eaa1f096.rlib | 3264616 | 3263624 | 0.3% |
| libLLVM-15-rust-1.65.0-nightly.so | 109697528 | 109697528 | 0% |
| libmemchr-a8101b21344f81eb.rlib | 1431468 | 1389316 | 3% |
| libminiz_oxide-44bf32af694d4f3a.rlib | 1001630 | 992254 | 1% |
| libobject-af2efcb345913075.rlib | 8630976 | 8630408 | 0% |
| libpanic_abort-6da9002fe89b2e2a.rlib | 11180 | 11180 | 0% |
| libpanic_unwind-c2d7eb851cafcedd.rlib | 39498 | 37186 | 5.9% |
| libproc_macro-4b89ad880cf901ef.rlib | 4199496 | 4189040 | 0.3% |
| librustc_demangle-e1681d60af5e6ee1.rlib | 591966 | 586742 | 0.9% |
| librustc_std_workspace_alloc-091abb8a2f3ef39a.rlib | 5180 | 5180 | 0% |
| librustc_std_workspace_core-6f37b8376277b571.rlib | 6722 | 6722 | 0% |
| librustc_std_workspace_std-879df989f3ba3bc5.rlib | 8384 | 8384 | 0% |
| libstd-b6dbfa4842ad1467.rlib | 15548624 | 15324024 | 1.5% |
| libstd-b6dbfa4842ad1467.so* | 7205640 | 7071784 | 2% |
| libstd_detect-4062402ee65c0247.rlib | 507760 | 504168 | 0.7% |
| libtest-c7b87c602ca2d4e9.rlib | 4840816 | 4835280 | 0.1% |
| libtest-c7b87c602ca2d4e9.so* | 1491912 | 1488984 | 0.2% |
| libunicode_width-b08619fcd1265a63.rlib | 162844 | 162844 | 0% |
| libunwind-58a66e84df914c1c.rlib | 46256 | 46256 | 0% |
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.
☀️ Try build successful - checks-actions
Build commit: ec24c7b6173ddb76e1308214ea2b4bd1520ab4fb (ec24c7b6173ddb76e1308214ea2b4bd1520ab4fb)
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.
| mean1 | range | count2 | |
|---|---|---|---|
| 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.
| mean1 | range | count2 | |
|---|---|---|---|
| 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.
| mean1 | range | count2 | |
|---|---|---|---|
| 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
☀️ Try build successful - checks-actions
Build commit: 9f7d8a6c2c3311e9e7231f533709680c98854e8b (9f7d8a6c2c3311e9e7231f533709680c98854e8b)
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.
| mean1 | range | count2 | |
|---|---|---|---|
| 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.
| mean1 | range | count2 | |
|---|---|---|---|
| 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.
| mean1 | range | count2 | |
|---|---|---|---|
| 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
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
📌 Commit 41263d2 has been approved by lqd,GuillaumeGomez
It is now in the queue for this repository.
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.
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
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.
| mean1 | range | count2 | |
|---|---|---|---|
| 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.
| mean1 | range | count2 | |
|---|---|---|---|
| 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.
| mean1 | range | count2 | |
|---|---|---|---|
| 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
bors added a commit to rust-lang-ci/rust that referenced this pull request
rustc_metadata: Encode even less doc comments
The fact that def_id is in the tcx.privacy_access_levels(()) table is not very meaningful, especially after rust-lang#102026, is_exported (or is_reachable in the worst case) is what you need.
Follow up to rust-lang#98450.
r? @GuillaumeGomez @lqd
RalfJung pushed a commit to RalfJung/miri that referenced this pull request
rustc_metadata: Encode even less doc comments
The fact that def_id is in the tcx.privacy_access_levels(()) table is not very meaningful, especially after rust-lang/rust#102026, is_exported (or is_reachable in the worst case) is what you need.
Follow up to rust-lang/rust#98450.
r? @GuillaumeGomez @lqd
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request
Add documentation for TyCtxt::visibility
We encountered this issue while working on rust-lang/rust#98450.
cc @lqd
r? @cjgillot
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request
Add documentation for TyCtxt::visibility
We encountered this issue while working on rust-lang/rust#98450.
cc @lqd
r? @cjgillot
Labels
Area: The testsuite used to check the correctness of rustc
This PR was explicitly merged by bors.
The performance regression has been triaged.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.