-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switch CoreCLR WeakReference to unified managed implementation #77196
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
src/libraries/System.Private.CoreLib/src/System/WeakReference.T.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/WeakReference.T.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/WeakReference.T.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/WeakReference.T.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/WeakReference.T.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/WeakReference.cs
Outdated
Show resolved
Hide resolved
On a microbenchmark with common/ordinary objects I see:
The reasons why managed Set is a bit slower:
Microbenchmark that I use: using System.Diagnostics;
internal class Program
{
private static void Main(string[] args)
{
object o = new object();
WeakReference<object>[] warr1 = new WeakReference<object>[1000];
for(int i = 0; i < warr1.Length; i++)
{
warr1[i] = new WeakReference<object>(o);
}
WeakReference<object>[] warr2 = new WeakReference<object>[1000];
for (int i = 0; i < warr2.Length; i++)
{
warr2[i] = new WeakReference<object>(null);
}
for (; ; )
{
TestGetObj(warr1);
TestSetObj(warr1);
TestGetNull(warr2);
System.Console.WriteLine();
}
}
private static void TestGetObj(WeakReference<object>[] warr1)
{
Stopwatch sw = Stopwatch.StartNew();
for (int iter = 0; iter < 1000000; iter++)
{
for (int i = 0; i < warr1.Length; i++)
{
if (!warr1[i].TryGetTarget(out _))
{
throw null;
}
}
}
sw.Stop();
System.Console.WriteLine("Get obj:" + sw.ElapsedMilliseconds);
}
private static void TestSetObj(WeakReference<object>[] warr1)
{
Stopwatch sw = Stopwatch.StartNew();
for (int iter = 0; iter < 1000000; iter++)
{
for (int i = 0; i < warr1.Length; i++)
{
warr1[i].SetTarget(warr1);
}
}
sw.Stop();
System.Console.WriteLine("Set obj:" + sw.ElapsedMilliseconds);
}
private static void TestGetNull(WeakReference<object>[] warr2)
{
Stopwatch sw = Stopwatch.StartNew();
for (int iter = 0; iter < 1000000; iter++)
{
for (int i = 0; i < warr2.Length; i++)
{
if (warr2[i].TryGetTarget(out _))
{
throw null;
}
}
}
sw.Stop();
System.Console.WriteLine("Get null:" + sw.ElapsedMilliseconds);
}
} Sample output with changes (based off main branch), lower is better:
Baseline - Recently published 7.0 bits:
|
I think this PR is ready to be reviewed/merged. |
return Unsafe.As<ComAwareWeakReference>(GCHandle.InternalGet(taggedHandle & ~HandleTagBits)).Target; | ||
} | ||
|
||
[MethodImpl(MethodImplOptions.NoInlining)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very trivial method. Is it really worth it to disable inlining for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason. It is not even a part of some hot path any more, so there is little difference if it is inlined or not. I will let JIT decide.
|
||
~ComAwareWeakReference() | ||
{ | ||
GCHandle.InternalFree(_weakHandle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set _weakHandle
to make sure that we crash in case there is a handle resurrection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class is intentionally internal - to make sure we control use of its instances. We should never expose it publicly and each instance should have only one reference to it - from the corresponding WeakReference
via a strong handle.
That should guarantee:
- noone can do
lock(this)
and therefore we can uselock(this)
ourselves. - finalization is truly final. Since the owning handle is severed when WeakReference is finalized, the ComAwareWeakReference instance is completely unreachable from other objects when it is finalized.
I think we do not need to worry about resurrection, finalization while still constructing, use after finalization, finalization concurrent with use and other finalization issues.
These assumptions would not work with publicly exposed instances, but I think it is safe to assume here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it should all work fine when there are no bugs. Setting the _weakHandle
to null is a very cheap way to ensure that we get a nice crash in case there are bugs.
|
||
~ComInfo() | ||
{ | ||
Marshal.Release(_pComWeakRef); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set _pComWeakRef
to null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use the same assumptions as with ComAwareWeakReference
- the instance should be finalized only once and be completely unreachable after finalization, so we do not need to zero the references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#77196 (comment) applies here as well.
if (pComWeakRef == 0) | ||
return null; | ||
|
||
return new ComInfo(pComWeakRef, wrapperId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will leak pComWeakRef
reference when new ComInfo
fails (very rare).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, could be worth wrapping in try/catch - just in case there is OOM or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to care about a possibility of asynchronous exception (like a ThreadAbort) after the instance is created and before the constructor finishes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we generally do not harden code against ThreadAbort any more, so only OOM is an actionable concern here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not care about thread abort.
* underlying COM object as long as it has not been released by all of its strong | ||
* references. | ||
*/ | ||
HNDTYPE_WEAK_NATIVE_COM = 9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GC/VM interface change. cc @dotnet/gc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change looks ok, assuming this enum is not shared between clrgc and coreclr? We need to ensure that latest clrgc continues to work with .net 7 runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything in gcinterface.h is shared between clrgc and coreclr.
We need to ensure that latest clrgc continues to work with .net 7 runtime.
The changes under src/coreclr/gc need to be reverted if you would like to maintain this invariant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
continues to work with .net 7 runtime
Is this a temporary requirement?
Have we already revved the API version for the frozen string literals work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have added a new method to IGCHeap as part of the frozen literals work, that does not prevent new clrgc + old coreclr from working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. Considering how much GC and VM are coupled, it feels like compat requirement like this is not sustainable for long. Hopefully this is just for the short term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why that's not sustainable. we just don't delete things from the interface. yes, it'd be a bit nice to be able to delete things but considering the benefit it seems like the right tradeoff.
} | ||
|
||
// see: syncblk.h | ||
private const int IS_HASHCODE_BIT_NUMBER = 26; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is CoreCLR-specific. It would be better for it to live in CoreCLR-specific file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check if this can be factored out.
This will apply to NativeAOT as well, when it supports COM weak references, but may not be ever applicable to Mono.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will apply to NativeAOT as well
Actually I am not sure. I kind of assumed the interop info is handled the same way, but that is not necessarily so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AaronRobinsonMSFT can you comment on support for COM weak reference support in NativeAOT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think support makes sense longer term - particularly as we move forward on ComWrappers
. I mentioned support in some Not Yet Implemented behavior in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should support weak COM references at some point within the context of overall COM support in NativeAOT.
I assume it is just a matter of dev cost and prioritization.
That is why I have added NYI call outs for NativeAOT here - so it would be clear where it will plug in.
src/libraries/System.Private.CoreLib/src/System/ComAwareWeakReference.cs
Show resolved
Hide resolved
{ | ||
FCThrow(kNullReferenceException); | ||
FC_INNER_RETURN(IWeakReference*, ObjectToComWeakRefFramed(pObject, pWrapperId, GetEEFuncEntryPointMacro(ComAwareWeakReferenceNative::ObjectToComWeakRef))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather split this into a FCall without HelperMethodFrame for the fast path, and regular QCall for the heavy lifting. (One of my very long-term goals is to get rid of all FCalls with HMF.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually considered making the fast path check entirely in managed code. I already check for SyncBlock presence, so could check for interop info as well. I had concerns about whether that would be too much looking into native details from the managed code or whether that is safe if GC happens.
I will take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually considered making the fast path check entirely in managed code.
I think it would be pushing too much sync block details to managed code. I do not think we want to do that at this point.
@@ -7858,10 +7858,6 @@ void CALLBACK DacHandleWalker::EnumCallbackSOS(PTR_UNCHECKED_OBJECTREF handle, u | |||
data.Type = param->Type; | |||
if (param->Type == HNDTYPE_DEPENDENT) | |||
data.Secondary = GetDependentHandleSecondary(handle.GetAddr()).GetAddr(); | |||
#ifdef FEATURE_COMINTEROP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that SOS no longer has insight into weak references?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regular weak references did not change from the introspection point of view.
The thing that changed is that there is no need to special case native COM weak handles, since they no longer exist. The implementation uses regular managed objects now and they will be reachable in SOS walks like any other managed object.
internal static object? ComWeakRefToObject(IntPtr pComWeakRef, long wrapperId) | ||
{ | ||
// NativeAOT support for COM WeakReference is NYI | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this throw instead? I'm assume this impl now means the it is impossible to get back to a managed object from a WeakReference
. This makes sense I suppose because of the lack of general support, but I'd prefer a NotImplementedException
to make this clear rather than a silent response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think throwing is better.
I think all comments on this PR have been addressed. Let me know if I missed something. |
You can add a comment next to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice!
Co-authored-by: Maoni Stephens <[email protected]>
Thanks!!! |
} | ||
#endif // FEATURE_COMINTEROP | ||
GCPROTECT_END(); | ||
retRcw.Set(rcwRef); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VSadov I have not tried running this yet, but it does look a little odd to read from retRcw
right after turning off the GCPROTECT
on the variable. Maybe it is is OK because the GC is coop mode?
I could not find another example of ordering the GCPROTECT_END
before the ObjectHandleOnStack::Set, but I could a couple examples of turning it off after setting:
runtime/src/coreclr/vm/marshalnative.cpp
Lines 1307 to 1308 in 40df8f6
retType.Set(orType); | |
GCPROTECT_END(); |
runtime/src/coreclr/vm/assemblynative.cpp
Lines 824 to 826 in 40df8f6
retTypes.Set(orTypes); | |
GCPROTECT_END(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reference needs protection as long as we may be making calls that could trigger GC. In coop mode GC can happen only if we trigger it.
ObjectHandleOnStack::Set
does not trigger GC, so we can stop protecting before calling it.
This can be changed as a matter of style or consistency, but I think either way is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining coop mode!
Re: #75587
This is mostly about making the managed implementation aware of COM/
IWeakReference
. Switching by itself is trivial.COM/IWeakReference
support to the unified managed implementation (under #ifdef)HNDTYPE_WEAK_NATIVE_COM