Remove CACHE_KEY global

[GuillaumeGomez]

We realized in #80914 that the cache handling (through a global) needed to be updated to make it much easier to handle.

r? @jyn514

[jyn514]

I got as far as render and ran out of time.

I see a lot of changes adding the cache to print() - why was it needed? I had a hard time finding it in all the changes.

[GuillaumeGomez]

I got as far as render and ran out of time.

I see a lot of changes adding the cache to print() - why was it needed? I had a hard time finding it in all the changes.

Because we need the cache for both def_id and to print primitives. It's absolutely crazy how wide the usage of the cache is, it's just that it was "hidden" before. I'm quite happy that this change made it obvious.

[rust-log-analyzer]

The job x86_64-gnu-llvm-9 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

.................................................................................................... 9000/11255
.................................................................................................... 9100/11255
.................................................................................................... 9200/11255
..................................................i......i.......................................... 9300/11255
.........................................................................................iiiiii..iii 9400/11255
.................................................................................................... 9600/11255
.................................................................................................... 9700/11255
.................................................................................................... 9800/11255
.................................................................................................... 9900/11255
---
Suite("src/test/assembly") not skipped for "bootstrap::test::Assembly" -- not in ["src/tools/tidy"]
Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 27 tests
iiiiiiiiiiiiiiiiiiiiiiiiiii

 finished in 0.075 seconds
Suite("src/test/incremental") not skipped for "bootstrap::test::Incremental" -- not in ["src/tools/tidy"]
Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
---
Suite("src/test/debuginfo") not skipped for "bootstrap::test::Debuginfo" -- not in ["src/tools/tidy"]
Check compiletest suite=debuginfo mode=debuginfo (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 116 tests
iiiiiiiiii.i.i..i..i..ii....i.i....ii..........iiii.........i.....i...i.......ii.i.ii.....iiii.....i 100/116
test result: ok. 78 passed; 0 failed; 38 ignored; 0 measured; 0 filtered out; finished in 2.28s

 finished in 2.358 seconds
Suite("src/test/ui-fulldeps") not skipped for "bootstrap::test::UiFullDeps" -- not in ["src/tools/tidy"]
---
Building stage0 tool linkchecker (x86_64-unknown-linux-gnu)
   Compiling once_cell v1.4.1
   Compiling linkchecker v0.1.0 (/checkout/src/tools/linkchecker)
    Finished release [optimized] target(s) in 1.76s
std/primitive.str.html:1520: broken link - /checkout/obj/build/x86_64-unknown-linux-gnu/std/primitive.slice.html
std/primitive.str.html:1569: broken link - /checkout/obj/build/x86_64-unknown-linux-gnu/std/primitive.slice.html
std/primitive.str.html:1777: broken intra-doc link - [<code>begin</code>, <code>end</code>]
std/primitive.reference.html:158: broken link - /checkout/obj/build/x86_64-unknown-linux-gnu/std/primitive.slice.html
std/primitive.reference.html:329: broken link - /checkout/obj/build/x86_64-unknown-linux-gnu/std/primitive.slice.html
thread 'main' panicked at 'found some broken links', src/tools/linkchecker/main.rs:111:9


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/linkchecker" "/checkout/obj/build/x86_64-unknown-linux-gnu/doc"
expected success, got: exit code: 101

[GuillaumeGomez]

And fixed! I wonder about the performance impact here though... Let's see.

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

[bors]

⌛ Trying commit d128d8c0521603d0728e95332b345e02478c4371 with merge debe71105b9f695cc73e8fa32554977127b531cf...

[bors]

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

[rust-timer]

Queued debe71105b9f695cc73e8fa32554977127b531cf with parent 9bc8b00, future comparison URL.

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

[rust-timer]

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

[GuillaumeGomez]

No difference (or too little to be seen).

[jyn514]

No difference (or too little to be seen).

This is not true - script-servo went up by about .5% of the total. The docs for Cache say:

This structure purposefully does not implement Clone because it's intended to be a fairly large and expensive structure to clone. Instead this adheres to Send so it may be stored in a Arc instance and shared among the various rendering threads.

So I expect most of the slowdown is because there's so many more places the cache is used. I think the only place you actually need it is for primitive_locations, right? Could you change this to pass that down instead of the full cache?

Before doing that, though, it's very strange to me that you need to pass a dummy cache in places. We discussed on discord that without that change, rustdoc starts showing impls in the wrong place, like impl Add<str> for String on the str page. I don't think adding a dummy cache to hack around that is a good idea, it will just make things hard to fix later. Instead I would prefer to figure out why the behavior is broken when the cache is already filled. cc #74780, #74879.

Maybe it would be easier if you separate primitives from the cache structure, and kept primitive types as a thread-local, while moving everything else to DocContext? I think that would still allow your changes in #80883 without needing a transmute or to deal with why primitives are broken; plus it would be much less churn since you don't need to add Cache to GetDefId.

[GuillaumeGomez]

No difference (or too little to be seen).

This is not true - script-servo went up by about .5% of the total. The docs for Cache say:

This structure purposefully does not implement Clone because it's intended to be a fairly large and expensive structure to clone. Instead this adheres to Send so it may be stored in a Arc instance and shared among the various rendering threads.

So I expect most of the slowdown is because there's so many more places the cache is used. I think the only place you actually need it is for primitive_locations, right? Could you change this to pass that down instead of the full cache?

I don't see how it could change anything, we're passing a reference, not by move. Also, this is at the very bottom of call stack, meaning that we'd have primitive_locations passed down everywhere.

Before doing that, though, it's very strange to me that you need to pass a dummy cache in places. We discussed on discord that without that change, rustdoc starts showing impls in the wrong place, like impl Add<str> for String on the str page. I don't think adding a dummy cache to hack around that is a good idea, it will just make things hard to fix later. Instead I would prefer to figure out why the behavior is broken when the cache is already filled. cc #74780, #74879.

No, it's an exact replica of how it currently works: we switch the empty cache with a filled one after running fold_crate on the new cache. Before that, we need to use an empty cache, otherwise it changes the behavior. In that regard, we just make this weird behavior obvious but we don't change it.

Maybe it would be easier if you separate primitives from the cache structure, and kept primitive types as a thread-local, while moving everything else to DocContext? I think that would still allow your changes in #80883 without needing a transmute or to deal with why primitives are broken; plus it would be much less churn since you don't need to add Cache to GetDefId.

I think it'd be much better to not have any globals. In that regard, even though we have an extra parameter in a lot of places, it's at least a lot more clear. Once again: it allowed us to see that the cache was used in multiple places and that it needed to not be filled in some cases.

[GuillaumeGomez]

Updated!

[bors]

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

[GuillaumeGomez]

Rebased.

[bors]

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

[GuillaumeGomez]

Fixed conflicts.

[GuillaumeGomez]

Let's give it anther try... And let's hope that if there is an error, we'll be able to see it this time.

@bors: r=jyn514

[bors]

📌 Commit d78e1ed has been approved by jyn514

[bors]

⌛ Testing commit d78e1ed with merge 613ef74...

[bors]

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 613ef74 to master...