New Type System.Runtime.CompilerServices.DependentHandle<TPrimary, TSecondary>

[shmuelie]

Background and Motivation

There is an internal struct System.Runtime.CompilerServices.DependentHandle that is currently only used by System.Runtime.CompilerServices.ConditionalWeakTable<TKey, TValue>. The struct allows for a dependency between two objects to be created by a third type without modifying the first two classes, and only using up a pointer's worth of memory in the third type.

I have personally find use for such a class, in my Weak Delegates system for example. While in there I had to use slightly convoluted tricks to make it work I believe a more proper implementation included in the BCL would be useful.

Proposed API

namespace System.Runtime.CompilerServices
{
    public struct DependentHandle
    {
        public DependentHandle(object? target, object? dependent);

        public bool IsAllocated { get; }

        public object? Target { get; set; }
        public object? Dependent { get; set; }

        (object? Target, object? Dependent) TargetAndDependent { get; }

        public void Free();
    }
}

Open Questions

Should DependentHandle implement IDisposable? If so, we should expose the same on GCHandle

Should the constructor be private and have a static Alloc method to match GCHandle?

Should TargetAndDependent be a property or a method?

[svick]

What is the point of having both Free() and Dispose()?

[shmuelie]

No real reason. The internal type has Free() and doesn't implement IDisposable. I added IDisposable and Dispose(). Having Free() doesn't make sense now. I'll remove it.

[karelz]

API review:
We need more details on the use case. Why do we need this?

Random notes:

• Is it better Tuple with Dispose?
• Is the key value to keep object alive?

[shmuelie]

Pretty much it's useful for cases where you could use System.Runtime.CompilerServices.ConditionalWeakTable<TKey, TValue> but only need one entry in the table.

Primary uses cases are caches or linking classes that you can't modify (pretty much the uses cases for System.Runtime.CompilerServices.ConditionalWeakTable<TKey, TValue>`)

[weltkante]

I might add that DependentHandle is the conditional counterpart to already existing GCHandle. You might also want to add the counterpart to WeakReference<T> (maybe name it ConditionalWeakReference<TKey, TValue>) to avoid everyone having to implement his own, but that is just convenience and we could live without that; exposing the core functionality however is not optional.

As far as use-case is concerned, this is very important when implementing libraries for weak event handling or weak delegates. To work around the BCL not exposing DependentHandle all our applications are currently using reflection to get hold of the internal API (similar to what was linked in the original post) because ConditionalWeakTable is too much overhead for us. I'd welcome it if that was not necessary.

[Sergio0694]

Continuing the conversation here from #53296.

As per @jkotas' comment (here):

"[...] Dependent handle performance is not that bad, dependent handles are never going to free so one will have to be always careful about what they are used for. I think we are doing diservice by not exposing dependent handle as is, and forcing people to use workarounds such as unnecessary complex CWT usage patterns where dependent handle would be much simple solution, or unsupported private reflection hacks."

As suggested by @tannergooding, if we could get a comment in this issue as well to sum up why it'd be worth it to expose this type, and whether or not the proposal here is fine or would need any changes, then we could hopefully get this issue marked as ready to review so we can get it looked at in one of the upcoming API review streams, which would be awesome 😄

[tannergooding]

@jkotas, do you think there is additional work/discussion required here with regards to the proposed API?

If the signature looks alright as is (either exactly matching the internal or being "type-safe" as proposed) and there is no expected GC work, this seems relatively simple to mark ready-for-review so we can get in for .NET 6.

Notably it does differ from the internal signature, which is:

internal struct DependentHandle
{
    public DependentHandle(object primary, object? secondary);
 
    public bool IsAllocated { get; }

    public object? GetPrimary();
    public object? GetPrimaryAndSecondary(out object? secondary);
 
    public void SetPrimary(object? primary);
    public void SetSecondary(object? secondary);

    public void Free();
}

[jkotas]

We need to hammer out the details. Here are some questions on my mind:

• Is genericness going to add overhead? Are we going to be able to switch CWT to use the generic wrapper? If not, would it better to make it non-generic like regular GCHandle? Maybe your change to turn Unsafe.* into JIT intrinsics changes the equation here.
• null should be allowed for both primary and secondary everywhere.
• Free vs. Dispose. I think Dispose would be better. I do not really like how GCHandle.Free works, pretty much everybody has to call it as if (handle.IsAllocated) handle.Free(). Separately, it may be nice to add Dispose/IDisposable to GCHandle.
• What is the overriden ToString going to do? I do not think it makes sense to override ToString. GCHandle or WeakReference do not have overridden ToString either.

[tannergooding]

Is genericness going to add overhead? Are we going to be able to switch CWT to use the generic wrapper? If not, would it better to make it non-generic like regular GCHandle?

I don't have a particular preference here. I think given the use-case and the fact that related types, like GCHandle, are using object I'm leaning towards being non-generic here as well. Users can always cast or use Unsafe.As (same as required with GCHandle).

null should be allowed for both primary and secondary everywhere.

👍

Free vs. Dispose. I think Dispose would be better. I do not really like how GCHandle.Free works, pretty much everybody has to call it as if (handle.IsAllocated) handle.Free(). Separately, it may be nice to add Dispose/IDisposable to GCHandle.

I'm fine with Dispose and also exposing it on GCHandle makes sense to me provided there is no push-back from @Maoni0

What is the overriden ToString going to do? I do not think it makes sense to override ToString. GCHandle or WeakReference do not have overridden ToString either.

Makes sense to me.

[Sergio0694]

For reference, this would be the proposed API surface taking into account all of @jkotas' and @tannergooding's comments:

namespace System.Runtime.InteropServices
{
    public struct DependentHandle : IDisposable
    {
        public DependentHandle(object? primary, object? secondary);

        public bool IsAllocated { get; }

        public object? GetPrimary();
        public object? GetPrimaryAndSecondary(out object? secondary);

        public void SetPrimary(object? primary);
        public void SetSecondary(object? secondary);

        public void Dispose();
    }
}

I'll also add that personally I agree with all of that too: the type doesn't really have to be generic and people can always cast or unsafe-cast if needed, and this way the type will have less overhead and complexity, and potentially be more flexible as well. Same about ToString() not actually being needed, and for using Dispose() instead of Free(), with it then also just being a no-op in case you called it multiple times on the same instance (as per IDisposable spec) instead of throwing like GCHandle.Free()).

EDIT: added the namespace to be System.Runtime.InteropServices both for consistency with GCHandle, and also because that's the namespace for DependentHandle today as well. Are there any other open questions on the updated proposal? 🙂

[shmuelie]

Just some quick replies on the OG proposal questions

Is genericness going to add overhead? Are we going to be able to switch CWT to use the generic wrapper? If not, would it better to make it non-generic like regular GCHandle? Maybe your change to turn Unsafe.* into JIT intrinsics changes the equation here.

The genericness was mostly a personal thing (I hate having to use Object) and never did any testing if there was performance hit. While I personally don't like the idea of adding new types that aren't generic, as has been said before, this is such a low-level API that it isn't as big a deal.

null should be allowed for both primary and secondary everywhere.

Agreed. API was simply before C# 8. This is also why I converted the API to a TryXXX pattern, which isn't as useful now.

Free vs. Dispose. I think Dispose would be better. I do not really like how GCHandle.Free works, pretty much everybody has to call it as if (handle.IsAllocated) handle.Free(). Separately, it may be nice to add Dispose/IDisposable to GCHandle.

Sums up what my wrapper did.

What is the overriden ToString going to do? I do not think it makes sense to override ToString. GCHandle or WeakReference do not have overridden ToString either.

Was mostly a debugging helper (Makes it very quick to see if IsAllocated is true). While I agree not much is gained by it, I would replace it with a DebuggerDisplayAttribute.

[weltkante]

I had a similar proposal #30759 (naming it ConditionalWeakReference instead of exposing the internal name, but thats just naming and doesn't matter)

Considering that both proposals are about exactly the same thing under a different name, and I was being told that it going into .NET 6 was not an option without reworking the GC, I'm surprised this suddenly gets so much support. Happy if this gets accepted, in this case you probably can close the other proposal.

Also note that the implementation of DependentHandle in the GC has a bug if the values of a DependentHandle refer back to the DependentHandle. This kind of cycle will apparently never get collected, causing a memory leak. Exposing this as a primitive makes the bug/leak likely more severe. (ConditionalWeakTable is usually used as a static, DependentHandle as a primitive will likely not be used as a static and suffer under the leak more severly.) Refer to #12255 for the original report.

[Sergio0694]

From the comments I see in #12255 and specifically this commment, that doesn't really seem like a bug per se to me, rather than just an implementation detail causing this admittedly unintuitive behavior in that very specific case. That said, @jkotas said here that that's not really a priority and not worth losing performance in ConditionalWeakTable<,> over for, and other than that I'm not sure I follow how that would be related to this proposal. As in, the core of the issue is that the DependentHandle objects being created by ConditionalWeakTable<,> weren't being freed, but that wouldn't really apply in this case since users of DependentHandle would explicitly be responsible for calling Dispose() on it. That is to say, I don't think that is a bug per se, and even if it was considered so, I think that'd only apply to ConditionalWeakTable<,> itself and not this proposal, which seems fine on its own 🙂

[Maoni0]

could someone please explain what scenarios would be made better by implementing IDisposable for this?

I've seen DH scanning show up in CPU profiles quite often with our 1st party workloads (not surprising as scanning DHs has never been optimized ; and DHs are only scanned during the stop-the-world phase of a background GC). also because we haven't gone and optimized this, we also haven't added much instrumentation to help easily show how long the pauses are caused by DH so you will need to rely on CPU profiles (I'm fixing this part in .NET 6 though, since I've seen this causing perf problems). in the doc for this, if it goes in, the perf aspect should absolutely be emphasized. so do your perf measurement diligently if you anticipate to use these at large scale (I've seen over and over again that folks tested something locally and concluded it's ok then got surprised by prod perf because their prod was so much more stressful than local).

[rickbrew]

FWIW I've occasionally had to add methods like Free() to struct types that already implement IDisposable for precisely one reason: otherwise C++/CLI code can't call it (as far as I could figure out anyway, and I wrestled with the compiler for quite a bit on this issue).

C++/CLI prohibits calling foo->Dispose() in favor of requiring you to call delete Foo; (which just calls Dispose()). However, it seems to bug out on structs, still disallowing foo.Dispose() from being called and erroring when you try to use delete Foo; (because, duh, foo isn't a ref type). There is no equivalent to C#'s using construct, either, you have to do the try/finally yourself.

But, C++/CLI is finally able to fade away, especially with all the new goodies in C# 7, 8, 9 (spans, function pointers, etc.) and things like System.Runtime.CompilerServices.Unsafe.

I've only done this for a few "low-level" supporting types that were used in my interop/glue code (e.g. Direct2D et. al. wrappers). And, you can just use a helper method over in C# to enable C++/CLI code to call Dispose(). So I don't think this is something that should be of concern, it's just a historical anecdote.

[weltkante]

that doesn't really seem like a bug per se to me, rather than just an implementation detail causing this admittedly unintuitive behavior in that very specific case

For ConditionalWeakTable its an implementation detail, since its intended to be used as a static value, yet it still has a Finalizer which disposes the DependentHandles - in that context it can be considered a bug, because the Finalizer will never be called even if the entire object graph is no longer accessible. Once you expose DependentHandle it will become more of a problem when people store it in other objects with shorter lifetimes than a static field. From a correctness perspective, using a Finalizer to dispose the DependentHandle is not a valid implementation given the current GC behavior unless you have full control over the entire object graph reachable from the DependentHandle.

I'm not arguing against exposing DependentHandle (I'm already using DependentHandle myself via reflection) - but if its exposed it should be documented that Disposing in a Finalizer will not work reliably (i.e. never finalizes if cycles are present). Otherwise people will test their implementation in simple scenarios and it looks like it works just fine, but in real-life scenarios when cycles start to appear it suddenly leaks.

[Sergio0694]

Replying to @Maoni0:

"could someone please explain what scenarios would be made better by implementing IDisposable for this?"

I'm sure @jkotas will be able to elaborate more on this, but my understanding from the previous conversation is that in particular the way GCHandle.Free() needed to be used was problematic due to the fact that calling it multiple times resulted in a crash, meaning that consumers would always have to manually check IsAllocated and then call Free(), whereas IDisposable.Dispose() as per spec is guaranteed to be fine if called multiple times, and just result in a no-op if the instance has already been disposed. Just a guess, but looking at GCHandle.Free() I'd imagine a Dispose() implementation could just be to do an Intelocked.Exchange call on the internal handle, and then simply do nothing if the returned handle was already null (instead of throwing).

"I've seen DH scanning show up in CPU profiles quite often with our 1st party workloads (not surprising as scanning DHs has never been optimized ; [...] so do your perf measurement diligently if you anticipate to use these at large scale"

We should definitely ensure the docs for DependentHandle contain all the relevant info on the type and how it works, and potential pitfalls that using it might lead to (just like the docs page for GCHandle does). Just to reiterate Jan's point here though (and from his comment here), the thing is that with DependentHandle not being exposed today, consumers needing the functionality are still not blocked, but they just have to go through the extra hoops of either using CWT and getting even more overhead on top of what they need, or to simply hack DependentHandle through reflection (which then also obviously make their code way more brittle and not really portable anymore), so as far as I can tell the whole situation is just worse off as things are today.

Replying to @weltkante:

"[...] (i.e. never finalizes if cycles are present). Otherwise people will test their implementation in simple scenarios and it looks like it works just fine, but in real-life scenarios when cycles start to appear it suddenly leaks."

From what I can see in the linked issue (here), and also from some tests I run locally, it isn't actually the case that the issue is caused by cycles per se, rather than primary objects in dependent handles remaining alive, and therefore keeping the entire dependent object graph alive. That is to say, the repro from that issue is:

object key = new object();
while (true) {
    var table = new ConditionalWeakTable<object, Tuple<object, byte[]>>();
    table.Add(key, new Tuple<object, byte[]>(table, new byte[1000000]));

    GC.Collect();
}

Which causes a leak, yes, but only because the key is still alive. I can definitely agree that might be unintuitive given that the CWT instances have been thrown away and that the behavior is a result of the dependent handles only being freed from the CWT finalizer (which is an implementation detail, sure), which is not running here since key is alive, but I'm saying I don't really think this is a bug in general. If you change that code to this:

object key = new object();

for (int i = 0; i < 10_000; i++)
{
    var table = new ConditionalWeakTable<object, Tuple<object, byte[]>>();
    table.Add(key, new Tuple<object, byte[]>(table, new byte[1000000]));

    GC.Collect();
}

key = null;

GC.Collect();

Then it'll allocate about 10GB of garbage, then properly collect everything just fine. I'm just saying, just for clarity, that we should not refer to this behavior as a bug, as it really isn't - DependentHandle is doing exactly what it's saying it does 😄

Likewise, reference cycles are actually handled too, eg. if you try this:

while (true)
{
    var foo = new { Table = new ConditionalWeakTable<object, Tuple<object, byte[]>>() };

    foo.Table.Add(foo, new Tuple<object, byte[]>(foo, new byte[1000000]));

    GC.Collect();
}

There will be no memory leaks either, and all the object graphs created for each iteration will be collected as well just fine 🙂

NOTE: this would be more related to #12255 specifically, but figured I'd mention this in this issue since as well since it's still related to DependentHandle anyway, and it's stuff we might want to at least mention in some remarks in the API docs 👍

[Maoni0]

I agree with @KevinCathcart this is not an ephemeron so we shouldn't call it so.

as far as Disposable goes, my intention was obviously not to say that it's not popular because it doesn't exist, it's that it's not a common scenario that one would want to use using on a GC handle. I'm sure you can always find a few cases where people want to do something, my point was how common it is. I would think the answer is not for GCHandle but it wouldn't surprise me a huge amount if someone proved me wrong. however as I said my main opposition there is not about whether to use Dispose or not; it's more about having inconsistency.

[jkoritzinsky]

I can definitely think of a few cases in interop where being able to use using for a GC handle would be convenient. Let's take a scenario where the native code takes a callback that takes a pointer-sized state parameter, and the callback is guaranteed to be executed before the native call returns. Something like the following:

void DoCallback(void* state, void(*callback)(void*));

If the developer in C# wants to use this API with the least overhead, they would define a P/Invoke to it as follows:

[DllImport("nativelib")]
public static unsafe extern void DoCallback(IntPtr state, delegate*<IntPtr, void> callback);

And the callback:

[UnmanagedCallersOnly]
public static void Callback(IntPtr state)
{
      GCHandle handle = GCHandle.FromIntPtr(state);
      object state = handle.Target;
}

Then they could use it as follows:

using (GCHandle handle = GCHandle.Alloc(state))
{
    DoCallback(handle, &Callback);
}

This style is common enough that the example given for using GCHandles in the docs does this exact pattern (without the using of course).

[Maoni0]

@jkoritzinsky I agree it's a bit more convenient to do it the using way for this example, to save a handle.Free call. I'm guessing the reason why I had never heard a request, until folks mentioned on this issue, to have GCHandle implement IDisposable is because when there's a scenario like what you pointed out, it's straightforward to just write handle.Free - it's well scoped and usually the scope is very small like in your example. but I really have no strong opinion here, if you tell me that "providing the convenience here is very important" I will gladly accept that :-)

[jkotas]

it's straightforward to just write handle.Free

It is not that straightforward. You have to typically write it in try finally to ensure that the handle is disposed even when the exception thrown - the example in the GCHandle docs is not 100% production quality code because of it can leak. using generates all this boilerplate for you.

[Sergio0694]

@jkotas Now that we've updated the API proposal and settled on one that from what I can see the team thinks is fine, what do you consider the risk be to ship this in .NET 6? In other words, could we get this added to the .NET 6 milestone at this point? 🙂
(I would also be happy to contribute the PR (after API review) if the team wanted to make this up for grabs)

For some more context (following our conversation from my previous proposal #53296): I'm planning to add the .NET 6 target to the Windows Community Toolkit, and at the same time I'm working on a new major version of the MVVM Toolkit we're shipping as part of it (don't have an exact timeline yet but likely by the end of the year). Having access to DependentHandle is important for us to be able to finally achieve a non-allocating broadcast functionality in our WeakReferenceMessenger type, which is something we'd really like to achieve also as a differentiating factor with other MVVM libraries exposing some event aggregator / message bus functionality. Here's the latest benchmarks for WeakReferenceMessenger specifically, which is our default messenger type: we've optimized this quite a bit already (and I have more planned for vNext) but currently the lack of a way to enumerate ConditionalWeakTable<K, V> without allocating an enumerator every single time is blocking us to just get that down to 0. With DependentHandle being public I plan to just make an internal copy of CWT<,> to use specifically there, with a value enumerator that would solve the issue (and without exposing that as an API as originally proposed, which I agree would've been less ideal).

Method	Mean	Error	Ratio	Gen 0	Allocated
MVVMToolkit	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

With the cutoff date for new features into .NET 6 being about July 17th and getting close and there being a number of other blocking issues already, just wanted to make sure this proposal was being considered for this next release once the schedule for API review gets past these next blocking issues that are there. Thanks! 😄

cc. @michael-hawker

[jkotas]

do you consider the risk be to ship this in .NET 6? In other words, could we get this added to the .NET 6 milestone at this point?

I do not see this a particularly risky. As you have said, we are getting close to cut off, so the problem is the band-width. It is up to @dotnet/area-system-runtime-compilerservices to decide whether they have band-width to deal with this.

[michael-hawker]

The MVVM Toolkit messenger class is pretty amazing. It's super simple to use, but really powerful and easy to use anywhere.

It's been really amazing to see @Sergio0694's work on optimizing it's performance and it's really a big differentiation for the library for these APIs.

To say we have zero allocations would be a game changer for highlighting this differentiation for the Windows Community Toolkit.

[bartonjs]

Video

• Based on the usage, there's a recommendation to change the Target and Dependent properties to method pairs to avoid debugger inspection introducing Heisenbugs.
• There are a lot of mixed feelings about the TargetAndDependent property. The most popular opinion is to be GetTargetAndDependent(), but "be Decompose" and "be a property" also have their camps.
• There was a suggestion that Free should be Dispose, but there is pushback on that.
  • "Dispose" should be from IDisposable.
  • IDisposable.Dispose() should support multiple calls without negative side effects.
  • Since the idempotentcy seems unlikely given the raw state of this type, it should not be IDisposable.
• Recommend moving the type to System.Runtime since it has nothing / very little to do with compilers, but has to do with garbage collection.

namespace System.Runtime
{
    public struct DependentHandle
    {
        public DependentHandle(object? target, object? dependent);

        public bool IsAllocated { get; }

        public object? Target { get; set; }
        public object? Dependent { get; set; }

        (object? Target, object? Dependent) TargetAndDependent { get; }

        public void Free();
    }
}

[tannergooding]

Modulo the Free vs Dispose discussion since this isn't idempotent, the points left above are:

• API review has recommendations but which option is chosen (keep as is or change as recommended) is being left up to @jkotas to decide "yay" or "nay" as best appropriate for the API

[jkotas]

Target and Dependent properties to method pairs to avoid debugger inspection introducing Heisenbugs.

VS debugger inspection affects GC behavior in number of subtle ways. It is by design and I do not see it as a problem. GCHandle.Target has the same hypothetical problem and I do not recall anybody ever reporting it as an issue.

I do not have a strong opinion on the method vs. properties. It is purely about ones artistic preferences as mentioned during the API review.

IDisposable

@bartonjs I was not able to follow your argument against IDisposable / Dispose.

We have recent prior art such as struct MemoryHandle : IDisposable. What is the difference between MemoryHandle and this one that makes Dispose/IDisposable to be ok on one, but not the other?

[jkotas]

IDisposable.Dispose() should support multiple calls without negative side effects.
Since the idempotentcy seems unlikely given the raw state of this type, it should not be IDisposable

It is very straightforward to make the Dispose implementation idempotent. Dispose will do nothing if the internal handle that this wraps is null, and set the internal handle pointer to null otherwise. It will behave same way as Dispose on MemoryHandle.

[Sergio0694]

As mentioned above, I'd be happy to pick this up and work on a PR (especially given that we're relatively tight on time), and we can always address/incorporate further feedbacks as they come. Agreed with @jkotas that making Dispose() safe would be fine, we can just do Interlocked.Exchange with null and branch, and get a no-op after the first call (like other types do as well) 😄

Just to recap, the currently proposed API surface (with updates) would be this then:

namespace System.Runtime
{
    public struct DependentHandle : IDisposable
    {
        public DependentHandle(object? target, object? dependent);

        public bool IsAllocated { get; }

        public object? Target { get; set; }
        public object? Dependent { get; set; }

        public (object? Target, object? Dependent) GetTargetAndDependent();

        public void Dispose();
    }
}

I've added IDisposable as discussed (can always change this mid-PR), and changed the TargetAndDependent property to be a method since Jan said either is fine for him, and I remember @bartonjs saying he'd prefer a method during API review.

Pinging @tannergooding as he's been driving this during API review as well, feel free to assign the issue to me if it's alright 🙂

[jkotas]

making Dispose() safe would be fine, we can just do Interlocked.Exchange with null and branch

It makes it thread-safe, not generally safe.

[Sergio0694]

Right, I meant it'd make it safe with respect to single or multi-threaded disposals to the same instance. If the value was copied in multiple places (including boxing, etc.) then it'd still fall apart on multiple disposals, sure. Not sure if that'd be a concern in this case considering the fact it's a very specialized type? Happy to change the implementation there with any feedbacks 🙂

[bartonjs]

then it'd still fall apart on multiple disposals, sure. Not sure if that'd be a concern in this case considering the fact it's a very specialized type?

That's a concern that I have, yes. Especially because I don't have a gut feeling for what happens when multiple value-copies get disposed. If it's bad, like a double-Free, then manually calling Free is better (to me) -- because then it's a clear "stop, and think about ownership here". If that's actually guaranteed safe, then my concern goes away.