[not-fixup] - Reduce memory consumption for Buffered File Writer

[ahrav]

Description:

The current buffer handling allows buffers to frequently grow larger than necessary. When writing content smaller than the buffer threshold but larger than the buffer capacity, the buffer pool no longer serves its intended purpose. This unnecessary buffer growth causes a spike in memory usage.

This PR checks the write content size against the buffer's remaining capacity. If the buffer cannot accommodate the content, instead of doubling the buffer for a small amount of data, the buffer is grown to the exact size that's needed.. This approach reduces the buffered file writer's overall memory footprint.

Analysis of logging output revealed this pattern:

DATA SIZE: 64, BUFFER SIZE: 16371, CAPACITY: 16384
DATA SIZE: 62, BUFFER SIZE: 32726, CAPACITY: 32768
DATA SIZE: 44, BUFFER SIZE: 2026, CAPACITY: 2048
DATA SIZE: 57, BUFFER SIZE: 2038, CAPACITY: 2048
DATA SIZE: 49, BUFFER SIZE: 4063, CAPACITY: 4096
DATA SIZE: 51, BUFFER SIZE: 8154, CAPACITY: 8192
DATA SIZE: 18, BUFFER SIZE: 16376, CAPACITY: 16384
DATA SIZE: 61, BUFFER SIZE: 2038, CAPACITY: 2048
DATA SIZE: 50, BUFFER SIZE: 4069, CAPACITY: 4096
DATA SIZE: 38, BUFFER SIZE: 2037, CAPACITY: 2048
DATA SIZE: 58, BUFFER SIZE: 4048, CAPACITY: 4096
DATA SIZE: 41, BUFFER SIZE: 2030, CAPACITY: 2048
DATA SIZE: 68, BUFFER SIZE: 1983, CAPACITY: 2048
DATA SIZE: 40, BUFFER SIZE: 2035, CAPACITY: 2048
DATA SIZE: 75, BUFFER SIZE: 4071, CAPACITY: 4096
DATA SIZE: 52, BUFFER SIZE: 1998, CAPACITY: 2048
DATA SIZE: 63, BUFFER SIZE: 4036, CAPACITY: 4096
DATA SIZE: 36, BUFFER SIZE: 2015, CAPACITY: 2048
DATA SIZE: 52, BUFFER SIZE: 4063, CAPACITY: 4096
DATA SIZE: 42, BUFFER SIZE: 2038, CAPACITY: 2048
DATA SIZE: 63, BUFFER SIZE: 4084, CAPACITY: 4096
DATA SIZE: 9, BUFFER SIZE: 2041, CAPACITY: 2048
DATA SIZE: 40, BUFFER SIZE: 4093, CAPACITY: 4096
DATA SIZE: 27, BUFFER SIZE: 8182, CAPACITY: 8192
DATA SIZE: 65, BUFFER SIZE: 2034, CAPACITY: 2048
DATA SIZE: 97, BUFFER SIZE: 2029, CAPACITY: 2048
DATA SIZE: 56, BUFFER SIZE: 2006, CAPACITY: 2048
DATA SIZE: 49, BUFFER SIZE: 2019, CAPACITY: 2048
DATA SIZE: 52, BUFFER SIZE: 2014, CAPACITY: 2048
DATA SIZE: 44, BUFFER SIZE: 2023, CAPACITY: 2048
DATA SIZE: 51, BUFFER SIZE: 2043, CAPACITY: 2048
DATA SIZE: 46, BUFFER SIZE: 2032, CAPACITY: 2048
DATA SIZE: 70, BUFFER SIZE: 2024, CAPACITY: 2048
DATA SIZE: 9, BUFFER SIZE: 4096, CAPACITY: 4096
DATA SIZE: 79, BUFFER SIZE: 1996, CAPACITY: 2048
DATA SIZE: 30, BUFFER SIZE: 2032, CAPACITY: 2048
DATA SIZE: 101, BUFFER SIZE: 4093, CAPACITY: 4096
DATA SIZE: 46, BUFFER SIZE: 2007, CAPACITY: 2048
DATA SIZE: 1, BUFFER SIZE: 4096, CAPACITY: 4096
DATA SIZE: 55, BUFFER SIZE: 2005, CAPACITY: 2048
DATA SIZE: 33, BUFFER SIZE: 4085, CAPACITY: 4096
DATA SIZE: 64, BUFFER SIZE: 2025, CAPACITY: 2048
DATA SIZE: 47, BUFFER SIZE: 4077, CAPACITY: 4096

In all cases, the buffer was doubled despite needing only a small increase to accommodate the write content.

Profiles before and after this change:

Additionally, some performance profiles:

Checklist:

• Tests passing (make test-community)?
• Lint passing (make lint this requires golangci-lint)?

[rosecodym]

Why is this a bufferPool method? The receiver's not involved and I can't think of a reason it needs to be.

[ahrav]

I have a following PR that adds some metrics to this method, so I figured by adding it as a method made sense:
Ex;

func (bp *bufferPool) growBufferWithSize(buf *bufferWrapper, size int) {
	// Grow the buffer to accommodate the new data.
	bp.metrics.recordGrowth(size)
	buf.Grow(size)
}

Maybe it's not worth it since it's confusing and i'll handle the metrics recording inline.

[mcastorina]

I think that makes sense, however, from the POV of treating a PR as an individual change, I agree that it should be inlined here, and the followup PR can break it out into a helper function/method if needed.

[ahrav]

done.

[rosecodym]

I understand that the purpose of this change is to save memory, but it's doing so by losing the guaranteed (amortized) O(1) write time. This is probably fine (since it's all "constant" anyway, given our hard threshold) but I think it would be good to explicitly acknowledge it in a comment somewhere so that some future maintainer doesn't see it and get concerned.

[ahrav]

Could you elaborate a bit more on this? My understanding was that without this growth the underlying buffer was going to have to be grown anyways. The only difference being we are controlling the growth instead of doubling the growth by default. If we don't grow the buffer here, the default behavior would be w.Write would allocate a new buffer with twice the cap and copy the current contents into the new buffer.

I might be missing something that feels important in your comment.

[ahrav]

nvm i ripped it out.

[mcastorina]

These detector changes seem irrelevant to this PR - is this other work leaking in?

[ahrav]

forgot to rebase

[mcastorina]

I think that makes sense, however, from the POV of treating a PR as an individual change, I agree that it should be inlined here, and the followup PR can break it out into a helper function/method if needed.

[mcastorina]

totalSizeNeeded is bufferLength + len(data), so is growSize equivalent to len(data)?

That seems wrong, or I'm misunderstanding what growSize means.

[ahrav]

No, the growSize is the length of the current buffer + the amount of data needed to be written to the buffer. It's the size the buffer needs to be to handle all the existing data and any new data. If you call Grow with a size that is less than less than the remainder of the buffer nothing happens. This is why you can't do something linke totalSizeNeeded - w.buf.Cap() you need to use the length for it to grow correctly.

[rosecodym]

I had to read this part like four times as well. I was going to complain until I noticed that that the old code was equally confusing so I decided to just blame the buffer interface.

[ahrav]

Yea I am ashamed to say I spent WAY too long trying to fully grok the buffer interface... there are definitely some gotchas that took me way way too long to comprehend. 😞

[rosecodym]

Don't be ashamed! Intuitive API design is difficult and sometimes impossible. Some stuff is just hard to understand.

[rosecodym]

Is this change now just ensuring that all buffers are at a particular minimum size?

[rosecodym]

What's the effect of this change?

[ahrav]

Should be identical exepct we use CopyBuffer elsewhere in the codebase so I was trying to be consistent.

// CopyBuffer is identical to Copy except that it stages through the
// provided buffer (if one is required) rather than allocating a
// temporary on

[ahrav]

Is this change now just ensuring that all buffers are at a particular minimum size?

Correct, an few optimizations around copying buffers.

[ahrav]

Small cleanup here.

[rosecodym]

What are the optimizations around copying buffers? I can see the one where we drop large buffers upon return instead of reusing them, but are there others?

[ahrav]

What are the optimizations around copying buffers? I can see the one where we drop large buffers upon return instead of reusing them, but are there others?

ah, just the use of WriteTo in the two places. it's supposed to be a bit more performant.