Open kerams opened 2 years ago
Tagging subscribers to this area: @dotnet/area-extensions-caching See info in area-owners.md if you want to be subscribed.
Author: | kerams |
---|---|
Assignees: | - |
Labels: | `api-suggestion`, `untriaged`, `area-Extensions-Caching` |
Milestone: | - |
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.
Is there a reason this has to be
ReadOnlyMemory<byte>
and notReadOnlySpan<byte>
?
Because you can't use Span
over an async
suspension point. See for instance Stream.WriteAsync
-system-threading-cancellationtoken)), 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.
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.
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.
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.
@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?
Update: see IBufferDistributedCache
here.
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
?
The HybridCache
work adds a new optional backend API which sits alongside IDistributedCache
and fills this niche, see: https://github.com/dotnet/runtime/issues/100290 - however this new API should mostly be for backend implementations - think "I'm Cosmos, and I want my existing distributed cache backend to allocate less".
For application code, @julealgon is correct that HybridCache
would be the obvious switch here, removing the need for user code to even worry about this API (other than to register a backend cache)
Background and motivation
IDistributedCache.Set
andSetAsync
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 withReadOnlyMemory<byte>
parameters would alleviate this issue (supposing the actual caching implementation itself supportsMemory
, which StackExchange.Redis does).API Proposal
Additions to
Microsoft.Extensions.Caching.Distributed.IDistributedCache
:API Usage
Alternative Designs
No response
Risks
No response