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.

Sign up for GitHub

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

Jump to bottom

Split test=Speed into SpeedBulk and SpeedSmall and report weighted average for Small key speed test #293

Merged
merged 4 commits into from
Sep 28, 2024
Merged

Split test=Speed into SpeedBulk and SpeedSmall and report weighted average for Small key speed test #293

merged 4 commits into from
Sep 28, 2024

Conversation

darkk
Copy link
Contributor

@darkk darkk commented Aug 30, 2024

I hope, that also addresses the goal that @wangyi-fudan had in #113 while being way more "stable" in computational terms.

It gives results looking like that:

$ ./SMHasher --test=SpeedSmall --extra SipHash
--- Testing SipHash "SipHash 2-4 - SSSE3 optimized" GOOD
...
Small key speed test -   64-byte keys -   231.26 cycles/hash
Average                                   151.602 cycles/hash
Average DNS query (| 99.8% of query.log)  123.685 cycles/hash
Average DNS name (| 99.4% of top-1m.csv)  118.621 cycles/hash
Average DNS name (| 99.6% of 200MiB.zone) 114.610 cycles/hash
Average UMASH (| 70.0% of startup-1M.bz2) 139.403 cycles/hash

or

$ SMHASHER_SMALLKEY_MAX=256 ./SMHasher --test=SpeedSmall --extra SipHash
--- Testing SipHash "SipHash 2-4 - SSSE3 optimized" GOOD
...
Small key speed test -  255-byte keys -   688.32 cycles/hash
Average                                   374.459 cycles/hash
Average DNS query (|100.0% of query.log)  124.329 cycles/hash
Average DNS name (|100.0% of top-1m.csv)  120.263 cycles/hash
Average DNS name (|100.0% of 200MiB.zone) 115.890 cycles/hash
Average UMASH (| 99.9% of startup-1M.bz2) 192.407 cycles/hash

Possible improvements are:

  • computed distributions instead of tabulated ones might reduce size of SMHasher binary a bit, but 8 KiB of doubles is not that much anyway
  • fit a linear model to extrapolate timings for keys having length greater than N instead of running a test, but I'm not that confident that it's useful unless we have real-world dataset with key-lengths being somewhat large

rurban
rurban requested changes Aug 30, 2024
View reviewed changes
main.cpp Outdated Show resolved Hide resolved
@darkk darkk requested a review from rurban August 30, 2024 21:07
@darkk darkk force-pushed the master branch 2 times, most recently from 8cc0101 to 4cd75f8 Compare September 2, 2024 18:46
@darkk
Copy link
Contributor Author

darkk commented Sep 4, 2024

macOS build is, indeed, fixed, but it highlights verification fails:

1:            superfast - Verification value 0x0C80403A ....... FAIL! (Expected 0x6306A6FE)
1:             nmhash32 - Verification value 0x9905BCB1 ....... FAIL! (Expected 0x12A30553)
1:            nmhash32x - Verification value 0x74B572DA ....... FAIL! (Expected 0xA8580227)
1:            k-hashv32 - Verification value 0x9FBDD2B1 ....... FAIL! (Expected 0xB69DF8EB)
1:            k-hashv64 - Verification value 0x70E67C61 ....... FAIL! (Expected 0xA6B7E55B)
1:              polymur - Verification value 0xA612032C ....... FAIL! (Expected 0x4F894810)

Similar story happened to MSVC=1, PLATFORM=x86, it fails following verification values:

              FNV128 - Verification value 0x000000BA ....... FAIL! (Expected 0xBCAA1426)
             polymur - Verification value 0xA612032C ....... FAIL! (Expected 0x4F894810)

@darkk
Copy link
Contributor Author

darkk commented Sep 5, 2024

@rurban please, tell me, should I do anything to re-request review of this PR explicitly or are you just busy and I should just wait?

@rurban
Copy link
Owner

rurban commented Sep 5, 2024 via email

@darkk
Copy link
Contributor Author

darkk commented Sep 5, 2024

👌 Got you. Thanks for your reply.

Constant arrays are currently gone, I've dropped them all as curating a "representative" dataset is a whole different story.

I replaced those constant arrays with SMHASHER_SMALLKEY_WEIGHTS environment variable that is a list of space-separated weights for test=SpeedSmall and the variable has no default value at this moment.

As far as I understand, you're okay with some hard-coded weights examples, so I'll add them as a separate file with references to data sources and will make one of them a default value for SMHASHER_SMALLKEY_WEIGHTS.

@darkk darkk marked this pull request as draft September 5, 2024 15:00
@darkk
Speedup timer_mips() 2.7x times with CLOCK_MONOTONIC_COARSE
2ace13b 
It reduces timer_mips() overhead by ≈385 ticks, from ≈613 to ≈228
as measured by "timer resolution" warning.

CLOCK_MONOTONIC_COARSE works fine as timer_mips() needs precision of 1s
and the lowest one for CLOCK_MONOTONIC_COARSE is 10ms in case of HZ=100.
@darkk
Fix macOS build with Xcode 15.4 and -arch arm64, see also 8a384af
72e22d7 
Fixes rurban#284 by disabling PMP_Multilinear not only on aarch64, but also
on arm64. Those are the same things under different labels.
@darkk
Split Speed into SpeedBulk and SpeedSmall, add ENV{SMHASHER_SMALLKEY_…
139a0f8 
…MAX}

It adds SMHASHER_SMALLKEY_MAX environment variable to override default
value of the longest "Small key" for hash and changes default value from
31 to 32 to make the Average a bit more fair to the hashes reading the
memory word-by-word (dword, qword) and not byte-by-byte.

SMHASHER_SMALLKEY_MIN is also added as a counterpart to benchmark hashes
when the range of small key lengths is known.
@rurban
Copy link
Owner

rurban commented Sep 6, 2024

Sounds better, thanks

@darkk
Add weighted average to SpeedSmall report
296cb0c 
Weights coming from two datasets are hard-coded: DNS domain lengths and
UMASH traces.  Custom one might be passed via ENV{SMHASHER_SMALLKEY_WEIGHTS}

It partly addresses the question at rurban#113

What is the "real" average cycles/hash value for a given hash function?

We can't know, but we can estimate it better if we assume that the
function timing does not depend on input (that's not true for hashes
based on multiplication) and we know distribution of key length in
advance (that might be somewhat known for certain classes of inputs,
but the distribution varies across classes measurably).
@darkk
Copy link
Contributor Author

darkk commented Sep 6, 2024

I've found a "representative" data source for DNS domain lengths sample, so I've added it and UMASH distribution to SpeedTest.cpp.

I agree, that these data-points do not belong to main.cpp at all. I think, SpeedTest.cpp is a good place for the numbers, but if it's not, tell me, I'll move them to separate SpeedSmallReport.cpp.

@darkk darkk marked this pull request as ready for review September 6, 2024 12:58
@rurban rurban self-assigned this Sep 6, 2024
@rurban
Copy link
Owner

rurban commented Sep 6, 2024 •
edited
Loading

Looks good now, I think. Will test it tomorrow. Esp the failing verifications look troublesome

This was referenced Sep 11, 2024
Open
Closed
@darkk
Copy link
Contributor Author

darkk commented Sep 11, 2024

the failing verifications look troublesome

That's true. However, they were previously hidden by corresponding builds failing altogether, so I suggest to treat them as separate issues: #294 #295 and #296

@darkk
Copy link
Contributor Author

darkk commented Sep 27, 2024

@rurban I've fixed all the issues GitHub CI highlighted in the following patch set: darkk/smhasher@master...macos

Should I push those patches to this PR or should it be a separate one?

@rurban
Copy link
Owner

rurban commented Sep 27, 2024

Seperate please

@darkk darkk mentioned this pull request Sep 27, 2024
Merged
@darkk darkk marked this pull request as draft September 27, 2024 15:17
@rurban rurban marked this pull request as ready for review September 28, 2024 17:21
@rurban rurban merged commit 850e1bd into rurban:master Sep 28, 2024
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rurban rurban Awaiting requested review from rurban

Assignees

@rurban rurban

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@darkk @rurban