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.

Sign up for GitHub

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

Jump to bottom

[wasm] Enable the log profiler #107434

Merged
merged 24 commits into from
Sep 12, 2024
Merged

[wasm] Enable the log profiler #107434

merged 24 commits into from
Sep 12, 2024

Conversation

jeromelaban
Copy link
Contributor

@jeromelaban jeromelaban commented Sep 5, 2024 •
edited
Loading

This change adds the support for the log profiler discussed in #107312 for webassembly.

The inclusion follows the support for the browser and aot profilers.

Closes #107312

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 5, 2024
@jeromelaban jeromelaban force-pushed the dev/jela/log-profiler branch from 55c005e to c10694a Compare September 6, 2024 01:25
@jeromelaban jeromelaban mentioned this pull request Sep 6, 2024
Closed
26 tasks
@jeromelaban jeromelaban marked this pull request as ready for review September 6, 2024 11:30
@pavelsavara pavelsavara added arch-wasm WebAssembly architecture os-browser Browser variant of arch-wasm area-Diagnostics-mono and removed area-Build-mono labels Sep 6, 2024
pavelsavara
pavelsavara reviewed Sep 6, 2024
View reviewed changes
Copy link
Member

@pavelsavara pavelsavara left a comment

Choose a reason for hiding this comment

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

@jeromelaban what's the size of libmono-profiler-log.a ?

src/mono/browser/runtime/profiler.ts Outdated Show resolved Hide resolved
src/mono/browser/runtime/startup.ts Outdated Show resolved Hide resolved
src/mono/mono/profiler/CMakeLists.txt Outdated Show resolved Hide resolved
@jeromelaban
Copy link
Contributor Author

@pavelsavara interestingly, the dotnet.native.wasm seems to be smaller by 20 bytes with the log profiler included.

pavelsavara
pavelsavara previously approved these changes Sep 6, 2024
View reviewed changes
@pavelsavara
Copy link
Member

adding <WasmProfilers>log;</WasmProfilers> WBT would be great.
Probably to src\mono\wasm\Wasm.Build.Tests\WasmNativeDefaultsTests.cs

@pavelsavara pavelsavara added this to the 10.0.0 milestone Sep 11, 2024
This was referenced Sep 11, 2024
Open
Open
Open
Open
Open
Open
Closed
@ilonatommy
Treat a list of loggers as one argument.
ab0b6e4 
Avoid "MSBUILD : error MSB1005: Specify a property and its value."
@build-analysis build-analysis bot mentioned this pull request Sep 11, 2024
Open
3 tasks
@pavelsavara
Copy link
Member

From Jerome
image

@build-analysis build-analysis bot mentioned this pull request Sep 12, 2024
Open
@pavelsavara pavelsavara mentioned this pull request Sep 12, 2024
Merged
4 tasks
@lewing lewing mentioned this pull request Sep 12, 2024
Open
@lewing lewing merged commit 34ba8c7 into dotnet:main Sep 12, 2024
107 of 109 checks passed
@build-analysis build-analysis bot mentioned this pull request Sep 12, 2024
Open
@pavelsavara
Copy link
Member

pavelsavara commented Sep 13, 2024 •
edited
Loading

There is about 1kb size growth in the range for the default build.

But I can't tell if this is the .wasm file or DLLs. There are other changes in the diff.

image

@radekdoulik could you please advise ?

jtschuster pushed a commit to jtschuster/runtime that referenced this pull request Sep 17, 2024
@jeromelaban @pavelsavara
@ilonatommy @jtschuster
[wasm] Enable the log profiler (dotnet#107434)
bd6af7a 
* feat(wasm): Enable the log profiler

* chore: Disable for wasi

* chore: Add log profiler docs

* chore: Adjust flush logs syntax

* chore: Adjust defines, add conditionals, remove Module dependency

* chore: Adjust buffer_unlock exclusion

* chore: Adjust doc

* chore: Add log profiler sample

* chore: Add more sample logging

* chore: Remove unused makefile target

* chore: Remove icall, use jit interception to take heap sot

* chore: Remove unused logs

* chore: Remove unused dependency

* Revert "chore: Remove unused dependency"

This reverts commit 33221c4.

* remove ENABLE_BROWSER_PROFILER from default build

* Draft of WBT.

* Miss-commit, there's no profiler.js in the sample.

* Shift the responsibility of checking profile's size to the browser.

* Test linking of all 3 types of loggers + running one of them.

* link all 3 at the same time

* fix     [Fact]

* Treat a list of loggers as one argument.

Avoid "MSBUILD : error MSB1005: Specify a property and its value."

* fix?

---------

Co-authored-by: pavelsavara <pavel.savara@gmail.com>
Co-authored-by: Ilona Tomkowicz <itomkowicz@microsoft.com>
Co-authored-by: Ilona Tomkowicz <32700855+ilonatommy@users.noreply.github.com>
Co-authored-by: Pavel Savara <pavelsavara@microsoft.com>
sirntar pushed a commit to sirntar/runtime that referenced this pull request Sep 30, 2024
@jeromelaban @pavelsavara
@ilonatommy @sirntar
[wasm] Enable the log profiler (dotnet#107434)
e9230b8 
* feat(wasm): Enable the log profiler

* chore: Disable for wasi

* chore: Add log profiler docs

* chore: Adjust flush logs syntax

* chore: Adjust defines, add conditionals, remove Module dependency

* chore: Adjust buffer_unlock exclusion

* chore: Adjust doc

* chore: Add log profiler sample

* chore: Add more sample logging

* chore: Remove unused makefile target

* chore: Remove icall, use jit interception to take heap sot

* chore: Remove unused logs

* chore: Remove unused dependency

* Revert "chore: Remove unused dependency"

This reverts commit 33221c4.

* remove ENABLE_BROWSER_PROFILER from default build

* Draft of WBT.

* Miss-commit, there's no profiler.js in the sample.

* Shift the responsibility of checking profile's size to the browser.

* Test linking of all 3 types of loggers + running one of them.

* link all 3 at the same time

* fix     [Fact]

* Treat a list of loggers as one argument.

Avoid "MSBUILD : error MSB1005: Specify a property and its value."

* fix?

---------

Co-authored-by: pavelsavara <pavel.savara@gmail.com>
Co-authored-by: Ilona Tomkowicz <itomkowicz@microsoft.com>
Co-authored-by: Ilona Tomkowicz <32700855+ilonatommy@users.noreply.github.com>
Co-authored-by: Pavel Savara <pavelsavara@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Oct 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ilonatommy ilonatommy ilonatommy left review comments

@pavelsavara pavelsavara pavelsavara approved these changes

@lewing lewing Awaiting requested review from lewing lewing is a code owner

@BrzVlad BrzVlad Awaiting requested review from BrzVlad BrzVlad is a code owner

@lambdageek lambdageek Awaiting requested review from lambdageek

@steveisok steveisok Awaiting requested review from steveisok steveisok is a code owner

@marek-safar marek-safar Awaiting requested review from marek-safar

@maraf maraf Awaiting requested review from maraf maraf is a code owner

@akoeplinger akoeplinger Awaiting requested review from akoeplinger akoeplinger is a code owner

Assignees

@pavelsavara pavelsavara

@ilonatommy ilonatommy

Labels
arch-wasm WebAssembly architecture area-Diagnostics-mono community-contribution Indicates that the PR has been added by a community member os-browser Browser variant of arch-wasm
Projects
None yet
Milestone
10.0.0
Development

Successfully merging this pull request may close these issues.

[Wasm] Support for the mono log profiler for memory profiling
5 participants
@jeromelaban @pavelsavara @steveisok @ilonatommy @lewing