Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Use sync.Pool to share per-connection transport write buffer. #6309
Use sync.Pool to share per-connection transport write buffer. #6309
Changes from 16 commits
0f1058d
63f4ba3
4faff38
f594b29
ec75cb1
95bde71
a243c24
a7ededc
16345bb
c8036d0
09407f4
0b42642
e1bdb20
9b97327
d274f35
00e8de7
9aeb148
195a467
82a400e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check failure on line 142 in dialoptions.go
GitHub Actions / tests (vet, 1.20)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the significance of the
* 2
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC this matches the legacy behavior, which always doubled for reasons nobody knows but we don't want to change now due to it affecting existing applications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. Do you mind expanding on the legacy behavior you are describing? It seems like if there is no shared buffer pool, the buffer is allocated per the input size. It only seems to be in the case of the buffer pool that we're allocating 2x the size (and this seems to be the PR the write buffer pool was added)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may be right.. The old code multiplied by 2 when doing this allocation, so it seems we have changed the behavior for legacy users already:
https://github.com/grpc/grpc-go/blame/d524b409462c601ef3f05a7e1fba19755a337c77/internal/transport/http_util.go#L321
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - it just felt somewhat bizarre because this is the very PR that also removed that same
* 2
in the default path (see line 321 above), but added the* 2
for the buffer pool path.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think it's worth updating, I created #6983