add ReadOnlyMemory<RedisKeyOrValue> and Lease<byte> for Lua and Execute

[pairbit]

(outdated)

Tasks:

• add ReadOnlyMemory<RedisKeyOrValue> API for Lua
• add Lease<byte>? API for Lua
• add ScriptEvaluateReadOnlyAsync again if the script does not exist.
• add rent args for key-prefix path
• add ReadOnlyMemory<RedisKeyOrValue> API for Execute
• add Lease<byte>? API for Execute
• add IScriptRequestDisposer to Execute (to return memory to pools, including for fire-and-forget mode)

Links
#2346
#2843
#2844

Please consider this PR, this issue has been raised for a long time

[mgravell]

There's a couple of reasons I've been deferring on this for a little bit...

If we're thinking "efficiency", there's the problem of the return value, and managing it in a way that is flexible and efficient.

I'm not quite ready to release it on the world yet, but a large part of the IO rewrite is "yet to come", and explicitly targets flexible read / write custom command scenarios, including Lua (but also any ad-hoc commands). I'm torn between compromising on an eval[sha] now, bs giving you the real thing "soon" - trust me, it'll be much easier than this.

Thoughts?

[pairbit]

I understand that you want to give the maximum. But the perfect is the enemy of the good. The solution I propose will completely satisfy many and will be a compromise. For most tasks, the current solution will be sufficient. But I understand it's not perfect.

I'd really appreciate your consideration of my merge request. It's a good temporary compromise.

[pairbit]

I'm currently in dire need of the ScriptEvaluateLease and ExecuteLease methods.
In my projects I use only lua scripts and direct call commands, and I really miss the rent on return.

If you don't like the idea of ​​introducing a new RedisKeyOrValue or ScriptRequestDisposer type, I'm willing to live with their absence.

I am willing to listen to your terms on which you would agree to add methods with a Lease return.

I apologize for being stubborn, I have been waiting for these changes for a very long time.

[mgravell]

I'll try to look today. Balancing competing needs is always tricky. I'm not against reasonable APIs. The return value is the trickiest bit, since this can technically be an arbitrary tree of different data types.

Thinking outside of the box: how might a lease + RespReader work for you? I.e. the lease isn't a value, but an API over the tree? If you're accessing a scalar or BLOB: this is then one line to get the blob, and I could provide helper APIs for those.

[pairbit]

I understand that the Lease<byte> object only works with a scalar result, which is fine for me since I'm working with blob. I try to avoid trees and arrays as a result.

Thinking outside of the box: how might a lease + RespReader work for you?

Returning lease + RespReader would be a great solution!

After the review, please give me your opinion on my idea with IScriptRequestDisposer, which returns all data associated with a request to pools after writing. IScriptRequestDisposer could not be used for ScriptEvaluate because the message can be used twice.

[mgravell]

OK; I can see that I have "push" access; if it is OK with you, I'm going to take one half of this (the input API, i.e. RedisKeyOrValue etc), but totally change the response half, pushing to your branch. I understand that efficiently accessing BLOB payloads is a key goal, so I will prioritize that aspect. Let me know if this plan concerns you.

[mgravell]

Making good progress locally; just a heads-up: I need to remove the IRequestDisposer scenario - that doesn't really work safely as-written (because of -MOVED etc), and it will actually become redundant soon. For now, the caller must own the lifetime via await; it is acknowledged that this today won't work well with fire-and-forget (bit of a footgun there, which we'll document), but when the IO rewrite work is complete, the serialize moves way further out, and that problem goes away.

[mgravell]

Hi Ivan - thanks for this, the underlying idea (typed key/value args, a low-allocation reply path for EVAL and ad-hoc commands) is exactly right and is the reason I wanted to get this in.

I've pushed a single commit to this branch that reworks the implementation pretty much top to bottom, so I want to be upfront about the scope before you look: the request-side shape survives (RedisKeyOrValue, keys-and-values-through-EVAL), but the response side, the RedisKeyOrValue internals, the public API names, and the
KeyPrefixedDatabase wiring are all different from what you had.

Highlights:

• The lease-returning response type became RespResult
  (IDisposable, raw/undecoded, pooled buffer) with ExecuteResp/
  ScriptEvaluateResp/ScriptEvaluateReadOnlyResp (+ Async) as the
  final names.
• RedisKeyOrValue's internal layout changed from an unsafe union to
  a plain RedisValue + prefix-byte-array pair, mainly so it can
  actually participate in KeyPrefixed key-prefixing (the original
  layout couldn't).
• IRequestDisposer is gone - it wasn't safe to fire per-WriteImpl
  given retries/redirects; RespResult.Dispose() covers what it was
  for.
• Docs (Execute.md, Scripting.md) and a fuller test pass (unit +
  live-server + mock tests, including array/tree replies) came along
  with it.
• The PR now includes the work to un-gate the RespReader API

Full details are in the commit message. Would genuinely like your take - on the API shape, anything I've missed from the original design intent, or anywhere you think this went the wrong way.

The tests deliberately include examples related to the "lease" scenario - both using ReadLease(), and (separately) copying the raw payload out from the RespReader:

     [Fact]
    public async Task ScriptEvaluateResp_ScalarBlob_ReadLease()
    {
        await using var conn = Create();
        var db = conn.GetDatabase();

        using var result = db.ScriptEvaluateResp("return 'hello world'", default, default);
        Assert.False(result.IsNull);

        using var lease = result.ReadScalar().ReadLease();
        Assert.Equal("hello world", Encoding.UTF8.GetString(lease!.Span));
        // ^^^ you wouldn't do this in real code - there is a reader.ReadString() method!
    }

    [Fact]
    public async Task ScriptEvaluateResp_ScalarBlob_CopyToCallerBuffer()
    {
        await using var conn = Create();
        var db = conn.GetDatabase();

        using var result = db.ScriptEvaluateResp("return 'hello world'", default, default);
        var reader = result.ReadScalar();
        byte[] buffer = new byte[reader.ScalarLength()]; // in reality: your own lease
        var copied = reader.CopyTo(buffer);
        Assert.Equal("hello world", Encoding.UTF8.GetString(buffer, 0, copied));
        // ^^^ you wouldn't do this in real code - there is a reader.ReadString() method!
    }

[pairbit]

Excellent solution. I couldn't have asked for anything better.

I have a question.
In my code I will use the following approach with renting:

var values = ArrayPool<RedisValue>.Shared.Rent(1);
values[0] = "myvalue";
var canReturn = true;
try
{
    using RespResult result = db.ScriptEvaluateResp(script, values: values.AsMemory(0, 1));
    // use result...
}
catch (RedisServerException)
{
    throw;
}
catch // I don't like it
{
    canReturn = false; 
    throw;
}
finally
{
    if (canReturn) ArrayPool<RedisValue>.Shared.Return(values, clearArray: true);
}

I don't like catch without specifics.
An ArgumentException or InvalidOperationException may be thrown.
Is there a more precise way to determine whether a message will be reused?

[pairbit]

I don't like that i have to rent the buffer twice to get a BulkString.
The first time, we lease the entire RespResult response.

using var respResult = await db.ScriptEvaluateRespAsync(script, keys, values)

The second time we rent to get the value.

if (result.Prefix == RESPite.Messages.RespPrefix.BulkString)
{
    using var lease = result.ReadScalar().ReadLease();
}

For large blobs this may not be effective.
I propose to inherit the RespResult class from IMemoryOwner<byte> and add the AsBulkString method, which will return the memory chunk cut off from RespResult.
For example:

Task<IMemoryOwner<byte>> RunScript()
{
	using RespResult result = db.ScriptEvaluateResp(script, keys, values);
	return result.AsBulkString(); //or AsScalar();
}

What do you say?

[mgravell]

I say that's technically impossible. A strict interpretation of RESP3 means that a payload body does not need to be a single contiguous chunk - it can be streamed. This means at a minimum we'd need to consider ReadOnlySequence-byte. Unfortunately, ReadOnlySequence-byte doesn't have a well-understood lifetime metaphor.

However, in reality it almost certainly will be contiguous, and there are APIs to fetch that directly - for example, I believe TryGetSpan and various inline parse methods are zero copy.

It is also important to note that we need to avoid ambiguity over payload lifetime, which is a huge issue.

[mgravell]

Notes to self:

• defensive input copy on F+F
• check for functions support

[pairbit]

I was thinking about ROS. But the current RespResult implementation only uses contiguous memory blocks. I understand there's a TryGetSpan() method, but I don't want to externalize the RespResult class. Perhaps I could add TryGetScalar() that returns IMemoryOwner?

[mgravell]

I do think I can do something here with ref-counted memory, optimized for the linear case. I'll look. The calling code will need to dispose each, but it'll avoid a copy/lease unless it is genuinely discontiguous.

[mgravell]

I was thinking about...

I think we're near the same page; I can make something work - I have an idea. Technically it won't be a strict guarantee to not do an extra copy, but in reality it always will (not do an extra copy). So: yes, I can improve the current PR state that always does an extra copy.

[mgravell]

if (result.Prefix == RESPite.Messages.RespPrefix.BulkString)

Perfect reminder for me: I need to make the docs lean people towards IsScalar / IsAggregate - focusing on specific prefixes tends to not play well with RESP2 vs RESP3 (although you'll get away with it in your case).

[mgravell]

I don't like catch without specifics.

I don't disagree. When I finish the write half of the IO core rewrite, the answer will be "always". But I'll have a bit more of a think.

[mgravell]

But the current RespResult implementation only uses contiguous memory blocks.

You misunderstand me. The payload itself can be non-contiguous inside a contiguous buffer - split over multiple fragments. At least, theoretically. I don't think any real server actually ever returns streamed payloads, though!

[pairbit]

You misunderstand me. The payload itself can be non-contiguous inside a contiguous buffer - split over multiple fragments. At least, theoretically. I don't think any real server actually ever returns streamed payloads, though!

Thanks for the clarification. I didn't know about this possibility.

[pairbit]

I've already looked at your commit. It's amazing.

Remember when I mentioned the place where you lianize ROS?
Now you can pass that buffer directly to RespReader.
It may be a rare scenario, but it seems like it would be amazing, don't you think?

private void OnResponseFrame(RespPrefix prefix, ReadOnlySequence<byte> payload)
{
    if (payload.IsSingleSegment)
    {
        OnResponseFrame(prefix, payload.FirstSpan, ref SharedNoLease);
    }
    else
    {
        var len = checked((int)payload.Length);
        var memoryPool = BridgeCouldBeNull?.Multiplexer.RawConfig.ResponseBufferPool ?? MemoryPool<byte>.Shared;
        var memoryOwner = memoryPool.Rent(len);
        Span<byte> oversized = memoryOwner.Memory.Span.Slice(0, len);

        payload.CopyTo(oversized);
		
        // set buffer in RespReader
        OnResponseFrame(prefix, oversized, ref memoryOwner);

        memoryOwner?.Dispose();
    }
}

[pairbit]

Are these final changes? When is the merger planned?

[mgravell]

Sorry, long weekend, looking again now!

Now you can pass that buffer directly to RespReader.

That's probably a step too far, or at least one to defer until we have the full end-to-end IO v3 core working - it would mix two buffer pools with delicate handling. For now I'd rather focus on reliability.

Are these final changes? When is the merger planned?

I need to think a little bit more about the inbound memory lifetime - that's my job for today.