Possible false optimization in RecyclableMemoryStream.EnsureCapacity ?

[sgorozco]

Hi, please forgive me if this is not the proper channel to address this question. I'm still very intimidated by the whole concept of Open Source.

While reviewing the EnsureCapacity(long newCapacity) method's source code, I noticed the following optimization :

// Let's save some re-allocs of the blocks list
var blocksRequired = (newCapacity / this.memoryManager.BlockSize) + 1;
if (this.blocks.Capacity < blocksRequired)
{
   this.blocks.Capacity = (int)blocksRequired;
}

By setting the capacity of the blocks list to the exact number of required blocks, I think that the code is achieving the opposite goal stated on the comments (saving reallocations) and instead forces a reallocation of the list every time the number of required blocks increases, if even by one (although it keeps memory waste at minimum).

Wouldn't it be better to let the capacity grow exponentially as it happens if we let the Capacity be auto-managed by the backing List{byte[]} instance? Or perhaps if the goal has changed to minimize memory waste, reflect this on the comment instead?

[benmwatson]

I think I'd prefer to keep the logic and perhaps update the comment. If a user of this class wants to pre-allocate a known amount, they can always set the Capacity property on the stream. By adding a block at a time, the assumption was that most streams would be satisfied in one, maybe two, blocks.

But I'll have to think about it. It's a small thing, and the allocation of the List doesn't impact LOH allocs (or shouldn't...one would hope to not need that many blocks), so letting it manage its own size might be ok.

[sgorozco]

Thanks for answering. By the way, IMHO the code is extremely clean and well thought, and the documentation is probably the best documented library I've had the pleasure to work with!

[benmwatson]

Going to close this as wont-fix because it's a bit of a toss-up whether this will improve things. If I remove the explicit Capacity increase, then it's possible the blocks list will be re-allocated multiple times for this one call. OTOH, if I let it manage its own capacity, then it's possible future calls will be saved from re-allocating--which is the optimization you're proposing.

It's not clear to me which approach is better, so I'm going to leave as-is for now, with the more explicit management in-place.