bootstrap: Enable rustdoc mergeable CCI for std and internal docs

[camelid]

View all comments

This feature is unstable but will be stabilized soon, and this is a good way of dogfooding it to make sure it works properly. It should have no effect on the generated docs, but it provides a significant speedup. For example, I measure a 3x speedup locally (3m 11s -> 1m 1s) for x doc src/tools -- note that this is with the latest rustdoc perf improvements (#159854).

r? @Kobzol

[rustbot]

Kobzol is not on the review rotation at the moment.
They may take a while to respond.

[Kobzol]

Let's try the main x64 Linux dist job, I will compare the docs build time before/after.

Btw, it would be nice to add a comment on top of the added argument, which explains why is it there.

@bors try jobs=dist-x86_64-linux

[camelid]

@GuillaumeGomez @notriddle do you think this failure is due to a bug in mergeable CCI? It's a little weird because it seems to pass on LLVM 22.2 but not 22.1 -- so could be spurious? But it's suspicious that it's related to the search index.

[Kobzol]

I kinda doubt it is spurious. However, bootstrap does a lot of weird crimes related to the created and removal of various doc things being built in the build directory, could be caused by that. I'll take a look at it tomorrow.

[camelid]

Yes, I suspect it's a bug in rustdoc. It looks like the LLVM 22.2 CI disables js-std tests.

[camelid]

Looks like it's actually a cargo bug, where cargo isn't passing the --resource-suffix to the final merge step. Actually cargo isn't passing the RUSTDOCFLAGS at all. We should probably pass the same flags to the final merge invocation as to each crate's rustdoc invocation. This conflates two somewhat different things, but we already share the same flags across all crates so sharing it with the merge makes sense I think. Working with @weihanglo on a fix in cargo.

[rust-bors]

💔 Test for bf99269 failed: CI. Failed job:

• try - dist-x86_64-linux (web logs, enhanced plaintext logs)

[camelid]

cc rust-lang/cargo#17269

[Kobzol]

The try build failure looks like it might be some different error, maybe related to some cursedness when documenting the standard library?

##[group]Documenting stage1 library{alloc, compiler_builtins, core, panic_abort, panic_unwind, proc_macro, profiler_builtins, rustc-std-workspace-core, std, std_detect, sysroot, test, unwind} in JSON format (stage1 -> stage1, x86_64-unknown-linux-gnu)
warning: output filename collision at /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/dist/build/alloc/4e4894d7dbb95523/out/alloc.json
  |
  = note: the lib target `alloc` in package `alloc v0.0.0 (/checkout/library/alloc)` has the same output filename as the lib target `alloc` in package `alloc v0.0.0 (/checkout/library/alloc)`
  = note: this is a known bug where multiple crates with the same name use the same path; see <https://github.com/rust-lang/cargo/issues/6313>

[weihanglo]

Looks like it's actually a cargo bug, where cargo isn't passing the --resource-suffix to the final merge step. Actually cargo isn't passing the RUSTDOCFLAGS at all.

This is an unresolved question. See rust-lang/cargo#17269 (comment)

[camelid]

Came across another issue. Currently, when building compiler/tool docs, we do separate cargo invocations but have them all end up in the same output directory. So that rustc, rustdoc, cargo, clippy, etc. all share one docs site (/nightly/nightly-rustc). This works out of the box with legacy shared CCI because rustdoc updates the finalized artifacts with each build. However, with shared CCI, we run one finalize per docs target (i.e. one for rustc, one for rustdoc, one for clippy, etc.), which causes the CCI (crates.js, search-index, etc.) to be overwritten.

I'm not sure what the best fix for this is. Ideally, we would probably have bootstrap tell Cargo to not run finalize for the intermediate targets and just do one finalize at the end. It also suggests that enabling mergeable CCI by default in Cargo is a moderately breaking change for workflows that depend on separate cargo doc invocations working well together.

[camelid]

@notriddle @weihanglo do you have any thoughts about the above issue?

[weihanglo]

@camelid #160098 (comment)

Came across another issue. Currently, when building compiler/tool docs, we do separate cargo invocations but have them all end up in the same output directory. So that rustc, rustdoc, cargo, clippy, etc. all share one docs site (/nightly/nightly-rustc). This works out of the box with legacy shared CCI because rustdoc updates the finalized artifacts with each build. However, with shared CCI, we run one finalize per docs target (i.e. one for rustc, one for rustdoc, one for clippy, etc.), which causes the CCI (crates.js, search-index, etc.) to be overwritten.

I'm not sure what the best fix for this is. Ideally, we would probably have bootstrap tell Cargo to not run finalize for the intermediate targets and just do one finalize at the end. It also suggests that enabling mergeable CCI by default in Cargo is a moderately breaking change for workflows that depend on separate cargo doc invocations working well together.

Haven't checked how bootstrap builds docs, though I think it should just work, as cargo doc is additive (in order to follow how rustdoc works and keep backward-compat): https://github.com/rust-lang/cargo/blob/9c930d9f8d5ec08afd4bd69dbdcba2a336fbefe3/tests/testsuite/doc.rs#L3773-L3913

It might be possible that bootstrap is using -Zskip-rustdoc-fingerprint. See also rust-lang/cargo#9404

View changes since the review

[camelid]

Bootstrap is in fact using -Zskip-rustdoc-fingerprint. However, I think the issue is more fundamental than that because rustdoc assumes that finalize is always run exactly once per doc site. If you run finalize for rustc and then for a tool, it will just overwrite the shared files for rustc with ones specific to the tool.

[weihanglo]

Not sure. I thought the feature was additive hence the test in Cargo (it ran cargo doc -p foo then -p dep and then -p bar sequentially).

If it were not they Cargo probably can't ship it as a default for backward compatibility reasons.

What are those shared files being overwritten? Were they safe-from-overwrite previously without -Zrustdoc-mergeable-info?

[camelid]

The cargo test doesn't actually check the contents of the shared files as far as I can tell. Rustdoc is still additive for the crate docs (e.g. foo/struct.Bar.html will not be overwritten). But with mergeable CCI, sequential cargo doc invocations will clobber the shared files like the search index.

The shared files were deserialized, updated, and reserialized on every single rustdoc invocation before. With mergeable CCI, each crate's contribution to the CCI is serialized separately and the merge step combines them all into the final shared files. One possible fix for this backcompat issue is that the merge step could check for any existing shared files in the output directory and use those as the basis for its merge step. It would be nice to avoid that if we can though and have the merge be authoritative rather than being invoked any number of times.

[notriddle]

However, with shared CCI, we run one finalize per docs target (i.e. one for rustc, one for rustdoc, one for clippy, etc.), which causes the CCI (crates.js, search-index, etc.) to be overwritten.

As long as the same Cargo build directory is used for all three of these builds, that isn’t supposed to be a problem. The feature implements --doc-meta-dir as a directory specifically so that this can still work.

Did you try the feature and have it not work?

[camelid]

Yes, I tried this branch locally with e.g. x doc src/tools/rustdoc, and the crates.js file and search index are both broken. Crate documentation exists for all the rustc crates, but they are not in the crates.js file or search index.

[camelid]

The specific problem is that merging assumes it is given all meta-dir inputs and does not import existing finalized outputs. Since bootstrap invokes cargo doc on rustc separately from each of the tools, we run the merge step for each one, without providing meta-dir inputs from the other invocations.

[weihanglo]

Yes, I tried this branch locally with e.g. x doc src/tools/rustdoc, and the crates.js file and search index are both broken. Crate documentation exists for all the rustc crates, but they are not in the crates.js file or search index.

That may be due to skipping rustdoc-fingeprint. The fingerprint file tracks previous rustdoc run in order to collect all mergeable info across multiple cargo doc invocations.

[camelid]

Hmm, so what is in rustdoc-fingerprint? Does it include all the meta dirs that were generated along the way?

[camelid]

@rustbot blocked

[notriddle]

Okay, I rebased this one onto main.

[notriddle]

@bors try jobs=dist-x86_64-linux

[rust-bors]

☀️ Try build successful (CI)
Build commit: 9e9efb5 (9e9efb5d6e9e97f42d4c195140b7c197a5e66fa6)
Base parent: 8925ea3 (8925ea358a0f265ca61026aadc7ecc506c545cbe)

[Kobzol]

Looks like this made docs building ~20% faster in the dist job? Nice!

[Kobzol]

That being said, I'd still like to understand what exactly is needed for this, and if we can implement it without introducing a new mode.

[notriddle]

@Kobzol

It's needed because:

1. The search index (the index that powers this feature) needs to contain all of the crates in the nightly-rustc project. In particular, I'd prefer if it contained Clippy, Rustdoc, and Rustc, since those crates share type checker stuff and the ability to search all three at once is convenient.
2. For every crate that rustdoc currently documents, it has to load the search index from the doc output dir, and rebuild the search index with the new crate added to it. Loading the search index requires $O(\text{crates})$ work, so doing it once for every crate means we're doing $O(\text{crates}^2)$ work overall.
3. It would be more efficient, instead, if each crate wrote its data separately, and then the final search index was generated at the end by merging them all at once. Obviously, this would make the work linear instead of quadratic. For the record, Hoogle and Sherlodoc have a similar index-generating step.
4. We call this "Mergeable Cross-Crate-Information." Cargo stores it in the build directory, and supplies it to Rustdoc in a separate phase that runs after everything else. When we eventually stabilize this feature, it will be invisible to (most) end users. cargo doc will just be faster.
5. So, in order for crates to share their cross-crate info, we need them to share a build directory.
6. Tools, like Rustdoc and Cargo, don't normally share a build directory with Rustc.
7. To make them share a build directory while generating documentation, without forcing them to share a build directory while compiling, I added a new mode.

[Kobzol]

Thanks for the detailed explanation!

Bootstrap is doing something hacky, instead of combining docs from multiple crates within a single cargo doc invocation (which is the normal case), it wants to combine docs across multiple cargo doc invocations, for "unrelated" projects, rustc, clippy, etc.

If I understood it correctly, with the mergeable info, cargo doc executes the merge step as a separate rustdoc command at the end of its execution.

You propose to run cargo doc for rustc, clippy, etc. with a shared build dir, so that each cargo doc execution will perform the final merge step for all docs built so far.

I wonder, could we instead build things in their own separate build dirs, and then manually call the rustdoc command that merges the data together, and pass it the separate build dirs, to produce the final documentation?

[notriddle]

I wonder, could we instead build things in their own separate build dirs, and then manually call the rustdoc command that merges the data together, and pass it the separate build dirs, to produce the final documentation?

Yes, that's possible. To accomplish this, bootstrap needs to know where cargo puts the CCI data in its build dir, so bootstrap can supply the path to rustdoc.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Kobzol]

Just to explain why I'm asking about this, I consider the fact that bootstrap shares the output doc directory to be a gross hack that I wanted to get rid of for some time. It causes unrelated doc steps to invalidate each other, and it is currently implemented in a very implicit way in bootstrap, which makes it harder to do some refactorings that I'd like to make.

I want to keep the final merged documentation, but only do that when we actually run x dist rust-docs, not when building doc steps for individual projects. And I want to arrive at that target state via some other mechanism than by sharing the intermediate output dir.

Replacing sharing of the output dir by sharing the target dir is perhaps a bit better, but I still consider it hacky.

So ideally, I'd like to build all docs independently, and then have an explicit step that combines them into a separate docs directory which we will then ship.

[notriddle]

@Kobzol

I've written a commit that implements pretty much what you mentioned. notriddle@2115296c11a

If you're happy with the way it works, I can push it to either this PR or a new one.

[weihanglo]

@Kobzol

I've written a commit that implements pretty much what you mentioned. notriddle@2115296c11a

If you're happy with the way it works, I can push it to either this PR or a new one.

I skimmed through the impl. Does that mean Cargo's -Zskip-rustdog-fingerprint is no longer needed?
https://github.com/rust-lang/cargo/blob/af373f761aa4e42b093c2e453078fd73c8ce54dd/src/workspace/features.rs#L928

[notriddle]

I tested without it, and it still seems to work. notriddle@7aa06d7e764