Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Optimize for small strings to close gap with FNV #30

Closed
wants to merge 3 commits into from
Closed

RFC: Optimize for small strings to close gap with FNV #30

wants to merge 3 commits into from

Conversation

adamreichold
Copy link

Over at Tantivy, I proposed standardizing on rustc-hash by replacing the remaining usages of FNV. However, it appears FNV still beats rustc-hash for very short strings.

Hence I added a microbenchmark here exploring this which showcase this:

test few_large::fnv   ... bench:      19,621 ns/iter (+/- 120)
test few_large::fx    ... bench:       9,355 ns/iter (+/- 166)
test few_medium::fnv  ... bench:       9,356 ns/iter (+/- 98)
test few_medium::fx   ... bench:       7,898 ns/iter (+/- 115)
test few_small::fnv   ... bench:       7,300 ns/iter (+/- 60)
test few_small::fx    ... bench:       7,337 ns/iter (+/- 109)
test few_tiny::fnv    ... bench:       6,936 ns/iter (+/- 50) <- significant win for FNV
test few_tiny::fx     ... bench:       7,513 ns/iter (+/- 84)
test many_large::fnv  ... bench:   3,883,669 ns/iter (+/- 26,968)
test many_large::fx   ... bench:   2,233,341 ns/iter (+/- 14,412)
test many_medium::fnv ... bench:   2,071,061 ns/iter (+/- 14,305)
test many_medium::fx  ... bench:   1,677,142 ns/iter (+/- 10,292)
test many_small::fnv  ... bench:   1,471,003 ns/iter (+/- 25,075)
test many_small::fx   ... bench:   1,336,677 ns/iter (+/- 8,678)
test some_large::fnv  ... bench:     309,672 ns/iter (+/- 22,118)
test some_large::fx   ... bench:     196,661 ns/iter (+/- 4,512)
test some_medium::fnv ... bench:     158,861 ns/iter (+/- 2,246)
test some_medium::fx  ... bench:     151,262 ns/iter (+/- 3,039)
test some_small::fnv  ... bench:     119,939 ns/iter (+/- 3,410) <- small win for FNV
test some_small::fx   ... bench:     120,153 ns/iter (+/- 749)

I also made two changes. First using only safe code but changing the output values which does close the gap. And then using a bit of unsafe (but still passing Miri) to polyfill for slice::as_chunks. After that, we do beat FNV in the two above cases but regress in one other even we generally improve:

test few_large::fnv   ... bench:      19,846 ns/iter (+/- 174)
test few_large::fx    ... bench:       8,206 ns/iter (+/- 70)
test few_medium::fnv  ... bench:       9,318 ns/iter (+/- 94)
test few_medium::fx   ... bench:       6,917 ns/iter (+/- 71)
test few_small::fnv   ... bench:       7,288 ns/iter (+/- 69)
test few_small::fx    ... bench:       6,679 ns/iter (+/- 69)
test few_tiny::fnv    ... bench:       6,884 ns/iter (+/- 82)
test few_tiny::fx     ... bench:       6,421 ns/iter (+/- 72)
test many_large::fnv  ... bench:   3,838,407 ns/iter (+/- 67,395)
test many_large::fx   ... bench:   1,912,792 ns/iter (+/- 21,136)
test many_medium::fnv ... bench:   2,079,771 ns/iter (+/- 16,490)
test many_medium::fx  ... bench:   1,580,259 ns/iter (+/- 19,684)
test many_small::fnv  ... bench:   1,482,612 ns/iter (+/- 13,102)
test many_small::fx   ... bench:   1,634,143 ns/iter (+/- 15,159) <- we regress and loose to FNV
test some_large::fnv  ... bench:     317,275 ns/iter (+/- 20,146)
test some_large::fx   ... bench:     169,852 ns/iter (+/- 1,669)
test some_medium::fnv ... bench:     158,647 ns/iter (+/- 4,668)
test some_medium::fx  ... bench:     135,808 ns/iter (+/- 556)
test some_small::fnv  ... bench:     120,479 ns/iter (+/- 1,543)
test some_small::fx   ... bench:     115,738 ns/iter (+/- 1,121)

So, first I wanted to ask if upstream is interested in optimizing for small strings to make rustc-hash more generally useful at all. And then if breaking value stability is possible at all or whether such a change cannot be done even as a breaking change.

@Noratrieb
Copy link
Member

Noratrieb commented Jan 18, 2024

Can you make a PR to https://github.com/rust-lang/rust/ patching rustc-hash to point to your branch? I can then start a rustc benchmark, as that's the benchmark we care about. If it doesn't regress, we can move forward and I'll review it.

@adamreichold
Copy link
Author

Can you make a PR to https://github.com/rust-lang/rust/ patching rustc-hash to point to your branch? I can then start a rustc benchmark, as that's the benchmark we care about. If it doesn't regress, we can move forward and I'll review it.

This is rust-lang/rust#120093

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2024
…s, r=<try>

[DO NOT MERGE] perf: test rustc-hash variant optimized for small strings.

I would be glad if a perf run could be performed for the modification proposed in rust-lang/rustc-hash#30 which came out of FNV beating fxhash for small strings when used in Tantivy. (Note that this is not changing the structure of rustc-hash, it just tries to optimize the implementation for handling the remaining bytes below the word size which appears to dominate performance for short strings.)
…alue stability.

This merges the two rounds for 2 and 1 remaining bytes to reduce their overhead
but keeps the separate round for 4 remaining bytes to ensure sufficient hash
quality for strings of up to 7 bytes.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2024
…s, r=<try>

[DO NOT MERGE] perf: test rustc-hash variant optimized for small strings.

I would be glad if a perf run could be performed for the modification proposed in rust-lang/rustc-hash#30 which came out of FNV beating fxhash for small strings when used in Tantivy. (Note that this is not changing the structure of rustc-hash, it just tries to optimize the implementation for handling the remaining bytes below the word size which appears to dominate performance for short strings.)
@adamreichold
Copy link
Author

The rustc benchmarks seem to indicate that the extra rounds for the tail bytes are critical for quality and cannot be merged, i.e. just scheduling changes will not make fxhash competitive with FNV for three-byte strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants