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

rustdoc: Store intra-doc links in Cache instead of on items directly #83833

Merged
merged 1 commit into from
Apr 7, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Apr 4, 2021

Items are first built after rustdoc creates the TyCtxt. To allow
resolving the links before the TyCtxt is built, the links can't be
stored on clean::Item directly.

Helps with #83761. Opening this early because I think it might decrease memory usage.

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. labels Apr 4, 2021
@rust-highfive
Copy link
Collaborator

Some changes occurred in intra-doc-links.

cc @jyn514

@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 Apr 4, 2021
@jyn514
Copy link
Member Author

jyn514 commented Apr 4, 2021

@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 Apr 4, 2021
@bors
Copy link
Contributor

bors commented Apr 4, 2021

⌛ Trying commit e7e384048d837ecba22235e884aeaca5a32668b1 with merge 687f724d5dd477b34197157d39c27f0d608a032c...

@jyn514 jyn514 changed the title Store intra-doc links in Cache instead of on items directly rustdoc: Store intra-doc links in Cache instead of on items directly Apr 4, 2021
@jyn514 jyn514 mentioned this pull request Apr 4, 2021
6 tasks
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 4, 2021

💔 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 Apr 4, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member Author

jyn514 commented Apr 4, 2021

@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 Apr 4, 2021

⌛ Trying commit 6beac7ada1af649615cafd2f2a83bff81a20b718 with merge 96bcaeeb0e14a08764e7c4fe479898bfee094c27...

@bors
Copy link
Contributor

bors commented Apr 4, 2021

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

@rust-timer
Copy link
Collaborator

Queued 96bcaeeb0e14a08764e7c4fe479898bfee094c27 with parent 2616ab1, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (96bcaeeb0e14a08764e7c4fe479898bfee094c27): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@rustbot rustbot 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 Apr 4, 2021
@GuillaumeGomez
Copy link
Member

Interesting, the performance are reduced a bit (max +0.9%). However, we have a nice improvement on the memory usage (max -1.9%).

@GuillaumeGomez
Copy link
Member

Looks good to me. Maybe @camelid wants to review again?

@jyn514
Copy link
Member Author

jyn514 commented Apr 5, 2021

Oh, I forgot to try using ThinVec. Let me do that real quick.

@jyn514
Copy link
Member Author

jyn514 commented Apr 5, 2021

Oh, I forgot to try using ThinVec. Let me do that real quick.

This currently breaks because it needs the changes from #83821. @camelid do you want me to wait for those to compare the performance? Note that the results will be slightly inaccurate because they'll be comparing different base commits.

@GuillaumeGomez
Copy link
Member

Well, you can always add a FIXME to give it a try until then if it's not merged before @camelid comes around.

@jyn514
Copy link
Member Author

jyn514 commented Apr 5, 2021

@bors r=GuillaumeGomez

@bors
Copy link
Contributor

bors commented Apr 5, 2021

📌 Commit 5667879d51b061f690340316a5f48c4e1e668402 has been approved by GuillaumeGomez

@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 Apr 5, 2021
@camelid
Copy link
Member

camelid commented Apr 6, 2021

Oh, I forgot to try using ThinVec. Let me do that real quick.

This currently breaks because it needs the changes from #83821. @camelid do you want me to wait for those to compare the performance? Note that the results will be slightly inaccurate because they'll be comparing different base commits.

I'm not sure if using ThinVec would actually improve performance. I think BTreeMap already stores values behind a pointer. Based on a quick test in the Playground, ThinVec does not change the size of Cache.intra_doc_links. But maybe it would still make a difference in some other way?

@bors
Copy link
Contributor

bors commented Apr 6, 2021

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

@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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 6, 2021
@jyn514
Copy link
Member Author

jyn514 commented Apr 6, 2021

Hmm, I just realized ThinVec won't help much anyway because the vec will never be empty, the intra doc pass only creates it if it's going to add a link.

Items are first built after rustdoc creates the TyCtxt. To allow
resolving the links before the TyCtxt is built, the links can't be
stored on `clean::Item` directly.
@jyn514
Copy link
Member Author

jyn514 commented Apr 6, 2021

@bors r=GuillaumeGomez rollup=never

@bors
Copy link
Contributor

bors commented Apr 6, 2021

📌 Commit 9e11902 has been approved by GuillaumeGomez

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 6, 2021
@bors
Copy link
Contributor

bors commented Apr 7, 2021

⌛ Testing commit 9e11902 with merge 1c158b6...

@bors
Copy link
Contributor

bors commented Apr 7, 2021

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 1c158b6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 7, 2021
@bors bors merged commit 1c158b6 into rust-lang:master Apr 7, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 7, 2021
@jyn514 jyn514 deleted the no-resolver branch April 7, 2021 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. 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
Development

Successfully merging this pull request may close these issues.

None yet

8 participants