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

Optimizations for encoding and decoding #46

Merged
merged 13 commits into from
Oct 19, 2018
Merged

Optimizations for encoding and decoding #46

merged 13 commits into from
Oct 19, 2018

Conversation

gbaz
Copy link
Collaborator

@gbaz gbaz commented Oct 18, 2018

Supersedes #45 now. Includes changes to improve decoding and encoding.

This replaces decodeWire with a function that does direct parsing instead of via the cereal library. The most important element is that it also replaces the varint128 decoding with a fully unrolled version, which should greatly improve performance.

On top of that we also swap to use IntMap throughout and get rid of the Seqs for lists, which should also cut some modest overhead.

On encodings, we use unpacked constructors for the Builder which is a pair of a length and bytestring builder, and also unroll the encoding of varint128s for some modest gain.

@gbaz gbaz mentioned this pull request Oct 18, 2018
@gbaz gbaz changed the title Optimizations Optimizations for encoding and decoding Oct 18, 2018
@gbaz
Copy link
Collaborator Author

gbaz commented Oct 18, 2018

All the decode stuff has been through review. Relevant tests are passing. After we're confident in the encoding stuff I'd like to merge.

Copy link
Collaborator

@j6carey j6carey left a comment

Choose a reason for hiding this comment

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

I looked at the encoding logic (which I know best) and some parts of the decoding logic (with which I am less familiar). I have some suggestions. The main thing is I'd like a unit test of the unrolled encode/decode.

src/Proto3/Wire/Builder.hs Show resolved Hide resolved
src/Proto3/Wire/Builder.hs Outdated Show resolved Hide resolved
src/Proto3/Wire/Builder.hs Outdated Show resolved Hide resolved
src/Proto3/Wire/Builder.hs Outdated Show resolved Hide resolved
src/Proto3/Wire/Builder.hs Outdated Show resolved Hide resolved
src/Proto3/Wire/Decode.hs Outdated Show resolved Hide resolved
src/Proto3/Wire/Decode.hs Show resolved Hide resolved
@gbaz gbaz merged commit 3885014 into master Oct 19, 2018
Copy link
Collaborator

@j6carey j6carey left a comment

Choose a reason for hiding this comment

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

Thanks for those edits; looks good to me (though again I'm less familiar with the decoding side of things).

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

2 participants