Slightly optimize hash map stable hashing

[Kobzol]

I was profiling some of the rustc-perf benchmarks locally and noticed that quite some time is spent inside the stable hash of hashmaps. I tried to use a SmallVec instead of a Vec there, which helped very slightly.

Then I tried to remove the sorting, which was a bottleneck, and replaced it with insertion into a binary heap. Locally, it yielded nice improvements in instruction counts and RSS in several benchmarks for incremental builds. The implementation could probably be much nicer and possibly extended to other stable hashes, but first I wanted to test the perf impact properly.

Can I ask someone to do a perf run? Thank you!

[rust-highfive]

r? @matthewjasper

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

[jackh726]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 6fd9f81c09c27edcf702f3be7cedaa5b465a16a9 with merge 6ed7c0251b508f4b31a6b15623addde734ae2172...

[Kobzol]

Well, this is embarrassing 😅 I assumed that iterating a BinaryHeap would return the items in a sorted order (a bit of an API footgun, although I understand why it doesn't do that). And I was wondering why it was so fast.. now it probably won't be nearly as good, but let's see.

[Kobzol]

@jackh726 Ok I'm not actually sure if that last failure is caused by the PR or if it's spurious?

[jackh726]

@bors try

[bors]

⌛ Trying commit 3b9da308ebfd15a41bf400a14ab99f8a61427963 with merge 871b2837e9311a4b8b20f8ec57ee7463d1714c64...

[bors]

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

[rust-timer]

Queued 871b2837e9311a4b8b20f8ec57ee7463d1714c64 with parent 6dc08b9, future comparison URL.

[leonardo-m]

I assumed that iterating a BinaryHeap would return the items in a sorted order (a bit of an API footgun, although I understand why it doesn't do that).

No problem. Take a look at the "Heap Sort" algorithm :)

[Kobzol]

No problem. Take a look at the "Heap Sort" algorithm :)

I assumed that into_iter() would behave like into_iter_sorted. Which I didn't know existed, otherwise it would be obvious :) At least I'm not alone :D

[rust-timer]

Finished benchmarking commit (871b2837e9311a4b8b20f8ec57ee7463d1714c64): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

• Very large regression in instruction counts (up to 8.6% on incr-full builds of clap-rs)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

[Kobzol]

Sigh, back to the drawing board I guess. It would really help to make this sort faster, but my usual tricks (parallelizing the sort or using radix sort) probably can't be applied here. Let's see if the SmallVec alone helps with anything, if not, I'll close the PR.

Can someone please run another perf run? :) Thanks.

[joshtriplett]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 8031200d5bc4f4b8356a514e2b765d3ccfaa6870 with merge b66e21c62a53fcc9fd91130c8b45d414d64e0616...

[joshtriplett]

@bors r=@Mark-Simulacrum

[bors]

📌 Commit e4b4d18 has been approved by Mark-Simulacrum

[bors]

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

[michaelwoerister]

I just saw this now. Very interesting! Note that we don't actually need to "sort" the hash table contents before hashing. We just need to have them in a deterministic order. Maybe that can be exploited somehow?

[Kobzol]

Hmm, so, any idea how to produce a deterministic order without sorting? :D And without depending on the insertion order I suppose.

If all hash map insertions have the same order and the hash function is deterministic, we could probably just iterate the items of the hashmap as they are laid out in the map. But that would depend on the insertion order, which seems to be too brittle (basically this was the reason why we decided to postpone https://perf.rust-lang.org/compare.html?start=55ccbd090d96ec3bb28dbcb383e65bbfa3c293ff&end=7b55f66571824a0ba6e2ee9303d94854dcc8f378 until we make sure that the insertion order won't break anything).

[the8472]

Hmm, so, any idea how to produce a deterministic order without sorting?

An idea how to produce a stable hash without order: Use a commutative function.

The problem with that is that it requires the hash for each item to be computed separately before it can be merged with a commutative function. The initialization and finish() costs of SipHasher might be significant enough that the overhead of hashing elements individually might outweigh the gains from not having to sort. In that case some FastStableHasher would be needed.

[bors]

⌛ Testing commit e4b4d18 with merge 58457bb...

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 58457bb to master...

[rust-timer]

Finished benchmarking commit (58457bb): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

[Kobzol]

@the8472 from my benchmarks it looked like hashing and sorting takes a similar amount of time here, based on the workload. So keeping the hashing + a commutative function might theoretically work. I'll try it, thanks.