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

[browser][MT] fix library tests, remove MonoWasmBuildVariant=perftrace #87549

Merged
merged 4 commits into from
Jun 20, 2023
Merged

[browser][MT] fix library tests, remove MonoWasmBuildVariant=perftrace #87549

merged 4 commits into from
Jun 20, 2023

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Jun 14, 2023 •
edited
Loading

Multi-threading tests:

  • fix the msbuild scripts to detect MT build when building library tests and set
  • disable V8 and NodeJS for threading tests
  • removed perftrace runtime flavor
  • removed MonoWasmBuildVariant==perftrace
  • made IsThreadingSupported respect build flavor via IsBrowserThreadingSupported and --use-threads
  • removed most of event-pipe wasm sample
  • removed DISABLE_WASM_USER_THREADS and MonoWasmThreadsNoUser
  • fixed mono_wasm_execute_timer for WS without Main
  • added COOP into SimpleServer
  • fixed problem with SharedArrayBuffer in WS

This PR is not fixing tests which are actually broken, they are on optional CI pipeline, which doesn't fail the build.

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-VM-threading-mono labels Jun 14, 2023
@pavelsavara pavelsavara added this to the 8.0.0 milestone Jun 14, 2023
@pavelsavara pavelsavara self-assigned this Jun 14, 2023
@ghost
Copy link

ghost commented Jun 14, 2023

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

Issue Details

null

Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-VM-threading-mono
Milestone: 8.0.0

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm-libtests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

At least some lib tests are passing on MT, for example
Log
Log

V8 doesnn't have threads and EventTarget class, we should run all tests on the browser.
NodeJS is failing with #87556 and we should disable it for now.

This was referenced Jun 14, 2023
Closed
Closed
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@pavelsavara pavelsavara marked this pull request as ready for review June 19, 2023 11:01
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara requested a review from lewing June 19, 2023 11:01
@pavelsavara
- rebased
7028044 
- removed perftrace runtime flavor
- removed MonoWasmBuildVariant==perftrace
- made IsThreadingSupported respect build flavor via IsBrowserThreadingSupported and --use-threads
- removed most of event-pipe wasm sample
- removed DISABLE_WASM_USER_THREADS and MonoWasmThreadsNoUser
- fixed mono_wasm_execute_timer for WS without Main
- added COOP into SimpleServer
- fixed problem with SharedArrayBuffer in WS
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara changed the title [browser][MT] fix library tests [browser][MT] fix library tests, remove MonoWasmBuildVariant=perftrace Jun 19, 2023
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@SamMonoRT
Copy link
Member

@pavelsavara @radical - do we need any corresponding changes in any performance pipelines/lanes ?

This was referenced Jun 20, 2023
Closed
Closed
maraf
maraf reviewed Jun 20, 2023
View reviewed changes
src/mono/sample/wasm/browser-eventpipe/README.md Outdated Show resolved Hide resolved
src/mono/wasm/test-main.js Outdated Show resolved Hide resolved
@pavelsavara
Copy link
Member Author

@pavelsavara @radical - do we need any corresponding changes in any performance pipelines/lanes ?

@SamMonoRT , this PR is removing perftrace build flavor, which is very similar to /p:MonoWasmBuildVariant=multithread.It doesn't have user threads only dedicated diagnostic thread.

I think we don't have any performance pipelines/lanes for multi-threaded build flavor yet, right ?
We will eventually need such lane for /p:MonoWasmBuildVariant=multithread, but right now, we don't pass enough unit tests yet on this flavor.

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

pavelsavara commented Jun 20, 2023 •
edited
Loading

Here are MT test failures this uncovered

Timeout probably because of the sample setup.
Wasm.Browser.EventPipe.Sample

Maybe we need bigger thread pool or something is eating/leaking the threads.
ThreadStartException: Thread failed to start.
System.Threading.Tasks.Tests
System.Transactions.Local.Tests
System.Threading.Tests

System.Threading.Tests.TimerFiringTests.ManyTimers_EachTimerDoesFire [10:42:43] info: Not all timers fired, 10000 left of 10000
When main thread blocks with .Wait timers and GC doesn't work.
System.Threading.Timer.Tests

@pavelsavara pavelsavara merged commit 1784251 into dotnet:main Jun 20, 2023
@lambdageek lambdageek mentioned this pull request Jun 20, 2023
Merged
pavelsavara added a commit to pavelsavara/runtime that referenced this pull request Jun 21, 2023
@pavelsavara
fix bug introduced in dotnet#87549
18ccbe2
@pavelsavara pavelsavara mentioned this pull request Jun 21, 2023
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Jul 20, 2023
@pavelsavara pavelsavara deleted the browser_mt_tests branch September 2, 2024 15:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@maraf maraf maraf approved these changes

@lambdageek lambdageek lambdageek approved these changes

@kg kg kg approved these changes

@radical radical Awaiting requested review from radical

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

@thaystg thaystg Awaiting requested review from thaystg

@vargaz vargaz Awaiting requested review from vargaz

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

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

Assignees

@pavelsavara pavelsavara

Labels
arch-wasm WebAssembly architecture area-VM-threading-mono
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@pavelsavara @SamMonoRT @radical @kg @lambdageek @maraf