rustdoc: Don't load all extern crates unconditionally #83738

jyn514 · 2021-04-01T03:02:33Z

Instead, only load the crates that are linked to with intra-doc links.

This doesn't help very much with any of rustdoc's fundamental issues
with freezing the resolver, but it at least fixes a stable-to-stable
regression, and makes the crate loading model somewhat more consistent
with rustc's. I tested and it unfortunately does not help at all with #82496.

Closes #68427. Opened #83761 to track the more fundamental issues.
r? @petrochenkov cc @eddyb @ollie27

src/test/run-make/unused-extern-crate/panic-item.rs

petrochenkov · 2021-04-01T19:36:12Z

src/librustdoc/core.rs

+                        Err(_) => continue,
+                    };
+                    self.resolver.borrow_mut().access(|resolver| {
+                        let _ = resolver.resolve_str_path_error(


If you save the resolution results for later instead of throwing them away, then you'll no longer need the resolver later on.
That's basically the idea behind fixing the resolver cloning/freezing issue.

petrochenkov · 2021-04-01T19:39:04Z

Let me know if you want me to open a separate issue for not freezing the resolver.

Yes, could you create the issue?
The fix is to pretty much do what this PR does but more extensively - resolve everything that needs to be resolved early and save the results for later.
Once Resolver::clone_outputs is removed the problem can be considered fixed.

jyn514 · 2021-04-01T22:25:24Z

Yes, could you create the issue?

Sure thing, I opened #83761. I left a question about the approach, but it's not related to this PR.

petrochenkov · 2021-04-02T05:47:47Z

@bors r+

bors · 2021-04-02T05:47:48Z

📌 Commit cc69c65 has been approved by petrochenkov

…rochenkov rustdoc: Don't load all extern crates unconditionally Instead, only load the crates that are linked to with intra-doc links. This doesn't help very much with any of rustdoc's fundamental issues with freezing the resolver, but it at least fixes a stable-to-stable regression, and makes the crate loading model somewhat more consistent with rustc's. I tested and it unfortunately does not help at all with rust-lang#82496. Closes rust-lang#68427. Let me know if you want me to open a separate issue for not freezing the resolver. r? `@petrochenkov` cc `@eddyb` `@ollie27`

…rochenkov rustdoc: Don't load all extern crates unconditionally Instead, only load the crates that are linked to with intra-doc links. This doesn't help very much with any of rustdoc's fundamental issues with freezing the resolver, but it at least fixes a stable-to-stable regression, and makes the crate loading model somewhat more consistent with rustc's. I tested and it unfortunately does not help at all with rust-lang#82496. Closes rust-lang#68427. Let me know if you want me to open a separate issue for not freezing the resolver. r? ``@petrochenkov`` cc ``@eddyb`` ``@ollie27``

jyn514 · 2021-04-02T20:35:44Z

Ok, I added that. Not sure how to test it worked from linux. I also had to add crate_type = "lib" or it gave a weird error about conflicting outputs:

error: the generated executable for the input file "/home/joshua/rustc3/src/test/rustdoc-ui/auxiliary/panic-item.rs" conflicts with the existing directory "/home/joshua/rustc3/build/x86_64-unknown-linux-gnu/test/rustdoc-ui/unused-extern-crate/auxiliary/panic-item"

petrochenkov · 2021-04-03T09:55:46Z

@bors r+

bors · 2021-04-03T09:55:47Z

📌 Commit e4244e3 has been approved by petrochenkov

bors · 2021-04-03T10:59:28Z

⌛ Testing commit e4244e3 with merge 640ce99...

bors · 2021-04-03T13:23:37Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 640ce99 to master...

klensy · 2021-04-04T14:15:28Z

Somehow this works good for cargo-doc, but not for other:
https://perf.rust-lang.org/compare.html?start=ccd997592bf545fc54a9cce31f1280f833915ba1&end=640ce99bfe70375a24c6775a937d6a258b40398b&stat=instructions%3Au

jyn514 · 2021-04-04T14:20:08Z

@klensy this is a bug fix - it doesn't really matter what the performance is as long as it's not a giant regression. Cargo is probably lower because it has a lot of --extern crates that no longer need to be loaded.

klensy · 2021-04-04T14:24:43Z

@jyn514 No, i mean, look at -doc full builds: for cargo-doc there some changes in executions delta column, but literally zero changes for others in that column (didn't checked all of them).

jyn514 · 2021-04-04T14:29:05Z

I don't understand what you're trying to tell me. On the "detailed self profile results" page I see no change in time, which is what I'd expect from this PR. If there's an inconsistency between that and instructions that sounds like an issue with perf.rlo, I think there's some ongoing work to measure instructions for self-profile.

klensy · 2021-04-04T14:41:38Z

cargo-doc: https://perf.rust-lang.org/detailed-query.html?commit=640ce99bfe70375a24c6775a937d6a258b40398b&base_commit=ccd997592bf545fc54a9cce31f1280f833915ba1&benchmark=cargo-doc&run_name=full , Executions delta diff not zero

any(?) other -doc profile : Executions delta equal to zero
https://perf.rust-lang.org/detailed-query.html?commit=640ce99bfe70375a24c6775a937d6a258b40398b&base_commit=ccd997592bf545fc54a9cce31f1280f833915ba1&benchmark=clap-rs-doc&run_name=full
https://perf.rust-lang.org/detailed-query.html?commit=640ce99bfe70375a24c6775a937d6a258b40398b&base_commit=ccd997592bf545fc54a9cce31f1280f833915ba1&benchmark=regex-doc&run_name=full
https://perf.rust-lang.org/detailed-query.html?commit=640ce99bfe70375a24c6775a937d6a258b40398b&base_commit=ccd997592bf545fc54a9cce31f1280f833915ba1&benchmark=webrender-doc&run_name=full

If there's an inconsistency between that and instructions that sounds like an issue with perf.rlo, I think there's some ongoing work to measure instructions for self-profile.

Or cargo-doc works somehow special, or it issue with profiler, yes.

Mark-Simulacrum · 2021-04-04T14:46:56Z

Those runs suggest that there is some difference in the documented code for cargo and clap-rs, and this particular PR sees a larger improvement in terms of less queries being run on the cargo benchmark (compared to the other ones you've cited). That's not really a problem, it's pretty intentional that our benchmarks differ in what they look like to the compiler (otherwise there'd be no point in having two different ones) -- I'm trying to understand what problem or note you're trying to make :)

…crate-load, r=jyn514 Revert "Don't load all extern crates unconditionally" Fixes rust-lang#84738. This reverts rust-lang#83738. For the "smart" load of external crates, we need to be able to access their items in order to check their doc comments, which seems, if not impossible, quite complicated using only the AST. For some context, I first tried to extend the `IntraLinkCrateLoader` visitor by adding `visit_foreign_item`. Unfortunately, it never enters into this call, so definitely not the right place... I then added `visit_use_tree` to then check all the imports outside with something like this: ```rust let mut loader = crate::passes::collect_intra_doc_links::IntraLinkCrateLoader::new(resolver); ast::visit::walk_crate(&mut loader, krate); let mut items = Vec::new(); for import in &loader.imports_to_check { if let Some(item) = krate.items.iter().find(|i| i.id == *import) { items.push(item); } } for item in items { ast::visit::walk_item(&mut item); for attr in &item.attrs { loader.check_attribute(attr); } } ``` This was, of course, a failure. We find the items without problems, but we still can't go into the external crate to check its items' attributes. Finally, `@jyn514` suggested to look into the [`CrateLoader`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_metadata/creader/struct.CrateLoader.html), but it only seems to provide metadata (I went through [`CStore`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_metadata/creader/struct.CStore.html) and [`CrateMetadata`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_metadata/rmeta/decoder/struct.CrateMetadata.html)). I think we are too limited here (with AST only) to be able to determine the crates we actually need to import, but it's very likely that I missed something. Maybe `@petrochenkov` or `@Aaron1011` have an idea? So until we find a way to make it work completely, we need to revert it to fix the ICE. Once merged, we'll need to re-open rust-lang#68427. r? `@jyn514`

Reland rust-lang#83738: "rustdoc: Don't load all extern crates unconditionally" I hopefully found all the bugs 🤞 time for a take two. See the last commit for details on what went wrong before. r? `@petrochenkov` (but feel free to reassign to Guillaume if you don't have time.) Closes rust-lang#68427. Includes a fix for rust-lang#84738.

…arth Rollup of 11 pull requests Successful merges: - rust-lang#87832 (Fix debugger stepping behavior with `match` expressions) - rust-lang#88123 (Make spans for tuple patterns in E0023 more precise) - rust-lang#88215 (Reland rust-lang#83738: "rustdoc: Don't load all extern crates unconditionally") - rust-lang#88216 (Don't stabilize creation of TryReserveError instances) - rust-lang#88270 (Handle type ascription type ops in NLL HRTB diagnostics) - rust-lang#88289 (Fixes for LLVM change 0f45c16) - rust-lang#88320 (type_implements_trait consider obligation failure on overflow) - rust-lang#88332 (Add argument types tait tests) - rust-lang#88340 (Add `c_size_t` and `c_ssize_t` to `std::os::raw`.) - rust-lang#88346 (Revert "Add type of a let tait test impl trait straight in let") - rust-lang#88348 (Add field types tait tests) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

…trochenkov rustdoc: Go back to loading all external crates unconditionally This *continues* to cause regressions. This code will be unnecessary once access to the resolver happens fully before creating the tyctxt (rust-lang#83761), so load all crates unconditionally for now. To minimize churn, this leaves in the code for loading crates selectively. "Fixes" rust-lang#84738. Previously: rust-lang#83738, rust-lang#85749, rust-lang#88215 r? `@petrochenkov` cc `@camelid` (this should fix the "index out of bounds" error you had while looking up `crate_name`).

…ochenkov rustdoc: Go back to loading all external crates unconditionally This *continues* to cause regressions. This code will be unnecessary once access to the resolver happens fully before creating the tyctxt (rust-lang#83761), so load all crates unconditionally for now. To minimize churn, this leaves in the code for loading crates selectively. "Fixes" rust-lang#84738. Previously: rust-lang#83738, rust-lang#85749, rust-lang#88215 r? `@petrochenkov` cc `@camelid` (this should fix the "index out of bounds" error you had while looking up `crate_name`).

jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-resolve Area: Name resolution A-metadata Area: Crate metadata labels Apr 1, 2021

rust-highfive assigned petrochenkov Apr 1, 2021

This comment has been minimized.

Sign in to view

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 1, 2021

jyn514 force-pushed the only-load-some-crates branch from e10e1dc to 4b0178b Compare April 1, 2021 03:06

This comment has been minimized.

Sign in to view

jyn514 force-pushed the only-load-some-crates branch from 4b0178b to 36d6800 Compare April 1, 2021 04:03

jyn514 mentioned this pull request Apr 1, 2021

Fix impl being removed excessively #82496

Closed

This comment has been minimized.

Sign in to view

jyn514 force-pushed the only-load-some-crates branch from 36d6800 to 3d0961e Compare April 1, 2021 13:39

petrochenkov reviewed Apr 1, 2021

View reviewed changes

petrochenkov 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 1, 2021

jyn514 force-pushed the only-load-some-crates branch from 3d0961e to cc69c65 Compare April 1, 2021 21:57

jyn514 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 Apr 1, 2021

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 2, 2021

jyn514 mentioned this pull request Apr 2, 2021

rustdoc: Stop cloning the resolver #83761

Closed

6 tasks

JohnTitor mentioned this pull request Apr 2, 2021

Rollup of 7 pull requests #83777

Closed

JohnTitor mentioned this pull request Apr 2, 2021

Rollup of 6 pull requests #83778

Closed

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 3, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 3, 2021

bors merged commit 640ce99 into rust-lang:master Apr 3, 2021

rustbot added this to the 1.53.0 milestone Apr 3, 2021

jyn514 deleted the only-load-some-crates branch April 3, 2021 15:25

jyn514 mentioned this pull request Apr 10, 2021

Rustdoc ICE: Referencing a workspace crate's root in an intra-doc link triggers an ICE #84046

Closed

This was referenced Apr 30, 2021

Index out of bounds when running cargo doc in rustc_metadata/src/creader.rs #84738

Closed

intra-rustdoc-link: Cannot use core in core crate #73445

Open

GuillaumeGomez mentioned this pull request May 27, 2021

Revert "Don't load all extern crates unconditionally" #85749

Merged

jyn514 mentioned this pull request Jul 4, 2021

rustdoc only: duplicate lang item panic_halt #68427

Closed

Manishearth mentioned this pull request Aug 26, 2021

Rollup of 11 pull requests #88371

Merged

jyn514 mentioned this pull request Nov 1, 2021

rustdoc: Go back to loading all external crates unconditionally #90489

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: Don't load all extern crates unconditionally #83738

rustdoc: Don't load all extern crates unconditionally #83738

jyn514 commented Apr 1, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

petrochenkov Apr 1, 2021

petrochenkov commented Apr 1, 2021

jyn514 commented Apr 1, 2021

petrochenkov commented Apr 2, 2021

bors commented Apr 2, 2021

jyn514 commented Apr 2, 2021

petrochenkov commented Apr 3, 2021

bors commented Apr 3, 2021

bors commented Apr 3, 2021

bors commented Apr 3, 2021

klensy commented Apr 4, 2021

jyn514 commented Apr 4, 2021 •

edited

Loading

klensy commented Apr 4, 2021

jyn514 commented Apr 4, 2021

klensy commented Apr 4, 2021

Mark-Simulacrum commented Apr 4, 2021

rustdoc: Don't load all extern crates unconditionally #83738

rustdoc: Don't load all extern crates unconditionally #83738

Conversation

jyn514 commented Apr 1, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

petrochenkov Apr 1, 2021

Choose a reason for hiding this comment

petrochenkov commented Apr 1, 2021

jyn514 commented Apr 1, 2021

petrochenkov commented Apr 2, 2021

bors commented Apr 2, 2021

jyn514 commented Apr 2, 2021

petrochenkov commented Apr 3, 2021

bors commented Apr 3, 2021

bors commented Apr 3, 2021

bors commented Apr 3, 2021

klensy commented Apr 4, 2021

jyn514 commented Apr 4, 2021 • edited Loading

klensy commented Apr 4, 2021

jyn514 commented Apr 4, 2021

klensy commented Apr 4, 2021

Mark-Simulacrum commented Apr 4, 2021

jyn514 commented Apr 1, 2021 •

edited

Loading

jyn514 commented Apr 4, 2021 •

edited

Loading