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

flate: Remove unneeded bounds checks (effectively one less) #439

Merged
merged 1 commit into from
Sep 16, 2021

Conversation

klauspost
Copy link
Owner

@klauspost klauspost commented Sep 16, 2021

(no measurable difference)

@orijbot
Copy link

orijbot commented Sep 16, 2021

@klauspost klauspost merged commit 61a4f62 into master Sep 16, 2021
@klauspost klauspost deleted the flate-matchlen-bounds-check branch September 16, 2021 16:17
@odeke-em
Copy link

@klauspost very interesting that despite your mention of no noticeable differences, on Linux the benchmarks show many improvements from that bounds removal per https://dashboard.github.orijtech.com/benchmark/f6cd51a3d9144c73954783b91ad4492d
image

@klauspost
Copy link
Owner Author

@odeke-em Half of these are unrelated, one is benchmarking an stdlib function and shows +6%

So to be honest I don't really trust these numbers.

Gunzip is completely unrelated.
Transport is benchmarking stdlib, nothing in this package.
DecodeRandom is decompression and unrelated.
2 x EncodeTwainConstant is unrelated.

So 5 of 9 is in unrelated code.

My own numbers of the differences above (n=1, though)

BenchmarkEncodeDigitsSpeed1e6-32        6565961       6561624       -0.07%
BenchmarkEncodeDigitsCompress1e6-32     30598651      30685776      +0.28%
BenchmarkEncodeDigitsSL1e4-32           51189         50338         -1.66%
BenchmarkEncodeTwainSpeed1e6-32         6500970       6524704       +0.37%

@klauspost
Copy link
Owner Author

My own numbers are way below the noise of my own system.

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.

3 participants