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

buffer writes when sending protobufs #182

Merged
merged 2 commits into from
Aug 6, 2018
Merged

buffer writes when sending protobufs #182

merged 2 commits into from
Aug 6, 2018

Conversation

marten-seemann
Copy link
Contributor

@marten-seemann marten-seemann commented Aug 5, 2018

The Protobuf writer performs multiple small writes when writing a message. We need to buffer these, otherwise we'll send out a packet for each Write call.

This will reduce the number of packets sent when providing records by 75%.

Verified that it works by looking at the QUIC packet log. Every DHT record is now sent in a single packet.

@ghost ghost assigned marten-seemann Aug 5, 2018
@ghost ghost added the status/in-progress In progress label Aug 5, 2018
The protobuf writer performs multiple small writes when writing a
message. We need to buffer these, otherwise we'll send out a packet for
each Write call.
Copy link
Contributor

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

LGTM, great find!

@whyrusleeping
Copy link
Contributor

also cc @bigs, dht perf :)

Copy link
Contributor

@bigs bigs left a comment

Choose a reason for hiding this comment

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

yeah, this is wonderful! thanks marten

@Stebalien Stebalien merged commit 4d49d8e into master Aug 6, 2018
@ghost ghost removed the status/in-progress In progress label Aug 6, 2018
@Stebalien Stebalien deleted the buffer-writes branch August 6, 2018 19:50
@dirkmc
Copy link
Contributor

dirkmc commented Aug 13, 2018

Great find 👍

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