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

rustdoc: Optimize and refactor doc link resolution #96135

Merged
merged 6 commits into from
Apr 20, 2022
Merged

rustdoc: Optimize and refactor doc link resolution #96135

merged 6 commits into from
Apr 20, 2022

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Apr 17, 2022 •
edited
Loading

One more subset of #94857 that should bring perf improvements rather than regressions + a couple more optimizations on top of it.
It's better to read individual commits and their descriptions to understand the changes.
The may_have_doc_links optimization is not very useful here, but it's much more important for #94857.

Closes #96079

@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

Some changes occurred in clean/types.rs.

cc @camelid

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 17, 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 Apr 17, 2022
@petrochenkov
Copy link
Contributor Author

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

bors commented Apr 17, 2022

⌛ Trying commit afd1ce9da528a376d8bf532fc73ea6b30f96081f with merge 17cff29288d7f9f1378ebd548ff5a4994de8c57c...

@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 17, 2022
@bors

This comment was marked as resolved.

Sign in to view
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 17, 2022
@rust-log-analyzer

This comment was marked as resolved.

Sign in to view
@petrochenkov
Copy link
Contributor Author

@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 17, 2022

⌛ Trying commit 76077f923a7536b844a854078dcfa093524c5c27 with merge cd5dce912ebe17db000b68c530333a49ad85d462...

@petrochenkov petrochenkov removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 17, 2022
@bors
Copy link
Contributor

bors commented Apr 17, 2022

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

@rust-timer
Copy link
Collaborator

Queued cd5dce912ebe17db000b68c530333a49ad85d462 with parent 1ec2c13, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cd5dce912ebe17db000b68c530333a49ad85d462): comparison url.

Summary:

  • Primary benchmarks: mixed results
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 6 2 14 19 20
mean2 0.4% 0.5% -1.2% -0.8% -0.7%
max 0.4% 0.7% -2.1% -1.0% -2.1%

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.

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

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot added perf-regression Performance regression. 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 18, 2022
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Apr 18, 2022 •
edited
Loading

Seems like you're almost done in here! :D

@petrochenkov
Copy link
Contributor Author

@GuillaumeGomez
This PR should be ready.

tt-muncher seems to fluctuate a lot for reasons unrelated to this PR.
stm32f4 has a huge number of items with doc attributes, so the ~0.4% regression from encoding metadata is an expected tradeoff for faster documentation generation.

@petrochenkov
Copy link
Contributor Author

@GuillaumeGomez
Updated with the test from #96216.

@Kobzol
Copy link
Contributor

Kobzol commented Apr 19, 2022

markdown_links is very expensive and is responsible for majority of runtime regression in #94857, preprocess_link is also somewhat expensive.

Are there any specific crates in the benchmark suite that use these doc links a lot? I can try to profile them and look for potential bottlenecks.

@GuillaumeGomez
Copy link
Member

Thanks @petrochenkov ! Let's go then. :)

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 19, 2022

📌 Commit ca5c752 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 19, 2022
@GuillaumeGomez
Copy link
Member

@bors rollup=never

@petrochenkov
Copy link
Contributor Author

@Kobzol
Standard library.
If you build rustdoc with #94857 and run it on hello-world, you'll see markdown_links at the top of the profile inside early_resolve_intra_doc_links.
(It happens because with #94857 rustdoc pulls impl items with their docs from dependency crates too eagerly.)

@bors
Copy link
Contributor

bors commented Apr 20, 2022

⌛ Testing commit ca5c752 with merge d39864d...

@bors
Copy link
Contributor

bors commented Apr 20, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 20, 2022
@bors bors merged commit d39864d into rust-lang:master Apr 20, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 20, 2022
This was referenced Apr 20, 2022
Merged
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d39864d): comparison url.

Summary:

  • Primary benchmarks: mixed results
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 6 0 14 20 20
mean2 0.3% N/A -1.3% -0.7% -0.9%
max 0.3% N/A -2.7% -1.0% -2.7%

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 20, 2022
@bors
Auto merge of rust-lang#96187 - GuillaumeGomez:potential-intra-doc-li…
879aff3 
…nks-filtering, r=notriddle

Prevent `<>` links to be interpreted for intra-doc links

As discussed in [this thread](rust-lang#96135 (comment)). As mentioned, the intra-doc RFC states that `<>` links shouldn't be potential intra-doc links:  https://rust-lang.github.io/rfcs/1946-intra-rustdoc-links.html#no-autolinks-style.

I renamed `markdown_links` into `potential_intra_doc_markdown_links` to make it more obvious what it's doing.

cc `@petrochenkov`
r? `@notriddle`
@Kobzol Kobzol mentioned this pull request Apr 20, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 21, 2022
@bors
Auto merge of rust-lang#96260 - Kobzol:rustdoc-idmap, r=petrochenkov
de1bc00 
rustdoc: Optimize IdMap

Slightly optimizes `IdMap`, which is hot in `markdown_links` (context [here](rust-lang#96135 (comment))). There are more improvements that can be made near this place, but this seemed like an easy win locally (although I tried it on top of rust-lang#94857, so let's see what happens without that PR).

r? `@petrochenkov`
@GuillaumeGomez GuillaumeGomez mentioned this pull request Apr 23, 2022
Closed
@rylev
Copy link
Member

rylev commented Apr 26, 2022

@petrochenkov said:

stm32f4 has a huge number of items with doc attributes, so the ~0.4% regression from encoding metadata is an expected tradeoff for faster documentation generation.

Given that besides that benchmark, this is a win (and sometimes a big one), I'll mark this as triaged.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 26, 2022
@petrochenkov petrochenkov mentioned this pull request Apr 26, 2022
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 3, 2022
@JohnTitor
Rollup merge of rust-lang#96447 - petrochenkov:docregr, r=GuillaumeGomez
a4afb49 
rustdoc: Resolve doc links on fields during early resolution

Another subset of rust-lang#94857 which fixes rust-lang#96429.

This case regressed in rust-lang#96135 when `may_have_doc_links`-based filtering was introduced.
Before that filtering structs could collect traits in scope for their fields, but after the filtering structs won't collect anything if they don't have doc comments on them, so we have to visit fields too.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2022
@bors
Auto merge of rust-lang#96447 - petrochenkov:docregr, r=GuillaumeGomez
21d613b 
rustdoc: Resolve doc links on fields during early resolution

Another subset of rust-lang#94857 which fixes rust-lang#96429.

This case regressed in rust-lang#96135 when `may_have_doc_links`-based filtering was introduced.
Before that filtering structs could collect traits in scope for their fields, but after the filtering structs won't collect anything if they don't have doc comments on them, so we have to visit fields too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notriddle notriddle notriddle left review comments

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

@jyn514 jyn514 jyn514 left review comments

@camelid camelid camelid left review comments

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.

Compiler error on nightly cargo doc invocation on fltk-rs repository