Reduce allocations in Interner

[ReubenBond]

Cleans up the Interner class so that allocations do not occur on the hot path.

• Avoid allocating a closure for the createFunc parameter if possible
• Avoid closure allocations within Interner.FindOrCreate
• Converted to WeakReference<T> instead of WeakReference in order to simplify the code.

[ReubenBond]

Functionals passed except for ActivationCollectorShouldNotCollectBusyStatelessWorkers, which failed in CI but passed locally

[jason-bragg]

@dotnet-bot test this please

[jason-bragg]

&& obj != null?

[ReubenBond]

That's guaranteed by TryGetTarget

[gabikliot]

you are not worried about the different atomicity semantic snow (AddOrUpdate vs. what you do now)?
I am not saying its wrong now, just drawing your attention that you changed it.

[ReubenBond]

We were using AddOrUpdate just like a setter (our update lambda just replaces the original value). There is no added safety by performing AddOrUpdate over just using the index setter.

[gabikliot]

It has a different atomicity now - before that it GetOrAdd was atomic, now it is not. Is it a problem?

[ReubenBond]

The Func passed to GetOrAdd are invoked outside of any lock. This effectively inlines the logic so that we do not need to allocate that closure.

This is the GetOrAdd code:

TValue resultingValue;
if (this.TryGetValue(key, out resultingValue)) return resultingValue;
this.TryAddInternal(
  key,
  valueFactory(key),
  updateIfExists: false,
  acquireLock: true,
  out resultingValue);
return resultingValue;

There's no point allocating the valueFactory closure (captures creatorFunc), it makes more sense (in my opinion) to just call TryGetValue and if the value doesn't exist or is no longer valid, then to create and set the value.

Additionally, the purpose of this class is to cache immutable values (or am I wrong about this?) - if the values stored in this class do mutate, then that's an issue, i,.e, user is abusing this class.

Thanks for calling this out, by the way. I should have preemptively added a comment here to discuss my reasoning.

[sergeybykov]

What if under if (cacheEntry == null) we simply called cacheEntry = internCache.GetOrAdd() like before to maintain the behavior in the add case? While the code looks safe to me for how it's used, it's not equivalent to the internal implementation of GetOrAdd and can create multiple copies of the cached item. Not a big deal though IMO.

---

In reply to: 91662953 [](ancestors = 91662953)

[ReubenBond]

@sergeybykov

it's not equivalent to the internal implementation of GetOrAdd and can create multiple copies of the cached item

GetOrAdd can, too, since it's executed outside of the lock (look at valueFactory(key) above). The reason to go with this route over GetOrAdd is that this avoids allocations and offers the same semantics/guarantees.

[sergeybykov]

Maybe I'm missing something. Doesn't
updateIfExists: false,
acquireLock: true,
prevent it from creating multiple copies?

[ReubenBond]

@sergeybykov the copy is made before the call. Note that the signature of the function is

private bool TryAddInternal(TKey key, TValue value, bool updateIfExists, bool acquireLock, out TValue resultingValue)

The call is made to obtain value prior to invoking TryAddInternal

[ReubenBond]

The GetOrAdd docs say this:

If you call GetOrAdd simultaneously on different threads, addValueFactory may be called multiple times, but its key/value pair might not be added to the dictionary for every call.

[ReubenBond]

A benchmark verifies that no allocations are performed in the new iteration:

Host Process Environment Information:
BenchmarkDotNet.Core=v0.9.9.0
OS=Microsoft Windows NT 6.2.9200.0
Processor=Intel(R) Xeon(R) CPU E3-1230 v5 3.40GHz, ProcessorCount=8
Frequency=3328123 ticks, Resolution=300.4697 ns, Timer=TSC
CLR=MS.NET 4.0.30319.42000, Arch=32-bit RELEASE
GC=Non-concurrent Server
JitModules=clrjit-v4.6.2006.0

Type=InternerBenchmarks  Mode=Throughput

      Method |      Median |    StdDev |  Gen 0 | Gen 1 | Gen 2 | Bytes Allocated/Op |
------------ |------------ |---------- |------- |------ |------ |------------------- |
    Interner | 109.9614 ns | 2.6255 ns |      - |     - |     - |               0.01 |
 OldInterner | 190.1600 ns | 4.0172 ns | 225.00 |     - |     - |              78.76 |

The benchmark code was simple:

public class InternerBenchmarks
{
    private const int NumKeys = 1;
    private const int NumIterationsPerKey = 2;
    private readonly Interner<string, Tuple<string, int>> interner = new Interner<string, Tuple<string, int>>();
    private readonly OldInterner<string, Tuple<string, int>> oldInterner = new OldInterner<string, Tuple<string, int>>();
    private readonly List<string> keys;

    public InternerBenchmarks()
    {
        this.keys = new List<string>(NumKeys);
        for (var i = 0; i < NumKeys; i++)
        {
            this.keys.Add(i.ToString());
        }
    }

    [Benchmark]
    public int Interner()
    {
        var result = 0;
        for (int i = 0; i < NumIterationsPerKey; i++)
        {
            foreach (var key in keys)
            {
                result += this.interner.FindOrCreate(key, k => Tuple.Create(k, k.Length)).Item2;
            }
        }

        return result;
    }

    [Benchmark]
    public int OldInterner()
    {
        var result = 0;
        for (int i = 0; i < NumIterationsPerKey; i++)
        {
            foreach (var key in keys)
            {
                result += this.oldInterner.FindOrCreate(key, () => Tuple.Create(key, key.Length)).Item2;
            }
        }

        return result;
    }
}

[ReubenBond]

Apologies for the prod, but is someone able to look at this?