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

THRIFT-5490: Use pooled buffer for THeaderTransport #2492

Merged
merged 1 commit into from Dec 17, 2021

Conversation

fishy
Copy link
Member

@fishy fishy commented Dec 16, 2021

Client: go

Instead of binding 2 buffers (read/write) to each THeaderTransport, grab
one from the pool to be used for the whole read/write, and return it
back to the pool after the read/write is done. This would help reduce
the memory footprint from idle connections.

  • Did you create an Apache Jira ticket? (not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

@fishy
Copy link
Member Author

fishy commented Dec 16, 2021

Note: similar things can be done in TFrameTransport (and maybe a few others), but I plan to do that in a follow up PR(s).

if err != nil {
return NewTTransportExceptionFromError(err)
}
if _, err := io.Copy(writer, &t.writeBuffer); err != nil {
if _, err := io.Copy(writer, t.writeBuffer); err != nil {
return NewTTransportExceptionFromError(err)
}
if err := writer.Close(); err != nil {

Choose a reason for hiding this comment

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

Curious. If we return an error on line 655 would that mean the writer is never closed? Reason why I ask is because at first glance, NewTransformWriter could potentially allocate a new zlib writer (line 221) which would never get closed. But I could just be missing something in my understand of how this works

Copy link
Member Author

Choose a reason for hiding this comment

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

it's a local variable, so it will be garbage collected later after it's no longer reachable by any code.

Copy link
Member

@dcelasun dcelasun left a comment

Choose a reason for hiding this comment

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

LGTM

@fishy
Copy link
Member Author

fishy commented Dec 17, 2021

Added a unit test to make sure that a THeaderTransport can be reused. Will merge this in after some staging test is completed.

Client: go

Instead of binding 2 buffers (read/write) to each THeaderTransport, grab
one from the pool to be used for the whole read/write, and return it
back to the pool after the read/write is done. This would help reduce
the memory footprint from idle connections.
@fishy
Copy link
Member Author

fishy commented Dec 17, 2021

not seeing issues on staging. merging.

@fishy fishy merged commit d582a86 into apache:master Dec 17, 2021
@fishy fishy deleted the go-theader-memory branch December 17, 2021 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants