-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[wasm-mt] Disable and fix remaining failing tests on Browser with multi-threading and perf tracing enabled #75286
[wasm-mt] Disable and fix remaining failing tests on Browser with multi-threading and perf tracing enabled #75286
Conversation
Tagging subscribers to 'arch-wasm': @lewing Issue DetailsThis PR
|
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
Tests that previously passed are now failing:
This PR will need some additional work to fix all the remaining failing tests in the multi-threaded lane. I wonder why it started failing now and how it's connected to the disabled tests. |
Sample stack trace of some failing
|
…-ep-fix-remaining-failing-tests
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-wasm |
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
This reverts commit 1e5c88e.
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
@kg is it reasonable to use 16 web workers for CI? Are we testing something that will never happen in real code? |
16 is definitely too much. In practice I don't think you'll see more than 8 real threads available on regular user machines anytime soon, and you could end up being limited to way less based on core count. (I don't know how the browser decides on the limit). It makes sense to run some tests with high and low counts to test those scenarios though. |
@simonrozsival It would be good to understand what the maximum required degree of parallelism is for those dataflow tests - ie: do they really need to run at least 8 threads in parallel, or is the threadpool just spinning up as many threads as it can because there are a lot of async tasks. If the dataflow tests really have a high degree of required parallelism, we should make an issue to make simplified tests for threaded wasm. If it's the threadpool, we should make an issue to make it understand that thread creation on wasm can fail sometimes and try to recover. I don't think we should throw as many workers as we can at a test until it is passing, if we're past what a regular desktop browser would support. |
@lambdageek OK, I'll look deeper into the codebase |
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
@lambdageek The problem seems to be |
@simonrozsival Let's go with 8 and create an issue to look at these tests again. |
@lambdageek when that particular test is disabled, it sometimes succeeds, and sometimes it gets stuck. I tried finding a subset of BTW In the last test run I noticed that the |
Ok, fair enough. Let's go with 8 workers for now.
I think the tests got rewritten using the new template which uses the wasm app host which I think knows when to add the additional headers on its own |
…ing-failing-tests
This got dropped in 4486805 as it moved to building a custom RunScriptCommand, but missed referencing |
When we feel confident about these, the tests can be enabled outside |
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
The |
This PR
browser-eventpipe
sample to make it CI friendly