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][MT] unify pthread pool size to 40 #94204

Merged
merged 14 commits into from
Nov 2, 2023

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Oct 31, 2023

  • bump default thread pool size to 40 and unify to that default for all tests
  • limit managed thread pool size to 10
  • see which existing active issues are caused by the same reason
  • keep ActiveIssue tracking it, because we know that it's too large default

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

ghost commented Oct 31, 2023

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

Issue Details
Author: pavelsavara
Assignees: pavelsavara, ilonatommy
Labels:

arch-wasm, area-VM-threading-mono, os-browser

Milestone: 9.0.0

@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

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara marked this pull request as ready for review November 1, 2023 16:45
@kg
Copy link
Contributor

kg commented Nov 2, 2023

Why 40 if the managed limit is 10? 40 is a lot

@pavelsavara
Copy link
Member Author

Why 40 if the managed limit is 10? 40 is a lot

  • Because there are tests which also allocate non-thread-pool managed threads.
  • Possibly something is still leaking threads.
  • I know that managed portable thread pool would release pthread only after some delay.
  • I also know that mono will only release attached pthread on next attempt to allocate another.
    But there could be browser loop run that emscripten need to have before it could re-use the released pthread.

But I don't claim this is a solution, this is a way how to see more unit tests running.

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

MT lib tests pass with this change
image

MT debugger is silenced
image

All other CI issues are known issues.

@pavelsavara pavelsavara merged commit c5da1b1 into dotnet:main Nov 2, 2023
192 of 199 checks passed
@pavelsavara pavelsavara deleted the browser_mt_pool_size2 branch November 2, 2023 16:41
@ghost ghost locked as resolved and limited conversation to collaborators Dec 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-VM-threading-mono os-browser Browser variant of arch-wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants