[Mono.Android] Guard GREF/WREF logging P/Invokes behind Logger.LogGlobalRef check#10785
[Mono.Android] Guard GREF/WREF logging P/Invokes behind Logger.LogGlobalRef check#10785simonrozsival wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes .NET for Android runtime JNI reference management by avoiding managed→native P/Invoke transitions for GREF/WREF logging when global-ref logging is disabled, aligning the behavior with the existing local-ref logging guards.
Changes:
- Wrapped
_monodroid_gref_log_new/_deleteand_monodroid_weak_gref_new/_deleteP/Invokes inif (Logger.LogGlobalRef)checks. - Avoided building thread/stacktrace logging payloads when global-ref logging is disabled.
There was a problem hiding this comment.
Guarding _monodroid_gref_log_new() (and the related delete/weak gref P/Invokes) behind Logger.LogGlobalRef means the native GREF/WREF counters will no longer be updated when GREF logging is disabled. This changes the behavior of RuntimeNativeMethods._monodroid_gref_get() / Java.Interop.Runtime.GlobalReferenceCount and also disables the gref_gc_threshold full-GC trigger in CreateGlobalReference for the default (logging-off) case.
If the goal is to avoid the managed→native transition but keep the counter/threshold behavior, consider tracking GREF/WREF counts in managed code (e.g., Interlocked fields in this manager) and performing the threshold check unconditionally, while only doing stacktrace construction + native logging when Logger.LogGlobalRef is enabled.
There was a problem hiding this comment.
Ok, good copilot saw it, too!
There was a problem hiding this comment.
I think this doesn't have _log_ in the name, it's actually deleting? So, we can't change this here?
There was a problem hiding this comment.
I think this doesn't have _log_ in the name, it's actually creating a new gref? So, we can't change this here?
There was a problem hiding this comment.
If you iterate on this PR, maybe you can put at the top instead:
if (!Logger.LogGlobalRef) {
return;
}
So, the diff would be a lot smaller: no indentation.
There was a problem hiding this comment.
I agree checking in managed is better, can we remove the equivalant Logger.LogGlobalRef in native now? Or can it get called from places in native code where we'd need to check in both places?
…balRef check The AndroidObjectReferenceManager overrides for CreateGlobalReference, DeleteGlobalReference, CreateWeakGlobalReference, and DeleteWeakGlobalReference were unconditionally calling native logging P/Invokes on every invocation, even when GREF logging was disabled. While the native side does an early return when LOG_GREF is off, the managed-to-native transition itself has significant overhead that adds up in tight loops (e.g. GC bridge processing with thousands of refs). Changes: - Guard all 4 logging P/Invoke calls behind `if (Logger.LogGlobalRef)` early-return checks, matching the existing local ref pattern - Add managed `_grefc` and `_weak_grefc` counters using Interlocked to replace the native counter P/Invokes - Keep the gref_gc_threshold full-GC trigger working unconditionally - Switch GlobalReferenceCount/WeakGlobalReferenceCount properties to read from managed counters via Volatile.Read Performance (physical device, CoreCLR, Release, 3740 objects): - Bridge cleanup: 385ms → 4ms (~100x improvement) - Per-object NewGlobalRef overhead: ~7000µs → ~0µs
d4ce8ca to
99aec81
Compare
|
This is going to require a different approach, but it's definitely part of what's needed to make #10748 viable. |
Summary
The
AndroidObjectReferenceManageroverrides forCreateGlobalReference,DeleteGlobalReference,CreateWeakGlobalReference, andDeleteWeakGlobalReferencewere unconditionally calling native logging P/Invokes on every invocation, even when GREF logging was disabled (Logger.LogGlobalRef == false).While the native side does an early return when
LOG_GREFis off, the managed-to-native transition itself has significant overhead that adds up in tight loops (e.g. GC bridge processing on CoreCLR, which swaps GREF↔WREF for thousands of objects per collection).Changes
_grefcand_weak_grefccounters usingInterlocked.Increment/Decrementto maintain reference counts without P/Invokegref_gc_thresholdfull-GC trigger working unconditionally (uses managed counter)GlobalReferenceCountandWeakGlobalReferenceCountproperties to read from managed counters viaVolatile.Readinstead of native P/Invoke gettersImpact
base.*delegation + managed counter update. Eliminates 4+ unnecessary managed→native transitions per GREF/WREF operation._grefccounter.Performance
Measured on physical device (Samsung, CoreCLR, Release):
NewGlobalRefcost dropped from ~7000µs to ~0µs (the 7ms was the P/Invoke overhead)