Please add these commands to playground/CommandInfoUpdater/SupportedCommand.cs and run the CommandInfoUpdater tool with --force, then ensure that the produced json files (RespCommandDocs.json & RespCommandInfo.json) are the same (it might move these commands around as it's alphabetizing the commands & sub-commands). Thank you!

done

Why a 1 case switch instead of an if?

fixed

Should an error be reported if valueSpan is too small? Just raising an exception is kinda hostile.

What is reported if the set fails?

More generally, the Exists call on L45 won't keep the key from being created by the time we get here (or to line 107).

Perhaps a SETEX with NX (and dealing with the responses) would be a better approach? Redis has a REPLACE flag to control that behavior, but that can be future work.

Same note as above on SETEX.

Same "why switch and not if"-note.

fixed

This could be pretty big, right? So unconditionally stackalloc'ing is dangerous, probably get a rented array past a certain limit (64 or 128 are probably reasonable?).

Not sure if renting here is a good idea, if this method will get called, it's probably going to be multiple times, the renting and disposing might be overall less efficient than just heap allocating.

For now i changed it to heap alloc when above 128.

Why copy byte-by-byte and not use a Span CopyTo method?

done

Max length here is like 10 right? You can just stackalloc that instead of allocating an array.

done

nit: C# convention is to PascalCase tuple members. So (long Length, byte PayloadStart).

done

This whole method is pretty allocate-y, which worries me if it's ever extensively used.

I agree bit @kevin-montrose here. A common approach is to have small static method to first calculate the required buffer size (it can be simple switch expression) for the caller (i.e. in our case NetworkDUMP method) to know how much buffer it needs (to slice from existing buffer, stackalloc, rent etc., doesn't matter).

Then the actual encoding method takes a Span<byte> output argument which ownership belongs to caller. Now we could do the encoding on this buffer without allocations. You can see this pattern all over .NET with TryWrite and other encoding/serialization methods.

Rename to SkipRDBRestoreChecksumValidation to clarify that this skip is specifically for the restore command (we have checksum validations in other unrelated places).

I named it like this because that's how it's named in the redis source code skip_checksum_validation. Also in the redis it only can be se as a build option and not in redis.conf.

it is fine to diverge from other resp caches in such respects. having a conf option is clearly more powerful, and we can name it in a way that makes sense for garnet.

I'd like to suggest change of approach to this length decoding. Exception handling is expensive, to avoid it you can use the Try-pattern seen a lot in the .NET world. This way caller can decide how to handle failure, i.e. throw expensive exception or to maybe just write RESP error without EH catching logic?

The method should also only concern itself with the length decoding and leave the slicing of value type byte in the beginning to the caller who hands the input buffer to this method.

My alternate approach:

    public static bool TryReadLength(ReadOnlySpan<byte> input, out int length, out int bytesRead)
    {
        length = 0;
        bytesRead = 0;
        if (input.Length < 1) return false;
        
        var firstByte = input[0];
        switch (firstByte >> 6)
        {
            case 0:
                bytesRead = 1;
                length = firstByte & 0x3F;
                return true;
            case 1 when input.Length > 1:
                bytesRead = 2;
                length = ((firstByte & 0x3F) << 8) | input[1];
                return true;
            case 2:
                bytesRead = 5;
                return BinaryPrimitives.TryReadInt32BigEndian(input.Slice(1), out length);
            default: 
                return false;
        }
    }

With this approach, the JITted assembly will be much tighter: https://www.diffchecker.com/UMTqkaAE/

I agree bit @kevin-montrose here. A common approach is to have small static method to first calculate the required buffer size (it can be simple switch expression) for the caller (i.e. in our case NetworkDUMP method) to know how much buffer it needs (to slice from existing buffer, stackalloc, rent etc., doesn't matter).

Then the actual encoding method takes a Span<byte> output argument which ownership belongs to caller. Now we could do the encoding on this buffer without allocations. You can see this pattern all over .NET with TryWrite and other encoding/serialization methods.

Excellent! This rent still needs to be returned to the ArrayPool<byte> after we're done. I'd also recommend bumping the threshold to 256 bytes for it to be worth it.

This is the common pattern:

byte[]? rentedBuffer = null;
var buffer = totalLength <= 256 
    ? stackalloc byte[256] // constant sized stackallocs result better codegen (We should bunch of stack left here). 
    : (rentedBuffer = ArrayPool<byte>.Shared.Rent(totalLength));

then after we're done:

if (rentedBuffer is not null)
    ArrayPool<byte>.Shared.Return(rentedBuffer);

done

I had small mistake in my snippet, sorry! 😅 (Off by ones, am I right?)

done