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

[dbnode] ReadBits to use optimized Read path #2205

Merged
merged 19 commits into from Mar 18, 2020
Merged

Conversation

rallen090
Copy link
Collaborator

@rallen090 rallen090 commented Mar 11, 2020

What this PR does / why we need it:

The ReadBits path originally always made individual ReadByte calls for each 8 bit sequence. However, Read path is optimized to read multiple full bytes in a row if the iterator is aligned (i.e. no remaining current bits) and so we use that path for ReadBits instead now.

PRE
image

POST
image

PRE
6164           1940648 ns/op

POST
6813           1746332 ns/op

(1940648 - 1746332)/1940648 = ~10%

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:


Does this PR require updating code package or user-facing documentation?:


arnikola
arnikola previously approved these changes Mar 11, 2020
Copy link
Collaborator

@arnikola arnikola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, what kind of difference does it make?

edit: looked at the graph, seems substantial

@codecov
Copy link

codecov bot commented Mar 11, 2020

Codecov Report

Merging #2205 into master will increase coverage by <.1%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2205     +/-   ##
========================================
+ Coverage    72.4%   72.4%   +<.1%     
========================================
  Files        1022    1022             
  Lines       88865   88904     +39     
========================================
+ Hits        64386   64415     +29     
- Misses      20162   20167      +5     
- Partials     4317    4322      +5
Flag Coverage Δ
#aggregator 82% <0%> (-0.1%) ⬇️
#cluster 85.3% <0%> (ø) ⬆️
#collector 82.8% <0%> (ø) ⬆️
#dbnode 79.1% <0%> (ø) ⬆️
#m3em 59.3% <0%> (-15.1%) ⬇️
#m3ninx 77.7% <0%> (+3.3%) ⬆️
#m3nsch 19.6% <0%> (-31.5%) ⬇️
#metrics 17.5% <0%> (ø) ⬆️
#msg 74.8% <0%> (-0.2%) ⬇️
#query 68.9% <0%> (ø) ⬆️
#x 49.6% <0%> (-33.6%) ⬇️

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 72ca215...795287c. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 11, 2020

Codecov Report

Merging #2205 into master will decrease coverage by 1.7%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2205     +/-   ##
========================================
- Coverage    72.0%   70.2%   -1.8%     
========================================
  Files        1022     983     -39     
  Lines       88793   86281   -2512     
========================================
- Hits        63966   60650   -3316     
- Misses      20505   21500    +995     
+ Partials     4322    4131    -191     
Flag Coverage Δ
#aggregator 71.1% <ø> (-11.1%) ⬇️
#cluster 85.3% <ø> (ø)
#collector 82.8% <ø> (+11.4%) ⬆️
#dbnode 74.0% <100.0%> (-4.9%) ⬇️
#m3em 57.1% <ø> (-16.3%) ⬇️
#m3ninx 68.2% <ø> (-4.2%) ⬇️
#m3nsch 51.1% <ø> (ø)
#metrics 17.6% <ø> (ø)
#msg 74.9% <ø> (+1.9%) ⬆️
#query 68.4% <ø> (ø)
#x 75.9% <ø> (-7.3%) ⬇️

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 07e7df8...d2c9c98. Read the comment docs.

@arnikola arnikola dismissed their stale review March 11, 2020 20:40

re-evaluating numbers, double checking this is working as intended

@robskillington robskillington changed the title ReadBits to use optimized Read path [dbnode] ReadBits to use optimized Read path Mar 13, 2020
Copy link
Collaborator

@robskillington robskillington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rallen090 rallen090 merged commit da7ced3 into master Mar 18, 2020
@rallen090 rallen090 deleted the ra/read-bits-read-byte branch March 18, 2020 14:35
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.

None yet

3 participants