Make Microsoft.Extensions.Caching.Memory.MemoryCache support generics

[yangmsft]

Background and Motivation

We use MemoryCache extensively in our project and found that MemoryCache could greatly benefit from 2 features:

1. Most of the times we use strings as keys, and look ups are mostly case insensitive. If an IEqualityComparer can be supported then we can avoid a lot of unnecessary allocations by normalizing the casing of strings
2. Sometimes we use value types (int) as keys; since MemoryCache forces keys to be objects right now there are a lot of unnecessary boxing costs

Making MemoryCache support generics can solve both issues, since we can enforce TKey to be IEquatable

Proposed API

To fully preserve backward compatibility, we can make a generic interface, and make original MemoryCache implement that generic interface:

namespace Microsoft.Extensions.Caching.Memory
{
+    public interface IMemoryCache<TKey, TValue> : IDisposable {
     }

namespace Microsoft.Extensions.Caching.Memory
{
-    public interface IMemoryCache : IDisposable {
+    public interface IMemoryCache : IMemoryCache<object, object> {
     }

namespace Microsoft.Extensions.Caching.Memory
{
+    public class MemoryCache<TKey, TValue> : IMemoryCache<TKey, TValue> where TKey : IEquatable<TKey> {
     }

namespace Microsoft.Extensions.Caching.Memory
{
     public interface MemoryCache : IMemoryCache {
     }

Usage Examples

var cache = new MemoryCache<string, string>(cacheOptions, comparer: StringComparer.OrdinalIgnoreCase);
cache["key"] = "value";
// Now this returns "value" as well
var value = cache["KEY"];

var cache = new MemoryCache<long, string>(cacheOptions);
// No more boxing
cache["value".GetHashCode()] = "value";

Alternative Designs

N/A

Risks

The only risk is that this can potentially produce significant memory pressure if a consumer has a large number of different concrete MemoryCache<> types. Considering common usages of this cache, it is highly unlikely (Please correct me if I am wrong).

I am glad to help publish a PR for the proposed change as we would love to consume it if the design is approved!

[ghost]

Tagging subscribers to this area: @eerhardt, @maryamariyan, @michaelgsharp
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and Motivation

We use MemoryCache extensively in our project and found that MemoryCache could greatly benefit from 2 features:

1. Most of the times we use strings as keys, and look ups are mostly case insensitive. If an IEqualityComparer can be supported then we can avoid a lot of unnecessary allocations by normalizing the casing of strings
2. Sometimes we use value types (int) as keys; since MemoryCache forces keys to be objects right now there are a lot of unnecessary boxing costs

Making MemoryCache support generics can solve both issues, since we can enforce TKey to be IEquatable

Proposed API

To fully preserve backward compatibility, we can make a generic interface, and make original MemoryCache implement that generic interface:

namespace Microsoft.Extensions.Caching.Memory
{
+    public interface IMemoryCache<TKey, TValue> : IDisposable {
     }

namespace Microsoft.Extensions.Caching.Memory
{
-    public interface IMemoryCache : IDisposable {
+    public interface IMemoryCache : IMemoryCache<object, object> {
     }

namespace Microsoft.Extensions.Caching.Memory
{
+    public class MemoryCache<TKey, TValue> : IMemoryCache<TKey, TValue> where TKey : IEquatable<TKey> {
     }

namespace Microsoft.Extensions.Caching.Memory
{
     public interface MemoryCache : IMemoryCache {
     }

Usage Examples

var cache = new MemoryCache<string, string>(cacheOptions, comparer: StringComparer.OrdinalIgnoreCase);
cache["key"] = "value";
// Now this returns "value" as well
var value = cache["KEY"];

var cache = new MemoryCache<long, string>(cacheOptions);
// No more boxing
cache["value".GetHashCode()] = "value";

Alternative Designs

N/A

Risks

The only risk is that this can potentially produce significant memory pressure if a consumer has a large number of different concrete MemoryCache<> types. Considering common usages of this cache, it is highly unlikely (Please correct me if I am wrong).

I am glad to help publish a PR for the proposed change as we would love to consume it if the design is approved!

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

[eerhardt]

@davidfowl / @maryamariyan - this looks a lot like #48567, but only part of it.

Do you think this public API refactoring could be done without any breaking changes?

[yangmsft]

@eerhardt Not sure if the question is directed towards me, but IMHO, it should be possible to do this change without any breaking changes. The proposed design should already cover full backward compatibility (please correct me if I'm wrong) :)

[eerhardt]

The part I'm concerned about is

namespace Microsoft.Extensions.Caching.Memory
{
-    public interface IMemoryCache : IDisposable {
+    public interface IMemoryCache : IMemoryCache<object, object> {
     }

This is probably a source breaking change, assuming IMemoryCache<TKey, TValue> is defined as:

public interface IMemoryCache<TKey, TValue> : IDisposable {
        bool TryGetValue(TKey key, out TValue value);
        ICacheEntry CreateEntry(TKey key);
        void Remove(TKey key);
     }

The way it would be a break is if someone has an explicitly implemented interface:

public class MyCache : IMemoryCache
{
    bool IMemoryCache.TryGetValue(object key, out object value) { }
}

Severity	Code	Description	Project	File	Line	Suppression State
Error	CS0539	'MyCache.TryGetValue(object, out object)' in explicit interface declaration is not found among members of the interface that can be implemented	ConsoleApp1	C:\Users\eerhardt\source\repos\ConsoleApp10\ConsoleApp1\Program.cs	50	Active

I'm not sure if it is would be a binary breaking change or not. Typically changing an interface's base interface is a breaking change, in general. See https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-changes.md#bucket-1-public-contract

Adding an interface to the set of base types of an interface

[yangmsft]

Thanks for pointing out the issue! In that case, can we leave IMemoryCache/MemoryCache untouched, and start with

public interface IMemoryCache<TKey, TValue> : IDisposable {
        bool TryGetValue(TKey key, out TValue value);
        ICacheEntry CreateEntry(TKey key);
        void Remove(TKey key);
     }

Then, we can define the generic type with

public class MemoryCache<TKey, TValue> : IMemoryCache, IMemoryCache<TKey, TValue>
{
...
}

This should be similar to how containers in System.Collections and System.Collections.Generic behave, and should be fully backward compatible (since we are not changing existing memory cache implementation at all).

Since MemoryCache and MemoryCache<TKey, TValue> may have lots of logic in common, we can probably further create a base class that hosts the common methods (let's say its name is MemoryCacheBase):

+ internal abstract class MemoryCacheBase {

- public class MemoryCache : IMemoryCache {
+ public class MemoryCache : MemoryCacheBase, IMemoryCache {

- public class MemoryCache<TKey, TValue> : IMemoryCache, IMemoryCache<TKey, TValue> {
+ public class MemoryCache<TKey, TValue> : MemoryCacheBase, IMemoryCache, IMemoryCache<TKey, TValue> {

Or alternatively, maybe we can consider MemoryCache<TKey, TValue> as the new base class since MemoryCache is really a special case where both keys and values are objects:

- public class MemoryCache : IMemoryCache {
+ public class MemoryCache : MemoryCache<object, object>, IMemoryCache {

I assume inserting an internal base class is allowed according to the official guideline, since we are not changing the public facing contracts defined in interface.

What do you think?

[eerhardt]

I assume inserting an internal base class is allowed according to the official guideline, since we are not changing the public facing contracts defined in interface.

You can insert a new base class to a class, but it needs to be public. You can't have a public class derive from an internal class.

At this point of introducing a completely new interface, I think we are getting really close to duplicating #48567.

[yangmsft]

You can insert a new base class to a class, but it needs to be public. You can't have a public class derive from an internal class.

Got it. Then we should let MemoryCache inherit MemoryCache<TKey, TValue>:

public class MemoryCache : MemoryCache<object, object>, IMemoryCache {

At this point of introducing a completely new interface, I think we are getting really close to duplicating #48567.

Agreed. The purpose of this issue is to introduce a generic version of MemoryCache, and is indeed a subset of that issue. We can definitely close this one if it is preferred; just want to make sure there is closure to either this issue or the more comprehensive one so that there is improvement planned for MemoryCache, since it is being used a lot and a lot of proposed improvements are in high demand :)

[eerhardt]

@maryamariyan @davidfowl - any thoughts on whether this proposal should stand on its own, or be closed as a dupe of #48567?

[maryamariyan]

Closing as dupe of #48567