Reduce weak ref blocking with java interop

src/coreclr/gc/env/gcenv.object.h

@@ -21,6 +21,9 @@ extern bool g_oldMethodTableFlags;
 // Bits stolen from the sync block index that the GC/HandleTable knows about (currently these are at the same
 // positions as the mainline runtime but we can change this below when it becomes apparent how NativeAOT will
 // handle sync blocks).
+#ifdef FEATURE_JAVAMARSHAL
+#define BIT_SBLK_BRIDGE_PENDING             0x80000000
+#endif // FEATURE_JAVAMARSHAL
 #define BIT_SBLK_GC_RESERVE                 0x20000000
 #define BIT_SBLK_FINALIZER_RUN              0x40000000
 

src/coreclr/gc/gcbridge.cpp

@@ -261,6 +261,7 @@ static DynPtrArray g_scanStack, g_loopStack;
 // Objects from crossref handles registered with RegisterBridgeObject
 static DynPtrArray g_registeredBridges;
 static DynPtrArray g_registeredBridgesContexts;
+static DynPtrArray g_registeredBridgeHandles;
 
 // As we traverse the graph, which ColorData objects are accessible from our current position?
 static DynPtrArray g_colorMergeArray;
@@ -1086,10 +1087,12 @@ static void BridgeFinish()
 #endif
 }
 
-void BridgeResetData()
+void BridgeResetData(bool resetPendingBridgeHandles)
 {
     DynPtrArrayEmpty(&g_registeredBridges);
     DynPtrArrayEmpty(&g_registeredBridgesContexts);
+    if (resetPendingBridgeHandles)
+        DynPtrArrayEmpty(&g_registeredBridgeHandles);
     DynPtrArrayEmpty(&g_scanStack);
     DynPtrArrayEmpty(&g_loopStack);
     EmptyObjectBuckets();
@@ -1105,6 +1108,17 @@ void RegisterBridgeObject(Object* object, uintptr_t context)
     DynPtrArrayAdd(&g_registeredBridgesContexts, (void*)context);
 }
 
+void RegisterPendingBridgeHandle(uintptr_t handle)
+{
+    DynPtrArrayAdd(&g_registeredBridgeHandles, (void*)handle);
+}
+
+uintptr_t* GetPendingBridgeHandles(size_t* count)
+{
+    *count = (size_t)DynPtrArraySize(&g_registeredBridgeHandles);
+    return (uintptr_t*)g_registeredBridgeHandles.data;
+}
+
 uint8_t** GetRegisteredBridges(size_t* pNumBridges)
 {
     *pNumBridges = (size_t)g_registeredBridges.size;
@@ -1298,6 +1312,12 @@ MarkCrossReferencesArgs* ProcessBridgeObjects()
 
     ResetObjectsHeader();
 
+    for (int i = 0; i < DynPtrArraySize(&g_registeredBridges); i++)
+    {
+        Object* object = (Object*)DynPtrArrayGet(&g_registeredBridges, i);
+        object->GetHeader()->SetBit(BIT_SBLK_BRIDGE_PENDING);
+    }
+
     BridgeFinish();
 
     return args;

src/coreclr/gc/gcbridge.h

@@ -9,7 +9,7 @@
 #include "common.h"
 #include "gcinterface.h"
 
-void BridgeResetData();
+void BridgeResetData(bool resetPendingBridgeHandles);
 MarkCrossReferencesArgs* ProcessBridgeObjects();
 
 // Decides whether this collection should hand a fresh set of cross references to the client.
@@ -18,6 +18,8 @@ MarkCrossReferencesArgs* ProcessBridgeObjects();
 bool ShouldProcessBridgeObjects();
 
 void RegisterBridgeObject(Object *object, uintptr_t context);
+void RegisterPendingBridgeHandle(uintptr_t handle);
+uintptr_t* GetPendingBridgeHandles(size_t* count);
 uint8_t** GetRegisteredBridges(size_t *pNumBridges);
 
 #endif // FEATURE_JAVAMARSHAL

src/coreclr/gc/gcimpl.h

@@ -334,6 +334,8 @@ class GCHeap : public IGCHeapInternal
     virtual int RefreshMemoryLimit();
 
     virtual void NullBridgeObjectsWeakRefs(size_t length, void* unreachableObjectHandles);
+
+    virtual uintptr_t* GetPendingBridgeHandles(size_t* count);
 };
 
 #endif  // GCIMPL_H_

src/coreclr/gc/gcinterface.h

@@ -1074,6 +1074,8 @@ class IGCHeap {
     virtual void DiagWalkHeapWithACHandling(walk_fn fn, void* context, int gen_number, bool walk_large_object_heap_p) PURE_VIRTUAL
 
     virtual void NullBridgeObjectsWeakRefs(size_t length, void* unreachableObjectHandles) PURE_VIRTUAL;
+
+    virtual uintptr_t* GetPendingBridgeHandles(size_t* count) PURE_VIRTUAL;
 };
 
 #ifdef WRITE_BARRIER_CHECK

src/coreclr/gc/interface.cpp

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 #include "gcinternal.h"
+#include "gcbridge.h"
 
 #ifdef SERVER_GC
 namespace SVR
@@ -2737,6 +2738,17 @@ void GCHeap::NullBridgeObjectsWeakRefs(size_t length, void* unreachableObjectHan
 #endif
 }
 
+uintptr_t* GCHeap::GetPendingBridgeHandles(size_t* count)
+{
+#ifdef FEATURE_JAVAMARSHAL
+    return ::GetPendingBridgeHandles(count);
+#else
+    assert(false);
+    *count = 0;
+    return nullptr;
+#endif
+}
+
 HRESULT GCHeap::WaitUntilConcurrentGCCompleteAsync(int millisecondsTimeout)
 {
 #ifdef BACKGROUND_GC

src/coreclr/gc/objecthandle.cpp

@@ -1504,6 +1504,8 @@ void CALLBACK GetBridgeObjectsForProcessing(_UNCHECKED_OBJECTREF* pObjRef, uintp
     if (!g_theGCHeap->IsPromoted(*ppRef))
     {
         RegisterBridgeObject(*ppRef, *pExtraInfo);
+        if (lp2 != 0)
+            RegisterPendingBridgeHandle((uintptr_t)pObjRef);
     }
 }
 
@@ -1514,8 +1516,9 @@ uint8_t** Ref_ScanBridgeObjects(uint32_t condemned, uint32_t maxgen, ScanContext
     LOG((LF_GC | LF_CORPROF, LL_INFO10000, "Building bridge object graphs.\n"));
     uint32_t flags = HNDGCF_NORMAL;
     uint32_t type = HNDTYPE_CROSSREFERENCE;
+    bool shouldProcessBridgeObjects = ShouldProcessBridgeObjects();
 
-    BridgeResetData();
+    BridgeResetData(shouldProcessBridgeObjects);
 
     HandleTableMap* walk = &g_HandleTableMap;
     while (walk) {
@@ -1527,14 +1530,14 @@ uint8_t** Ref_ScanBridgeObjects(uint32_t condemned, uint32_t maxgen, ScanContext
                     HHANDLETABLE hTable = walk->pBuckets[i]->pTable[uCPUindex];
                     if (hTable)
                         // or have a local var for bridgeObjectsToPromote/size (instead of NULL) that's passed in as lp2
-                        HndScanHandlesForGC(hTable, GetBridgeObjectsForProcessing, uintptr_t(sc), 0, &type, 1, condemned, maxgen, HNDGCF_EXTRAINFO | flags);
+                        HndScanHandlesForGC(hTable, GetBridgeObjectsForProcessing, uintptr_t(sc), shouldProcessBridgeObjects, &type, 1, condemned, maxgen, HNDGCF_EXTRAINFO | flags);
                 }
             }
         walk = walk->pNext;
     }
 
     // The callee here will free the allocated memory.
-    if (ShouldProcessBridgeObjects())
+    if (shouldProcessBridgeObjects)
     {
         MarkCrossReferencesArgs *args = ProcessBridgeObjects();
 

src/coreclr/nativeaot/Runtime/ObjectLayout.h

@@ -5,8 +5,13 @@
 // Low-level types describing GC object layouts.
 //
 
+#include "volatile.h"
+
 // Bits stolen from the sync block index that the GC/HandleTable knows about (currently these are at the same
 // positions as the mainline runtime).
+#ifdef FEATURE_JAVAMARSHAL
+#define BIT_SBLK_BRIDGE_PENDING             0x80000000
+#endif // FEATURE_JAVAMARSHAL
 #define BIT_SBLK_GC_RESERVE                 0x20000000
 #define BIT_SBLK_FINALIZER_RUN              0x40000000
 
@@ -22,6 +27,7 @@ class ObjHeader
 
 public:
     uint32_t GetBits() { return m_uSyncBlockValue; }
+    uint32_t GetBitsAcquire() { return VolatileLoad(&m_uSyncBlockValue); }
     void SetBit(uint32_t uBit);
     void ClrBit(uint32_t uBit);
     void SetGCBit() { m_uSyncBlockValue |= BIT_SBLK_GC_RESERVE; }

src/coreclr/nativeaot/Runtime/interoplibinterface_java.cpp

@@ -26,6 +26,19 @@ namespace
     Volatile<bool> g_GCBridgeActive = false;
     CLREventStatic g_bridgeFinished;
 
+    void ClearPendingBridgeBits(
+        uintptr_t* handles,
+        size_t handleCount)
+    {
+        for (size_t i = 0; i < handleCount; i++)
+        {
+            OBJECTHANDLE handle = reinterpret_cast<OBJECTHANDLE>(handles[i]);
+            Object* object = ObjectFromHandle(handle);
+            if (object != nullptr)
+                object->GetHeader()->ClrBit(BIT_SBLK_BRIDGE_PENDING);
+        }
+    }
+
     void ReleaseGCBridgeArgumentsWorker(
         MarkCrossReferencesArgs* args)
     {
@@ -55,19 +68,17 @@ void JavaMarshalNative::TriggerClientBridgeProcessing(
 {
     _ASSERTE(GCHeapUtilities::IsGCInProgress());
 
-    if (g_GCBridgeActive)
-    {
-        // Release the memory allocated since the GCBridge
-        // is already running and we're not passing them to it.
-        ReleaseGCBridgeArgumentsWorker(args);
-        return;
-    }
+    size_t pendingBridgeHandleCount;
+    uintptr_t* pendingBridgeHandles = GCHeapUtilities::GetGCHeap()->GetPendingBridgeHandles(&pendingBridgeHandleCount);
+
+    _ASSERTE(!g_GCBridgeActive);
 
     // Not initialized
     if (g_MarkCrossReferences == NULL)
     {
         // Release the memory allocated since we
         // don't have a GC bridge callback.
+        ClearPendingBridgeBits(pendingBridgeHandles, pendingBridgeHandleCount);
         ReleaseGCBridgeArgumentsWorker(args);
         return;
     }
@@ -122,12 +133,14 @@ extern "C" void QCALLTYPE JavaMarshal_FinishCrossReferenceProcessing(
         pThisThread->DisablePreemptiveMode();
 
         GCHeapUtilities::GetGCHeap()->NullBridgeObjectsWeakRefs(length, unreachableObjectHandles);
+        size_t pendingBridgeHandleCount;
+        uintptr_t* pendingBridgeHandles = GCHeapUtilities::GetGCHeap()->GetPendingBridgeHandles(&pendingBridgeHandleCount);
+        ClearPendingBridgeBits(pendingBridgeHandles, pendingBridgeHandleCount);
 
         IGCHandleManager* pHandleManager = GCHandleUtilities::GetGCHandleManager();
         OBJECTHANDLE* handles = (OBJECTHANDLE*)unreachableObjectHandles;
         for (size_t i = 0; i < length; i++)
             pHandleManager->DestroyHandleOfUnknownType(handles[i]);
-
         g_GCBridgeActive = false;
         g_bridgeFinished.Set();
 
@@ -139,12 +152,21 @@ extern "C" void QCALLTYPE JavaMarshal_FinishCrossReferenceProcessing(
 
 FCIMPL2(FC_BOOL_RET, GCHandle_InternalTryGetBridgeWait, OBJECTHANDLE handle, OBJECTREF* pObjResult)
 {
-    if (g_GCBridgeActive)
+    Object* object = ObjectFromHandle(handle);
+    if (g_GCBridgeActive && object != nullptr &&
+        (object->GetHeader()->GetBitsAcquire() & BIT_SBLK_BRIDGE_PENDING) != 0)
+    {
+        FC_RETURN_BOOL(false);
+    }
+
+    // See explanation in Interop::TryGetObjectFromHandleWithoutBridgeWait
+    Object* confirmedObject = ObjectFromHandle(handle);
+    if (confirmedObject != object)
     {
         FC_RETURN_BOOL(false);
     }
 
-    *pObjResult = ObjectFromHandle(handle);
+    *pObjResult = confirmedObject;
     FC_RETURN_BOOL(true);
 }
 FCIMPLEND

src/coreclr/vm/interoplibinterface.h

@@ -150,6 +150,10 @@ class Interop
 
     static bool IsGCBridgeActive();
 
+    static bool TryGetObjectFromHandleWithoutBridgeWait(
+        _In_ OBJECTHANDLE handle,
+        _Out_ Object** result);
+
     static void WaitForGCBridgeFinish();
 
     static void TriggerClientBridgeProcessing(

src/coreclr/vm/interoplibinterface_java.cpp

@@ -20,6 +20,23 @@ namespace
     Volatile<bool> g_GCBridgeActive = false;
     CLREvent* g_bridgeFinished = nullptr;
 
+    void ClearPendingBridgeBits(
+        _In_reads_(handleCount) uintptr_t* handles,
+        size_t handleCount)
+    {
+        LIMITED_METHOD_CONTRACT;
+
+        for (size_t i = 0; i < handleCount; i++)
+        {
+            OBJECTHANDLE handle = reinterpret_cast<OBJECTHANDLE>(handles[i]);
+            Object* object = OBJECTREFToObject(ObjectFromHandle(handle));
+            if (object != nullptr)
+            {
+                object->GetHeader()->ClrBit(BIT_SBLK_BRIDGE_PENDING);
+            }
+        }
+    }
+
     void ReleaseGCBridgeArgumentsWorker(
         _In_ MarkCrossReferencesArgs* args)
     {
@@ -51,6 +68,42 @@ bool Interop::IsGCBridgeActive()
     return g_GCBridgeActive;
 }
 
+bool Interop::TryGetObjectFromHandleWithoutBridgeWait(
+    _In_ OBJECTHANDLE handle,
+    _Out_ Object** result)
+{
+    LIMITED_METHOD_CONTRACT;
+
+    // g_GCBridgeActive is Volatile<bool> and pending bit clear uses InterlockedAnd. These should be set with release
+    // stores guaranteeing the order: weak-reference nulling -> pending-bit clearing -> g_GCBridgeActive = false
+    Object* object = OBJECTREFToObject(ObjectFromHandle(handle));
+    if (g_GCBridgeActive && object != nullptr &&
+        (object->GetHeader()->GetBitsAcquire() & BIT_SBLK_BRIDGE_PENDING) != 0)
+    {
+        return false;
+    }
+
+    // If object is nullptr, the handle value is stable, refetching is harmless
+    // If bridge is not active, refetching the handle should guarantee we get the right value
+    // since `g_GCBridgeActive` is Volatile<bool>
+    // The only remaining interesting case is gc bridge being active, with non-null object which
+    // had the bridge pending bit not set.
+    //
+    // If the bridge pending bit was never set because the object was not due for collection, handle value is stable
+    // If the bridge pending bit was set but got cleared in the meantime due to this code racing with the bridge
+    // finisher, refetching the handle will guarantee that we see the new value of the handle, because the
+    // bit is cleared with InterlockedAnd which does a release barrier (guaranteeing that the potential handle nulling
+    // was already published).
+    Object* confirmedObject = OBJECTREFToObject(ObjectFromHandle(handle));
+    if (confirmedObject != object)
+    {
+        return false;
+    }
+
+    *result = confirmedObject;
+    return true;
+}
+
 void Interop::WaitForGCBridgeFinish()
 {
     CONTRACTL
@@ -80,20 +133,18 @@ void Interop::TriggerClientBridgeProcessing(
     }
     CONTRACTL_END;
 
-    if (g_GCBridgeActive)
-    {
-        // Release the memory allocated since the GCBridge
-        // is already running and we're not passing them to it.
-        ReleaseGCBridgeArgumentsWorker(args);
-        return;
-    }
+    size_t pendingBridgeHandleCount;
+    uintptr_t* pendingBridgeHandles = GCHeapUtilities::GetGCHeap()->GetPendingBridgeHandles(&pendingBridgeHandleCount);
+
+    _ASSERTE(!g_GCBridgeActive);
 
     bool gcBridgeTriggered = JavaNative::TriggerClientBridgeProcessing(args);
 
     if (!gcBridgeTriggered)
     {
         // Release the memory allocated since the GCBridge
         // wasn't trigger for some reason.
+        ClearPendingBridgeBits(pendingBridgeHandles, pendingBridgeHandleCount);
         ReleaseGCBridgeArgumentsWorker(args);
         return;
     }
@@ -122,11 +173,13 @@ void Interop::FinishCrossReferenceProcessing(
         GCX_COOP();
 
         GCHeapUtilities::GetGCHeap()->NullBridgeObjectsWeakRefs(length, unreachableObjectHandles);
+        size_t pendingBridgeHandleCount;
+        uintptr_t* pendingBridgeHandles = GCHeapUtilities::GetGCHeap()->GetPendingBridgeHandles(&pendingBridgeHandleCount);
+        ClearPendingBridgeBits(pendingBridgeHandles, pendingBridgeHandleCount);
 
         IGCHandleManager* pHandleManager = GCHandleUtilities::GetGCHandleManager();
         for (size_t i = 0; i < length; i++)
             pHandleManager->DestroyHandleOfUnknownType(((OBJECTHANDLE*)unreachableObjectHandles)[i]);
-
         g_GCBridgeActive = false;
         g_bridgeFinished->Set();
     }