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

mem: use slice capacity instead of length, to determine whether to pool buffers or directly allocate them #7702

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

PapaCharlie
Copy link
Contributor

@PapaCharlie PapaCharlie commented Oct 4, 2024

Fixes #7631

As the issue states, mem.NewBuffer would not pool buffers with a length below the pooling threshold but whose capacity is actually larger than the pooling threshold. This can lead to buffers being leaked.

RELEASE NOTES:

  • mem: use slice capacity instead of length, to determine whether to pool buffers or directly allocate them

Copy link

linux-foundation-easycla bot commented Oct 4, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: PapaCharlie / name: Paul Chesnais (aed354b)
  • ✅ login: easwars / name: Easwar Swaminathan (402d56e)
  • ✅ login: purnesh42H / name: Purnesh Dixit (eeb6971)

Copy link

codecov bot commented Oct 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.32%. Comparing base (98d1550) to head (402d56e).
Report is 31 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7702      +/-   ##
==========================================
- Coverage   81.80%   81.32%   -0.49%     
==========================================
  Files         361      368       +7     
  Lines       27827    36772    +8945     
==========================================
+ Hits        22765    29906    +7141     
- Misses       3861     5636    +1775     
- Partials     1201     1230      +29     
Files with missing lines Coverage Δ
mem/buffers.go 88.18% <100.00%> (+4.34%) ⬆️

... and 322 files with indirect coverage changes

As the issue states, `mem.NewBuffer` would not pool buffers with a length below
the pooling threshold but whose capacity is actually larger than the pooling
threshold. This can lead to buffers being leaked.
Copy link
Contributor

@purnesh42H purnesh42H left a comment

Choose a reason for hiding this comment

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

lgtm

@purnesh42H
Copy link
Contributor

@dfawley @easwars for second review

@easwars easwars changed the title Address #7631 by correctly pooling large-capacity buffers mem: use slice capacity instead of length, to determine whether to pool buffers or directly allocate them Oct 8, 2024
Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

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

LGTM, modulo minor nits.

mem/buffers.go Outdated Show resolved Hide resolved
mem/buffers_test.go Outdated Show resolved Hide resolved
@easwars easwars assigned PapaCharlie and unassigned easwars and dfawley Oct 8, 2024
@easwars easwars added this to the 1.68 Release milestone Oct 8, 2024
@purnesh42H purnesh42H modified the milestones: 1.68 Release, 1.69 Release Oct 16, 2024
@purnesh42H purnesh42H merged commit f8e5d8f into grpc:master Oct 24, 2024
15 checks passed
purnesh42H added a commit to purnesh42H/grpc-go that referenced this pull request Oct 29, 2024
…ol buffers or directly allocate them (grpc#7702)

* Address grpc#7631 by correctly pooling large-capacity buffers

As the issue states, `mem.NewBuffer` would not pool buffers with a length below
the pooling threshold but whose capacity is actually larger than the pooling
threshold. This can lead to buffers being leaked.

---------

Co-authored-by: Purnesh Dixit <purneshdixit@google.com>
Co-authored-by: Easwar Swaminathan <easwars@google.com>
purnesh42H added a commit to purnesh42H/grpc-go that referenced this pull request Oct 29, 2024
…ol buffers or directly allocate them (grpc#7702)

* Address grpc#7631 by correctly pooling large-capacity buffers

As the issue states, `mem.NewBuffer` would not pool buffers with a length below
the pooling threshold but whose capacity is actually larger than the pooling
threshold. This can lead to buffers being leaked.

---------

Co-authored-by: Purnesh Dixit <purneshdixit@google.com>
Co-authored-by: Easwar Swaminathan <easwars@google.com>
purnesh42H added a commit to purnesh42H/grpc-go that referenced this pull request Oct 29, 2024
…ol buffers or directly allocate them (grpc#7702)

* Address grpc#7631 by correctly pooling large-capacity buffers

As the issue states, `mem.NewBuffer` would not pool buffers with a length below
the pooling threshold but whose capacity is actually larger than the pooling
threshold. This can lead to buffers being leaked.

---------

Co-authored-by: Purnesh Dixit <purneshdixit@google.com>
Co-authored-by: Easwar Swaminathan <easwars@google.com>
purnesh42H added a commit that referenced this pull request Oct 30, 2024
misvivek pushed a commit to misvivek/grpc-go that referenced this pull request Nov 7, 2024
…ol buffers or directly allocate them (grpc#7702)

* Address grpc#7631 by correctly pooling large-capacity buffers

As the issue states, `mem.NewBuffer` would not pool buffers with a length below
the pooling threshold but whose capacity is actually larger than the pooling
threshold. This can lead to buffers being leaked.

---------

Co-authored-by: Purnesh Dixit <purneshdixit@google.com>
Co-authored-by: Easwar Swaminathan <easwars@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mem.NewBuffer() should look at slice capacity?
4 participants