Skip to content

Interpreter block-count PGO for WebAssembly CoreCLR - #132721

Open
pavelsavara wants to merge 26 commits into
dotnet:mainfrom
pavelsavara:wasm_collect_PGO
Open

pavelsavara wants to merge 26 commits into
dotnet:mainfrom
pavelsavara:wasm_collect_PGO

Conversation

@pavelsavara

@pavelsavara pavelsavara commented Aug 24, 2026

Copy link
Copy Markdown
Member

Summary

Instruments the CoreCLR interpreter with block-count PGO probes on WebAssembly and adds a JavaScript trigger to collect the profile over EventPipe, so dotnet-pgo can produce an .mibc for R2R precompilation. This is the profile production side of PGO-on-WebAssembly; consumption (crossgen2 on WASM) is tracked separately.

This targets the single-threaded browser/WASI interpreter (the offline PGO-collection config, PERFTRACING_DISABLE_THREADS); the feature is compiled out on multithreaded WASM.

Part of #130524. Implements #130517 and #130518.

Instrumentation (#130517)

  • New INTOP_PGO_COUNT interpreter opcode (single-threaded browser/WASI only) that increments a native uint32_t counter allocated via allocPgoInstrumentationBySchema, so counters outlive the EventPipe session and wrap as the profile format expects.
  • InterpCompiler::InstrumentBlockCounts emits BasicBlockIntCount probes at block heads only — method entry plus branch/switch/loop targets, restricted to the original IL range (m_ILCodeSizeFromILHeader, so synthetic finally/epilog IL for synchronized/async methods is skipped) — gated by DOTNET_InterpPgo with an optional DOTNET_InterpPgoMethods method filter.
  • The alloc*/get* PGO interface methods move to the shared CEECodeGenInfo base so the JIT and interpreter share one implementation; the tiering gate is relaxed for the interpreter, target-scoped to browser/WASI.
  • FEATURE_PGO is enabled for WASM independently.
  • The instrumentation (opcode, probe emission, enable flags) is gated on PERFTRACING_DISABLE_THREADS, so multithreaded (WasmEnableThreads) builds never emit the counter and can't race on the increment.

Flush over EventPipe

  • Accumulated counts are emitted to JitInstrumentationDataVerbose events on EventPipe session stop via a new ep_rt_session_stopping hook. CoreCLR calls PgoManager::EmitInstrumentationDataToEventPipe() (Mono and NativeAOT are no-ops). The hook runs before the EventPipe lock is taken, since emitting events re-enters the write path; the stopping session's keyword mask is captured under the lock in stop_session and passed to the hook (ep_rt_session_stopping(id, session_mask)), so the runtime tests the keyword without dereferencing a session a concurrent stop could free.
  • EventPipe emission is separated from the text-file export. EmitInstrumentationDataToEventPipe() only fires the events; the DOTNET_WritePGOData text dump stays in WritePgoData(), driven solely by the process-shutdown path — an on-demand trace collection never writes the text file.
  • Shutdown coordination: WritePgoData() emits to EventPipe only under !PERFTRACING_DISABLE_THREADS. On single-threaded WASM the session-stopping hook is the sole EventPipe emitter, so a method is never delivered twice into a session EventPipe stops during shutdown (which dotnet-pgo rejects as a duplicate chunk after a method's final chunk); threaded desktop still emits at shutdown as before.
  • The emission is routed into only the stopping session: the flushing thread is briefly marked as a rundown thread bound to that session, so events go through ep_session_write_event instead of broadcasting to every enabled session (the same mechanism EventPipe uses for method/assembly rundown at teardown).
  • On multithreaded WASM the flush is compiled out; a PORTABILITY_ASSERT, gated on the stopping session's JitInstrumentationData keyword, flags a genuine PGO-collection attempt on that unsupported config without tripping on unrelated (CPU/GC/counters) sessions.

JS trigger (#130518)

  • collectPgoTrace() diagnostic client (js://pgo) starts a trace with the JitInstrumentationData keyword — mask aligned to the IBC keyword set dotnet-pgo consumes — and auto-downloads the .nettrace after a default 10s window. The stop timer only stops the session it started.
  • Collection is one-shot per process: because the interpreter counters are cumulative and flushed once, a second collectPgoTrace is rejected rather than re-emitting cumulative data that dotnet-pgo would drop as a restarted chunk sequence. Restart the app to collect again.

Notes

  • Method identity in the events is MVID + metadata token + IL offset, which is engine-independent, so no PerfMap is required for instrumentation PGO.
  • dotnet-pgo must reference the IL-trimmed linked/*.dll (whose MVID matches the running app), not the untrimmed runtime pack.
  • Docs added to src/mono/wasm/features.md.

Validation

  • Browser CoreCLR interpreter builds clean.
  • End-to-end verified: browser run → .nettrace with JitInstrumentationDataVerbose events → dotnet-pgo → valid .mibc.

Note

This PR description was drafted with GitHub Copilot.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@pavelsavara pavelsavara added this to the 12.0.0 milestone Aug 24, 2026
@pavelsavara pavelsavara added arch-wasm WebAssembly architecture os-browser Browser variant of arch-wasm labels Aug 24, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

Comment thread src/coreclr/vm/jitinterface.cpp Outdated
@pavelsavara

pavelsavara commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

Blazor WASM PGO profile/trace https://gist.github.com/pavelsavara/70de5d2c5a7575f35eba0a72fc9e0abb

@pavelsavara

Copy link
Copy Markdown
Member Author
image

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved moderate findings affect counter correctness, session-specific flushing, trace collection, and end-to-end validation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds CoreCLR WebAssembly interpreter block-count PGO instrumentation and browser-side EventPipe trace collection for dotnet-pgo/R2R workflows.

Changes:

  • Adds WASM interpreter probes, shared PGO allocation, and configuration.
  • Adds EventPipe flushing and collectPgoTrace().
  • Adds documentation, build integration, and end-to-end validation.
File summaries
File Reviewed change / final review note
src/native/libs/System.Native.Browser/diagnostics/types.ts Adds the PGO EventPipe keyword.
src/native/libs/System.Native.Browser/diagnostics/index.ts Exposes the PGO collector.
src/native/libs/System.Native.Browser/diagnostics/dotnet-pgo-trace.ts Implements timed trace collection. moderate (1 vote): stale timers can stop a later session; associate the timer with its original session.
src/native/libs/System.Native.Browser/diagnostics/diagnostic-server-js.ts Supports startup js://pgo tracing. moderate (1 vote): add coverage for startup registration and downloaded traces.
src/native/libs/System.Native.Browser/diagnostics/client-commands.ts Defines the PGO EventPipe command.
src/native/libs/Common/JavaScript/types/public-api.ts Declares the diagnostics API.
src/native/libs/Common/JavaScript/loader/dotnet.d.ts Updates loader typings.
src/native/eventpipe/ep.c Invokes the session-stopping hook. moderate (1 vote): session-agnostic flushing broadcasts duplicate PGO chunks; make flushing session-aware or only flush when appropriate.
src/native/eventpipe/ep-rt.h Declares the lifecycle hook. nit (3 votes): correct the inaccurate EventPipe-lock contract comment.
src/mono/wasm/Wasm.Build.Tests/Wasm.Build.Tests.csproj Includes dotnet-pgo in test payloads.
src/mono/wasm/Wasm.Build.Tests/Blazor/EventPipeDiagnosticsTests.cs Adds end-to-end PGO validation. moderate (2 votes): use the trimmed linker directory. moderate (3 votes): assert BasicBlockIntCount data, not only method presence.
src/mono/wasm/features.md Documents WASM PGO usage. nit (1 vote): align DLL identity guidance with the tool’s actual CodeView/PDB GUID validation.
src/mono/nuget/Microsoft.NET.Sdk.WebAssembly.Pack/build/Microsoft.NET.Sdk.WebAssembly.Browser.CoreCLR.targets Integrates the browser CoreCLR build settings.
src/mono/mono/eventpipe/ep-rt-mono.h Adds the Mono no-op lifecycle hook.
src/coreclr/vm/pgo.h Declares PGO instrumentation flushing.
src/coreclr/vm/pgo.cpp Flushes accumulated instrumentation data.
src/coreclr/vm/jitinterface.h Exposes shared PGO interface methods.
src/coreclr/vm/jitinterface.cpp Shares PGO allocation with the interpreter. moderate (1 vote): limit the tiering-gate relaxation to the interpreter callback.
src/coreclr/vm/interpexec.cpp Executes PGO counter probes. moderate (1 vote): threaded builds can race on the counter; use synchronized counters or exclude them. moderate (2 votes): use the unsigned counter type to avoid signed overflow and match the schema.
src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h Adds the CoreCLR lifecycle hook declaration.
src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.cpp Connects EventPipe stopping to PGO flushing. moderate (2 votes): prevent duplicate chunks when sessions overlap.
src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h Adds the AOT no-op hook.
src/coreclr/interpreter/interpconfigvalues.h Defines interpreter PGO settings.
src/coreclr/interpreter/inc/intops.def Adds the PGO counter opcode.
src/coreclr/interpreter/eeinterp.cpp Initializes interpreter PGO instrumentation.
src/coreclr/interpreter/compiler.h Stores interpreter instrumentation state and helpers.
src/coreclr/interpreter/compiler.cpp Emits block-head probes. moderate (1 vote): increment the unsigned BasicBlockIntCount counter with an unsigned type.
src/coreclr/inc/clrconfigvalues.h Adds interpreter PGO configuration.
src/coreclr/clrfeatures.cmake Enables PGO for WASM.
Review details

Suppressed comments (7)

src/coreclr/interpreter/compiler.cpp:8757

  • BasicBlockIntCount is an unsigned four-byte counter (see corjit.h/PgoFormat.cs), but this executes a signed int32_t increment. A hot interpreted method can eventually overflow INT32_MAX, which is undefined behavior in C++, and the access does not match the schema's unsigned representation. Use a uint32_t* (or an equivalent unsigned increment) here.
        int32_t *pCounter = (int32_t*)(pInstrumentationData + pSchema[i].Offset);

src/coreclr/vm/interpexec.cpp:2071

  • INTOP_PGO_COUNT is compiled for threaded browser/WASI builds too: WasmEnableThreads=true removes PERFTRACING_DISABLE_THREADS, while this opcode is guarded only by the target. Multiple workers can race on this read-modify-write, and session stopping can read the same counter concurrently, so counts can be lost or undefined. Use an atomic/interlocked counter with a synchronized snapshot, or explicitly exclude threaded builds.
                    (*(int32_t*)pMethod->pDataItems[ip[1]])++;

src/coreclr/vm/jitinterface.cpp:13095

  • CEECodeGenInfo is the common base of both CEEJitInfo and CInterpreterJitInfo, so this condition also relaxes the JIT's tiering-eligibility gate whenever DOTNET_InterpPGO=1. Any JIT PGO phase can then allocate instrumentation for non-tiering-eligible methods, and a later JIT schema can replace an interpreter schema for the same method in PgoManager. Keep the relaxation limited to the interpreter callback rather than this shared implementation.
    // Only try instrumenting tiering-eligible methods, unless interpreter PGO is enabled, in
    // which case we instrument every method for offline profile collection.
    MethodDesc* pMD = (MethodDesc*)ftnHnd;
    if (pMD->IsEligibleForTieredCompilation() || InterpreterPgoInstrumentationEnabled())
    {

src/mono/wasm/features.md:471

  • The conversion tool currently validates CodeView/PDB GUIDs (src/coreclr/tools/dotnet-pgo/Program.cs:1304-1322) and explicitly notes that it does not match MVIDs (:1340). This documentation therefore attributes Dll mismatch to an MVID check that dotnet-pgo does not perform; please align the guidance with the actual identity check (or update the tool and docs together) so users do not diagnose the wrong cause.
`--reference` must point at assemblies whose **MVID** matches the modules recorded in the trace, otherwise
`dotnet-pgo` reports `Dll mismatch ...` (or `Unknown ModuleID` for the affected methods). On browser/wasm
the assemblies loaded by the runtime are the **IL-trimmed** ones: `PublishTrimmed`/ILLink rewrites each
assembly and **generates a fresh MVID**, then those trimmed DLLs are converted to the fingerprinted
`*.wasm` files in `_framework` (webcil preserves the MVID byte-for-byte). So the trace records the
**trimmed** MVIDs, which do **not** match the untrimmed assemblies in the runtime pack

src/native/eventpipe/ep.c:808

  • ep_rt_session_stopping() is called for every stop_session(id), but the hook has no session ID and WritePgoData() uses the normal EventPipe write path. Those events are broadcast to every still-live session, so stopping an unrelated or earlier diagnostic session flushes the complete PGO dataset into this trace; the later PGO-session stop flushes it again. dotnet-pgo rejects a new chunk after a method's final chunk and drops that method, making traces unreliable when sessions overlap. Make the hook session-aware/target the write, or flush only once when the final relevant session stops.
		// Give the runtime a chance to emit any pending end-of-session data (e.g. block-count PGO)
		// into the still-live session. This must run before taking the EventPipe lock: emitting events
		// re-enters the write path, which requires the lock not be held.
		ep_rt_session_stopping ();

src/native/libs/System.Native.Browser/diagnostics/diagnostic-server-js.ts:179

  • The existing PGO test invokes collectPgoTrace from an already-running page, so it does not exercise this new js://pgo startup registration. A failure in createDiagConnectionJs or the startup=true setup would leave the documented pre-managed-code capture broken while the test still passes. Add a startup-port case that verifies the downloaded trace.
        if (scenarioName.startsWith("js://pgo")) {
            collectPgoTrace({}, true);

src/native/libs/System.Native.Browser/diagnostics/dotnet-pgo-trace.ts:32

  • The timeout callback is detached from the session it was created for and stops whatever session is currently in the global pgoSession. If the first session closes early and a second trace starts before the first timeout fires, the stale timer will stop the second trace prematurely. Capture/check the original session before sending the stop command.
        Module.safeSetTimeout(() => {
            stopPgoTrace();
        }, 1000 * durationSeconds);
  • Files reviewed: 28/29 changed files
  • Comments generated: 5
  • Review effort level: Lite

Comment thread src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.cpp Outdated
Comment thread src/coreclr/vm/interpexec.cpp Outdated
Comment thread src/mono/wasm/Wasm.Build.Tests/Blazor/EventPipeDiagnosticsTests.cs
Comment thread src/mono/wasm/Wasm.Build.Tests/Blazor/EventPipeDiagnosticsTests.cs Outdated
Comment thread src/native/eventpipe/ep-rt.h Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unresolved moderate issues remain around threaded WASM counters, session-stop safety, and incremental browser-bundle dependencies.

Review details

Suppressed comments (8)

Previously missed (1) — in code that hasn't changed since the last review.

src/native/libs/System.Native.Browser/diagnostics/dotnet-pgo-trace.ts:1

  • This new module is imported by the browser bundle but is missing from src/native/libs/Common/JavaScript/CMakeLists.txt's ROLLUP_TS_SOURCES dependency list. As a result, incremental builds can keep using a stale generated bundle after this file changes (or after a fix is made here); add it to that list so Rollup is rerun when the module changes.

src/coreclr/interpreter/compiler.cpp:2255

  • m_emitInterpPGO is enabled for all browser/WASI builds, including builds where FeatureMultithreading leaves PERFTRACING_DISABLE_THREADS undefined. In that configuration multiple interpreter threads can execute the same probe, but INTOP_PGO_COUNT performs a plain uint32_t increment, so counts are lost or raced; the session-stop path also explicitly has no synchronized flush. Either gate this instrumentation to the threadless configuration or use atomic counters and add the matching threaded flush implementation before exposing it there.
    m_emitInterpPGO = s_interpPgoEnabled
        && (InterpConfig.InterpPGOMethods().isEmpty()
            || InterpConfig.InterpPGOMethods().contains(compHnd, m_methodHnd, m_classHnd, &m_methodInfo->args));

src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.cpp:209

  • WasmEnableThreads=true reaches this branch when the public collectPgoTrace session stops, but the only behavior is PORTABILITY_ASSERT; the trace therefore cannot be collected safely (and the plain counter increments are also unsynchronized on threaded execution). Either implement synchronized counting/flush for threaded WASM or explicitly disable/reject this API/configuration before a session is started instead of exposing a stop path that aborts or drops the block counts.
	// Multithreaded WASM: interpreter block-count PGO has no synchronized flush path yet. This hook runs for
	// every stopping session, so only trip when the stopping session actually enabled the PGO keyword (a real
	// collection attempt on this unsupported config); unrelated sessions (CPU/GC/counters) are unaffected.

src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.cpp:210

  • This branch runs for every EventPipe session stop in a threaded browser/WASI build, not only for a session that enabled JitInstrumentationDataVerbose. Because PERFTRACING_DISABLE_THREADS is omitted when multithreading is enabled, stopping an otherwise unrelated counters, CPU-sampling, or GC-dump session reaches PORTABILITY_ASSERT and aborts the process. Gate the unsupported-path assertion on the stopping session's PGO keyword (and leave unrelated sessions as a no-op), or implement the threaded flush path.
#elif defined(FEATURE_PGO) && (defined(TARGET_BROWSER) || defined(TARGET_WASI))
	// Multithreaded WASM: interpreter block-count PGO has no synchronized flush path yet. This hook runs for
	// every stopping session, so only trip when the stopping session actually enabled the PGO keyword (a real
	// collection attempt on this unsupported config); unrelated sessions (CPU/GC/counters) are unaffected.
	extern EventPipeEvent *EventPipeEventJitInstrumentationDataVerbose;

src/coreclr/vm/interpexec.cpp:2073

  • INTOP_PGO_COUNT is compiled for threaded WASM too, because PERFTRACING_DISABLE_THREADS is omitted when FeatureMultithreading=true; interpreter executions can therefore update the same counter concurrently. The plain ++ is a data race and loses increments, producing incorrect hotness (and undefined C++ behavior). Use an atomic increment for the 32-bit counter storage, or disable emission of these probes for threaded WASM together with the unsupported flush path.
                INTOP_CASE(INTOP_PGO_COUNT)
                    // Increment the block-count PGO counter whose address is stored as a data item.
                    (*(uint32_t*)pMethod->pDataItems[ip[1]])++;
                    ip += 2;

src/mono/wasm/Wasm.Build.Tests/Blazor/EventPipeDiagnosticsTests.cs:288

  • This only checks that some method in the generated MIBC has BasicBlockIntCount; it does not verify that the expected IncrementCount method has block data. A regression that drops instrumentation or execution counts for the method under test would still pass as long as another runtime method is instrumented. Parse the dump's per-method InstrumentationData (or otherwise scope the assertion) and require the expected method's record to contain BasicBlockIntCount.
        Assert.Contains("BasicBlockIntCount", dumpText);

src/native/eventpipe/ep.c:816

  • This lock only protects the session-id lookup; it does not keep the session alive while ep_rt_session_stopping(id) runs. A concurrent ep_disable(id) can pass the same check, unpublish/free the session, and let this callback dereference the stale ID (the CoreCLR callback calls ep_session_get_mask and writes to that session). Serialize stop callbacks or hold an explicit session lifetime/reference across the callback while still keeping the EventPipe lock released for event emission.
		EP_LOCK_ENTER (section1)
			is_active_session = is_session_id_in_collection (id);
		EP_LOCK_EXIT (section1)

		if (is_active_session)
			ep_rt_session_stopping (id);

src/native/libs/System.Native.Browser/diagnostics/index.ts:16

  • The new module is imported here, but it is missing from src/native/libs/Common/JavaScript/CMakeLists.txt's explicit ROLLUP_TS_SOURCES list (that list is the custom command's DEPENDS). Incremental native builds can therefore keep an old browser bundle without collectPgoTrace/js://pgo after this file is added or edited; add the new path to that dependency list.
import { collectPgoTrace } from "./dotnet-pgo-trace";
  • Files reviewed: 28/29 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 16, 2026 11:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved critical findings affect block-count completeness, EventPipe session safety, and repeated trace collection.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

src/native/libs/System.Native.Browser/diagnostics/index.ts:16

  • This import adds a new Rollup module, but src/native/libs/Common/JavaScript/CMakeLists.txt's explicit ROLLUP_TS_SOURCES dependency list does not include diagnostics/dotnet-pgo-trace.ts. Because the Rollup custom command depends on that list, subsequent edits to this module alone will not invalidate the browser bundle and can leave the deployed collectPgoTrace implementation stale. Add the new module to the dependency list.

src/coreclr/interpreter/compiler.cpp:8685

  • This target gate includes WASI, but the current CoreCLR WASI build sets FEATURE_EVENT_TRACE=0 and FEATURE_PERFTRACING=0 in src/coreclr/CMakeLists.txt, and the new JavaScript trigger is browser-only. WASI can therefore allocate these counters but cannot start an EventPipe session or flush them into a profile, so the advertised browser/WASI collection path is not usable. Add equivalent WASI diagnostics plumbing or remove WASI from this feature gate until it is supported.
#if (defined(TARGET_BROWSER) || defined(TARGET_WASI)) && defined(PERFTRACING_DISABLE_THREADS)

src/coreclr/vm/jitinterface.cpp:13069

  • This gate is target-scoped but not restricted to PERFTRACING_DISABLE_THREADS. On a multithreaded Browser/WASI build, setting DOTNET_InterpPGO=1 makes the shared allocPgoInstrumentationBySchema path treat every method as eligible even though InstrumentBlockCounts and INTOP_PGO_COUNT are compiled out, so the build can pay for JIT PGO instrumentation without producing the promised interpreter profile (and a PGO trace stop later hits the unsupported-config assert). Include the single-thread feature gate here so the relaxation is disabled in the same configurations as the interpreter probes.
#if defined(TARGET_BROWSER) || defined(TARGET_WASI)
    static ConfigDWORD s_interpPgo;
    return s_interpPgo.val(CLRConfig::INTERNAL_InterpPGO) != 0;

src/native/libs/System.Native.Browser/diagnostics/diagnostic-server-js.ts:11

  • This new module is not present in src/native/libs/Common/JavaScript/CMakeLists.txt's ROLLUP_TS_SOURCES, which is the dependency list for the Rollup custom command. The initial bundle follows this import, but later edits to dotnet-pgo-trace.ts will not rerun the bundle during incremental builds, leaving stale diagnostics code. Add the new TypeScript file to that dependency list.
import { collectPgoTrace } from "./dotnet-pgo-trace";
  • Files reviewed: 28/29 changed files
  • Comments generated: 3
  • Review effort level: Lite

Comment on lines +8724 to +8725
if (bb->ilOffset == 0 || isBranchTarget[bb->index])
blocks.Add(bb);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is flip side of Vlad's question above.

Comment thread src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.cpp Outdated
Comment thread src/coreclr/vm/pgo.cpp Outdated
…ources

Capture the stopping session's keyword mask under the EventPipe lock in stop_session and pass it to ep_rt_session_stopping, so ep_rt_coreclr_session_stopping tests the JitInstrumentationData keyword without dereferencing a session pointer a concurrent stop may have freed. The single-threaded flush still uses the session pointer for the rundown-thread marker, which is safe under PERFTRACING_DISABLE_THREADS.

Add dotnet-pgo-trace.ts to ROLLUP_TS_SOURCES so incremental browser-bundle builds rerun Rollup when it changes.
Comment thread src/native/libs/System.Native.Browser/diagnostics/client-commands.ts Outdated
Per review feedback, avoid the ambiguous term 'desktop' (glossary-synonymous with .NET Framework) in the JitInstrumentationData keyword comment.
Copilot AI review requested due to automatic review settings September 16, 2026 14:24
Copilot stopped reviewing on behalf of pavelsavara due to an error September 16, 2026 14:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

Copilot reviewed 29 out of 30 changed files in this pull request and generated 3 comments.

Comment thread src/mono/wasm/Wasm.Build.Tests/Wasm.Build.Tests.csproj
Comment thread src/mono/wasm/Wasm.Build.Tests/Wasm.Build.Tests.csproj
Copilot AI review requested due to automatic review settings September 16, 2026 14:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved critical staging and moderate instrumentation and validation issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/coreclr/interpreter/compiler.cpp:8725

  • The JIT assigns zero weight to every block whose IL offset is absent from the block-count schema (src/coreclr/jit/fgprofile.cpp:321-342) and applies that value to all flow-graph blocks. With only entry and explicit branch targets recorded here, ordinary fall-through and exception-handler blocks are emitted as cold even when they execute; no consumer-side reconstruction fills them in before R2R. Emit a probe for every canonical IL block (or add an explicit reconstruction step) before generating the MIBC.
        if (bb->ilOffset == 0 || isBranchTarget[bb->index])
            blocks.Add(bb);

src/mono/wasm/Wasm.Build.Tests/Blazor/EventPipeDiagnosticsTests.cs:295

  • Checking only for BasicBlockIntCount verifies that a schema was emitted, not that the interpreter executed INTOP_PGO_COUNT or that any counter was incremented: a profile with all block values zero still satisfies this assertion. Since this test drives IncrementCount, also assert a non-zero block count for that method (or parse the dump/MIBC and reject zero-sum counters) so the end-to-end test covers the actual instrumentation behavior.
        // The method list alone can be populated by Jit method-start events; require actual block-count
        // instrumentation so the test fails if no INTOP_PGO_COUNT probe ran or the counters weren't flushed.
        Assert.Contains("BasicBlockIntCount", dumpText);
  • Files reviewed: 29/30 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/mono/wasm/Wasm.Build.Tests/Wasm.Build.Tests.csproj
Comment thread src/coreclr/vm/pgo.cpp Outdated
Comment thread src/native/eventpipe/ep.c
// flush into other open sessions, and pass it so the runtime can target only that session. This
// must run before the disable lock: emitting events re-enters the write path, which requires the
// lock not be held.
bool is_active_session = false;

@lateralusX lateralusX Sep 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can race and since the logic here is generic (not specific to single threaded platforms), I believe we either need to move it into a lower level and implement it only on single threaded platform or look into implementing this flush logic more broadly so it could work on multithreaded platforms as well. Let me think a little around this and what alternatives we might have.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood — this is your call on placement. For current state: the hook fires for every stopping session, but the CoreCLR side only does work under PERFTRACING_DISABLE_THREADS, and the mask it needs is now captured under the EventPipe lock in stop_session and passed in (ep_rt_session_stopping(id, session_mask)), so there's no off-lock session deref / UAF — the remaining question is purely where the generic hook lives. Happy to go either way: move the call site into a single-threaded-only path, or keep it generic if we later want a multithreaded flush. Let me know which you'd prefer and I'll restructure accordingly.

🤖 Reply drafted with GitHub Copilot.

@lateralusX lateralusX Sep 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long term, I would like to generalize the rundown mechanism so we can establish a scoped target session on a thread. EventPipe writes from that thread would then go only to that session, subject to its event filters. Rundown would build on that routing mechanism with a separate flag for rundown-specific behavior.

Supporting this on multithreaded runtimes would require session ownership/lifetime coordination: while a thread holds that scope, another stop must not disable or dispose the session underneath it. That is a larger change than we should take on in this PR, but could be worth exploring for .NET 12 because other scenarios may need it too.

For this PR, I suggest:

  1. Add a static session_stopping helper in ep.c, implemented only under PERFTRACING_DISABLE_THREADS and a no-op otherwise. Call it before the existing disable operation, within the existing preemptive GC scope.

  2. In that helper, acquire the configuration lock, validate the session, save the thread's previous rundown-session state (if one exists), and bind the thread to the stopping session. Release the lock before calling ep_rt_session_stopping. Afterward, reacquire the lock and restore the previous state, including if emission fails.

  3. Add a shared helper named ep_event_is_enabled_for_current_thread in shared EventPipe sources. If the current thread has a rundown session, use ep_event_is_enabled_by_mask(event, ep_session_get_mask(session)). Otherwise, fall back to ep_event_is_enabled(event). If the target session does not enable the event, return false rather than falling back to other sessions.

  4. Use that helper in the CoreCLR hook before generating the instrumentation data. This keeps the event-specific decision in CoreCLR and session validation/routing in EventPipe. The CoreCLR implementation shouldn't need to do any additional EP integration than calling ep_event_is_enabled_for_current_thread to decide if it should serialize the JIT instrumentation data.

This preserves the existing locking contracts for session validation and setup while allowing the runtime hook to emit events without holding the configuration lock.

We should explicitly document that the unlocked interval is safe only because this path runs on a threadless runtime and must not yield or reenter session teardown while calling into ep_rt_session_stopping. Neither the rundown-session pointer nor the enablement query keeps the session alive. Multithreaded support would require the session ownership mechanism described above, rather than simply removing the compile-time guard.

The thread-aware enablement helper would also remain useful after the future refactoring: it would consult the thread's explicit target session instead of its rundown-session pointer.

Extract EmitInstrumentationDataToEventPipe from WritePgoData so FlushInstrumentationData (the on-demand session-stop flush) emits only to EventPipe and never triggers the DOTNET_WritePGOData text-file dump. Guard the shutdown WritePgoData EventPipe emission with !PERFTRACING_DISABLE_THREADS so single-threaded WASM does not emit each method a second time into a session EventPipe stops during shutdown (which dotnet-pgo rejects as a duplicate chunk); threaded desktop still emits at shutdown as before.
collectPgoTrace now rejects a second collection after the first has run and flushed, since the interpreter block-count counters are cumulative and re-emitting them would produce a duplicate chunk sequence that dotnet-pgo drops. The latch is set only once a session actually started, so a setup that fails before starting still allows a retry.
# Conflicts:
#	src/mono/nuget/Microsoft.NET.Sdk.WebAssembly.Pack/build/Microsoft.NET.Sdk.WebAssembly.Browser.CoreCLR.targets
Copilot AI review requested due to automatic review settings September 17, 2026 12:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Critical build/test issues and unresolved WASI and block-count correctness concerns must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

src/coreclr/interpreter/compiler.cpp:8689

  • This adds the block-count producer to TARGET_WASI, but the current CoreCLR WASI configuration sets FEATURE_PERFTRACING=0 in src/coreclr/CMakeLists.txt:37-45; consequently the EventPipe hook and JitInstrumentationDataVerbose export are not built, and WASI has no corresponding JS trigger. A WASI run can allocate and increment counters that can never become an .mibc; either keep this instrumentation browser-only until WASI diagnostics are enabled, or add the missing export path before claiming WASI support.
#if (defined(TARGET_BROWSER) || defined(TARGET_WASI)) && defined(PERFTRACING_DISABLE_THREADS)
// Instrument each basic block with a block-count PGO probe. The counters are allocated by
// allocPgoInstrumentationBySchema (native PgoManager memory), so they persist independently of
// EventPipe session lifetime; the accumulated profile is flushed to the trace as
// JitInstrumentationDataVerbose events, which dotnet-pgo consumes to build an .mibc.

src/coreclr/interpreter/compiler.cpp:8725

  • This filter emits counts only for the entry and explicit branch targets, but the block-count consumer does not reconstruct omitted blocks: fgGetProfileWeightForBasicBlock returns zero when an IL offset has no schema entry (src/coreclr/jit/fgprofile.cpp:321-342), and fgIncorporateBlockCounts assigns that value to the block. Hot fall-through blocks and exception-handler entries will therefore be serialized as cold in the MIBC; emit every canonical real IL block or add reconstruction before producing the profile.
        if (bb->ilOffset == 0 || isBranchTarget[bb->index])
            blocks.Add(bb);

src/coreclr/interpreter/inc/intops.def:95

  • The TARGET_WASI branch is not active in the current CoreCLR WASI build: src/coreclr/CMakeLists.txt:37-45 sets FEATURE_PERFTRACING=0, and src/coreclr/interpreter/CMakeLists.txt:50-52 defines PERFTRACING_DISABLE_THREADS only when perf tracing is enabled. Consequently WASI emits no INTOP_PGO_COUNT and has no EventPipe flush path, so the advertised browser/WASI collection support is currently browser-only. Either enable the required WASI diagnostics plumbing or remove the WASI guard/claim until that follow-up lands.
#if (defined(TARGET_BROWSER) || defined(TARGET_WASI)) && defined(PERFTRACING_DISABLE_THREADS)
OPDEF(INTOP_PGO_COUNT, "pgo.count", 2, 0, 0, InterpOpLdPtr)
#endif

src/native/eventpipe/ep-rt.h:245

  • session_mask is not a keyword mask: ep_session_get_mask returns the single session-routing bit (1 << session->index), which is exactly what ep_event_is_enabled_by_mask expects. Calling it a keyword mask makes this hook's contract misleading and could cause a future implementation to pass provider keyword flags instead; describe it as the session bit/routing mask.
// is the session's keyword mask captured under the EventPipe lock, so the runtime can test provider
// keywords without dereferencing the session, which a concurrent stop may free once the lock is
  • Files reviewed: 29/30 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread src/mono/wasm/Wasm.Build.Tests/Wasm.Build.Tests.csproj
@AndyAyersMS

Copy link
Copy Markdown
Member

If the idea is for JIT to be able to leverage this data, we need to pay careful attention to the schema formation. In the JIT, count reconstruction from sparse profiles currently only runs with edge profiling, and the schema used for this must be one the JIT can recreate from IL analysis (probably tricky to pull off).

If you want to emit sparse block data we would need a new reconstruction algorithm in the JIT to try and infer the missing counts. Or maybe there is SPGO code in dotnet-pgo that can do likewise. That would free us from having to try and match the JIT's notion of basic block boundaries.

If the JIT is not the intended consumer then we can ignore all that.

Also note that class identity information can be quite useful (class histograms), as well as "value profiles". This is what lights up GDV and other advanced opts.

Comment thread src/coreclr/vm/pgo.cpp
typedef Holder<FILE*, DoNothing, CallFClose> FILEHolder;

void PgoManager::WritePgoData()
void PgoManager::EmitInstrumentationDataToEventPipe()

@lateralusX lateralusX Sep 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking through the source a little, a better name for this that is inline with the per method instrumentation data called by this method would be LogInstrumentationData().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arch-wasm WebAssembly architecture area-VM-coreclr os-browser Browser variant of arch-wasm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants