Repository navigation
Add GC bridge EventSource timing - #12849
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A couple of small but important robustness/consistency fixes are needed (notably around feature-gating and test brittleness) before the PR is safe to merge.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (3)
What changed in this PR
This PR adds the first production instrumentation call sites for the Microsoft.Android.Runtime EventSource provider by emitting GC bridge start/stop events and documenting their timing boundary, along with new/updated tests to validate emission and trimming behavior.
Changes:
- Emit GC bridge Start/Stop EventSource events around the managed portions of CoreCLR/NativeAOT GC bridge processing, with enablement latched per bridge round.
- Initialize the runtime EventSource during GC bridge setup (when the feature switch is enabled) to avoid first-use overhead during a GC bridge callback.
- Extend runtime/linker tests and documentation to validate/describe the GC bridge event contract and trimming expectations.
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Mono.Android-Tests/Android.Runtime/RuntimeEventSourceTests.cs | Updates provider contract test and adds a CoreCLR-only GC bridge round emission test. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/LinkerTests.cs | Extends linker test assertions to verify GC bridge call-site retention/removal and initialization trimming behavior. |
| src/Mono.Android/Microsoft.Android.Runtime/RuntimeEventSource.cs | Adds explicit provider initialization and changes GC bridge Start to return whether emission occurred. |
| src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs | Adds GC bridge start/stop emission around bridge callbacks with enablement latched on the serialized bridge thread. |
| Documentation/guides/tracing.md | Documents GC bridge event IDs 7/8, zero-payload contract, and precise timing boundary. |
4e1cabc to
35ffb03
Compare
4a3d342 to
4ac1a13
Compare
4ac1a13 to
6cd5a1e
Compare
6cd5a1e to
7d24261
Compare
## Summary Add the EventSource/EventPipe provider foundation for runtime diagnostics without defining events or adding production instrumentation call sites. - add the empty `Microsoft.Android.Runtime` provider in the trimmable `Mono.Android.dll` - use the standard `System.Diagnostics.Tracing.EventSource.IsSupported` feature switch as the sole compile/trim gate - expose that internal CoreLib switch through an internal `RuntimeFeature.EventSourceSupport` wrapper whose fallback matches CoreLib: enabled unless AppContext explicitly disables it - create the provider lazily so `EventSourceSupport=false` removes its implementation and guarded call paths from trimmed Release applications - reserve the future event ID and keyword contract in documentation only: - IDs 1-6 and keywords `0x1`/`0x2` for Java interop lifecycle/reachability from #12258 - IDs 7/8 and keyword `0x8` for GC bridge timing - IDs 9/10 and keyword `0x4` for trimmable type-map timing - document enabling EventSource support and enabling diagnostics separately for out-of-process `dotnet-trace` transport ## Scope This is Layer 1 and the bottom PR of registered GitHub stack `12855`: 1. #12843 — EventSource foundation 2. #12849 — GC bridge timing 3. #12851 — trimmable typemap timing 4. #12853 — deprecate and detach `Android.Runtime.TimingLogger` 5. #12854 — remove FastTiming from CoreCLR This layer intentionally does **not**: - define GC bridge events - define trimmable type-map events - define the Java interop events from #12258 - instrument any production call sites - instrument LLVM-IR type maps - change or remove FastTiming - change `Android.Runtime.TimingLogger` Each concrete event and its call sites are added by the later layer that owns that instrumentation. ## Validation - built `Mono.Android.csproj` - compiled `Xamarin.Android.Build.Tests` - passed the provider foundation test, which verifies the provider name/metadata, EventListener provider creation, and that the provider declares zero events - passed `RuntimeEventSourceFeatureSwitch(false/true)`, including runtimeconfig validation and linked-assembly inspection proving the nested provider implementation is removed when `EventSourceSupport=false` and retained when `EventSourceSupport=true` - built `Mono.Android.NET-Tests.csproj` with `EventSourceSupport=true` using the local Android workload
56454d7 to
5af8b98
Compare
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors · 1 warning · 0 suggestions
The production instrumentation is well scoped: provider initialization is feature-gated, Start enablement is latched across the serialized bridge round, Stop covers completion/notification, and the linker coverage preserves feature-off trimming. The new integration test has a race at its observation boundaries, however, and can fail on a leading Stop or trailing Start from an overlapping bridge round; the inline comment describes how to make the assertion deterministic.
CI is still in progress: 41 checks have succeeded, 2 are running, and the aggregate dotnet-android check is queued; no failures are currently reported.
Generated by Android PR Reviewer for #12849 · copilot · gpt56 · 105.4 AIC · ⌖ 11.2 AIC · ⊞ 26K
Comment /review to run again
Instrument complete managed GC bridge rounds with latched EventSource enablement, preserve trim-away behavior, and cover real CoreCLR bridge emission. Co-authored-by: Copilot App <[email protected]>
Rely on the fatal unmanaged callback boundary instead of resetting state for an unreachable subsequent round. Co-authored-by: Copilot App <[email protected]>
Replace the discard-based provider initialization trigger with an empty instance method. Co-authored-by: Copilot App <[email protected]>
Guard only EventSource emission and keep context validation on one unconditional path. Co-authored-by: Copilot App <[email protected]>
Rely on the native bridge's serialized processing invariant instead of thread-local state. Co-authored-by: Copilot App <[email protected]>
Complete bridge processing once, then gate only the timing Stop path so feature-off trimming remains effective. Co-authored-by: Copilot App <[email protected]>
Preserve lazy provider verification on MonoVM while recognizing managed GC bridge setup initializes it before tests run. Co-authored-by: Copilot App <[email protected]>
Handle Cecil methods without IL bodies when scanning for runtime EventSource calls. Co-authored-by: Copilot App <[email protected]>
Validate one adjacent Start/Stop pair under the listener lock instead of assuming the full asynchronous event stream is pair-aligned. Co-authored-by: Copilot App <[email protected]>
Remove the obsolete MonoVM provider-initialization branch after main removed UseMonoRuntime support. Co-authored-by: Copilot App <[email protected]>
6b8d252 to
7bbfc91
Compare
jonathanpeppers
left a comment
There was a problem hiding this comment.
I reran one of the test lanes, but otherwise looks good. 👍
## Summary Layer 3 of registered GitHub stack `12855`, above #12849 and foundation #12843. - own the complete trimmable typemap provider contract: event IDs 9/10, keyword `0x4`, task 2, and `JavaToManaged` / `ManagedToJava` direction payloads - instrument trimmable typemap cache-population misses in both directions - check `RuntimeFeature.EventSourceSupport` and listener/keyword enablement only inside the `ConcurrentDictionary.GetOrAdd` miss factories - latch Start emission and pair Stop in `finally`, including failed and throwing backend lookups - preserve linker removal when `EventSourceSupport=false` - document exact miss-only semantics and payloads Cache hits perform no feature-switch or EventSource enablement check and emit no events. The Stop payload has no success/found field, matching the provider contract. This layer remains scoped to the trimmable typemap path on the current CoreCLR-only runtime model. It does not add legacy runtime assumptions and does not modify TimingLogger or FastTiming. ## Validation - `./dotnet-local.sh build src/Mono.Android/Mono.Android.csproj -v:minimal` - `./dotnet-local.sh test bin/TestDebug/net10.0/Xamarin.Android.Build.Tests.dll --filter "Name~RuntimeEventSourceFeatureSwitch"` — 2 passed - trimmable CoreCLR on-device `Mono.Android.NET-Tests`, categories `TypeMap,GCBridge` — 7 passed, 1 expected skip - default CoreCLR on-device `Mono.Android.NET-Tests`, category `TypeMap` — 4 expected trimmable-only skips; no default-path timing assertion remains ## Stack - Base: #12849 / `simonrozsival-gc-bridge-timing` - This layer: #12851 - Next layer: #12853 - Registered GitHub stack: `12855`
## Summary - mark `Android.Runtime.TimingLogger` obsolete with guidance to use `Stopwatch` or `EventSource` - replace its native FastTiming-backed implementation with managed monotonic timing while preserving the `Logger.LogTiming` opt-in gate, `monodroid-timing` logcat tag, default message, elapsed format, and idempotent state transitions - remove the remaining `monodroid_timing_start`/`monodroid_timing_stop` declarations, CoreCLR dispatch entries, NativeAOT stubs, and native managed-timing sequence allocator ## Stack Layer 4 of registered GitHub stack `12855`, above #12851, #12849, and #12843. - Base: `main` (the lower layers have merged) - This layer: #12853 - Top layer: #12854 FastTiming instrumentation, timing properties/modes, dump handling, call sites, and FastTiming tests are intentionally retained here and deferred to Layer 5. ## Validation - `Mono.Android.csproj` build with API compatibility checks - `native-clr.csproj` x86_64 build - `native-nativeaot.csproj` x86_64 build The APK descriptions now use the versions from `main`. The Mono runtime and its generated P/Invoke table were removed upstream, so no Mono-specific validation remains in this layer.
## Summary Layer 5 and the top PR of registered GitHub stack `12855`, above merged #12853, #12851, #12849, and #12843. - remove native FastTiming instrumentation, event storage, formatting, summaries, configuration, and self-overhead measurement - remove the shared FastTiming implementation/static library and all CoreCLR timing call sites - remove FastTiming properties/modes, JNI/export dump entrypoint, broadcast receiver, manifest overlay, packaging hooks, and `_AndroidFastTiming` - retain the exact plain `timing` log category for the obsolete managed `Android.Runtime.TimingLogger` - remove the obsolete `FastTimingTests` device test - direct runtime/JIT/loader diagnostics to EventPipe and Android bridge/type-map diagnostics to `Microsoft.Android.Runtime` - refresh CoreCLR APK size references from a local rebuild after rebasing onto current `main` ## Size The four `BuildReleaseArm64` CoreCLR references were regenerated locally after rebasing onto `main` (`64920590083e2d50e0d99d716ce2d82c46bdb4f9`): | Variant | APK delta | `libmonodroid.so` | Delta | Assembly-store delta | |---|---:|---:|---:|---:| | Simple CoreCLR | -12,288 B | 158,792 → 120,096 B | -38,696 B (-24.37%) | +512 B | | Simple CoreCLR + R8 | -16,384 B | 158,792 → 120,096 B | -38,696 B (-24.37%) | +664 B | | XForms CoreCLR | -12,288 B | 151,488 → 120,096 B | -31,392 B (-20.72%) | -192 B | | XForms CoreCLR + R8 | -12,288 B | 151,488 → 120,096 B | -31,392 B (-20.72%) | -200 B | The size matrix passed all four `BuildReleaseArm64` regression tests after copying the freshly generated `.apkdesc` files. The earlier managed-size investigation used isolated merge-base/PR builds and extracted every assembly. It found no managed DLL size growth: all extracted assemblies had identical uncompressed totals before and after this PR. The small assembly-store changes above are compressed/layout differences, not a ~100 KiB managed-code increase. ## Startup Prior balanced startup measurements showed overlapping distributions and no measurable startup effect; no startup regression or improvement is claimed. ## Validation - rebased onto current `main` and resolved `host.cc` plus four APK reference conflicts - full `make all CONFIGURATION=Release` - CoreCLR APK size regression tests: 4 passed - CoreCLR EventSource linker feature-switch tests: 2 passed - CoreCLR `RunWithLogging` property regression test verifies `debug.dotnet.log=default,assembly,timing` - trimmable type-map tests: 1,277 passed, 1 skipped - native no-op incrementality: `_ConfigureRuntimes` and `_BuildAndroidRuntimes` skip with no CMake/Ninja execution - managed `TimingLogger` and lower-layer EventSource/type-map contract sources remain unchanged ## Stack - Final base: `main` - This layer: #12854 - Registered GitHub stack: `12855`

Summary
Layer 2 of registered GitHub stack
12855, built on the EventSource foundation merged by #12843.Microsoft.Android.RuntimeGC bridge events (IDs 7/8, keyword0x8, task 1)EnsureAllContextsAreOurs; Stop after collected-context processing,JavaMarshal.FinishCrossReferenceProcessing, andAndroidRuntimeInternal.NotifyBridgeProcessingFinishedThis intentionally does not add typemap lookup instrumentation, modify
TimingLogger, or remove/disableFastTiming.Validation
Mono.Android.Runtime.csprojbuildMono.Android.csprojbuildRuntimeEventSourceFeatureSwitchlinker tests: 2 passedAndroidTypeMapImplementation=llvm-ir: 3 passed, 1 pre-existing ignoredAndroidTypeMapImplementation=trimmable: 3 passed, 1 pre-existing ignoredStack
mainmain12855