Don't lock up 256KiB buffers when adding small files

[Stebalien]

This is part of #4505.

It's WIP because we still need to use some form of buffer pool. As is, allocating and throwing away 256KiB buffers is killing GC.

[Stebalien]

So, this uses the buffer pool from go-msgio. Should we break that into a separate package? It's used all over the place.

I'd use a chunker-specific buffer pool but we allow different sized chunkers...

[kevina]

This might cause a slight slow down due to the extra copying, especially when the buffer is almost pull, but in practice I don't think this will be a problem.

Looks like the P.R. was updated when I wrote this. Need to have another look.

[whyrusleeping]

It would be great if we could figure out a way to put all these buffers back into the buffer pool in the common case

[kevina]

( @Stebalien for future reference, I find it annoying when people do a rebase that makes a major change. (In this case using the mpool from go-msgio). At least I would appreciate it if you just add new commits and save rebasing for fixups. )

[Stebalien]

It would be great if we could figure out a way to put all these buffers back into the buffer pool in the common case.

That would be nice but would require some form of refcounting on blocks (at least given the current interfaces).

( @Stebalien for future reference, I find it annoying when people do a rebase that makes a major change. (In this case using the mpool from go-msgio). At least I would appreciate it if you just add new commits and save rebasing for fixups. )

The new commit completely changed my approach so I figured I'd just do it over. I could have opened a new PR but I figured nobody had had looked at this one yet (that's when I start trying to preserve history). Sorry about that.

[kevina]

@Stebalien How is using a memory pool helping here? In the original code before the rebase you reused the 256Kib buffer unless the buffer was full. You are doing the same basic thing here. I don't think it is worth the extra dependency unless we provide (and use) a method to return the returned byte to the pool.

[kevina]

More to the point, it looks like you are just using the mpool as a better allocator, is the default go allocator so bad that it helps to the a custom run?

Or is it the case that NewSizeSplitter is called for each new file, in which case I withdraw my objection.

No Longer Applies.

[Stebalien]

Or is it the case that NewSizeSplitter is called for each new file, in which case I withdraw my objection.

It is. I'm using it for the case where we add a bunch of small files. Without this, memory usage is still unstable because the GC lags a bit.

The alternative is to re-use the splitter somehow. I toyed with a reset function but that would be a more invasive change.

[kevina]

It is. I'm using it for the case where we add a bunch of small files. Without this, memory usage is still unstable because the GC lags a bit.

Ok.

I was going to say what's one more dependency but:

So, this uses the buffer pool from go-msgio. Should we break that into a separate package? It's used all over the place.

Yes i think we should. I am not particularly happy with bringing in a whole package just to use one of its utility sub-packages.

Other than that it LGTM.