Skip to content
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] profiler setup and env variables #113339

Merged
merged 11 commits into from
Mar 12, 2025

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Mar 10, 2025

  • set DOTNET_DiagnosticPorts by DiagnosticPorts MSBuild property, same as other mono platforms
  • new WasmEnvironmentVariable item list
  • new WasmAppBuilder.EnvVariables
  • set FeaturePerfTracing when WasmPerfTracing
  • fixed enablePerfTracing || enableBrowserProfiler for profiler events cwraps
  • simplified mono profilers setup
  • split LogProfilerTest and BrowserProfilerTest

Contributes to #76316

- new WasmEnvironmentVariable item list
- new WasmAppBuilder.EnvVariables
- fixed enablePerfTracing || enableBrowserProfiler for profiler events
- simplified mono profiler setup
- split LogProfilerTest and BrowserProfilerTest
@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-Diagnostics-mono os-browser Browser variant of arch-wasm labels Mar 10, 2025
@pavelsavara pavelsavara added this to the 10.0.0 milestone Mar 10, 2025
@pavelsavara pavelsavara self-assigned this Mar 10, 2025
Copy link
Contributor

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

@pavelsavara pavelsavara marked this pull request as ready for review March 11, 2025 09:16
@Copilot Copilot bot review requested due to automatic review settings March 11, 2025 09:16

Choose a reason for hiding this comment

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

PR Overview

This PR enhances the WebAssembly profiler setup and environment variable configuration for .NET by refining profiler initialization, updating test assets, and modifying runtime and build tasks to better support browser and log profilers.

  • Introduces a new EnvVariables item in WasmAppBuilder and updates BootJsonData to use a Dictionary for environment variables.
  • Splits and renames profiler tests to clearly separate LogProfiler and BrowserProfiler scenarios.
  • Adjusts runtime initialization and cwraps configuration to support new profiler setup logic.

Reviewed Changes

File Description
src/mono/wasm/testassets/WasmBasicTestApp/App/BrowserProfilerTest.cs Adds a new browser profiler test.
src/mono/wasm/Wasm.Build.Tests/DiagnosticsTests.cs Renames and updates tests for log and browser profiler support.
src/mono/wasm/testassets/WasmBasicTestApp/App/wwwroot/main.js Updates test case names and profiler configuration strings.
src/tasks/WasmAppBuilder/WasmAppBuilder.cs Adds EnvVariables property and integrates it into boot.json generation.
src/mono/browser/runtime/profiler.ts Refactors profiler initialization logic for log profiler.
src/mono/browser/runtime/startup.ts Updates environment variable and profiler initialization logic.
src/mono/browser/runtime/cwraps.ts Adjusts condition for including profiler cwraps based on profiler configuration.
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/BootJsonData.cs Changes environmentVariables type from object to Dictionary for clarity.
src/mono/browser/runtime/es6/dotnet.es6.lib.js Cleans up profiler and runtime configuration properties.
src/mono/wasm/testassets/WasmBasicTestApp/App/LogProfilerTest.cs Renames test class to align with log profiler testing.
src/mono/browser/runtime/jiterpreter.ts Updates condition to include browser profiler support in jiterpreter profiling.

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/mono/browser/runtime/profiler.ts:46

  • Since the assertion guarantees that options.takeHeapshot is defined, the subsequent 'if (options.takeHeapshot)' check and its else branch may be redundant. Consider refactoring this conditional to streamline the logic.
mono_assert(options.takeHeapshot, "Log profiler is not enabled, the takeHeapshot method must be defined in LogProfilerOptions.takeHeapshot");
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Changes look good to me, nothing strikes me as incorrect, although I don't have a lot of context.

My understanding is that this is enabled through the build, and then flows down to the runtime, with the most relevant change being the ability to add environment variables through the build on demand (which was not possible before).

Am I missing something?

@pavelsavara
Copy link
Member Author

(which was not possible before).

It was/is possible on runtime, there is JS and managed API for it.
It was also possible to hack it via specially crafted value of WasmExtraConfig msbuild items. But it's very clumsy.

pavelsavara and others added 6 commits March 11, 2025 12:28
- remove WASM_PERFTRACING from rollup
- remove mono_wasm_diagnostic_imports and include it always
@pavelsavara pavelsavara merged commit 2b394ae into dotnet:main Mar 12, 2025
31 of 33 checks passed
@pavelsavara pavelsavara deleted the browser_env branch March 12, 2025 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm WebAssembly architecture area-Diagnostics-mono os-browser Browser variant of arch-wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants