Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add (ReadOnly)Memory<byte> implicit conversions to RedisKey #2578

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kevin-montrose
Copy link
Contributor

At a high level, this just changes RedisKey.KeyPrefix to a ReadOnlyMemory (from a byte[]).

The change is slightly more complicated to deal with Prepend/Append.

Also noticed this line ; which looks like a place where prefix wouldn't be written? "Fixed" it and all tests still seem to pass.

…eeding to copy memories into arrays when dealing with RedisKeys; expands tests to cover all the new prepend/append possibilities; address an oversite(?) in PhysicalConnection, and adapt to use memory instead of byte[]
@mgravell
Copy link
Collaborator

which looks like a place where prefix wouldn't be written?

well, historically I believe it was "value only" or "prefix+value" - it is your change that makes "prefix only" a thing; it bothers me a little that if we are using key-prefix (which is non-trivial usage - aspnet exposes it, for example), then this gets kicked into non-working; I had a separate design somewhere that refactored the value part of the key instead, using object+int+int fields, with type testing to support:

  • string, char[], MemoryManager<char> (all handled as ReadOnlyMemory<char>)
  • byte[], MemoryManager<byte> (all handled as ReadOnlyMemory<byte>)

I wonder if that's the way we should go here

@mgravell
Copy link
Collaborator

here was the core part from last time I looked at this:

using System.Buffers;
using System.Runtime.InteropServices;
 
Describe(default);
 
string s = "abcdefg";
Describe(s);
Describe(""); // should look like default
Describe(s.AsMemory());
Describe(s.AsMemory().Slice(2,3));
 
var bytes = new byte[42];
Describe(bytes);
Describe(Array.Empty<byte>()); // should look like default
Describe(bytes.AsMemory());
Describe(bytes.AsMemory().Slice(10, 3));
 
var chars = new char[42];
Describe(chars);
Describe(chars.AsMemory());
Describe(chars.AsMemory().Slice(10, 3));
 
// not shown: custom memory managers
 

static void Describe(Prototype value)
{
    if (value.TryGetValue(out ReadOnlyMemory<byte> bytes))
    {
        Console.WriteLine($"Value is {bytes.Length} bytes");
    }
    if (value.TryGetValue(out ReadOnlyMemory<char> text))
    {
        Console.WriteLine($"Value is {text.Length} chars");
    }
    Console.WriteLine("---");
    Console.WriteLine();
}
readonly struct Prototype
{
    private readonly object? _obj;
    private readonly int _start, _length;
 
    private Prototype(object? obj, int start, int length)
    {
        if (length is 0)
        {
            // don't track the obj, then
            this = default;
        }
        else
        {
            _obj = obj;
            _start = start;
            _length = length;
        }
    }
    public static implicit operator Prototype(string value) => new(value, 0, value is null ? 0 : value.Length);
 
    public static implicit operator Prototype(byte[] value) => new(value, 0, value is null ? 0 : value.Length);
    public static implicit operator Prototype(char[] value) => new(value, 0, value is null ? 0 : value.Length);
 
    public static implicit operator Prototype(Memory<char> value) => (Prototype)(ReadOnlyMemory<char>)value;
    public static implicit operator Prototype(ReadOnlyMemory<char> value)
    {
        // string and array are pretty common
        if (MemoryMarshal.TryGetString(value, out var text, out int start, out int length))
        {
            return new Prototype(text, start, length);
        }
        if (MemoryMarshal.TryGetArray(value, out var segment))
        {
            return new Prototype(segment.Array, segment.Offset, segment.Count);
        }
        // this is pretty rare
        if (MemoryMarshal.TryGetMemoryManager(value, out MemoryManager<char>? mgr, out start, out length))
        {
            return new Prototype(mgr, start, length);
        }
        // we don't expect this
        return Fail();
    }
 
    public static implicit operator Prototype(Memory<byte> value) => (Prototype)(ReadOnlyMemory<byte>)value;
    public static implicit operator Prototype(ReadOnlyMemory<byte> value)
    {
        // array is pretty common
        if (MemoryMarshal.TryGetArray(value, out var segment))
        {
            return new Prototype(segment.Array, segment.Offset, segment.Count);
        }
        // this is pretty rare
        if (MemoryMarshal.TryGetMemoryManager(value, out MemoryManager<byte>? mgr, out var start, out var length))
        {
            return new Prototype(mgr, start, length);
        }
        // we don't expect this
        return Fail();
    }
    private static Prototype Fail() => throw new InvalidOperationException("Unexpected ReadOnlyMemory<> scenario; please report this.");
 
    public bool TryGetValue(out ReadOnlyMemory<byte> value)
    {
        if (_length == 0)
        {
            value = default;
            return true;
        }
        switch (_obj)
        {
            case byte[] arr:
                value = new ReadOnlyMemory<byte>(arr, _start, _length);
                return true;
            case MemoryManager<byte> mgr:
                value = mgr.Memory.Slice(_start, _length);
                return true;
            default:
                value = default;
                return false;
        }
    }
    public bool TryGetValue(out ReadOnlyMemory<char> value)
    {
        if (_length == 0)
        {
            value = default;
            return true;
        }
        switch (_obj)
        {
            case string text:
                value = text.AsMemory().Slice(_start, _length);
                return true;
            case char[] arr:
                value = new ReadOnlyMemory<char>(arr, _start, _length);
                return true;
            case MemoryManager<char> mgr:
                value = mgr.Memory.Slice(_start, _length);
                return true;
            default:
                value = default;
                return false;
        }
    }
}

@mgravell
Copy link
Collaborator

btw, I'm not saying "you should go do this" - this is the what I'm more inclined towards, but that doesn't make it your problem

@kevin-montrose
Copy link
Contributor Author

which looks like a place where prefix wouldn't be written?

well, historically I believe it was "value only" or "prefix+value" - it is your change that makes "prefix only" a thing

Ah, I think that's true - I was just looking at the signatures.

@kevin-montrose
Copy link
Contributor Author

Oh, also if you're thinking deep thoughts about keys and values...

Some way (or documentation, if these ways exist) to get returned values backed by rented byte[]s would also be handy. Got a couple cases where GC pauses are murder, so slapping an ArrayPool in there would be nice. I'd call it a pretty advanced scenario, so the consumer would be responsible for actually returning rented arrays IMO.

@mgravell
Copy link
Collaborator

@kevin-montrose there is a "get lease" API for redis strings, which might be what you mean? Or do you mean the keys?

@kevin-montrose
Copy link
Contributor Author

@kevin-montrose there is a "get lease" API for redis strings, which might be what you mean? Or do you mean the keys?

@mgravell YUP, that'll do - couldn't find it (was looking for "pool" or something), and seems basically undocumented?

Ideally we'd even be able to reuse the Lease... but that's maybe overkill.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants