Speed up DefaultHasher, SipHasher, and SipHasher13.

[nnethercote]

This PR applies the speedups to SipHasher128 from #68914 to the Sip hashers in libcore, and also adds the missing write_* methods required so that they can benefit from the speedups. Default hashing of integers is now something like 2.5x faster, and default hash tables should be more competitive with hash tables from the fxhash crate.

r? @michaelwoerister

[nnethercote]

This should have negligible effect on rustc's own performance, because rustc uses fxhash everywhere. But let's check, just to make sure.

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 66a83ae9f40856e3e4a21c3e3ea2605fca7dbd44 with merge 22f6b02ddd18fb74ee6551aea9a7dde4607f3cd8...

[nnethercote]

Rust's Sip hashing is notorious for being slow, hence the existence of FnvHash{Map,Set} and FxHash{Map,Set}. This change is worth mentioning in the release notes. Is there a way to mark it for release note consideration?

[bors]

☀️ Try build successful - checks-azure
Build commit: 22f6b02ddd18fb74ee6551aea9a7dde4607f3cd8 (22f6b02ddd18fb74ee6551aea9a7dde4607f3cd8)

[rust-timer]

Queued 22f6b02ddd18fb74ee6551aea9a7dde4607f3cd8 with parent 1010408, future comparison URL.

[nnethercote]

We have four benchmarks with significant regressions.

regex-opt
avg: 2.2% min: -0.1% max: 4.7%
webrender-wrench-opt
avg: 1.5% min: 0.2% max: 3.3%
cargo-opt
avg: 1.2% min: -0.1% max: 2.0%
tokio-webpush-simple-opt
avg: 1.0% min: -0.0% max: 1.6%

They are all opt builds, which suggests that code generation is the problem. (check results all show tiny improvements, mostly -0.1% to -0.2%.)

I did a diff with Cachegrind on regex-opt-clean, which showed the largest regression, and it confirms that the extra time is entirely due to LLVM code. I tested just the first commit and it doesn't cause the regression; this is unsurprising because it only changes short_write but short_write isn't actually used until the second commit. The second commit adds all the missing write_xyz methods, presumably their presence is causing more code to be generated. I tried removing the #[inline] from short_write, but unfortunately that didn't help.

[nnethercote]

I tried a bunch of things such as de-inlining, removing write_* methods in various places, and even implemented sip hashing directly in DefaultHasher rather than it relying on SipHasher13, but at best I could only get rid of a bit less than half of the regression (using regex-opt-clean as the benchmark of choice). Hmm.

[michaelwoerister]

@nnethercote Thanks for the PR!

I can't really r+ things in the standard library. Would you mind splitting out the SipHasher128 parts and ask @rust-lang/libs to review the rest?

[Amanieu]

From the libs perspective, as long as SipHasher continues to return the same output (given a fixed seed) then there should be no problem. Maybe we could add some tests for that?

[michaelwoerister]

The official test vectors would be great.

[nnethercote]

I have split out the SipHasher128 changes in #69332.

We have a decision to make about what to do with the rest of the PR. I see three options.

• Simply remove sip::Hasher::short_write and sip::Hasher::write_{u8,usize} because they're dead code. And probably add a comment about that.

• Keep sip::Hasher::short_write, change it to match SipHasher128::short_write, and add a comment about it being unused due to the lack of write_* methods in the various other hashers. This is a bit silly, but would at least move sip::Hasher and SipHasher128 closer together.

• Do the full change: improve sip::Hasher::short_write and also add all the missing write_* methods. This will benefit every Rust program that uses HashMap/HashSet with default hashing (or DefaultHasher directly) at a cost of making compilation slower for a small fraction of programs. I can see reasonable people coming down on either side of this trade-off.

With respect to the final point, I did a local run and got better results than the CI run:

regex-opt
avg: 2.7% min: -0.1% max: 5.9%
webrender-wrench-opt
avg: 1.4% min: 0.3% max: 3.2%
cargo-opt
avg: 1.4% min: -0.1% max: 2.4%
issue-46449-debug
avg: -1.8% min: -2.1% max: -0.2%
tokio-webpush-simple-opt
avg: 1.3% min: -0.0% max: 2.0%
webrender-debug
avg: -1.0% min: -1.9% max: -0.1%
regression-31157-debug
avg: -1.0% min: -1.9% max: -0.1%
cargo-debug
avg: -1.3% min: -1.8% max: -0.1%
syn-debug
avg: -1.0% min: -1.7% max: -0.1%
coercions-debug
avg: -0.2%? min: -1.3%? max: 1.7%?
piston-image-debug
avg: -0.8% min: -1.5% max: -0.1%
deeply-nested-debug
avg: -1.0% min: -1.4% max: -0.2%
regex-debug
avg: -0.9% min: -1.4% max: -0.1%
cranelift-codegen-debug
avg: -0.9% min: -1.4% max: -0.1%
clap-rs-debug
avg: -0.9% min: -1.3% max: -0.0%
hyper-2-debug
avg: -0.9% min: -1.3% max: -0.2%
encoding-debug
avg: -0.7% min: -1.3% max: -0.1%
tokio-webpush-simple-debug
avg: -0.6% min: -1.0% max: 0.3%
ctfe-stress-4-opt
avg: -0.5%? min: -1.0%? max: -0.0%?
serde-serde_derive-debug
avg: -0.6% min: -0.9% max: -0.0%
ripgrep-debug
avg: -0.4% min: -0.9% max: -0.0%
token-stream-stress-check
avg: -0.9% min: -0.9% max: -0.9%
webrender-wrench-debug
avg: -0.3% min: -0.8% max: 0.2%
clap-rs-opt
avg: 0.2% min: -0.2% max: 0.8%
inflate-debug
avg: -0.5% min: -0.7% max: -0.1%
webrender-opt
avg: -0.4% min: -0.7% max: -0.1%
deep-vector-debug
avg: -0.5% min: -0.6% max: -0.3%
cranelift-codegen-opt
avg: -0.5% min: -0.6% max: -0.1%
futures-debug
avg: -0.4% min: -0.6% max: -0.1%
html5ever-debug
avg: -0.4% min: -0.6% max: -0.1%
encoding-opt
avg: -0.3% min: -0.5% max: -0.1%
token-stream-stress-debug
avg: -0.4% min: -0.4% max: -0.4%
token-stream-stress-opt
avg: -0.4% min: -0.4% max: -0.3%
await-call-tree-debug
avg: -0.4% min: -0.4% max: -0.2%
helloworld-check
avg: -0.3% min: -0.4% max: -0.2%
html5ever-opt
avg: -0.2% min: -0.4% max: -0.0%

The worst regressions are bigger than the best improvements, but there are quite a few more improvements than regressions. I looked at issue-46449-debug-check which had the best improvement, and again it was all differences in the LLVM back-end, which makes sense. The exact effect on compile times is going to vary a lot depending on the vagaries of inlining, and may end up being a wash.

Another interesting case is token-stream-stress-check-clean, which gets a 0.9% improvement due to the use of DefaultHasher somewhere (not sure where, maybe librustc_session?).

I'm leaning towards the third option (full PR) but I'm happy to hear other opinions.

cc @rust-lang/wg-compiler-performance

[nikic]

@nnethercote As this is trading off worse compile-time performance for better run-time performance, it would be nice to have some benchmark of what the actual run-time performance improvement would look like.

[nnethercote]

it would be nice to have some benchmark of what the actual run-time performance improvement would look like.

That will depend heavily on the code in question and how much it uses DefaultHasher/HashMap/HashSet with integers. (E.g. rustc itself sees very little speedup because it barely uses those.) Microbenchmarks show that integer hashing is about 2.5x faster, though.

[nnethercote]

I did some perf comparisons with rustc.

• rust0: normal rustc
• rust1: rust0 with all the FxHasher uses changed to DefaultHasher.
• rust2: rust1 with this PR's improvements applied.

rust1 is massively slower than rust0. On check-clean runs of all benchmarks, slowdowns ranged from 4.3% to 128.6%.

rust2 is significantly faster than rust1. On check-clean runs of all benchmarks, speed-ups ranged from 0.9% to 19.6%. Not nearly enough to gain back the losses going from rust0 to rust1, but it shows the potential benefits to any Rust program that uses DefaultHasher heavily, as rust1 does.

[nnethercote]

I rebased. Let's measure again, see if the perf results vary.

@bors try @rust-timer queue

[bors]

⌛ Trying commit 96c4e42 with merge 3fd2ff0fe14703eb6d09f9596264ddebb2b4b86d...

[bors]

☀️ Try build successful - checks-azure
Build commit: 3fd2ff0fe14703eb6d09f9596264ddebb2b4b86d (3fd2ff0fe14703eb6d09f9596264ddebb2b4b86d)

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 96c4e42 with merge d4aac45b9c1492286b3fd245a27eb35cbd66a89f...

[bors]

☀️ Try build successful - checks-azure
Build commit: d4aac45b9c1492286b3fd245a27eb35cbd66a89f (d4aac45b9c1492286b3fd245a27eb35cbd66a89f)

[rust-timer]

Queued d4aac45b9c1492286b3fd245a27eb35cbd66a89f with parent 79cd224, future comparison URL.

[nnethercote]

The latest CI perf run is slightly worse than the first one, with small regressions (~1%) on more of the benchmarks.

[nnethercote]

We have a decision to make about what to do with the rest of the PR. I see three options.

* Simply remove `sip::Hasher::short_write` and `sip::Hasher::write_{u8,usize}` because they're dead code. And probably add a comment about that.

I have created #69471 for this option. I have been having trouble deciding between option 1 and option 3, so I ended up settling on option 1 because it preserves existing behaviour. (I did write a comment explaining the runtime perf win that has been given up.)

[Dylan-DPC-zz]

Marking this as blocked on #69471

[nnethercote]

I think we can just close this in favour of #69471.