Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Add varint reader/writer - Closes #5219 & #5221 & #5229 & #5230 #5319

Merged
merged 8 commits into from
May 13, 2020
Merged

Add varint reader/writer - Closes #5219 & #5221 & #5229 & #5230 #5319

merged 8 commits into from
May 13, 2020

Conversation

shuse2
Copy link
Collaborator

@shuse2 shuse2 commented May 12, 2020 •
edited
Loading

What was the problem?

This PR resolves #5219 , resolves #5221, resolves #5229 and #5230

How was it solved?

Add reader and writer for varint and zigzag, which are used for sint and uint

How was it tested?

  • Add test cases for each of them
writeVarInt#uint32 x 4,865,062 ops/sec ±13.33% (82 runs sampled)
writeVarInt#uint64 x 432,718 ops/sec ±13.25% (78 runs sampled)
writeSignedVarInt#sint32 x 6,388,955 ops/sec ±1.44% (87 runs sampled)
writeSignedVarInt#sint64 x 274,677 ops/sec ±0.58% (91 runs sampled)
readVarInt#uint32 x 3,069,689 ops/sec ±0.52% (92 runs sampled)
readVarInt#uint64 x 183,194 ops/sec ±5.88% (85 runs sampled)
readSignedVarInt#sint32 x 3,136,256 ops/sec ±0.63% (86 runs sampled)
readSignedVarInt#sint64 x 182,973 ops/sec ±0.42% (91 runs sampled)

@shuse2
🌱 Add varint reader/writer
2c6eb09 
🌱 Add varint and zigzag encoding and tests
@shuse2 shuse2 self-assigned this May 12, 2020
MaximeGagnebin
MaximeGagnebin suggested changes May 13, 2020
View reviewed changes
Copy link
Contributor

@MaximeGagnebin MaximeGagnebin left a comment

Choose a reason for hiding this comment

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

I am not sure that it is the responsability of the varint encoder is to check the range of the varint. I left a few comments in this regard.

pablitovicente
pablitovicente approved these changes May 13, 2020
View reviewed changes
Copy link
Contributor

@pablitovicente pablitovicente left a comment

Choose a reason for hiding this comment

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

Other than Maxime comments looks good.

MaximeGagnebin
MaximeGagnebin suggested changes May 13, 2020
View reviewed changes
Copy link
Contributor

@MaximeGagnebin MaximeGagnebin left a comment

Choose a reason for hiding this comment

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

only tiny comments in test section, and a typo

MaximeGagnebin
MaximeGagnebin approved these changes May 13, 2020
View reviewed changes
Copy link
Contributor

@MaximeGagnebin MaximeGagnebin 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 from my side.

@shuse2 shuse2 removed the request for review from nazarhussain May 13, 2020 14:08
@shuse2 shuse2 merged commit cc3ca81 into development May 13, 2020
@shuse2 shuse2 deleted the 5219-add_varint_codec branch May 13, 2020 14:09
@shuse2 shuse2 mentioned this pull request Jul 13, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MaximeGagnebin MaximeGagnebin MaximeGagnebin approved these changes

@pablitovicente pablitovicente pablitovicente approved these changes

Assignees

@shuse2 shuse2

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create signedVarInt writer Create VarInt reader Create VarInt writer
3 participants
@shuse2 @pablitovicente @MaximeGagnebin