For ROM-based APIs (mostly ScriptEvaluate/Execute): render request keys and values into one pooled buffer up front

[mgravell]

tl;dr: fix the problem with using ReadOnlyMemory<...> leases with Execute / ScriptEvaluate:

Consider:

var lease = ...
lease[0] = "foo";
lease[1] = 42;
var result = conn.ExecuteAsync("some_cmd", lease);

Now: when can you return/reuse lease? It isn't simple.

1. commands can be queued, delayed, redirected (-MOVED, which also means they're written multiple times)
2. when fire-and-forget is used, you have no idea when it is "done"
3. if it timed out... is it done? could it ever be written again?
4. WithRetry complicates things even more

Long term, we know that the "write" half of the IO core rewrite is designed to solve all of this, but for now, we need it to just not suck, so:

1. when appropriate (ScriptEvaluate, Execute), pre-render the values to bytes, so we know the values are safe
2. when that doesn't cover everything (WithRetry), we use safe local copies, removing the problem from the caller
3. note that this also simplifies how WithKeyPrefix works, since it can remove some defenses

Why

ExecuteResp/ScriptEvaluate* take ReadOnlyMemory<RedisKeyOrValue> / ReadOnlyMemory<RedisKey> / ReadOnlyMemory<RedisValue> and hold the caller's memory until write time. But the request is not necessarily written before the call returns: it can sit in the backlog, or in a batch, and a fire-and-forget caller gets no completion signal at all. So a caller who rents an args array and returns it afterwards — which is exactly what the docs teach for the hot path — is racing the writer and loses silently: whatever is in the recycled array when we get there is what goes on the wire.

Rendering at the point of the call means the request stops referring to caller memory. It also moves the formatting off the writer thread, where it currently happens while holding the single-writer lock, so it should help throughput as well as correctness — and it is the direction the IO core rewrite goes anyway, so it is a down payment rather than a detour.

Shape

One buffer per request instead of one per argument. Entries are laid out back to back as a 4-byte length followed by that many payload bytes; a negative length marks a key, with ~length giving the real size. The complement rather than the negation, because -0 == 0 cannot distinguish a zero-length key from a zero-length value.

RedisKey and RedisValue can both already measure and copy themselves exactly, so there is no oversize estimate and no backfilling: measure, rent, fill.

Rents from RequestBufferPool when configured, falling back to ArrayPool<byte>.Shared, matching what the write path already does.

On lifetime

This is the part that killed IRequestDisposer, so it is worth being explicit about what is different: the whole thing is internal, so no caller participates in a lifetime protocol and there is no public surface to get stuck with.

Recycle takes the state by ref on purpose. As an instance method it would compile perfectly happily against a readonly field and silently operate on a defensive copy, stranding the buffer; as a ref parameter that same mistake is CS0192 at build time — verified, along with the fact that the instance form produces no warning at all.

The invariant ref cannot enforce is that the state must never be copied, since two owners means a double-return to the pool. That is documented on the type.

Done

• Wiring ExecMessage and ScriptEvalMessage to it — done. ScriptEvalMessage keeps the script itself out of the buffer, since it is needed intact for hash management and SCRIPT LOAD.

Considerations

• The recycle-on-definitely-final-completion rule. PrepareToResend and caller-side timeouts must not recycle; the RecordConnectionFailed drain, -ERR and normal completion must. Note Message.Complete's once-only latch keys off resultBox, which is null for fire-and-forget — so the buffer needs its own latch rather than riding on that one.
• Batches and transactions hold queued messages until Execute, so this changes the pool-pressure profile: a large batch will hold a rented buffer per queued command where today it holds references to caller memory.

Follow-on: the same guarantee has to hold through every wrapper, not just at the base database

The point of this type is that as soon as an Execute* call exits — sync, async, fire-and-forget, delayed in the backlog, redirected — the caller's own buffers are done and safe to recycle. That only actually holds if every layer between the caller and this rendering step preserves it. Two places don't yet:

• KeyPrefixedDatabase's ExecuteResp/ScriptEvaluateResp/ScriptEvaluateReadOnlyResp currently hold their own leased, prefixed copy open until the inner call completes with success or RedisServerException, because that used to be the earliest point at which the write was known to be over. Once the inner database renders synchronously at message-construction time (this PR), that copy is fully consumed the moment the inner call is made — the lease can be returned immediately after making that call, without waiting on its result at all. Needs review once the recycle lifecycle below lands, to confirm the assumption and simplify accordingly.
  • finding: KeyPrefixedDatabase already uses lease/return; we can now simplify the return logic here
• RetryDatabase captures the caller's ReadOnlyMemory<RedisKeyOrValue>/RedisKey/RedisValue by reference across every retry attempt, with no copy of its own. The first attempt renders synchronously same as any direct call, but a later retry re-reads the same captured memory - and by then the caller may already have recycled it, believing the call was done. RetryDatabase needs its own local copy/lease taken once, up front, before the retry loop starts, so a retry always reads back its own snapshot rather than memory the caller has moved on from.
• audit for other likely methods - HashImport maybe?

[pairbit]

Excellent. However, I would use ROS<byte>.
First, it avoids the need to pre-calculate the total length of the request.
Second, it eliminates the need to allocate memory for excessively large arrays.

For the final public API version, I would like to see a RespRequest structure containing a ROS<byte>.
This could completely eliminate the ROM<RedisKey> and ROM<RedisValue> types.

struct RespRequest
{
	public void WriteKey(ReadOnlySpan<byte> key)
	public void WriteValue(ReadOnlySpan<byte> key)
}
class DatabaseAsync
{
	Task<RespResult> ExecuteRespAsync(string command, RespRequest request, CommandFlags flags = CommandFlags.None);
	Task<RespResult> ScriptEvaluateRespAsync(string script, RespRequest request, CommandFlags flags = CommandFlags.None);
}

var request = db.NewRespRequest();
request.WriteKey('key1');
request.WriteValue('value1');
request.WriteKey('key2');
request.WriteValue('value2');
using var result = await db.ExecuteRespAsync(command, request);

Can I assume that the core I/O v3 is aimed at this?

[mgravell]

@pairbit we're not there yet; that's a larger piece of the "write" half of the IO rewrite. If you use excessively large inputs, you're already in a world of hurt - for now, I'm just after a reliable way of making the API work - in reality, it'll work fine in all reasonable scenarios. The blit cost is not zero, but it also isn't big enough to cause me concern during the transition period between now and short-term-future when we get a more optimal write path

[mgravell]

btw, if you look in the IO core PoC, you're not a million miles away from the approach, except I've split it out - effectively you get a <T> for the args/state, and an IRespWriter<T> that is responsible for write the args. This allows, for example, simple things like (42, "abc", DateTime.Now) as the args, with the writer dealing with the .WriteKey(args.Foo) etc - noting that in many cases, it is actually possible to use a Roslyn generator to do all the tricky bits! I have a plan, basically.