Optimize SipHash by reordering compress instructions by mat-1 · Pull Request #127226 · rust-lang/rust

mat-1 · 2024-07-01T23:07:12Z

This PR optimizes hashing by changing the order of instructions in the sip.rs compress macro so the CPU can parallelize it better. The new order is taken directly from Fig 2.1 in the SipHash paper (but with the xors moved which makes it a little faster). I attempted to optimize it some more after this, but I think this might be the optimal instruction order. Note that this shouldn't change the behavior of hashing at all, only statements that don't depend on each other were reordered.

It appears like the current order hasn't changed since its original implementation from 2012 which doesn't look like it was written with data dependencies in mind.

Running ./x bench library/core --stage 0 --test-args hash before and after this change shows the following results:

Before:

benchmarks:
    hash::sip::bench_bytes_4             7.20/iter +/- 0.70
    hash::sip::bench_bytes_7             9.01/iter +/- 0.35
    hash::sip::bench_bytes_8             8.12/iter +/- 0.10
    hash::sip::bench_bytes_a_16         10.07/iter +/- 0.44
    hash::sip::bench_bytes_b_32         13.46/iter +/- 0.71
    hash::sip::bench_bytes_c_128        37.75/iter +/- 0.48
    hash::sip::bench_long_str          121.18/iter +/- 3.01
    hash::sip::bench_str_of_8_bytes     11.20/iter +/- 0.25
    hash::sip::bench_str_over_8_bytes   11.20/iter +/- 0.26
    hash::sip::bench_str_under_8_bytes   9.89/iter +/- 0.59
    hash::sip::bench_u32                 9.57/iter +/- 0.44
    hash::sip::bench_u32_keyed           6.97/iter +/- 0.10
    hash::sip::bench_u64                 8.63/iter +/- 0.07

After:

benchmarks:
    hash::sip::bench_bytes_4             6.64/iter +/- 0.14
    hash::sip::bench_bytes_7             8.19/iter +/- 0.07
    hash::sip::bench_bytes_8             8.59/iter +/- 0.68
    hash::sip::bench_bytes_a_16          9.73/iter +/- 0.49
    hash::sip::bench_bytes_b_32         12.70/iter +/- 0.06
    hash::sip::bench_bytes_c_128        32.38/iter +/- 0.20
    hash::sip::bench_long_str          102.99/iter +/- 0.82
    hash::sip::bench_str_of_8_bytes     10.71/iter +/- 0.21
    hash::sip::bench_str_over_8_bytes   11.73/iter +/- 0.17
    hash::sip::bench_str_under_8_bytes  10.33/iter +/- 0.41
    hash::sip::bench_u32                10.41/iter +/- 0.29
    hash::sip::bench_u32_keyed           9.50/iter +/- 0.30
    hash::sip::bench_u64                 8.44/iter +/- 1.09

I ran this on my computer so there's some noise, but you can tell at least bench_long_str is significantly faster (~18%).

Also, I noticed the same compress function from the library is used in the compiler as well, so I took the liberty of copy-pasting this change to there as well.

Thanks @Semisol for porting SipHash for another project which led me to notice this issue in Rust, and for helping investigate. <3

rustbot · 2024-07-01T23:07:20Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Noratrieb · 2024-07-02T06:16:25Z

the primary instructions metrics won't change, but maybe we can notice a difference in cycles or wall time. not a requirement for merging though.
@bors try @rust-timer queue

Optimize SipHash by reordering compress instructions This PR optimizes hashing by changing the order of instructions in the sip.rs `compress` macro so the CPU can parallelize it better. The new order is taken directly from Fig 2.1 in [the SipHash paper](https://eprint.iacr.org/2012/351.pdf) (but with the xors moved which makes it a little faster). I attempted to optimize it some more after this, but I think this might be the optimal instruction order. Note that this shouldn't change the behavior of hashing at all, only statements that don't depend on each other were reordered. It appears like the current order hasn't changed since its [original implementation from 2012](rust-lang@fada46c#diff-b751133c229259d7099bbbc7835324e5504b91ab1aded9464f0c48cd22e5e420R35) which doesn't look like it was written with data dependencies in mind. Running `./x bench library/core --stage 0 --test-args hash` before and after this change shows the following results: Before: ``` benchmarks: hash::sip::bench_bytes_4 7.20/iter +/- 0.70 hash::sip::bench_bytes_7 9.01/iter +/- 0.35 hash::sip::bench_bytes_8 8.12/iter +/- 0.10 hash::sip::bench_bytes_a_16 10.07/iter +/- 0.44 hash::sip::bench_bytes_b_32 13.46/iter +/- 0.71 hash::sip::bench_bytes_c_128 37.75/iter +/- 0.48 hash::sip::bench_long_str 121.18/iter +/- 3.01 hash::sip::bench_str_of_8_bytes 11.20/iter +/- 0.25 hash::sip::bench_str_over_8_bytes 11.20/iter +/- 0.26 hash::sip::bench_str_under_8_bytes 9.89/iter +/- 0.59 hash::sip::bench_u32 9.57/iter +/- 0.44 hash::sip::bench_u32_keyed 6.97/iter +/- 0.10 hash::sip::bench_u64 8.63/iter +/- 0.07 ``` After: ``` benchmarks: hash::sip::bench_bytes_4 6.64/iter +/- 0.14 hash::sip::bench_bytes_7 8.19/iter +/- 0.07 hash::sip::bench_bytes_8 8.59/iter +/- 0.68 hash::sip::bench_bytes_a_16 9.73/iter +/- 0.49 hash::sip::bench_bytes_b_32 12.70/iter +/- 0.06 hash::sip::bench_bytes_c_128 32.38/iter +/- 0.20 hash::sip::bench_long_str 102.99/iter +/- 0.82 hash::sip::bench_str_of_8_bytes 10.71/iter +/- 0.21 hash::sip::bench_str_over_8_bytes 11.73/iter +/- 0.17 hash::sip::bench_str_under_8_bytes 10.33/iter +/- 0.41 hash::sip::bench_u32 10.41/iter +/- 0.29 hash::sip::bench_u32_keyed 9.50/iter +/- 0.30 hash::sip::bench_u64 8.44/iter +/- 1.09 ``` I ran this on my computer so there's some noise, but you can tell at least `bench_long_str` is significantly faster (~18%). Also, I noticed the same compress function from the library is used in the compiler as well, so I took the liberty of copy-pasting this change to there as well. Thanks `@semisol` for porting SipHash for another project which led me to notice this issue in Rust, and for helping investigate. <3

bors · 2024-07-02T06:17:36Z

⌛ Trying commit 16fc41c with merge 3e6e884...

bors · 2024-07-02T07:59:53Z

☀️ Try build successful - checks-actions
Build commit: 3e6e884 (3e6e884f6472e4bc72eb391e99194572f2006de4)

rust-timer · 2024-07-02T09:14:26Z

Finished benchmarking commit (3e6e884): comparison URL.

Overall result: no relevant changes - no action needed

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 may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 695.108s -> 696.414s (0.19%)
Artifact size: 327.55 MiB -> 327.51 MiB (-0.01%)

lcnr · 2024-07-02T09:21:28Z

r? @nnethercote

nnethercote · 2024-07-02T10:34:43Z

@bors r+

bors · 2024-07-02T10:34:45Z

📌 Commit 16fc41c has been approved by nnethercote

It is now in the queue for this repository.

bors · 2024-07-02T10:34:46Z

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

Urgau · 2024-07-02T11:34:49Z

@mat-1 Could you also send those improvements to https://github.com/rust-lang/rustc-stable-hash, which is in the progress of becoming the new place for the stable hashing stuff (including the siphasher).

ghost · 2024-07-02T11:41:08Z

Have you compared the generated assembly? I'm surprised that the backend doesn't already perform reordering as long as it doesn't violate data dependencies.

Running ./x bench library/core --stage 0 --test-args hash before and after this change shows the following results

You'll have to be very careful with those, especially on laptop CPUs. They measure walltime, not cpu-instructions. So turbo boosts, core placement, thermal limitations etc. can all affect the results.

https://std-dev-guide.rust-lang.org/development/perf-benchmarking.html

ghost · 2024-07-02T12:01:25Z

They measure walltime, not cpu-instructions.

The amount of executed instructions should be unaffected by this change. Only the amount of cycles it takes should improve if this change does what it should do by increasing IPC.

ghost · 2024-07-02T22:14:28Z

Have you compared the generated assembly? I'm surprised that the backend doesn't already perform reordering as long as it doesn't violate data dependencies.

Running ./x bench library/core --stage 0 --test-args hash before and after this change shows the following results

You'll have to be very careful with those, especially on laptop CPUs. They measure walltime, not cpu-instructions. So turbo boosts, core placement, thermal limitations etc. can all affect the results.

https://std-dev-guide.rust-lang.org/development/perf-benchmarking.html

Yep, I put it into Godbolt earlier.
Before: https://rust.godbolt.org/z/n8T5zTTME
After: https://rust.godbolt.org/z/Gqj5Mf9ET

And here's the benchmarks run 3 more times for good measure (I ran them alternating between master and my branch):

Before

hash::sip::bench_bytes_4             7.31/iter +/- 0.66
hash::sip::bench_bytes_7             8.72/iter +/- 0.56
hash::sip::bench_bytes_8             8.10/iter +/- 0.02
hash::sip::bench_bytes_a_16         10.11/iter +/- 0.24
hash::sip::bench_bytes_b_32         13.43/iter +/- 0.15
hash::sip::bench_bytes_c_128        37.73/iter +/- 0.30
hash::sip::bench_long_str          119.19/iter +/- 0.99
hash::sip::bench_str_of_8_bytes     11.20/iter +/- 0.13
hash::sip::bench_str_over_8_bytes   11.25/iter +/- 0.13
hash::sip::bench_str_under_8_bytes   9.98/iter +/- 0.85
hash::sip::bench_u32                 8.15/iter +/- 0.08
hash::sip::bench_u32_keyed           6.97/iter +/- 0.02
hash::sip::bench_u64                 8.65/iter +/- 0.06

hash::sip::bench_bytes_4             7.26/iter +/- 1.14
hash::sip::bench_bytes_7             8.96/iter +/- 1.06
hash::sip::bench_bytes_8             8.17/iter +/- 0.08
hash::sip::bench_bytes_a_16         10.13/iter +/- 0.46
hash::sip::bench_bytes_b_32         13.44/iter +/- 0.35
hash::sip::bench_bytes_c_128        37.71/iter +/- 0.58
hash::sip::bench_long_str          119.59/iter +/- 2.80
hash::sip::bench_str_of_8_bytes     11.25/iter +/- 0.26
hash::sip::bench_str_over_8_bytes   11.25/iter +/- 0.24
hash::sip::bench_str_under_8_bytes   9.94/iter +/- 0.38
hash::sip::bench_u32                 9.62/iter +/- 0.21
hash::sip::bench_u32_keyed           6.97/iter +/- 0.05
hash::sip::bench_u64                 8.63/iter +/- 0.22

hash::sip::bench_bytes_4             7.29/iter +/- 0.61
hash::sip::bench_bytes_7             9.00/iter +/- 0.40
hash::sip::bench_bytes_8             8.12/iter +/- 0.06
hash::sip::bench_bytes_a_16         10.11/iter +/- 0.05
hash::sip::bench_bytes_b_32         13.47/iter +/- 0.16
hash::sip::bench_bytes_c_128        37.70/iter +/- 0.35
hash::sip::bench_long_str          119.37/iter +/- 0.94
hash::sip::bench_str_of_8_bytes     11.23/iter +/- 0.07
hash::sip::bench_str_over_8_bytes   11.26/iter +/- 0.07
hash::sip::bench_str_under_8_bytes   9.98/iter +/- 0.11
hash::sip::bench_u32                 9.59/iter +/- 0.07
hash::sip::bench_u32_keyed           7.00/iter +/- 0.26
hash::sip::bench_u64                 8.67/iter +/- 0.07

After

hash::sip::bench_bytes_4             6.62/iter +/- 0.10
hash::sip::bench_bytes_7             8.27/iter +/- 1.38
hash::sip::bench_bytes_8             8.75/iter +/- 0.78
hash::sip::bench_bytes_a_16          9.83/iter +/- 0.07
hash::sip::bench_bytes_b_32         12.79/iter +/- 0.06
hash::sip::bench_bytes_c_128        32.41/iter +/- 0.10
hash::sip::bench_long_str          101.21/iter +/- 0.80
hash::sip::bench_str_of_8_bytes     10.71/iter +/- 0.06
hash::sip::bench_str_over_8_bytes   11.70/iter +/- 0.12
hash::sip::bench_str_under_8_bytes  10.43/iter +/- 0.39
hash::sip::bench_u32                10.41/iter +/- 0.79
hash::sip::bench_u32_keyed           9.50/iter +/- 0.24
hash::sip::bench_u64                 8.45/iter +/- 0.91

hash::sip::bench_bytes_4             6.76/iter +/- 0.32
hash::sip::bench_bytes_7             8.20/iter +/- 0.37
hash::sip::bench_bytes_8             8.48/iter +/- 0.69
hash::sip::bench_bytes_a_16          9.75/iter +/- 0.06
hash::sip::bench_bytes_b_32         12.70/iter +/- 0.09
hash::sip::bench_bytes_c_128        32.35/iter +/- 0.87
hash::sip::bench_long_str          101.03/iter +/- 0.72
hash::sip::bench_str_of_8_bytes     10.51/iter +/- 0.08
hash::sip::bench_str_over_8_bytes   11.57/iter +/- 0.14
hash::sip::bench_str_under_8_bytes  10.51/iter +/- 0.33
hash::sip::bench_u32                 9.03/iter +/- 0.22
hash::sip::bench_u32_keyed           9.50/iter +/- 0.24
hash::sip::bench_u64                 8.59/iter +/- 0.93

hash::sip::bench_bytes_4             6.61/iter +/- 0.16
hash::sip::bench_bytes_7             8.20/iter +/- 0.06
hash::sip::bench_bytes_8             8.58/iter +/- 0.61
hash::sip::bench_bytes_a_16          9.76/iter +/- 0.08
hash::sip::bench_bytes_b_32         12.73/iter +/- 0.10
hash::sip::bench_bytes_c_128        32.33/iter +/- 0.16
hash::sip::bench_long_str          101.43/iter +/- 1.55
hash::sip::bench_str_of_8_bytes     10.51/iter +/- 0.05
hash::sip::bench_str_over_8_bytes   11.62/iter +/- 0.11
hash::sip::bench_str_under_8_bytes  10.50/iter +/- 0.30
hash::sip::bench_u32                 9.03/iter +/- 0.18
hash::sip::bench_u32_keyed           9.51/iter +/- 0.10
hash::sip::bench_u64                 8.54/iter +/- 0.57

Interestingly, setting RUSTFLAGS='-C target-cpu=native' causes them to compile to the exact same assembly, but when running the benchmarks they're slower than my optimized non-native version, I'm not sure what to do about that.

Before

    hash::sip::bench_bytes_4             7.44/iter +/- 0.11
    hash::sip::bench_bytes_7             8.28/iter +/- 0.06
    hash::sip::bench_bytes_8             8.34/iter +/- 0.06
    hash::sip::bench_bytes_a_16         10.09/iter +/- 0.02
    hash::sip::bench_bytes_b_32         13.77/iter +/- 0.10
    hash::sip::bench_bytes_c_128        35.93/iter +/- 0.15
    hash::sip::bench_long_str          110.21/iter +/- 0.80
    hash::sip::bench_str_of_8_bytes     11.38/iter +/- 0.09
    hash::sip::bench_str_over_8_bytes   12.22/iter +/- 0.09
    hash::sip::bench_str_under_8_bytes   9.78/iter +/- 0.23
    hash::sip::bench_u32                 9.74/iter +/- 0.49
    hash::sip::bench_u32_keyed           8.54/iter +/- 0.06
    hash::sip::bench_u64                 9.84/iter +/- 0.08

After

    hash::sip::bench_bytes_4             7.36/iter +/- 0.24
    hash::sip::bench_bytes_7             8.24/iter +/- 0.09
    hash::sip::bench_bytes_8             8.42/iter +/- 0.92
    hash::sip::bench_bytes_a_16         10.14/iter +/- 0.08
    hash::sip::bench_bytes_b_32         13.86/iter +/- 8.14
    hash::sip::bench_bytes_c_128        35.89/iter +/- 0.09
    hash::sip::bench_long_str          109.86/iter +/- 0.56
    hash::sip::bench_str_of_8_bytes     11.66/iter +/- 0.27
    hash::sip::bench_str_over_8_bytes   11.71/iter +/- 0.12
    hash::sip::bench_str_under_8_bytes   9.72/iter +/- 0.18
    hash::sip::bench_u32                10.14/iter +/- 0.25
    hash::sip::bench_u32_keyed           8.59/iter +/- 0.04
    hash::sip::bench_u64                 9.85/iter +/- 0.04

ghost · 2024-07-04T04:03:48Z

⌛ Testing commit 16fc41c with merge f6fa358...

ghost · 2024-07-04T06:17:50Z

☀️ Test successful - checks-actions
Approved by: nnethercote
Pushing f6fa358 to master...

ghost · 2024-07-04T07:48:33Z

Finished benchmarking commit (f6fa358): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.2%, secondary -2.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.2%	[-2.2%, -2.2%]	1
Improvements ✅ (secondary)	-2.0%	[-4.5%, -0.8%]	6
All ❌✅ (primary)	-2.2%	[-2.2%, -2.2%]	1

Cycles

Results (secondary 7.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	11.4%	[10.2%, 12.3%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-2.7%, -2.4%]	2
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 720.847s -> 740.571s (2.74%)
Artifact size: 328.33 MiB -> 328.21 MiB (-0.04%)

Optimize SipHash by reordering compress instructions

16fc41c

rustbot assigned lcnr Jul 1, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 1, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 2, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 2, 2024

rustbot assigned nnethercote and unassigned lcnr Jul 2, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 2, 2024

mat-1 mentioned this pull request Jul 2, 2024

Optimize SipHash by reordering instructions in compress rust-lang/rustc-stable-hash#7

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 4, 2024

bors merged commit f6fa358 into rust-lang:master Jul 4, 2024

rustbot added this to the 1.81.0 milestone Jul 4, 2024

Conversation

mat-1 commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jul 1, 2024

Uh oh!

Noratrieb commented Jul 2, 2024

Uh oh!

This comment has been minimized.

bors commented Jul 2, 2024

Uh oh!

bors commented Jul 2, 2024

Uh oh!

This comment has been minimized.

rust-timer commented Jul 2, 2024

Overall result: no relevant changes - no action needed

Uh oh!

lcnr commented Jul 2, 2024

Uh oh!

nnethercote commented Jul 2, 2024

Uh oh!

bors commented Jul 2, 2024

Uh oh!

bors commented Jul 2, 2024

Uh oh!

Urgau commented Jul 2, 2024

Uh oh!

ghost commented Jul 2, 2024

Uh oh!

ghost commented Jul 2, 2024

Uh oh!

ghost commented Jul 2, 2024 • edited by mat-1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jul 4, 2024

Uh oh!

ghost commented Jul 4, 2024

Uh oh!

ghost commented Jul 4, 2024

Overall result: ❌ regressions - no action needed

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

mat-1 commented Jul 1, 2024 •

edited

Loading

ghost commented Jul 2, 2024 •

edited by mat-1

Loading