[API Proposal]: IDistributedCache overloads with ReadOnlyMemory<byte> support

[kerams]

Background and motivation

IDistributedCache.Set and SetAsync only take a byte array as the input. This is not ideal when working with oversized pooled arrays for example, because you need to reallocate the data out into a fixed-size array just to pass it into the cache. Having overloads with ReadOnlyMemory<byte> parameters would alleviate this issue (supposing the actual caching implementation itself supports Memory, which StackExchange.Redis does).

API Proposal

Additions to Microsoft.Extensions.Caching.Distributed.IDistributedCache:

void Set(string key, ReadOnlyMemory<byte> value, DistributedCacheEntryOptions options);
Task SetAsync(string key, ReadOnlyMemory<byte> value, DistributedCacheEntryOptions options, CancellationToken token = default(CancellationToken));

API Usage

public static async Task Cache(string key, string looongString, IDistributedCache cache, DistributedCacheEntryOptions options)
{
    var maxLength = Encoding.UTF8.GetMaxByteCount(looongString.Length);
    var pooled = ArrayPool<byte>.Shared.Rent(maxLength);

    try
    {
        var bytesWritten = Encoding.UTF8.GetBytes(looongString, pooled);
        await cache.SetAsync(key, pooled.AsMemory(0, bytesWritten), options);
    }
    finally
    {
        ArrayPool<byte>.Shared.Return(pooled);
    }
}

Alternative Designs

No response

Risks

No response

Tagging subscribers to this area: @dotnet/area-extensions-caching
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

IDistributedCache.Set and SetAsync only take a byte array as the input. This is not ideal when working with oversized pooled arrays for example, because you need to reallocate the data out into a fixed-size array just to pass it into the cache. Having overloads with ReadOnlyMemory<byte> parameters would alleviate this issue (supposing the actual caching implementation itself supports Memory, which StackExchange.Redis does).

API Proposal

Additions to Microsoft.Extensions.Caching.Distributed.IDistributedCache:

void Set(string key, ReadOnlyMemory<byte> value, DistributedCacheEntryOptions options);
Task SetAsync(string key, ReadOnlyMemory<byte> value, DistributedCacheEntryOptions options, CancellationToken token = default(CancellationToken));

API Usage

public static async Task Cache(string key, string looongString, IDistributedCache cache, DistributedCacheEntryOptions options)
{
    var maxLength = Encoding.UTF8.GetMaxByteCount(looongString.Length);
    var pooled = ArrayPool<byte>.Shared.Rent(maxLength);

    try
    {
        var bytesWritten = Encoding.UTF8.GetBytes(looongString, pooled);
        await cache.SetAsync(key, pooled.AsMemory(0, bytesWritten), options);
    }
    finally
    {
        ArrayPool<byte>.Shared.Return(pooled);
    }
}

Alternative Designs

No response

Risks

No response

Author:	kerams
Assignees:	-
Labels:	api-suggestion, untriaged, area-Extensions-Caching
Milestone:	-

[eerhardt]

Having overloads with ReadOnlyMemory parameters

Is there a reason this has to be ReadOnlyMemory<byte> and not ReadOnlySpan<byte>? My assumption is that since this is a "distributed cache", the value would need to be sent it to wherever it is being cached. I wouldn't expect the IDistributedCache to "hold on" to the ReadOnlyMemory<byte>, but instead send it.

[Clockwork-Muse]

Is there a reason this has to be ReadOnlyMemory<byte> and not ReadOnlySpan<byte>?

Because you can't use Span over an async suspension point. See for instance Stream.WriteAsync, which would almost certainly be called under the covers for any cross-machine distributed cache.

In theory the synchronous version could still use the Span versions (as Stream.Write does), but using Memory in both places for consistency might be preferable.

[kerams]

I would not mind contributing this change if approved and within the .NET 7 timeframe. I assume it'll just involve adding the methods and updating any surface area tests. Then I don't know how such changes flow downstream, among others, to the ASP.NET repo, where the Redis implementation will have to be updated because the build will break as soon as the new interface is imported.

In theory the synchronous version could still use the Span versions

StackeExchange.Redis concerns me personally the most, and since Span is not convertible to RedisValue, I don't see much use in Span parameters even in the sync version.

[eerhardt]

Span is not convertible to RedisValue

I think that is a good reason to leave the sync version using ReadOnlyMemory<byte>.

where the Redis implementation will have to be updated because the build will break as soon as the new interface is imported.

We won't be accepting a breaking change. Instead, the new methods would have to be "DIM"s - https://docs.microsoft.com/en-us/dotnet/csharp/whats-new/tutorials/default-interface-methods-versions.

[aKzenT]

Any update on this?

Who would need to approve this @eerhardt ? What are the next steps?

ASP.NET Core spend a lot of effort over the years avoiding even small allocations on each request. IDistributedCache is often used as a session store which is used on each request on many websites, often in combination with relatively large chunks of data and things like serialization. So it seems very worthwhile to be able to avoid unecessary allocations/copies in this common scenario.

[adamsitnik]

@davidfowl I can see that most of the implementations of this interface are provided by the ASP.NET Team. Whom should we ask for feedback about extending that interface with the proposed methods?

[jodydonetti]

Update: see IBufferDistributedCache here.

[julealgon]

Is there a reason for this proposal to still exist with the creation of the new HybridCache? Would there be any situation where one shouldn't just migrate from IDistributedCache to HybridCache?