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.

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

Don't store primitives in Crate #98694

Closed
wants to merge 1 commit into from
Closed

Don't store primitives in Crate #98694

wants to merge 1 commit into from

Conversation

camelid
Copy link
Member

@camelid camelid commented Jun 29, 2022

It can be used on-demand like all the other uses.

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

r? @GuillaumeGomez

(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 Jun 29, 2022
@camelid
Copy link
Member Author

camelid commented Jun 29, 2022

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

bors commented Jun 29, 2022

⌛ Trying commit 40f87e9080ff4793d31d7a2caa5b8926381b07e6 with merge 1c2bbd3c54cfe7f7c9eeb92061aba8b058199f8f...

@camelid camelid closed this Jun 29, 2022
@camelid camelid reopened this Jun 29, 2022
@camelid
Copy link
Member Author

camelid commented Jun 29, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@bors
Copy link
Contributor

bors commented Jun 29, 2022

⌛ Trying commit e6e332763ca401388dbf4d465bc4c9b520f5dfc7 with merge 7e0fe6422706156646410c8e6bb63d021ff30579...

jyn514
jyn514 reviewed Jun 29, 2022
View reviewed changes
src/librustdoc/clean/utils.rs
@@ -73,7 +73,7 @@ pub(crate) fn krate(cx: &mut DocContext<'_>) -> Crate {
}));
}

Crate { module, primitives, external_traits: cx.external_traits.clone() }
Crate { module, external_traits: cx.external_traits.clone() }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I'm not useful this change is since you still call crate.primitives(), you just don't store it afterwards.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but Crate is passed by value a lot, and this cuts down on its size. Might not help a ton, but I thought worth a try.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to cache ExternalCrate::primitives as a OnceCell like primitive_locations, but ran into issues since primitives is called for multiple different crates, so you can't use OnceCell and call it a day.

@bors
Copy link
Contributor

bors commented Jun 30, 2022

💔 Test failed - checks-actions

@bors bors 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 Jun 30, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member

Once you have fixed the CI failures, we can run a perf check to see the result.

@camelid
Don't store primitives in Crate
975151a 
It can be used on-demand like all the other uses.
@camelid 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 Jul 7, 2022
@GuillaumeGomez
Copy link
Member

Time for a new perf check.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@bors
Copy link
Contributor

bors commented Jul 7, 2022

⌛ Trying commit 975151a with merge cb872ab1aedfa67ece4d7960a246bd7f26f92807...

@bors
Copy link
Contributor

bors commented Jul 7, 2022

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

@rust-timer
Copy link
Collaborator

Queued cb872ab1aedfa67ece4d7960a246bd7f26f92807 with parent c461f7a, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cb872ab1aedfa67ece4d7960a246bd7f26f92807): comparison url.

Instruction count

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

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 2.1% 4.1% 2
Regressions 😿
(secondary)		 3.9% 6.4% 5
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 2.1% 4.1% 2

Cycles

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 5.0% 5.0% 1
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -2.2% -2.2% 1
All 😿🎉 (primary) N/A N/A 0

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 8, 2022
@camelid
Copy link
Member Author

camelid commented Jul 8, 2022

All right, looks like this just makes perf worse, and I don't think the code simplification is worth it.

Perhaps caching ExternalCrate::primitives is a better route.

@camelid camelid closed this Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@camelid @rust-highfive @rust-timer @bors @rust-log-analyzer @GuillaumeGomez @jyn514 @rustbot