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

Improved generateChecksum performance. (4x) #83

Closed
wants to merge 3 commits into from
Closed

Conversation

kixelated
Copy link
Contributor

The old code is egregious so I had to fix it. It would make a copy of
each packet just to unset 4 bytes. It would also create a crc32 table
each time, which is fortunately cached but requires a mutex lock.

benchmark                             old ns/op     new ns/op     delta
BenchmarkPacketGenerateChecksum-8     284           68.2          -75.99%

@enobufs
Copy link
Member

enobufs commented Oct 25, 2019

Relates to #49

The old code is egregious so I had to fix it. It would make a copy of
each packet just to unset 4 bytes. It would also create a crc32 table
each time, which is fortunately cached but requires a mutex lock.

benchmark                           old ns/op   new ns/op   delta
BenchmarkPacketGenerateChecksum-8   284         68.2        -75.99%
@codecov-io
Copy link

codecov-io commented Oct 27, 2019

Codecov Report

Merging #83 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #83      +/-   ##
=========================================
- Coverage   76.91%   76.9%   -0.01%     
=========================================
  Files          43      43              
  Lines        2712    2711       -1     
=========================================
- Hits         2086    2085       -1     
  Misses        497     497              
  Partials      129     129
Impacted Files Coverage Δ
packet.go 63.41% <100%> (-0.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2fe0b1...0327fde. Read the comment docs.

@AeroNotix
Copy link
Contributor

@kixelated please update the commit message as per the format in the CI log and I'll be happy to merge!

@AeroNotix
Copy link
Contributor

@kixelated hi! Would love to merge this. Please update the commit message and I'll click that merge button.

@Sean-Der
Copy link
Member

Sean-Der commented Nov 3, 2019

Fantastic, thank you so much @kixelated merged as ced24b2

Thanks for the approve @AeroNotix I just forced pushed to the branch and fixed it up :)

@Sean-Der Sean-Der closed this Nov 3, 2019
@Sean-Der Sean-Der deleted the checksum-perf branch November 3, 2019 21:33
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.

5 participants