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] more testing and cleanup #94466

Merged
merged 9 commits into from
Nov 9, 2023
Merged

[browser][MT] more testing and cleanup #94466

merged 9 commits into from
Nov 9, 2023

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Nov 7, 2023 •
edited
Loading

  • use mono_coop_sem_post for MT GC

Other

  • rename mono_threads_wasm_is_browser_thread to mono_threads_wasm_is_ui_thread
  • rename mono_threads_wasm_browser_thread_tid to mono_threads_wasm_ui_thread_tid
  • add mono_threads_wasm_async_run_in_target_thread_vii
  • rename diagnostics_cwraps to threading_cwraps
  • enable more MT tests

Fixes #72101
Fixes #91593

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-VM-threading-mono os-browser Browser variant of arch-wasm labels Nov 7, 2023
@pavelsavara pavelsavara added this to the 9.0.0 milestone Nov 7, 2023
@pavelsavara pavelsavara self-assigned this Nov 7, 2023
@ghost
Copy link

ghost commented Nov 7, 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, os-browser
Milestone: 9.0.0

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Nov 7, 2023
Open
Open
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara changed the title [browser][MT] more testing and clenup [browser][MT] more testing and cleanup Nov 7, 2023
@build-analysis build-analysis bot mentioned this pull request Nov 7, 2023
Closed
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara mentioned this pull request Nov 8, 2023
Closed
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara marked this pull request as ready for review November 8, 2023 17:09
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

kg
kg reviewed Nov 8, 2023
View reviewed changes
...Microsoft.Extensions.Logging.Console.Tests/Microsoft.Extensions.Logging.Console.Tests.csproj
@@ -5,10 +5,11 @@
<TestRuntime>true</TestRuntime>
<EnableDefaultItems>true</EnableDefaultItems>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<_WasmPThreadPoolSize Condition="'$(MonoWasmBuildVariant)' == 'multithread'">100</_WasmPThreadPoolSize>
Copy link
Member

Choose a reason for hiding this comment

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

😱

Copy link
Member Author

Choose a reason for hiding this comment

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

the test is allocating lot of threads. I suspect that we are still not freeing them fast enough.
If we adopt deputy design, we could have dynamic thread allocation.
It's obvious this is not sustainable. On the other hand, this is just a test, not production code.

@kg
Copy link
Member

kg commented Nov 8, 2023

Please add comments for each explicit threadpool size:

  • Why this big?
  • Can we shrink it later by fixing something?

@build-analysis build-analysis bot mentioned this pull request Nov 8, 2023
Closed
3 tasks
@pavelsavara
Copy link
Member Author

Second green pass in a row.
image

There are know failures #89409
And also unrelated osx failures.

@build-analysis build-analysis bot mentioned this pull request Nov 8, 2023
Closed
@pavelsavara pavelsavara merged commit 28dcc44 into dotnet:main Nov 9, 2023
142 of 156 checks passed
@pavelsavara pavelsavara deleted the browser_mt_baseline branch November 9, 2023 07:32
@pavelsavara pavelsavara mentioned this pull request Nov 9, 2023
Closed
31 tasks
@radical radical mentioned this pull request Nov 10, 2023
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kg kg kg approved these changes

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

@ilonatommy ilonatommy Awaiting requested review from ilonatommy

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

@vargaz vargaz Awaiting requested review from vargaz

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

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

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

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

@marek-safar marek-safar Awaiting requested review from marek-safar marek-safar is a code owner

Assignees

@pavelsavara pavelsavara

Labels
arch-wasm WebAssembly architecture area-VM-threading-mono os-browser Browser variant of arch-wasm
Projects
None yet
Milestone
9.0.0
2 participants
@pavelsavara @kg