add ResponseArrayPool

[pairbit]

No description provided.

[mgravell]

The change

- static StackExchange.Redis.Lease<T>.Create(int length, bool clear = true) -> StackExchange.Redis.Lease<T>!
+static StackExchange.Redis.Lease<T>.Create(int length, bool clear = true, System.Buffers.ArrayPool<T>? pool = null) -> StackExchange.Redis.Lease<T>!

is a hard "nope" - that would smash binary compatibility; at the minimum, we would need to add this via an overload. However, I wonder whether we should instead allow the least to be backed by a union of T[] / IMemoryOwner<T> (presumably via a single object field), and for the byte case we might use the existing response buffer. Thoughts?

[pairbit]

for the byte case we might use the existing response buffer. Thoughts?

perfect!

[pairbit]

I have updated Lease, but I am having trouble passing the IMemoryOwner object to the ResultProcessor<Lease>

[mgravell]

I can look tomorrow, but I assume it would be accessing the muxer via the connection to get the options, to pass into a new overload of AsLease

[mgravell]

ok, I have a plan here; merging this into my branch and evolving

[mgravell]

new approach summary (please weigh in here!)

• options has MemoryPool<byte>? ResponseBufferPool and RequestBufferPool - they are no longer "exotic" types
• if specified, type-testing is used to check for the new CycleBufferPool<T> which would allow you to add your own rules based on ReadOnlySequence<T> (which means the implementation has access to total size, last buffer size, number of buffers, etc), when accessed via CycleBuffer
• the Lease<byte> code also uses ResponseBufferPool, but the simple Rent(int) is used, not the ReadOnlySequence<T> version

Does that cover all bases?

[pairbit]

new approach summary (please weigh in here!)

• options has MemoryPool<byte>? ResponseBufferPool and RequestBufferPool - they are no longer "exotic" types
• if specified, type-testing is used to check for the new CycleBufferPool<T> which would allow you to add your own rules based on ReadOnlySequence<T> (which means the implementation has access to total size, last buffer size, number of buffers, etc), when accessed via CycleBuffer
• the Lease<byte> code also uses ResponseBufferPool, but the simple Rent(int) is used, not the ReadOnlySequence<T> version

I like. But I have a few comments:

1. Why are you getting a response buffer from RequestBufferPool? Is this not an error?

-var memoryPool = BridgeCouldBeNull?.Multiplexer.RawConfig.ResponseMemoryPool ?? MemoryPool<byte>.Shared;
+var memoryPool = BridgeCouldBeNull?.Multiplexer.RawConfig.RequestBufferPool ?? MemoryPool<byte>.Shared;

2. I thought you suggested setting the memoryOwner buffer (for the Lease class), which was received the first time, to call the OnResponseFrame method.

var memoryOwner = memoryPool.Rent(len);
Span<byte> oversized = memoryOwner.Memory.Span.Slice(0, len);

payload.CopyTo(oversized);

OnResponseFrame(prefix, oversized, ref memoryOwner);

memoryOwner?.Dispose();

Does that cover all bases?

3. That's enough for now. But I have last year's issuer Memory optimization for ScriptEvaluate #2843, and I would like to include it in the next iteration.

[mgravell]

1 yep, typo, thanks

2 I'll have to read when at a computer, my brain can't parse in isolation

[pairbit]

1 yep, typo, thanks

2 I'll have to read when at a computer, my brain can't parse in isolation

OK. I think the current changes are sufficient for a release. When can we expect a new version?