Proposal: ref struct ConditionalWeakTable<TKey, TValue>.GetEnumerator()

[Sergio0694]

Background and Motivation

The ConditionalWeakTable<TKey, TValue> is an extremely useful type that allows developers to leverage GC dependent handles (which are not directly available), along with some other nice features on top (eg. auto trimming, thread-safety by default, etc.). There is currently one drawback though when trying to use this type in code that's trying to be as fast and memory efficient as possible, which is that it is not possible to enumerate key-value pairs in a ConditionalWeakTable<TKey, TValue> table without incurring in unwanted allocations and extra overhead. Specifically:

• The GetEnumerator implementation returns a reference type enumerator, incurring in one allocation
• The enumerator type also has a finalizer, which makes the allocation slower even though it's not actually needed in most cases (given that when using foreach, Dispose() is always called anyway, which suppresses the finalizer).

Proposed API

This proposal is to expose a value-based enumerator, allowing developers to enumerate key-value pairs in ConditionalWeakTable<TKey, TValue> instances without incurring in any allocations at all, and with also some nice performance improvements on top (which are just a natural consequence of moving to a value-based enumerator in this case and removing the two performance penalties mentioned before).

The proposed API is this one:

namespace System.Runtime.CompilerServices
{
    public sealed class ConditionalWeakTable<TKey, TValue> : IEnumerable<KeyValuePair<TKey, TValue>>
        where TKey : class
        where TValue : class?
    {
+       public Enumerator GetEnumerator();
        
+       public ref struct Enumerator
+       {
+           public KeyValuePair<TKey, TValue> Current { get; }
+
+           public bool MoveNext();
+           public void Dispose();
+       }
    }
}

This is perfectly safe when used through the foreach pattern, given that the C# compiler will still generate the same calls as when using a normal iterator, with the same try/finally blocks and the final Dispose() call (which is needed here to decrement the internal reference count for the ConditionalWeakTable<TKey, TValue> instance).

I'd be happy to contribute this feature myself in case the proposal was approved 🙌

Usage Examples

We're using ConditionalWeakTable<TKey, TValue> in the WeakReferenceMessenger type in the MVVM Toolkit. Currently this type incurs in allocations when broadcasting messages solely for the Enumerator type being instantiated every single time. As a test, I've hacked the DependentHandle type in a benchmark project (by defining it in a dummy System.Private.CoreLib project and using [IgnoreAccessChecksTo] and PrivateAssets="All" so that the MVVM Toolkit would load the true one from CoreCLR instead, see here, and then I copied the ConditionalWeakTable<TKey, TValue> source to it while adding the new API mentioned above to it. I then proceeded to run the same benchmark (see here) across the original implementation of WeakReferenceMessenger, and one using this modified ConditionalWeakTable<TKey, TValue> implementation:

Method	Mean	Error	StdDev	Ratio	Allocated
Original	21.79 ms	0.296 ms	0.277 ms	1.00	192,029 B
Improved	18.66 ms	0.216 ms	0.191 ms	0.86	-

✅ Completely allocation free broadcast
✅ About 15% faster too while at it, which doesn't hurt

I reckon there might be many other scenarios where library authors are using ConditionalWeakTable<TKey, TValue> as well where the ability to iterate key-value pairs without incurring in extra allocations would be very welcome as well.

Alternative Designs

While this would still be perfectly safe under normal usage, I understand that it's still technically slightly less safe than the normal enumerator, given that developers could in theory (but also, why?) do weird stuff like copying the enumerator, use it manually outside of a foreach block, etc. To that I have two possible answers:

1. The ConditionalWeakTable<TKey, TValue> is already in the System.Runtime.CompilerServices namespace, which is intrinsically considered "unsafe" and "use at your own risk" in general, so it's assumed that developers using this type (or any of the others in this namespace in general) would know what they're doing. Because of this, I think adding this new API directly on ConditionalWeakTable<TKey, TValue> would be fine.
2. In case this proposed API was still considered not safe enough to add to ConditionalWeakTable<TKey, TValue> directly, an alternative proposal could be to just move the API to CollectionMarshal instead. This would be clunkier (the enumerator type would have to either be moved too or just not be usable directly from ConditionalWeakTable<TKey, TValue>, and it'd also need a new GetEnumerator() method on it just returning this, in order to be usable in a foreach block), but in turn it'd entirely separate this slightly more unsafe API into this new special-purposed type (which also contains a bunch of other unsafe APIs for other collection types).

I'd personally argue for 1) given the above (and especially because it'd make the new enumerator much less verbose to use), but at the end of the day I'd be happy with either as long as the core functionality is present, which is true in both cases 😄

Risks

Very low, as mentioned above. It'd be a niche API on an already niche type, in a niche namespace that's already being used to host all sorts of weird and unsafe APIs. Developers ever finding this should already be very much aware of how to use it properly (especially given that we would also include detailed XML docs to call out all the various potential pitfalls for incorrect usage).

Additional context

For reference, as mentioned above, we could use this API to completely remove memory allocations in our WeakReferenceMessenger implementation in the MVVM Toolkit (part of the Windows Community Toolkit). We particularly focused on speed and efficiency in our messenger types, which are currently far ahead of all other available MVVM libraries (eg. Prism, Caliburn, etc.). Here's some benchmarks with our current implementations:

BenchmarkDotNet=v0.13.0, OS=Windows 10.0.19042.1023 (20H2/October2020Update)
AMD Ryzen 7 2700X, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.100-preview.4.21255.9
  [Host]     : .NET 5.0.6 (5.0.621.22011), X64 RyuJIT
  DefaultJob : .NET 5.0.6 (5.0.621.22011), X64 RyuJIT

Method	Mean	Error	Ratio	Gen 0	Allocated
MVVMToolkitStrong	6.428 ms	0.0108 ms	0.08	-	-
MVVMToolkitWeak	20.883 ms	0.3085 ms	0.25	31.2500	192,009 B
MVVMLight	82.511 ms	1.5461 ms	1.00	10857.1429	45,920,081 B
Prism	216.294 ms	2.1281 ms	2.62	18000.0000	76,096,445 B
CaliburnMicro	465.634 ms	2.7784 ms	5.65	95000.0000	397,825,336 B

This proposed API would allow us to both completely eliminate allocations in the broadcast method for the weak messenger, as well as reducing the performance gap between our two messenger types (cc. @michael-hawker, Windows Community Toolkit lead).

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[michael-hawker]

Thanks @Sergio0694 for filing this! It's really amazing to see opportunities you've identified in .NET from the work you've done in the Toolkit. Let me know if there's anything I can do to help.

[Sergio0694]

Thanks @michael-hawker! Really hoping we can get this in so we can go completely allocation-free on .NET 6 😄
As the next steps here, we'll need an area owner (eg. @joperezr, who I see has already had a look at this proposal) to mark it as ready for review, so that when it passes the live API reviews (assuming it does, of course), I can just open a PR from my branch (I've already implemented these changes in my fork and I'm keeping it on par with main for now) 👍
I believe the cutoff date for .NET 6 is around July 17th to merge PRs, so we still have plenty of time to do all this.

Additional context

Leaving some more info here while I'm at it. The rationale for this proposal is twofold:

• Provide a noticeable memory usage improvement for ConditionalWeakTable<TKey, TValue> consumers
• Not introducing any kind of breaking changes or larger concerns with new APIs

With respect to the second point specifically, I'm aware there are two separate proposal to expose DependentHandle publicly (ie. #19459 and #30759), but as expressed by @Maoni0 in a comment (here), there's a concern in exposing DependentHandle directly as it'd require GC work to make it efficient enough (and that work is not coming in .NET 6), meaning neither of those proposals really has a chance of making it in .NET 6. With this one instead:

• There is no concern with new public types, as we're just using the existing ConditionalWeakTable<TKey, TValue> type
• There are no source/binary breaking changes (the existing enumerators were explicit interface implementations, thankfully)
• The new value-enumerator pattern really just offers a "free" performance/memory improvement to consumers. The fact the proposed type is a ref struct means we also don't have to worry about it accidentally being boxed. It would really only be used directly in a foreach block, with the compiler producing all the necessary logic to properly dispose it. The only possible way it could cause issues is if developers deliberately used it explicitly and incorrectly, which is (1) against the documentation and (2) we're already in System.Runtime.* anyway, which contains a whole bunch of (much more) unsafe APIs and already assumes developers should know how to use APIs in this namespace properly.
• To address @GrabYourPitchforks's point on Discord, that this issue could also be solved by some other data type (in this case, by just having DependentHandle being public), I agree that that'd be even better, but as mentioned above too this is still a good stopgap for the time being, and it'd also still offer a nice performance/memory boost that people that would still require the use of ConditionalWeakTable<TKey, TValue> even if DependentHandle was public. That is to say, I believe there's value in this proposal even with the assumption that DependentHandle will become public in the future.

This overall just seems like a good improvement for ConditionalWeakTable<TKey, TValue> users, with little to no risks and with an easy implementation, without any big API changes nor potentially problematic new types (such as DependentHandle) 😊

[Joe4evr]

What exactly necessitates that this enumerator be a stack-only struct?

[Sergio0694]

@Joe4evr having it be a ref struct enables the disposable ref struct-s feature from C# 8, so that the compiler will generate the try/catch block with a call to Dispose automatically when using a foreach statement (see here). Without ref instead, that Dispose method would just be ignored entirely (see here), and the enumerator would not be disposed, hence breaking the parent ConditionalWeakTable<TKey, TValue> instance. Additionally, it also gives us the guarantee that the enumerator can't be boxed, hence reducing the chance of misuse (also boxing this enumerator would defeat the whole point anyway) 🙂

[stephentoub]

having it be a ref struct enables the disposable ref struct-s feature from C# 8, so that the compiler will generate the try/catch block with a call to Dispose automatically when using a foreach statement (see here). Without ref instead, that Dispose method would just be ignored entirely (see here), and the enumerator would not be disposed

With a normal struct you'd just add : IDisposable. That C# feature was added for ref structs because they can't implement interfaces.

[Sergio0694]

Of course, but given that boxing that would both defeat the point of this enumerator (the existing one works well for that already) while still be more dangerous (eg. it wouldn't have the finalizer, so if you forgot to dispose it yourself you'd still end up breaking the table), I figured making it a ref struct would still add some more safety for this specific scenario and just make it better? 🤔

[stephentoub]

but given that boxing that would both defeat the point of this Enumerator

There's no boxing.

while still be more dangerous (eg. it wouldn't have the finalizer, so if you forgot to dispose it yourself you'd still end up breaking the table

foreach will use the Dispose in both cases. And if you manually enumerated, you'd still be on the hook for calling Dispose yourself in both cases.

[Sergio0694]

"There's no boxing."

That's not what I meant, let me rephrase that 😄
I'm aware that if you have a struct type implementing IDisposable, you don't get boxing when used in a using/foreach, since the Dispose call will just be a constrained callvirt and no boxing will be needed, that is fine. What I meant is that given that this enumerator type is inherently less safe than the existing reference type one, making it a ref struct just seemed to at least reduce the chance of errors in general (as in, you just couldn't possibly box it by accident if you were to use it directly).

That said, other than that I have no issues with just making it non-ref and implementing IDisposable, as you said the functionality and codegen would just remain exactly the same when used properly anyway. If someone really wanted to shoot themselves in the foot with this, that'd still be possible in either case anyway, and I don't think the API should worry about those cases too much anyway (especially given in what namespace we're in). I'm happy to change the proposal by adding the interface and removing the ref modifier if you prefer 👍

[Joe4evr]

making it a ref struct just seemed to at least reduce the chance of errors in general (as in, you just couldn't possibly box it by accident if you were to use it directly).

Most BCL collection types have regular struct enumerators, and I haven't heard anything about them being less safe or error-prone for users, and if they get boxed in favor of convenience (eg. LINQ), so be it. ¯\_(ツ)_/¯