rustdoc: Only store locations in Cache::extern_locations and calculate the other info on-demand

[tdelabro]

help #84588

[rust-highfive]

r? @ollie27

(rust-highfive has picked a reviewer for you, use r? to override)

[tdelabro]

r? @jyn514

[tdelabro]

Hey @jyn514, can I store extern_html_root_url as a new field of Cache, instead of the whole extern_locations HashMap ?

Or is there a way for me to access it a any time via the context. I feel like this is not possible because I would need the DocContext as in:

rust/src/librustdoc/core.rs

Lines 561 to 565 in 154858c

	let render_options = ctxt.render_options;
	let mut cache = ctxt.cache;
	krate = tcx.sess.time("create_format_cache", || {
	cache.populate(krate, tcx, &render_options.extern_html_root_urls, &render_options.output)
	});

[jyn514]

can I store extern_html_root_url as a new field of Cache, instead of the whole extern_locations HashMap ?

Yes, that's fine :) render_options is kind of a mess, I would rather we keep more things on Cache anyway.

[jyn514]

I think the rest of this is still in progress, right? Feel free to ping me when this is ready for review.

[tdelabro]

I have been able to replace all the usages of Cache::extern_locations by some on-demand computation, except for two of them, in src/librustdoc/html/format.rs, in the functions href and primitive_link. When I try to do so errors arise when running test/rustdoc, for tests such as issue-15318.rs.

My on-demand computation return Some(ExternalLocation::Local) while the value stored in the cache is ExternalLocation::Remote("http://example.com").
The reason why this happen is that, while the arguments passed to the function location are the same, the output change because the computer file system state has changed between those two calls.

I printed some debug inside the location function:

        debug!(?local_location, ?self.crate_num, ?extern_url, ?dst);
        debug!("{}", local_location.is_dir())

Here is the output for the call that fill the cache (inside rustdoc::formats::cache::Cache::populate):

DEBUG rustdoc::clean::types local_location="/Users/timothee/Documents/rust/build/x86_64-apple-darwin/test/rustdoc/issue-15318/issue_15318", self.crate_num=crate4, extern_url=None, dst="/Users/timothee/Documents/rust/build/x86_64-apple-darwin/test/rustdoc/issue-15318"
DEBUG rustdoc::clean::types false

And here is the output for the on-demand call (inside `rustdoc::html::format::primitive_link):

DEBUG rustdoc::clean::types local_location="/Users/timothee/Documents/rust/build/x86_64-apple-darwin/test/rustdoc/issue-15318/issue_15318", self.crate_num=crate4, extern_url=None, dst="/Users/timothee/Documents/rust/build/x86_64-apple-darwin/test/rustdoc/issue-15318"
DEBUG rustdoc::clean::types true

The arguments are the same but in the on-demand call we enter the following branch:

        if local_location.is_dir() {
            return Local;
        }

So my issue seem to be that a new directory have been created been created at /rust/build/x86_64-apple-darwin/test/rustdoc/issue-15318/issue_15318 while building the documentation. Which seems legit, I suppose creating those directories and files is ultimately the goal of building documentation, but cause some trouble here.

Do you see any way we can get out of this situation ? Maybe slightly postponing the directory creation ?

[jyn514]

Maybe slightly postponing the directory creation ?

I don't think this can work because these href() calls happen while rustdoc is writing the HTML files themselves to disk. What we could do instead is generate files to a temporary directory, then rename them all, something like this:

.
├── old_crate
└── tmp-build
    ├── new_crate1
    └── new_crate2

That will only do as many renames as there are crates, so it shouldn't be much slower - on most file systems, directory renames are basically free as long as they happen on the same filesystem.

That's a fairly large change, so it may be easier in the meantime to only remove name and src_root from the extern_locations map, and still calculate the ExternLocations themselves on-demand. That should still get most of the benefit.

[jyn514]

This looks great :) just want to do a perf run first.

[jyn514]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit b1ab5cf9f3f2fbfef892327240c28e94011b2d10 with merge 327229fc2f1d5d7595a5edd2bbfaf553d7397b38...

[jyn514]

@rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[jyn514]

@rust-timer queue b1ab5cf9f3f2fbfef892327240c28e94011b2d10

[rust-timer]

Awaiting bors try build completion.

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

[jyn514]

@bors try @rust-timer queue include=-doc

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 2cc2639 with merge e5668a2fb8fd709b737433c50a467ea877ab5f4b...

[bors]

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

[rust-timer]

Queued e5668a2fb8fd709b737433c50a467ea877ab5f4b with parent a45f0d7, future comparison URL.

[rust-timer]

Finished benchmarking try commit (e5668a2fb8fd709b737433c50a467ea877ab5f4b): 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

[jyn514]

Both perf and max-rss look like noise, but this is a nice cleanup :)

@bors r+ rollup

[bors]

📌 Commit 2cc2639 has been approved by jyn514

[jyn514]

Also, given how little perf impact there was, I wouldn't spend too much time on my idea in #84601 (comment) - shrinking doctree::Module or something like that might be more helpful.