@bors try @rust-timer queue

Awaiting bors try build completion

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

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

Queued 45cb4ac369e8d420fc735c61f879d661b8fd69ca with parent f9c2177, future comparison URL.

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

Is the memory usage lower?

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.

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.

@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.

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

Awaiting bors try build completion

⌛ Trying commit 1a552da314d08a32594c3084767c0b7fd2c08b68 with merge c0b91af3cd8f045998932334ba4ed2f40fc547ca...

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

Queued c0b91af3cd8f045998932334ba4ed2f40fc547ca with parent 8bc801b, future comparison URL.

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

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.

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

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.

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.

@bors r=Mark-Simulacrum

📌 Commit ca0b89a has been approved by Mark-Simulacrum

⌛ Testing commit ca0b89a with merge 18f3be7...

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