mem: introduce `mem` package to facilitate memory reuse #7432

PapaCharlie · 2024-07-22T17:59:17Z

This package is required for #7356 but is introduced early here to allow additional features to be built on top of it. It provides a mechanism to reuse and recycle buffers to reduce GC pressure.

RELEASE NOTES:

TBD

linux-foundation-easycla · 2024-07-22T17:59:21Z

✅login: PapaCharlie / (be4d4ab)
✅login: PapaCharlie / (be4d4ab, 5624e37)
✅login: PapaCharlie / (be4d4ab, 5624e37, dace88f)
✅login: PapaCharlie / (be4d4ab, 5624e37, dace88f, 14c288c)
✅login: PapaCharlie / (be4d4ab, 5624e37, dace88f, 14c288c, f6f8b48)
✅login: PapaCharlie / (be4d4ab, 5624e37, dace88f, 14c288c, f6f8b48, 5aa0784)
✅login: PapaCharlie / (be4d4ab, 5624e37, dace88f, 14c288c, f6f8b48, 5aa0784, 2ee31ca)
✅login: PapaCharlie / (be4d4ab, 5624e37, dace88f, 14c288c, f6f8b48, 5aa0784, 2ee31ca, 3a8cac5)
✅login: PapaCharlie / (be4d4ab, 5624e37, dace88f, 14c288c, f6f8b48, 5aa0784, 2ee31ca, 3a8cac5, aaca195)
✅login: easwars / (169406f)
✅login: easwars / (169406f, ae308d4)
✅login: easwars / (169406f, ae308d4, 01ac2c6)
✅login: easwars / (169406f, ae308d4, 01ac2c6, 1261d3e)
✅login: easwars / (169406f, ae308d4, 01ac2c6, 1261d3e, 50bda24)
✅login: easwars / (169406f, ae308d4, 01ac2c6, 1261d3e, 50bda24, 4d0eaa5)

The committers listed above are authorized under a signed CLA.

codecov · 2024-07-22T18:03:54Z

Codecov Report

Attention: Patch coverage is 88.88889% with 18 lines in your changes missing coverage. Please review.

Project coverage is 81.57%. Comparing base (6fa393c) to head (aaca195).

Files	Patch %	Lines
mem/buffer_slice.go	85.93%	8 Missing and 1 partial ⚠️
mem/buffer_pool.go	90.56%	2 Missing and 3 partials ⚠️
mem/buffers.go	91.11%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7432      +/-   ##
==========================================
+ Coverage   81.38%   81.57%   +0.19%     
==========================================
  Files         354      357       +3     
  Lines       27080    27242     +162     
==========================================
+ Hits        22040    22224     +184     
+ Misses       3831     3817      -14     
+ Partials     1209     1201       -8

Files	Coverage Δ
mem/buffers.go	`91.11% <91.11%> (ø)`
mem/buffer_pool.go	`90.56% <90.56%> (ø)`
mem/buffer_slice.go	`85.93% <85.93%> (ø)`

... and 23 files with indirect coverage changes

arvindbr8 · 2024-07-22T18:22:51Z

cc: @printchard

mem/buffers_test.go

mem/buffers.go

go.mod

mem/buffers.go

mem/buffers_test.go

mem/buffer_slice.go

mem/buffer_pool.go

easwars · 2024-07-22T23:55:10Z

mem/buffer_pool.go

+	1 << 20, // 1MB
+}
+
+var defaultBufferPool = func() *atomic.Pointer[BufferPool] {


Would it be better to declare a var defaultBufferPool *atomic.Pointer[BufferPool] and initialize it in an init function?

Since it's not exported I'm not too picky either way, but I agree that might be nicer.

Ok, went ahead and changed it along with removing the atomic.

mem/buffer_pool.go

This package is required for grpc#7356 but is introduced early here to allow additional features to be built on top of it. It provides a mechanism to reuse and recycle buffers to reduce GC pressure.

easwars · 2024-07-23T18:34:30Z

@PapaCharlie : Could you please reply to individual comments with how you addressed them instead of marking them as resolved, since the latter approach means that the reviewer has to open all the resolved comments to see what that comment was in the first place and whether it was addressed to their satisfaction. Thanks.

PapaCharlie · 2024-07-23T18:36:00Z

Ah sorry about that. I did reply, but I didn't publish them since I was in review mode still.. I just published my replies

- add more tests - use got before want in tests - cleanup some docstrings - wrap comments at 80-cols

- and add tests for the BufferSlice

easwars · 2024-07-25T19:07:56Z

I've pushed a few more commits with more tests and in the process of writing them, I found a bug in the Reader and also fixed it. PTAL.

easwars · 2024-07-25T19:11:12Z

At this point, I'm mostly OK with how this package looks.

@PapaCharlie : Could you please take a look at my commits to see if you are Ok with those changes.

@dfawley : Could you make a pass. Thanks.

easwars · 2024-07-25T19:12:07Z

This one comment is still unaddressed though: #7432 (comment)

I don't have a strong opinion there, but I was wondering if one is a preferred way of doing things over the other, and if so why? Thanks.

dfawley

Lots of small things, many of which are subjective so feel free to disagree (ideally with rationale)! Also some of these things can just be cleaned up in a later PR.

dfawley · 2024-07-25T19:57:16Z

mem/buffer_pool.go

+	"sync/atomic"
+)
+
+// BufferPool is a pool of buffers that can be shared, resulting in


Nit: that can be shared and reused? The "reuse" is the part that reduces allocations, right?

dfawley · 2024-07-25T19:59:03Z

mem/buffer_pool.go

+	1 << 20, // 1MB
+}
+
+var defaultBufferPool = func() *atomic.Pointer[BufferPool] {


Since it's not exported I'm not too picky either way, but I agree that might be nicer.

dfawley · 2024-07-25T20:00:43Z

mem/buffer_pool.go

+// created with NewBufferPool that uses a set of default sizes optimized for
+// expected workflows.
+func DefaultBufferPool() BufferPool {
+	return *defaultBufferPool.Load()


Hmmm, can we make this not an atomic in production code? I.e. in production the user needs to initialize it once at init time and we never use it until after init time. Tests need to change it dynamically, but generally, users shouldn't do that. (That doesn't have to block this PR.)

dfawley · 2024-07-25T20:01:12Z

mem/buffer_pool.go

+// # Testing Only
+//
+// This function should ONLY be used for testing purposes.
+func SetDefaultBufferPoolForTesting(pool BufferPool) {


I'd like to work on moving this to internal instead to keep the API surface clean. Unless we think users need this for their own testing. As above, this PR is probably OK this way.

dfawley · 2024-07-25T20:03:31Z

mem/buffer_pool.go

+// NewBufferPool returns a BufferPool implementation that uses multiple
+// underlying pools of the given pool sizes.
+func NewBufferPool(poolSizes ...int) BufferPool {


Nit: this should probably be NewTieredBufferPool since we may want to add other BufferPool implementations in the future that work differently. E.g. NewDynamicTierBufferPool or NewSimpleBufferPool or whatever.

dfawley · 2024-07-25T20:11:26Z

mem/buffer_slice.go

+// LazyMaterialize functions like Materialize except that it writes the data to a
+// single Buffer pulled from the given BufferPool. As a special case, if the
+// input BufferSlice only actually has one Buffer, this function has nothing to
+// do and simply returns said Buffer, hence it being "lazy".


This seems more like "smart" to me than "lazy"? I think "lazy" usually implies you don't do anything until you have to. (Feel free to disagree if you have any examples where I'm wrong.)

Also, you could maybe instead make this MaterializeBuffer or something and the other one MaterializeBytes to distinguish them? I feel like the behavior of this is really just "what it should be doing", given the function signature.

I wasn't very convinced with the term "lazy" here as well and had made a comment on one of the previous PRs. See: #7356 (comment)

Changed to Concatenate. I think it makes sense it's pretty descriptive that it's creating a new big buffer if needed but leaves room for the smart case to be smart.

dfawley · 2024-07-25T20:15:19Z

mem/buffer_slice.go

+// the given BufferSlice. For example, in the context of a http.Handler, the
+// following code can be used to copy the contents of a request into a
+// BufferSlice:
+//
+//	var out mem.BufferSlice
+//	n, err := io.Copy(mem.NewWriter(&out, pool), req.Body)


Nit: I prefer to leave code out of comments and use an Example test instead.

Added an example test for this.

dfawley · 2024-07-25T20:17:10Z

mem/buffers.go

+	if free != nil {
+		b.free = func() { free(data) }
+	}


Optional: Buffer contains data, immutably, so this could just do b := &Buffer{..., free: free} and invoke it with b.data when needed.

This is actually important because the Split function changes b.data, so capturing this here avoids potential pitfalls down the line. It was initially set up that way but became too complicated to manage

dfawley · 2024-07-25T20:18:15Z

mem/buffers.go

+// to the returned Buffer are released.
+//
+// Note that the backing array of the given data is not copied.
+func NewBuffer(data []byte, free func([]byte)) *Buffer {


Nit: maybe name the parameter onFree since it's invoked when Free is called?

Sure. Do you also want me to rename the field in Buffer?

Probably would be better, but that (or both, really) can be done as a cleanup later.

mem/buffers_test.go

- fix grammar in test error string - move SetDefaultBufferPoolForTesting to the `internal` package

This PR removes the following gRPC buffer pool flags: - `--grpc-server-recv-buffer-pool` - `--grpc-client-recv-buffer-pool` These flags are no longer necessary since gRPC introduced the `mem` package in version [1.66.0](https://github.com/grpc/grpc-go/releases/tag/v1.66.0). See: - https://github.com/grpc/grpc-go/releases/tag/v1.66.0 - grpc/grpc-go#7432 - grpc/grpc-go#7356

easwars requested review from easwars and dfawley July 22, 2024 18:14

easwars self-assigned this Jul 22, 2024

easwars added the Type: Feature New features or improvements in behavior label Jul 22, 2024

easwars added this to the 1.66 Release milestone Jul 22, 2024

easwars reviewed Jul 22, 2024

View reviewed changes

mem/buffers_test.go Outdated Show resolved Hide resolved

easwars changed the title ~~Introduce mem package~~ mem: introduce mem package to facilitate memory reuse Jul 22, 2024

easwars requested changes Jul 22, 2024

View reviewed changes

easwars reviewed Jul 22, 2024

View reviewed changes

easwars requested changes Jul 23, 2024

View reviewed changes

Introduce mem package

be4d4ab

This package is required for grpc#7356 but is introduced early here to allow additional features to be built on top of it. It provides a mechanism to reuse and recycle buffers to reduce GC pressure.

PapaCharlie force-pushed the pc/mem branch from e81206d to be4d4ab Compare July 23, 2024 00:31

Address comments

5624e37

easwars assigned PapaCharlie and unassigned easwars Jul 23, 2024

easwars and others added 6 commits July 23, 2024 19:47

add a few unit tests for Buffer; fix docstring for NopBufferPool

169406f

Minor tweaks to BufferSlice.Reader

dace88f

Improve test

14c288c

more tests and minor touchups

ae308d4

- add more tests - use got before want in tests - cleanup some docstrings - wrap comments at 80-cols

fix a bug in BufferSlice.Reader

01ac2c6

- and add tests for the BufferSlice

add prefix to test names to match type

1261d3e

easwars assigned dfawley Jul 25, 2024

dfawley reviewed Jul 25, 2024

View reviewed changes

dfawley removed their assignment Jul 25, 2024

easwars and others added 7 commits July 26, 2024 00:10

handle review comments

50bda24

- fix grammar in test error string - move SetDefaultBufferPoolForTesting to the `internal` package

handle more review comments from dfawley

4d0eaa5

Merge remote-tracking branch 'upstream/master' into pc/mem

f6f8b48

Address comments

5aa0784

Rename test

2ee31ca

Rename to onFree

3a8cac5

Change to MaterializeToBuffer

aaca195

easwars approved these changes Aug 1, 2024

View reviewed changes

dfawley approved these changes Aug 1, 2024

View reviewed changes

easwars merged commit 887d908 into grpc:master Aug 1, 2024
13 checks passed

ijsong mentioned this pull request Oct 21, 2024

feat: remove gRPC buffer pool flags kakao/varlog#894

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mem: introduce `mem` package to facilitate memory reuse #7432

mem: introduce `mem` package to facilitate memory reuse #7432

PapaCharlie commented Jul 22, 2024 •

edited by easwars

Loading

linux-foundation-easycla bot commented Jul 22, 2024 •

edited

Loading

codecov bot commented Jul 22, 2024 •

edited

Loading

arvindbr8 commented Jul 22, 2024

easwars Jul 22, 2024

dfawley Jul 25, 2024

easwars Jul 26, 2024

easwars commented Jul 23, 2024

PapaCharlie commented Jul 23, 2024

easwars commented Jul 25, 2024

easwars commented Jul 25, 2024

easwars commented Jul 25, 2024

dfawley left a comment

dfawley Jul 25, 2024

easwars Jul 26, 2024

dfawley Jul 25, 2024

dfawley Jul 25, 2024

easwars Jul 26, 2024

dfawley Jul 25, 2024

easwars Jul 26, 2024

dfawley Jul 25, 2024

easwars Jul 26, 2024

dfawley Jul 25, 2024 •

edited

Loading

easwars Jul 26, 2024

PapaCharlie Aug 1, 2024

dfawley Jul 25, 2024

easwars Jul 26, 2024

dfawley Jul 25, 2024

PapaCharlie Jul 30, 2024

dfawley Jul 25, 2024

PapaCharlie Aug 1, 2024

dfawley Aug 1, 2024

mem: introduce mem package to facilitate memory reuse #7432

mem: introduce mem package to facilitate memory reuse #7432

Conversation

PapaCharlie commented Jul 22, 2024 • edited by easwars Loading

linux-foundation-easycla bot commented Jul 22, 2024 • edited Loading

codecov bot commented Jul 22, 2024 • edited Loading

Codecov Report

arvindbr8 commented Jul 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Jul 23, 2024

PapaCharlie commented Jul 23, 2024

easwars commented Jul 25, 2024

easwars commented Jul 25, 2024

easwars commented Jul 25, 2024

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mem: introduce `mem` package to facilitate memory reuse #7432

mem: introduce `mem` package to facilitate memory reuse #7432

PapaCharlie commented Jul 22, 2024 •

edited by easwars

Loading

linux-foundation-easycla bot commented Jul 22, 2024 •

edited

Loading

codecov bot commented Jul 22, 2024 •

edited

Loading

dfawley Jul 25, 2024 •

edited

Loading