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.

Sign up for GitHub

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

Jump to bottom

kafka.writeBuffer #318

Merged
merged 2 commits into from
Jul 22, 2019
Merged

kafka.writeBuffer #318

merged 2 commits into from
Jul 22, 2019

Conversation

achille-roussel
Copy link
Contributor

@achille-roussel achille-roussel commented Jul 22, 2019 •
edited
Loading

This PR refactors the write paths to remove the intermediary buffer that we used to compute checksums of record batches. These temporary buffers would end up being really large as they would have to hold the entire batch, depending on the buffering strategy this could end up being multiple MB of memory allocated on each write to kafka.

Memory footprint on writes should be greatly reduced by this change:

benchmark               old ns/op     new ns/op     delta
BenchmarkConn/Write     1026737       1004170       -2.20%

benchmark               old MB/s     new MB/s     speedup
BenchmarkConn/Write     9.74         9.96         1.02x

benchmark               old allocs     new allocs     delta
BenchmarkConn/Write     10             7              -30.00%

benchmark               old bytes     new bytes     delta
BenchmarkConn/Write     14738         322           -97.82%

Varint encoding is also much faster now that we work on a local buffer:

benchmark                old ns/op     new ns/op     delta
BenchmarkWriteVarInt     34.5          9.70          -71.88%
BenchmarkReadVarInt      50.1          47.9          -4.39%

benchmark                old allocs     new allocs     delta
BenchmarkWriteVarInt     0              0              +0.00%
BenchmarkReadVarInt      0              0              +0.00%

benchmark                old bytes     new bytes     delta
BenchmarkWriteVarInt     0             0             +0.00%
BenchmarkReadVarInt      0             0             +0.00%

@achille-roussel achille-roussel requested review from stevevls, Pryz and rjenkins and removed request for stevevls July 22, 2019 18:56
rjenkins
rjenkins approved these changes Jul 22, 2019
View reviewed changes
Copy link
Contributor

@rjenkins rjenkins left a comment

Choose a reason for hiding this comment

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

lgtm!

stevevls
stevevls approved these changes Jul 22, 2019
View reviewed changes
Copy link
Contributor

@stevevls stevevls left a comment

Choose a reason for hiding this comment

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

Nice find.

@achille-roussel
Copy link
Contributor Author

Thanks for the reviews!

@achille-roussel achille-roussel merged commit ad7818b into master Jul 22, 2019
@achille-roussel achille-roussel deleted the write-buffer branch July 22, 2019 23:40
This was referenced Jul 23, 2019
Merged
Closed
@sagarkrkv sagarkrkv mentioned this pull request Aug 1, 2019
Closed
@sagarkrkv sagarkrkv mentioned this pull request Sep 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjenkins rjenkins rjenkins approved these changes

@stevevls stevevls stevevls approved these changes

@Pryz Pryz Awaiting requested review from Pryz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@achille-roussel @stevevls @rjenkins