Reduce weak ref blocking with java interop - #131952
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
This still needs some polishing, but I'm looking for feedback whether it would be feasible to borrow a per object bit from somewhere. Seems doable from the object header, but according to the comment there might be some friction with debug builds. cc @jkotas |
|
Tagging subscribers to this area: @anicka-net, @dotnet/gc |
There was a problem hiding this comment.
Pull request overview
This PR reduces blocking when resolving weak-reference targets during Android Java GC bridge processing by marking bridge objects that are pending client processing using a previously-unused object header bit. Weak-reference resolution only waits (returns “need to wait”) for objects currently marked as bridge-pending, instead of for all weak handles while bridge processing is active.
Changes:
- Repurposes the high syncblock/header bit as
BIT_SBLK_BRIDGE_PENDINGand uses it to mark bridge objects awaiting Java-side processing. - Tracks “pending bridge” handle cells during bridge graph construction and clears the pending bit when bridge processing completes (or is not triggered).
- Updates weak-handle fast-path (
GCHandleInternalTryGetBridgeWait) to consult bridge-pending state rather than unconditionally blocking when the bridge is active.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/syncblk.h | Renames/reassigns the previously-unused header bit to BIT_SBLK_BRIDGE_PENDING. |
| src/coreclr/vm/marshalnative.cpp | Routes weak-handle “try get” through the new bridge-pending-aware helper. |
| src/coreclr/vm/interoplibinterface.h | Adds Interop::TryGetObjectFromHandleWithoutBridgeWait declaration (FEATURE_JAVAMARSHAL). |
| src/coreclr/vm/interoplibinterface_java.cpp | Implements pending-bit check/clear for CoreCLR Java bridge flow. |
| src/coreclr/nativeaot/Runtime/ObjectLayout.h | Adds BIT_SBLK_BRIDGE_PENDING for NativeAOT parity. |
| src/coreclr/nativeaot/Runtime/interoplibinterface_java.cpp | Implements pending-bit check/clear for NativeAOT Java bridge flow. |
| src/coreclr/gc/objecthandle.cpp | Records pending handle cells and notes the active-bridge recomputation race (FIXME). |
| src/coreclr/gc/gcbridge.h | Exposes pending-handle tracking APIs. |
| src/coreclr/gc/gcbridge.cpp | Implements pending-handle tracking and sets BIT_SBLK_BRIDGE_PENDING on candidates. |
Suppressed comments (2)
src/coreclr/vm/interoplibinterface_java.cpp:127
- When g_GCBridgeActive is already true, this early-return path releases args but never clears BIT_SBLK_BRIDGE_PENDING on the objects just marked in ProcessBridgeObjects for this GC. That can leave stale pending bits behind, causing future weak-handle checks to spuriously block (especially on subsequent bridge-active cycles). Clear the pending bits for the current pending handle set before returning.
if (g_GCBridgeActive)
{
// FIXME: This should become unreachable once bridge graph recomputation is skipped while active.
// Release the memory allocated since the GCBridge
// is already running and we're not passing them to it.
ReleaseGCBridgeArgumentsWorker(args);
return;
src/coreclr/nativeaot/Runtime/interoplibinterface_java.cpp:77
- When g_GCBridgeActive is already true, this early-return path releases args but never clears BIT_SBLK_BRIDGE_PENDING on the objects just marked in ProcessBridgeObjects for this GC. That can leave stale pending bits behind, causing future weak-handle checks to spuriously block. Clear the pending bits for the current pending handle set before returning.
if (g_GCBridgeActive)
{
// FIXME: This should become unreachable once bridge graph recomputation is skipped while active.
// Release the memory allocated since the GCBridge
// is already running and we're not passing them to it.
ReleaseGCBridgeArgumentsWorker(args);
return;
}
| Object* object = OBJECTREFToObject(ObjectFromHandle(handle)); | ||
| if (g_GCBridgeActive && object != nullptr && | ||
| (object->GetHeader()->GetBits() & BIT_SBLK_BRIDGE_PENDING) != 0) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| *result = OBJECTREFToObject(ObjectFromHandle(handle)); | ||
| return true; |
There was a problem hiding this comment.
Wasn't that the whole purpose of it - reread twice? object can be stale here: it is loaded before the synchronization check. The bridge finisher can null this weak handle, clear the object's pending bit, and mark the bridge inactive before the above ifs, after which this returns the previously cached object even though the weak handle is now null.
There was a problem hiding this comment.
Hmm. This does seem like an actual problem and it is not related to the refactoring. The previous could could hit it as well, since we didn't have any memory ordering constraints.
There was a problem hiding this comment.
g_GCBridgeActive is Volatile<T>, so reading/writing enforces memory ordering. I think it should be sufficient to guarantee correctness before this change. Or am I missing something?
There was a problem hiding this comment.
The problem is that, while we have ordering between g_GCBridgeActive and either the bit access or the handle access, we don’t have any ordering between the bit access and the handle access .
Consider scenario of an object that was pending current bridge collection and will be ultimately marked as dead
- obj = *handle
- bridge is active and obj != nullptr
= at this exact point in time the bridge finisher nulls the handle and clears the bridge pending bit for the object (without any memory ordering between the operations) - we see the new value of the pending bit, as being not set (so we actually try to resolve the handle value)
- we read the handle again (we may or may not get the new null value)
- if we get the old value, we would resurrect the object incorrectly
There was a problem hiding this comment.
Added a new commit which should address this issue. I added an acquire load for the pending bit, the writing of the bit was already ordered with the weak ref nulling apparently due to InterlockedAnd.
I can't comment on copilot's conclusion on its own created benchmark (which also doesn't reflect gc behavior for user app, where GC is triggered once every couple of seconds, both in their repro and in the actual game where Rolf tested - https://gist.github.com/rolfbjarne/5fd7ac636132718170ea30dc1be2452c. The copilot comment reports collection rates of 40 per second, which is absurd in my opinion, especially for a game). I tested on the exact sample provided by the customer (https://github.com/hyvanmielenpelit/Net11FPSBenchmark) where, following each gc, there were waits on the UI thread of ~30ms. The sample extracted logic from their actual game. While doing other investigation, starting from the maui sample, I simply triggered GCs directly from the button callback which resulted in waits as well. Aside from users simply using weak reference that they shouldn't expect to have significant overhead, the C#-Java interop layer is filled with uses of weak reference checks for bridge objects (so any native event that needs to bubble up into C# would be blocking unnecessarily on the java gc finish and there are probably dozens of other scenarios). I'm also putting into perpsective that the change is rather simplistic and is a nice to have optimization regardless. |
I think it is ok. It should be under |
|
Correction — that "ruled out" line is mine and it's wrong. I've edited the original comment on #131370. The experiment behind it compared a weak-reference arm against a strong-reference control and found Symbolized off-CPU profiling says the opposite. Ablating it confirms the size. A prototype of this same fix moves the probe 42.7 → 49.3 fps and Two caveats:
Details and the raw runs: https://github.com/steveisok/android-gcbridge-investigation Note This analysis was generated with GitHub Copilot. |
|
This fix will help, but the majority of the problem will remain in the cost of the promotion of these objects. |
Bridge objects live in 2 worlds, .net and java so a .net bridge object has a correpsonding java peer. Collection of these objects is triggered by .net. When the .net peer is eligible for collection we build some graph over the set of dead objects and pass it over to java. Java triggers its own collection, collecting the java peers if they are dead as well. .NET android reports which bridge objects died on the java side so we can drop the gchandles for them. This will finally allow .net peers to die in the following collection (since they had to be promoted, given we don't know yet if java peers need to keep them alive or not). Currently obtaining the target of a weak ref blocks until the bridge processing is fully completed. This is the case also on mono and prevents 2 issues: - normal c# code checks a weak ref for some object. This can't immediately return correct information. If it returns true, the object gets resurrected and we can end up with a ref to a bridge objects that no longer has a java peer. If it returns false then that can be false as well if the object remains alive. - java code could call into managed, inserting a reference to a C# peer and afterward it could drop its own java peer. If this happens while C# gc ran but the java gc is still yet to start, both GC would see their peer as dead, even though it is alive. The .NET android interop obtains the C# peer ref also via weak reference, so this safely synchronizes with bridge processing. This PR keeps the weak reference wait only for bridge objects that are currently processed. For a weak ref target we need to determine whether the underlying object is pending bridge processing which is awkward to do efficiently because we would need to iterate over a set of handles or implement a lookup from obj address to associated cross reference handle. It turns out there is a free bit in the object header that we could use for this purpose. FIXME this has a race with redudndant bridge processing, because a new collection would dirty our g_registeredBridgeHandles.
7b98ec3 to
00a176a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/coreclr/vm/interoplibinterface_java.cpp:124
TriggerClientBridgeProcessingrelies on_ASSERTE(!g_GCBridgeActive)but has no retail guard. If this invariant is violated in a non-Debug build (e.g., redundant/overlapping bridge triggering), the code will proceed and can corrupt the pending-bridge bookkeeping and/or double-trigger client processing. The previous implementation handled this safely by releasingargsand returning.
size_t pendingBridgeHandleCount;
uintptr_t* pendingBridgeHandles = GetPendingBridgeHandles(&pendingBridgeHandleCount);
_ASSERTE(!g_GCBridgeActive);
bool gcBridgeTriggered = JavaNative::TriggerClientBridgeProcessing(args);
src/coreclr/nativeaot/Runtime/interoplibinterface_java.cpp:74
TriggerClientBridgeProcessinguses_ASSERTE(!g_GCBridgeActive)as the only protection against overlapping bridge triggers. In retail builds this becomes a no-op; if the invariant is ever violated, the function will proceed and can corrupt pending-handle state or trigger client bridge processing twice. A defensive runtime check (as existed before) would make this robust.
size_t pendingBridgeHandleCount;
uintptr_t* pendingBridgeHandles = GetPendingBridgeHandles(&pendingBridgeHandleCount);
_ASSERTE(!g_GCBridgeActive);
src/coreclr/gc/objecthandle.cpp:1533
- The comment above the
HndScanHandlesForGCcall no longer matches the newlp2usage (it is now a boolean indicating whether to record pending bridge handles, not a pointer for promotion data). This is likely to mislead future changes to the scanning callback contract.
// or have a local var for bridgeObjectsToPromote/size (instead of NULL) that's passed in as lp2
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/coreclr/vm/marshalnative.cpp:402
- The comment above this fast path is now misleading: this method no longer waits for bridge processing to finish, it only returns false for handles whose target is currently pending bridge processing. Please update the comment (and fix the typo) to match the new behavior, otherwise future readers may assume this blocks or fully synchronizes with the bridge.
FCIMPL2(FC_BOOL_RET, MarshalNative::GCHandleInternalTryGetBridgeWait, OBJECTHANDLE handle, Object **pObjResult)
{
FCALL_CONTRACT;
if (!Interop::TryGetObjectFromHandleWithoutBridgeWait(handle, pObjResult))
src/coreclr/vm/interoplibinterface_java.cpp:127
- Minor grammar: “wasn't trigger” should be “wasn't triggered”.
if (!gcBridgeTriggered)
{
// Release the memory allocated since the GCBridge
// wasn't trigger for some reason.
ClearPendingBridgeBits(pendingBridgeHandles, pendingBridgeHandleCount);
2b5a92a to
fa2867f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/coreclr/vm/interoplibinterface_java.cpp:82
- TryGetObjectFromHandleWithoutBridgeWait declares
resultas an_Out_parameter but returnsfalsewithout writing*result. Even though current callers likely ignore the value onfalse, this violates the contract and can lead to accidental use of an uninitialized out value by future callers.
Object* object = OBJECTREFToObject(ObjectFromHandle(handle));
if (g_GCBridgeActive && object != nullptr &&
(object->GetHeader()->GetBits() & BIT_SBLK_BRIDGE_PENDING) != 0)
{
return false;
}
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
This addresses an issue reported on .net11 preview, as a regression from .net10 mono. Ideally we would get this in by Friday, in time for RC1 snap. |
| Object* object = OBJECTREFToObject(ObjectFromHandle(handle)); | ||
| if (g_GCBridgeActive && object != nullptr && | ||
| (object->GetHeader()->GetBits() & BIT_SBLK_BRIDGE_PENDING) != 0) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| *result = OBJECTREFToObject(ObjectFromHandle(handle)); | ||
| return true; |
There was a problem hiding this comment.
Wasn't that the whole purpose of it - reread twice? object can be stale here: it is loaded before the synchronization check. The bridge finisher can null this weak handle, clear the object's pending bit, and mark the bridge inactive before the above ifs, after which this returns the previously cached object even though the weak handle is now null.
… and clearing of pending bit The bit clearing already had memory ordering since it was done via InterlockedAnd. For the read we add GetBitsAcquire which does an acquire load, preserving the ordering on the reader side.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/coreclr/gc/gcbridge.cpp:1319
ProcessBridgeObjectssetsBIT_SBLK_BRIDGE_PENDINGon all registered bridge objects even ifBuildSccCallbackData()fails to allocate and returnsnullptr. In that case, the caller skips triggering client bridge processing, so the pending bits may remain set until the objects die, and a later bridge cycle could spuriously treat them as “pending” and force weak-ref waits.
Consider only setting the pending bit when args != NULL (or clearing the bits on the failure path).
for (int i = 0; i < DynPtrArraySize(&g_registeredBridges); i++)
{
Object* object = (Object*)DynPtrArrayGet(&g_registeredBridges, i);
object->GetHeader()->SetBit(BIT_SBLK_BRIDGE_PENDING);
}
Adds the A/B harness and results for dotnet/runtime#131952, the upstream implementation of the precise weak-reference wait prototyped here. The two arms differ only by that PR, which branched directly off the merge commit of #131764, and share one System.Private.CoreLib.dll because the PR changes no managed code. It reproduces the prototype (42.8 -> 49.9 fps at 1200 peers/frame) but recovers only ~31% of lost frame time at nodes=300, the operating point that matches the shape reported in #131370 -- so the wait accounts for less of the damage as the peer rate falls toward the realistic regime, not more. Also records the measurement trap this cost: an incremental app build does not re-copy a changed libcoreclr.so out of the runtime pack, so both arms silently run identical binaries and the result looks like a change that did nothing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 91dd8b98-73db-41b0-8d8c-9e1c21b53f5f
|
Any chance for a review to get this into rc1 ? |
MAUI startup A/B on Samsung A16I tested this change against the exact runtime source used by the installed Build
The two APKs were cloned from one base APK and re-signed after replacing Measurement
Results
Bridge confirmationA separate diagnostic startup with GC logging enabled recorded:
That is an approximately 42 ms accepted bridge round during startup. The ConclusionOn this peer-heavy MAUI sample, selective weak-reference waiting produces a These local CoreCLR builds do not have the official runtime pack's PGO/BOLT |
Bridge objects live in 2 worlds, .net and java so a .net bridge object has a correpsonding java peer. Collection of these objects is triggered by .net. When the .net peer is eligible for collection we build some graph over the set of dead objects and pass it over to java. Java triggers its own collection, collecting the java peers if they are dead as well. .NET android reports which bridge objects died on the java side so we can drop the gchandles for them. This will finally allow .net peers to die in the following collection (since they had to be promoted, given we don't know yet if java peers need to keep them alive or not).
Currently obtaining the target of a weak ref blocks until the bridge processing is fully completed. This is the case also on mono and prevents 2 issues:
This PR keeps the weak reference wait only for bridge objects that are currently processed. For a weak ref target we need to determine whether the underlying object is pending bridge processing which is awkward to do efficiently because we would need to iterate over a set of handles or implement a lookup from obj address to associated cross reference handle. It turns out there is a free bit in the object header that we could use for this purpose.
Addresses #131370