Upgrade indexmap and use it more

[cuviper]

First this upgrades indexmap to 1.5.1, which is now based on hashbrown::raw::RawTable. This means it shares a lot of the same performance characteristics for insert, lookup, etc., while keeping items in insertion order.

Then across various rustc crates, this replaces a lot of Vec+HashMap pairs with a single IndexMap or IndexSet.

Closes #60608.
r? @eddyb

[cuviper]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 0215c595e7312af32d7b0ae700685c003e7daba2 with merge 45cb4ac369e8d420fc735c61f879d661b8fd69ca...

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: 45cb4ac369e8d420fc735c61f879d661b8fd69ca (45cb4ac369e8d420fc735c61f879d661b8fd69ca)

[rust-timer]

Queued 45cb4ac369e8d420fc735c61f879d661b8fd69ca with parent f9c2177, future comparison URL.

[rust-timer]

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

[leonardo-m]

Is the memory usage lower?

[cuviper]

FWIW, I purposely separated the commits, so we can easily back any of these out if one proves troublesome.

Is the memory usage lower?

The perf results include max-rss -- looks like a mixed bag.

IndexMap is still basically a Vec+Map internally, but it only stores the keys once, which can help memory.

[Mark-Simulacrum]

max rss is usually really noisy. But those results look like a plausible small win. Usually we can only tell when looking at graphs with a few commits before/after currently (once things are on master).

@cuviper, did you expect the performance regression here? The PR description sounded like things should be better, but the results look worse (at least instruction counts). I guess we need to get a cachegrind diff going.

[cuviper]

@Mark-Simulacrum I expected existing uses of indexmap to improve with the new hashbrown-based version, and the new uses to be roughly the same, but with a better abstraction. Compare to a prior test that saw 20% worse switching one part to the old indexmap -- at least that kind of hit isn't here anymore. 🙂

The losses here are mostly in -doc, so that might be something that can be specifically targeted further, perhaps backing something out if it can be isolated. The rest look like mixed ups and downs to me.

[Mark-Simulacrum]

Local profiling indicates that reverting just the Symbol change fixes most (maybe all) of the regression on token-stream-stress-check, so I've pushed that up now, let's see if the same holds across the board: @bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 1a552da314d08a32594c3084767c0b7fd2c08b68 with merge c0b91af3cd8f045998932334ba4ed2f40fc547ca...

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: c0b91af3cd8f045998932334ba4ed2f40fc547ca (c0b91af3cd8f045998932334ba4ed2f40fc547ca)

[rust-timer]

Queued c0b91af3cd8f045998932334ba4ed2f40fc547ca with parent 8bc801b, future comparison URL.

[rust-timer]

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

[cuviper]

Docs still show ~0.5% increase in instructions across the board, but everything else looks neutral or improved.

Flipping to cpu-cycles, it looks better almost everywhere, even for docs. It's quite plausible that the new hashbrown-based indexmap would have better IPC, with its SIMD lookups and all.

The reverted Symbol might fare better with custom index types -- indexmap-rs/indexmap#147 -- since a Symbol wraps a u32 instead of usize. But even with that, it may be that indexmap's little bit of extra indirection on keys is too harmful for those simple &'static str keys. Anyway, that can be explored in future work if we do land custom index types.

[Mark-Simulacrum]

r=me with comment resolved, and ideally the revert commit squashed in or amended with explanation

[Mark-Simulacrum]

Hm this looks a bit weird, is there a reason this isn't options.clone()? Did I miss something?

[cuviper]

The values field is a Cow. So before, we had a local &Vec<u128> that could be cloned and converted, and now we have a map and have to collect its values. I guess we could directly collect a Cow though -- I'll do that.

[cuviper]

By "revert commit squashed", do you mean to effectively rebase that change out of existence? That's fine with me.

[Mark-Simulacrum]

Yeah, either rebasing it out of existence or alternatively (maybe better, not sure) leaving it in but editing the message to say why we did so.

[cuviper]

I think it may go unnoticed in the commit history -- I think I'll cut those out and add a new commit with just a comment that Symbol could use indexmap if performance is resolved.

[cuviper]

@bors r=Mark-Simulacrum

[bors]

📌 Commit ca0b89a has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit ca0b89a with merge 18f3be7...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing 18f3be7 to master...