Add support for ComWrappers-based RCWs to the special native WeakReference support. #35819
Conversation
…rence support. Rename the WinRT weak reference handle to "native COM weak reference" handle.
…zinsky/runtime into comwrappers-weakreference
…appers-weakreference
…into comwrappers-weakreference
| // * be an RCW | ||
| // * respond to a QI for IWeakReferenceSource | ||
| // * succeed when asked for an IWeakReference* | ||
| // | ||
| // Note that *pObject should be GC protected on the way into this method | ||
| IWeakReference* GetWinRTWeakReference(OBJECTREF* pObject) | ||
| IWeakReference* GetComWeakReference(OBJECTREF* pObject) |
There was a problem hiding this comment.
Note that this is non-pay for play API when called from WeakReference constructor. What is this change going to do to performance of WeakReference constructor when called on regular COM object?
There was a problem hiding this comment.
When it's called by a regular COM object, it will go down the first if path and have basically the same performance characteristics as it did previously. Only non-builtin-COM objects will go down the else path that does the additional work.
| { | ||
| return nullptr; | ||
| pWeakReferenceSource = reinterpret_cast<IWeakReferenceSource*>(GetComIPFromObjectRef(pObject, IID_IWeakReferenceSource, false /* throwIfNoComIP */)); |
There was a problem hiding this comment.
Can this ever return non-null when the interopinfo on sync block is NULL? I am wondering whether we can have a short-circuit for null interopinfo up-front (potentially even in the caller of this method) and do this expensive COM related stuff only when the interop info is non-null.
There was a problem hiding this comment.
Or maybe there are even better ways how to make this pay-for-play by e.g. having the types that should participate in this marked with a managed interface.
Still seeing about 5% regression on the benchmark. Can we do better? |
@jkoritzinsky Lets try keeping the IUnknown* unknown = nullptr;
SyncBlock* syncBlock = (*pObject)->PassiveGetSyncBlock();
if (syncBlock != nullptr)
{
unknown = ComWrappersNative::GetIdentityForObject(syncBlock, IID_IWeakReference);
} |
|
Would it be possible to use the sync block check to early our for the built-in COM interop as well? |
…n the object has interop info attached.
|
I added a check before the call to GetComWeakReference and now we get a 9% average speed improvement on my local run of the posted benchmark. |
…ition to and from pre-emptive when calling into the external QI.
|
@jkotas can you do another review pass when you have a chance? |
|
@jkoritzinsky It seems like you have some unrelated changed in here. |
|
@AaronRobinsonMSFT which changes are you referring to? |
|
All the changes in |
|
There was no support on the native side for the “global ComWrappers for tracker support”. Only the global instance for marshalling was exposed in the runtime for the vm to use. |
|
Ah. @elinor-fung only did the marshalling side. That is why it looked so familiar. Thanks. |
ghost
locked as resolved and limited conversation to collaborators
Add support for ComWrappers-based RCWs to the special native WeakReference support.
Rename the WinRT weak reference handle to "native COM weak reference" handle.
Contributes to #35318
Fixes #35745