GC Hole for SpanByte and other ones

[hamarb123]

garnet/libs/storage/Tsavorite/cs/src/core/VarLen/SpanByte.cs

Line 61 in 1e5c7ad

return (byte*)Unsafe.AsPointer(ref payload);

this member is called here

garnet/libs/client/ClientSession/GarnetClientSessionMigrationExtensions.cs

Line 403 in 1e5c7ad

Buffer.MemoryCopy(key.ToPointerWithMetadata(), curr, key.Length, key.Length);

which this API is called from the public API TryWriteKeyValueSpanByte here

garnet/libs/client/ClientSession/GarnetClientSessionMigrationExtensions.cs

Line 296 in 1e5c7ad

if (!WriteSerializedSpanByte(ref key, ref value))

which takes ref SpanByte.

It is a GC hole since it's never pinned or otherwise fixed.

This is also a GC hole, since the memory is not fixed (you can also just use Unsafe.SizeOf here):

garnet/libs/client/Utility.cs

Line 28 in 1e5c7ad

return (int)((long)Unsafe.AsPointer(ref arr[1]) - (long)Unsafe.AsPointer(ref arr[0]));

This is also seems to be a GC hole, since the memory is never fixed on this public API:

garnet/libs/storage/Tsavorite/cs/src/core/VarLen/SpanByteComparer.cs

Line 25 in 1e5c7ad

byte* ptr = (byte*)Unsafe.AsPointer(ref spanByte);

(note this list is not exhaustive)

[hamarb123]

This is a GC hole:

garnet/libs/storage/Tsavorite/cs/src/core/Index/Interfaces/CallbackInfos.cs

Line 106 in 1e5c7ad

var valueAddress = (long)Unsafe.AsPointer(ref recordValue);

which is called by

garnet/libs/storage/Tsavorite/cs/src/core/Index/Interfaces/CallbackInfos.cs

Line 94 in 1e5c7ad

public readonly unsafe void ClearExtraValueLength<TValue>(ref RecordInfo recordInfo, ref TValue recordValue, int usedValueLength)

which is a public API

(found by @DaZombieKiller)

[badrishc]

None of these seem like real issues. We know SpanByte structs are simply a view over pre-pinned memory buffers (in Tsavorite) or pinned pre-pinned network buffers (in Garnet's network layer). Garnet is being used as a server. At best, it might be a case of things being marked public when they shouldn't have?

[EgorBo]

None of these seem like real issues.

This project seems to use Unsafe.AsPointer a lot and it's difficult to trace down whether input is guaranteed to be on stack or not - a lot of methods/APIs seem to have a hidden "contract" for their ref X parameters and it's easy to misuse in future. GC Holes are a serious issue - they're extremely difficult to diagnose and may randomly manifest as NRE or even AVE crashing the whole application down once a week. I haven't investigated all of the listed uses, but e.g. this one:

garnet/libs/client/Utility.cs

Lines 27 to 28 in c16e1dd

	T[] arr = new T[2];
	return (int)((long)Unsafe.AsPointer(ref arr[1]) - (long)Unsafe.AsPointer(ref arr[0]));

is a classic GC hole - GC may interupt in the middle of the method, move arr[0] pointer to a new location and, as the result, this function may return something negative or huge (pretty much, random).

PS: the project seems to also use GC.AllocateArray(, pinned: true) a lot - if it is used for short-lived objects it may create a terrible fragmentation in POH (when mixed with long-lived ones) as POH was designed specifically only for long-lived objects.

[badrishc]

Got it, the GetSize happens rarely and only at load so it's possible we did not encounter the issue in practice, that should be fixed in a linked PR.

GC.AllocateArray is being used only for large pages that need to be pooled and pinned forever. In fact I think in some paths we unpin when returning to the pool. We should review all uses and improve this over time.

[badrishc]

Note that in order to match and beat competing C++ caches, particularly for 99.9th percentile latencies, we had to do all the memory management ourselves when we could - which implied keeping pinned pages around for reuse over the lifetime of the server.

[badrishc]

a lot of methods/APIs seem to have a hidden "contract" for their ref X parameters and it's easy to misuse in future

This is correct. It was cumbersome/impossible to use byte* everywhere, particularly as C#'s type system prevented it being a generic parameter. Thankfully this is internal to the codebase, where we know exactly what we are doing. We are definitely open to other ways of handling this, that do not involve performance impact.

[DaZombieKiller]

particularly as C#'s type system prevented it being a generic parameter

This particular pain point can be worked around with a Pointer<T> struct, for example:

public readonly unsafe struct Pointer<T>
    where T : unmanaged // not needed in C# 11
{
    public readonly T* Value;
    public Pointer(T* value) => Value = value;
    public static implicit operator void*(Pointer<T> pointer) => pointer.Value;
    public static implicit operator T*(Pointer<T> pointer) => pointer.Value;
    // etc...
}

The main thing to keep in mind with this is that it can't be used as a return type in interop code, because it being a struct means it may be passed differently in the ABI.

I'd strongly suggest using a solution like this to handle generics instead of using refs to represent fixed addresses in the API. The usage of refs here is just so much more dangerous and much easier to get wrong despite how "safe" refs "feel" in comparison to pointers.

Even if you're as careful as possible, there is no safeguard from the language, runtime or type system that ensures you don't accidentally AsPointer an unfixed reference (something that is very easy to do by mistake when it's common to do in the codebase), and then you have a ticking GC hole time bomb.

This constructor is a good example:

garnet/libs/storage/Tsavorite/cs/src/core/VarLen/UnmanagedMemoryManager.cs

Lines 26 to 37 in c16e1dd

	/// <summary>
	/// Create a new UnmanagedMemoryManager instance at the given pointer and size
	/// </summary>
	/// <remarks>It is assumed that the span provided is already unmanaged or externally pinned</remarks>
	public UnmanagedMemoryManager(Span<T> span)
	{
	fixed (T* ptr = &MemoryMarshal.GetReference(span))
	{
	_pointer = ptr;
	_length = span.Length;
	}
	}

IMO it just shouldn't exist and callers should be required to use the pointer + length constructor, making it much less likely that they will accidentally pass in dangerous values. The convenience of just being able to pass a Span<T> in comes at a great safety cost that is only really mitigated by a small remark comment. Luckily, this constructor does not currently have any usages.

[badrishc]

Thanks for the details. Yes, it should be possible to use wrapper structs throughout and avoid using refs to represent fixed addresses. We will be happy address this soon. The UnmanagedMemoryManager should be easily adjusted as well.

Just wanted to mention that Garnet has been running for many months in production use cases without issues, so this is not an immediate concern for users. Since Garnet (and definitely none of these above-mentioned APIs) is not programmatically accessed, there isn't a danger of users misusing it, just we system developers and future contributors. Yet, it makes sense to avoid where possible without loss of performance.