-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[browser] samplepoint instrumentation into Mono profiler #112352
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Hi @pavelsavara, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 12 out of 27 changed files in this pull request and generated 1 comment.
Files not reviewed (15)
- src/mono/mono/metadata/jit-icall-reg.h: Language not supported
- src/mono/mono/mini/interp/interp.c: Language not supported
- src/mono/mono/mini/interp/jiterpreter.h: Language not supported
- src/mono/mono/mini/interp/mintops.def: Language not supported
- src/mono/mono/mini/interp/transform.c: Language not supported
- src/mono/mono/mini/mini-profiler.c: Language not supported
- src/mono/mono/mini/mini-runtime.c: Language not supported
- src/mono/mono/mini/mini.c: Language not supported
- src/mono/mono/mini/mini.h: Language not supported
- src/mono/mono/mini/trace.c: Language not supported
- src/mono/mono/mini/trace.h: Language not supported
- src/mono/mono/profiler/browser.c: Language not supported
- src/mono/browser/runtime/exports-binding.ts: Evaluated as low risk
- src/mono/browser/runtime/es6/dotnet.es6.lib.js: Evaluated as low risk
- src/mono/browser/runtime/jiterpreter-trace-generator.ts: Evaluated as low risk
Comments suppressed due to low confidence (3)
src/mono/browser/runtime/profiler.ts:96
- The new function 'mono_wasm_profiler_record' should be covered by tests to ensure it works as expected.
export function mono_wasm_profiler_record (method: MonoMethod, start: number): void {
src/mono/browser/runtime/jiterpreter-support.ts:1619
- Ensure 'frame' is a valid local variable before using it. Add a check to verify 'frame' is defined.
builder.local("frame");
src/mono/browser/runtime/jiterpreter-support.ts:1617
- The error message should include the opcode value for better debugging. Consider changing to
Unimplemented profiler event: ${opcode}
.
throw new Error(`Unimplemented profiler event ${opcode}`);
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (I'm not an expert on aot or mini jit)
- switch profiler_stack_frames to fixed size buffer
|
||
MonoBasicBlock *prev_cbb = cfg->cbb; | ||
cfg->cbb = bblock; | ||
mini_profiler_emit_samplepoint (cfg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem correct. We are inserting code at the end of the basic block. Assuming that it is common for a basic block to end with a branch opcode, this would make the sample point not always hit. For reference, insert_safepoint
adds the instruction at the beginning of the basic block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BrzVlad Could you please suggest/commit a fix, I was struggling with this. Many thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simple solution that I suspect could work would be to create a new tmp_bblock
here instead (NEW_BBLOCK) and set it as the cbb. Then you do the emit as before into tmp_bblock
and once you are done you just move all instructions together from the temporary basic block to the one that you were planning to emit. This would mean that you just update the successor of last_ins of tmp_bblock
to point to bblock->code
and vice versa, and then set bblock->code
to tmp_bblock->code
. Hopefully everything would work just like that.
Motivation
Collect sample stack traces in the single-threaded browser.
Single threaded environment can't have dedicated thread for "stop-the-world + collect stack traces" implementation.
So we need to collect samples by instrumenting the code instead.
Previously we implemented browser based profiler which collects "sample" on every method entry/leave. In realistic application, that generates too much traffic for the browser profiler.
This enhancement could also be used to implement sampling profiler with ST diagnostic server.
Goal
samplepoint
instrumentation into Mono profiler, use the existing enter/leave too.method_enter
event/instrumentation.Contributes to #76316
Non-goal: the browser profiler is not compatible with MT.
Changes
Interop
MINT_PROF_SAMPLEPOINT
method_samplepoint
INTERP_PROFILER_RAISE
Jiterp
append_profiler_event
calling newmono_jiterp_prof_enter
,mono_jiterp_prof_samplepoint
,mono_jiterp_prof_leave
AOT
mini_profiler_emit_samplepoint
call $mono_profiler_raise_method_samplepoint
browser profiler
sampleIntervalMs
for browser profiler. 1ms by default. All samples when set to0
.mono_wasm_profiler_now
andmono_wasm_profiler_record
browser.c
newshould_record_frame
profiler_stack_frames
and skip countersample_skip_counter
callspec
parametr to profiler allows to filter the instrumentation, for example by namespacecallspec=N:Sample.FooNs
How to use
Interpreter
dotnet workload install wasm-tools
In the project file
In your
main.js
or in your Blazor app
AOT
You can add following, so that the native code produced by AOT has symbols, which are visible in the browser devtools profiler.
callspec
If you want to filter profiled methods, you can use
callspec
Callspec is documented here https://github.com/dotnet/runtime/blob/main/docs/design/mono/diagnostics-tracing.md#trace-monovm-profiler-events-during-startup
Your for AOT needs to match callspec in the
browserProfilerOptions
in JS