[browser] single threaded diagnostic server #111910

pavelsavara · 2025-01-28T16:37:15Z

This is split from #110818 to make code review easier

Enabled by <FeaturePerfTracing>true</FeaturePerfTracing>

Single thread

ep_rt_thread_create and ep_rt_thread_mono_start_func
- it will run short, non-blocking jobs on the browser event loop instead of on dedicated threads
- streaming_loop_tick refactored from streaming_thread
- server_loop_tick from refactored server_thread
mono_wasm_ds_exec to process on tick on event loop
mono_main_thread_schedule_ds_job will register callback into queue for above
NOOP implementation of ep_rt_wait_event_wait
new FEATURE_PERFTRACING_SINGLE_THREADED -> PERFTRACING_MULTI_THREADED, PERFTRACING_SINGLE_THREADED
ds_ipc_stream_factory_get_next_available_stream only polls once and never blocks

Sampling

disabled sampling_thread because we can't do "stop-the-world and scan other threads"
new events ep_rt_sample_profiler_enabled and ep_rt_sample_profiler_disabled
new ep_rt_mono_sampling_provider_component_init
- install mono profiler instrumentation MONO_PROFILER_CALL_INSTRUMENTATION_ENTER
- this needs to happen before any managed code is executed
it has negative performance impact when workload & re-linked with FeaturePerfTracing
- this is just single condition per method enter (and later per safe-point)
- we could introduce separate FeaturePerfTracingSampling to enable/disable this if necessary

Web socket

FEATURE_PERFTRACING_PAL_WS
ds-ipc-pal-websocket.c and ds-ipc-pal-websocket.h they just forward to JavaScript
empty for now ds_rt_websocket_create, ds_rt_websocket_send, ds_rt_websocket_poll, ds_rt_websocket_recv, ds_rt_websocket_close

Other

include diagnostics_tracing-static.a in in-tree builds
Microsoft.NETCore.App.Runtime.Mono.perftrace.**RID** cleanup
fix SystemNative_GetCpuUtilization
fix crash dump file permissions for CI/helix

Next PRs

the rest of the implementation in JavaScript, for WebSocket and JS client
better sampling instrumentation via MINT_SDB_SEQ_POINT or similar
tests

Out of scope

pause runtime during startup
multi-threaded EP on browser - this will need some cross thread marshaling

dotnet-policy-service · 2025-01-28T16:37:46Z

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

pavelsavara · 2025-01-28T20:14:03Z

@lateralusX please review this first step

maraf · 2025-01-29T11:44:08Z

src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj

@@ -135,7 +135,7 @@
    <ILLinkDescriptorsXmls Include="$(ILLinkDirectory)ILLink.Descriptors.xml" />
    <ILLinkDescriptorsXmls Include="$(ILLinkDirectory)ILLink.Descriptors.OSX.xml" Condition="'$(TargetsOSX)' == 'true' or '$(TargetsMacCatalyst)' == 'true' or '$(TargetsiOS)' == 'true' or '$(TargetstvOS)' == 'true'" />
    <ILLinkDescriptorsXmls Include="$(CoreLibSharedDir)ILLink\ILLink.Descriptors.Shared.xml" />
-    <ILLinkDescriptorsXmls Condition="'$(FeaturePerfTracing)' == 'true'" Include="$(CoreLibSharedDir)ILLink\ILLink.Descriptors.EventSource.xml" />
+    <ILLinkDescriptorsLibraryBuildXml Condition="'$(FeaturePerfTracing)' == 'true'" Include="$(CoreLibSharedDir)ILLink\ILLink.Descriptors.EventSource.xml" />


Should this be moved to https://github.com/dotnet/runtime/blob/bfa02766bdde4657ddd1f734e274d43e6ba3c036/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems#L67 where others are?

I wonder what is rooting it for x64 NAOT, when trimmed

This make sure we don't link out the InitializeDefaultEventSources method called during Mono startup to setup runtime specific event sources. Mono root this function through above file and CoreClr does a similar thing, but CoreClr uses additional tasks to root methods called by runtime by scanning different source files, like corlib.h and generate a ILLink.Descriptors.xml as part of build. NativeAOT uses ModuleInitializer preventing this function from being linked out when running ilc on the published app but not exactly sure how its rooting it as part of S.P.C build, but I assume it ends up with similar mechanism.

btw, why do we need to change fro ILLinkDescriptorsXmls to ILLinkDescriptorsLibraryBuildXml for just his specific item?

We run ILLink again during publish build step on customer dev machine. This file needs to be there to protect it from trimming. With ILLinkDescriptorsXmls it didn't work for me. Is this the right way ?

For publish on dev machines, EventSource related logic is kept using linker msbuild property, EventSourceSupport. That is how it works on all other platforms so I assume it should work the same on WASM. It should keep all EventSource related managed code.

maraf · 2025-01-29T11:45:35Z

src/mono/browser/build/BrowserWasmApp.targets

@@ -362,6 +363,8 @@
      <EmscriptenEnvVars Include="ENABLE_JS_INTEROP_BY_VALUE=0" Condition="'$(WasmEnableJsInteropByValue)' != 'true'" />
      <EmscriptenEnvVars Include="WASM_ENABLE_SIMD=1" Condition="'$(WasmEnableSIMD)' != 'false'" />
      <EmscriptenEnvVars Include="WASM_ENABLE_SIMD=0" Condition="'$(WasmEnableSIMD)' == 'false'" />
+      <EmscriptenEnvVars Include="FEATURE_PERFTRACING=1" Condition="'$(FeaturePerfTracing)' != 'false'" />


Is this in contradiction with default value set on L6?

Nope, this is just passing the value (which doesn't have to be same as default) to emscripten as env variable.

Configuration=Debug => FeaturePerfTracing=true => FEATURE_PERFTRACING=1
Configuration=Release => FeaturePerfTracing='' => FEATURE_PERFTRACING=1

What is the reason to set FeaturePerfTracing based on Configuration?

CoreCLR includes EP always. In Browser, I would like to include it when workload+Debug. Unless user specified FeaturePerfTracing explicitly. I missed the Release -> empty branch. Fixed now.

In Mono we have components meaning that an app can either statically or dynamically enable parts of the runtime when linking the app, using the same runtime build. EventPipe uses this (hot reload, debugger as well) and our mobile platforms uses it to enable/disable EventPipe using msbuild flags when building the app. Recommendation is to exclude it for release builds that should be pushed to app stores, but include it for debug and profiling builds of the apps used for debugging or testing.

lateralusX

Mainly looked at EventPipe and Mono native code in this first review pass.

lateralusX · 2025-01-31T13:26:07Z

src/native/eventpipe/ep-shared-config.h.in

+#ifdef FEATURE_PERFTRACING_SINGLE_THREADED
+#define PERFTRACING_SINGLE_THREADED
+#else
+#define PERFTRACING_MULTI_THREADED


Looks like we now will have multiple different defines controlling this, in Mono it seems that we have an older DISABLE_THREADS and here we get two more. Do we really need to define PERFTRACING_MULTI_THREADED ? That should be the default for event pipe and its PERFTRACING_SINGLE_THREADED that should be the special scenarios. Since we already have the concept of DISABLE_THREADS in Mono, maybe we should use similar naming for eventpipe, like FEATURE_PERFTRACING_DISABLE_THREADS and PERFTRACING_DISABLE_THREADS?

lateralusX · 2025-01-31T13:35:28Z

src/mono/mono/component/CMakeLists.txt

+
+    if (DISABLE_THREADS)
+        set_target_properties(
+            eventpipe-mono-objects


Do we need the wider Mono specific DISABLE_THREADS here or is this just needed since we have some checks in diagnostics_server.c that currently uses it? If so, then maybe we should change that code to use the eventpipe define for single threaded/disabled threads. Alternative could be to define this based on the more generic event pipe as part of mono specific ds or ep config's.

Here we are passing it to src\native\eventpipe\CMakeLists.txt so that it could set FEATURE_PERFTRACING_SINGLE_THREADED

lateralusX · 2025-01-31T13:38:11Z

src/mono/mono/eventpipe/ep-rt-mono-runtime-provider.c

@@ -25,8 +25,14 @@ extern EVENTPIPE_TRACE_CONTEXT MICROSOFT_WINDOWS_DOTNETRUNTIME_RUNDOWN_PROVIDER_
 #define NUM_NANOSECONDS_IN_1_MS 1000000

 // Sample profiler.
+#if defined(PERFTRACING_MULTI_THREADED)


I will just make this comment once, but it applies through all changes and is basically an effect of the last comment in this review. Multithreaded is the default, so we shouldn't need an explicit define for multithreaded support, instead we should use one define disabling threading.

you prefer #if !defined(PERFTRACING_SINGLE_THREADED) everywhere, right ?

lateralusX · 2025-01-31T14:41:18Z

src/mono/mono/eventpipe/ep-rt-mono-runtime-provider.c

+#else // !PERFTRACING_MULTI_THREADED
+
+bool
+ep_rt_mono_sample_profiler_write_sampling_event_for_threads (


How frequent will this be called on WASM? When we run regular sample profiler this is called on the timer interval defined in __sampling_rate_in_ns. Looking further down it appears that this method will only be called once, is that correct?

Correct, I just want to capture the sampling_event and sampling_thread to be able to use it later.

lateralusX · 2025-01-31T15:22:10Z

src/mono/mono/eventpipe/ep-rt-mono-runtime-provider.c

+}
+
+static void
+method_enter (MonoProfiler *prof, MonoMethod *method, MonoProfilerCallContext *ctx)


Should we sample on every call? Sample profiler only samples at a specific time interval, like very ms, setup in __sampling_rate_in_ns. Maybe we should only sample based on the sample rate instead on every call to be more inline with how sampling works on all other platforms.

That's right, it needs to consider sample frequency. But also I will bring another instrumentation for like safepoint.
I will make those improvements in next PR

lateralusX · 2025-01-31T16:38:49Z

src/native/eventpipe/ds-ipc-pal-websocket.c

+
+
+	DiagnosticsIpcStream *stream = NULL;
+


EP_ASSERT (ipc->mode == DS_IPC_CONNECTION_MODE_CONNECT); ?

lateralusX · 2025-01-31T16:43:50Z

src/native/eventpipe/ds-ipc.c

@@ -455,6 +455,13 @@ ds_ipc_stream_factory_get_next_available_stream (ds_ipc_error_callback_func call
 		// clear the view.
 		dn_vector_clear (&ipc_poll_handles);
 	}
+#if defined(PERFTRACING_MULTI_THREADED)


Another option would be to keep while loop as is and then just doing a break in single threaded mode at end.

lateralusX · 2025-01-31T16:56:47Z

src/native/eventpipe/ep-sample-profiler.c

@@ -192,6 +194,9 @@ sample_profiler_enable (void)
 	if (!sample_profiler_load_profiling_enabled ()) {
 		sample_profiler_store_profiling_enabled (true);

+		ep_rt_sample_profiler_enabled ();
+
+#if defined(PERFTRACING_MULTI_THREADED)
 		EP_ASSERT (!ep_rt_wait_event_is_valid (&_thread_shutdown_event));
 		ep_rt_wait_event_alloc (&_thread_shutdown_event, true, false);


Little curious if ep_rt_wait functions are all noop on browser or if they are actually implemented? Regardless we could just make the code work on both single and multithreaded and only do a sampling_thread function implementation that is different, would remove the need to ifdefing inside this function.

There is only busy spin wait on browser, you can't really wait.

Regardless we could just make the code work on both single and multithreaded

Probably not

we need to enter from interp loop, rather than from here.

we capture only one stack trace

the code for that is different on current thread than scanning another thread

lateralusX · 2025-01-31T17:07:46Z

src/native/eventpipe/ep-session.c

+	}
+	if (!ep_session_write_all_buffers_to_file (session, events_written)) {
+		session->streaming_thread = NULL;
+		ep_disable ((EventPipeSessionID)session);


I think we need to preserve this call to happen outside the EP_GCX_PREEMP_ENTER/EXIT as it did before.

OK. Could you please explain why (for my education) ?

lateralusX · 2025-01-31T17:13:34Z

src/native/eventpipe/ep-session.c

+	}
+	return 0; // try again
+}
+
 EP_RT_DEFINE_THREAD_FUNC (streaming_thread)


Maybe it is simpler and more clear to just create two functions keeping the first error/state checking code and then call into another function implemented differently for single and multi threaded support, you can still call the loop tick, but it might just be as simple to inline that part into each implementation once you have two methods.

pavelsavara · 2025-01-31T18:25:43Z

src/mono/browser/browser.proj

@@ -424,6 +424,8 @@
      <_CmakeEnvironmentVariable Include="ENABLE_JS_INTEROP_BY_VALUE=0" Condition="'$(WasmEnableJsInteropByValue)' == 'false'"/>
      <_CmakeEnvironmentVariable Include="WASM_ENABLE_SIMD=1" Condition="'$(WasmEnableSIMD)' != 'false'" />
      <_CmakeEnvironmentVariable Include="WASM_ENABLE_SIMD=0" Condition="'$(WasmEnableSIMD)' == 'false'" />
+      <_CmakeEnvironmentVariable Include="FEATURE_PERFTRACING=1" Condition="'$(FeaturePerfTracing)' != 'false'" />


Maybe there is same problem that Maraf reported

pavelsavara · 2025-01-31T18:26:06Z

src/mono/browser/browser.proj

@@ -568,6 +570,8 @@
      <_MonoRollupEnvironmentVariable Include="WasmEnableThreads:$(WasmEnableThreads)" />
      <_MonoRollupEnvironmentVariable Include="WASM_ENABLE_SIMD:1" Condition="'$(WasmEnableSIMD)' != 'false'" />
      <_MonoRollupEnvironmentVariable Include="WASM_ENABLE_SIMD:0" Condition="'$(WasmEnableSIMD)' == 'false'" />
+      <_MonoRollupEnvironmentVariable Include="FEATURE_PERFTRACING:1" Condition="'$(FeaturePerfTracing)' != 'false'" />


Maybe there is same problem that Maraf reported

wip

3cd951f

pavelsavara added arch-wasm WebAssembly architecture os-browser Browser variant of arch-wasm labels Jan 28, 2025

pavelsavara added this to the 10.0.0 milestone Jan 28, 2025

pavelsavara self-assigned this Jan 28, 2025

dotnet-issue-labeler bot added the area-Build-mono label Jan 28, 2025

pavelsavara added 2 commits January 28, 2025 17:59

fix

3bbee71

fix

fe063de

pavelsavara changed the title ~~[browser] single threaded diagnosti server~~ [browser] single threaded diagnostic server Jan 28, 2025

chmod a+rw

d6c5821

pavelsavara marked this pull request as ready for review January 28, 2025 20:13

pavelsavara requested review from lewing, akoeplinger, maraf, steveisok, vitek-karas, lateralusX and MichalStrehovsky as code owners January 28, 2025 20:13

pavelsavara added area-Diagnostics-mono and removed area-Build-mono labels Jan 28, 2025

fix

cdad8b2

maraf reviewed Jan 29, 2025

View reviewed changes

pavelsavara added 2 commits January 29, 2025 14:25

Merge branch 'main' into browser_st_ep_step1

46aa990

revert chmod

3b711be

pavelsavara added 3 commits January 29, 2025 17:07

put it back

685f663

feedback

2af88c3

Merge branch 'main' into browser_st_ep_step1

a4d0ac8

build-analysis bot mentioned this pull request Jan 31, 2025

[Linux-arm64] ModuleNotFoundError: No module named 'requests' #112021

Open

lateralusX reviewed Jan 31, 2025

View reviewed changes

build-analysis bot mentioned this pull request Jan 31, 2025

[wasm][AOT] emcc : error - received SIGKILL (-9) #89402

Open

pavelsavara commented Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[browser] single threaded diagnostic server #111910

[browser] single threaded diagnostic server #111910

pavelsavara commented Jan 28, 2025 •

edited

Loading

dotnet-policy-service bot commented Jan 28, 2025

pavelsavara commented Jan 28, 2025

maraf Jan 29, 2025

pavelsavara Jan 29, 2025

lateralusX Jan 31, 2025

lateralusX Jan 31, 2025

pavelsavara Jan 31, 2025

lateralusX Jan 31, 2025 •

edited

Loading

maraf Jan 29, 2025

pavelsavara Jan 29, 2025

maraf Jan 31, 2025

pavelsavara Jan 31, 2025

pavelsavara Jan 31, 2025

lateralusX Jan 31, 2025 •

edited

Loading

lateralusX left a comment

lateralusX Jan 31, 2025

lateralusX Jan 31, 2025

pavelsavara Jan 31, 2025

lateralusX Jan 31, 2025

pavelsavara Jan 31, 2025

lateralusX Jan 31, 2025

pavelsavara Jan 31, 2025

lateralusX Jan 31, 2025

pavelsavara Jan 31, 2025

lateralusX Jan 31, 2025

lateralusX Jan 31, 2025

lateralusX Jan 31, 2025

pavelsavara Jan 31, 2025

pavelsavara Jan 31, 2025

lateralusX Jan 31, 2025

pavelsavara Jan 31, 2025 •

edited

Loading

lateralusX Jan 31, 2025

pavelsavara Jan 31, 2025

pavelsavara Jan 31, 2025

[browser] single threaded diagnostic server #111910

Are you sure you want to change the base?

[browser] single threaded diagnostic server #111910

Conversation

pavelsavara commented Jan 28, 2025 • edited Loading

dotnet-policy-service bot commented Jan 28, 2025

pavelsavara commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lateralusX Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lateralusX Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

lateralusX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelsavara Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelsavara commented Jan 28, 2025 •

edited

Loading

lateralusX Jan 31, 2025 •

edited

Loading

lateralusX Jan 31, 2025 •

edited

Loading

pavelsavara Jan 31, 2025 •

edited

Loading