fix: reduce memory held by deferred objects

[Stebalien]

bytes.Buffer will always overallocate (for performance) and we don't have a great way of predicting the correct size. So, instead, copy once when we're done.

We we're seeing 10x the memory usage expected in a network migration in Filecoin.

[ZenGround0]

Checking for understanding:

In the old code reusedBuf was overallocated compared to the data in d.Raw, i.e. capacity was greater than length. When we assign reusedBuf to the slice of Raw the over capacity is carried along with it. Then when we pass reusedBuf to bytes.NewBuffer it further overallocates more capacity than is needed by reusedBuf, amplifying the allocation further.

Is this correct?

[Stebalien]

Mostly the second part. In the case we care about, reusedBuf was always empty. But bytes.Buffer generally over-allocates for better performance.

[ZenGround0]

Checking for understanding:

In the old code the assignment of buf.Bytes() to d.Raw brought along the overallocated capacity. Now by assigning d.Raw to the append call this is no longer happening. d.Raw keeps its old capacity and the append call will only increase its capacity if the length of buf.Bytes() exceeds capacity.

Is this correct?

[Stebalien]

Ah, no. I'm keeping any overallocation from Raw (reusing the buffer) but removing any overallocation from the bytes Buffer by copying.

Note: in most cases, d.Raw will be nil. It won't be nil in some special cases where, e.g., we're re-using the same one in a loop. In that case, we don't care about a bit of overallocation as we're re-using space.

The issue with bytes.Buffer is that we're over-allocating entirely new space.

[masih]

LGTM aside from a coupe of minor comments. It would be great to add some benchmarks to this repo, specially for memory sensitive parts.

Question:
Why not

• pool Deferred,
• use bytes.Buffer in it instead of Raw []byte directly,
• and handle pool.Put within Close() error by making Deferred an io.Closer.

?

If I have not missed anything that should remove the need for copy here and make the the buffer/deferred management more efficient to manage. A user would simply borrow Deferred closers and close them off when done.

[masih]

WriteMajorTypeHeaderBuf taking buf seem to know precisely how many bytes are getting written. Ideally, we should grow the buffer before writing considering that function is called in a loop.

But doing that needs a bit more refactoring which seems to be beyond the scope of this PR. Please feel free to address this in a separate PR.

[Stebalien]

bytes.Buffer will expand by, IIRC, at least 512 bytes each time, so it shouldn't really matter.

[Stebalien]

Question:

We give Deferred to the user so the user would need to put it back in the pool (tends to be sketchy). By pooling bytes.Buffer, we can control the full lifecycle and don't need to ask the user to explicitly free it.

We could put a bytes.Buffer inside Deferred, but:

1. I wouldn't pool it (we give it to the user, they may hang on to a reference to the underlying byte slice).
2. That'll make it difficult to shrink the allocation.