Add a general-purpose property bag to Compilation

[pharring]

We have many places in our code base where we have ConditionalWeakTable<Compilation, ...>
Each one makes me cringe because of the potential memory leaks and additional GC pressure.
What people really want is to tie arbitrary data to the lifetime of a compilation.
We should instead just add general purpose property bag to the Compilation object and recommend that people use that instead of creating new CWTs.

[pharring]

I don't know if we have an existing "property bag" type we can use or if there's an example of an API we like. I expect the implementation is a ConcurrentDictionary<object, object>, although we wouldn't expose that directly.

[sharwell]

Please keep DotNetAnalyzers/StyleCopAnalyzers#1304 in mind as you implement this.

[RichiCoder1]

👍 A purpose-built, efficient way to do this would be fantastic.

[axel-habermaier]

Isn't one of the benefits of ConditionalWeakTable<,> over Dictionary<,> that it specifically does not cause memory leaks? As soon as no one else reference the compilation, it is garbage collected and removed from the table. References within the table itself don't count. Or is that not working when the same compilation is contained in multiple ConditionalWeakTable<,>?

[gafter]

I have just the right API for this. I will explain next week.

[pharring]

@axel-habermaier You're right. "memory leak" mischaracterizes the issue. Although, CWT doesn't scavenge old entries (call DependentHandle.Free) unless there is churn (add or remove) and who ever calls Remove on a CWT? Both key and value are held weakly, so they don't prevent collection.

@sharwell Can you expand on what I should look out for? I looked at the issue you mentioned. I a CWT<Compilation,...> was added in an analyzer. It's one of the motivating scenario for implementing the property bag. Are you just saying that your code would benefit from this proposed change?

@gafter looking forward the explanation!

[pharring]

The GC pressure I'm referring to comes from the DependentHandle allocations. DependentHandles in the GC are stored It's an additional table that the GC has to scan for references. It's pretty much a linear scan through that unordered table to mark additional rooted objects ("If primary is alive, then mark secondary as alive").

I've never actually seen this show up in profiles, but it's like a 'peanut butter' cost on every GC. CWT has its place for adding sparse annotations to an object graph. However, in Roslyn, Compilation is such an obvious target for annotation - especially in analyzers - that it makes sense to add this property bag. The lifetime is obvious and no dependent handles are required.

[sharwell]

@pharring Accessing the CWT is very expensive in concurrent code. We saw significant gains by ensuring the CWT is only accessed once per analyzer per compilation.

[pharring]

@sharwell Thanks for the explanation. Yes, the guts of CWT are protected by a single sync object which leads to lock contention in highly concurrent code. That's another reason to avoid it. We will use ConcurrentDictionary<object,object> for the property bag.

[MrJul]

Please consider using strongly-typed accessors around the bag to avoid obvious cast errors.

One pattern I often use for this case:

// sealed to force reference equality
public sealed class DataKey<T> { }

public class PropertyBag
{

    private readonly ConcurrentDictionary<object, object> _data = new ConcurrentDictionary<object, object>(); 

    public void SetData<T>(DataKey<T> key, T value)
    {
        _data[key] = value;
    }

    public bool TryGetData<T>(DataKey<T> key, out T value)
    {
        object untypedValue;
        if (_data.TryGetValue(key, out untypedValue))
        {
            value = (T) untypedValue;
            return true;
        }
        value = default(T);
        return false;
    }

}

Usage:

private static readonly DataKey<MyObject> _myObjectKey = new DataKey<MyObject>();
...
var myObject = new MyObject();
bag.SetData(_myObjectKey, myObject); // write
bag.TryGetData(myObjectKey, out myObject); // read: correctly typed, no cast

All accesses are strongly typed, you don't have to figure out a good unique key, and you can keep that key private.

[pharring]

@MrJul thanks. I agree that having generic helper methods (extension methods on Compilation) would help to avoid casts in caller code. But I don't think that it's necessary to introduce DataKey<T> to achieve that.

The ability to keep the keys private is a requirement, but I think that can be enforced in the callers - or not if they choose to share their well-known keys with others. If they want a private key, then, as you've shown, private static readonly object _myPrivateKey = new object(); is sufficient.

Therefore, I think DataKey<T> adds an extra concept to the interface without any benefit.

Still waiting to hear @gafter's suggestion.

[gafter]

@pharring I intend to do something close to what @MrJul said. I think the type safety is more important than merely hiding casts.

[gafter]

Please see #5708 as a draft of the approach I'm suggesting. I need help with naming.

[pharring]

@tmat asked for a list of use cases:
AnalyzerManager
DependentTypeFinder has two
SemanticModelWorkspaceServiceFactory
AliasSymbolCache has two
MetadataOnlyReference

And of course, the one that @sharwell mentioned above: DotNetAnalyzers/StyleCopAnalyzers#1304

[gafter]

I sense the prevailing wind is "Won't Fix". Is that right?