[browser] threads & js cleanup by pavelsavara · Pull Request #86278 · dotnet/runtime

pavelsavara · 2023-05-15T20:41:18Z

This is just cleanup. There are no intended functional changes.

TimerQueue

renamed SetTimeout to MainThreadScheduleTimer
renamed ReplaceNextSetTimeout to ReplaceNextTimer
renamed TimeoutCallback to TimerHandler
renamed mono_wasm_set_timeout to mono_wasm_main_thread_schedule_timer
renamed mono_set_timeout_exec to mono_wasm_execute_timer
use [UnmanagedCallersOnly] instead of mono_runtime_try_invoke

ThreadPool

renamed QueueCallback to MainThreadScheduleBackgroundJob using mono_main_thread_schedule_background_job
renamed Callback to BackgroundJobHandler
use [UnmanagedCallersOnly] instead of mono_runtime_try_invoke

JSSynchronizationContext

dropped unused RequestPumpCallback
renamed ScheduleBackgroundJob to MainThreadScheduleBackgroundJob using mono_main_thread_schedule_background_job

Jobs

split mono_threads_schedule_background_job into mono_current_thread_schedule_background_job and mono_main_thread_schedule_background_job
made background job queue TLS

MT Debugger

I added stricter assert about usage of legacy interop with MT and could make debugger tests fail more explicitly on it because they still use bind_static_method.

Other

fixed assert for install_sync_context
fixed browser-threads-minimal to pass on Windows
added mono_threads_wasm_async_run_in_target_thread
added mono_bound_thread_schedule_background_job in preparation for next step
more logging in the sample
cleanup in legacy interop, more asserts about threads
moved conv_string to different file
removed unused _get_class_name, _get_type_name, _get_type_aqn methods
added named wrapper Function into mono_wasm_bind_cs_function and mono_wasm_bind_js_function only under Debug build of the runtime. It makes debugging easier because names the exported/imported function are visible on the stack trace.
fixed how config is applied in the worker via normalizeConfig after it arrived by message.

ghost · 2023-05-15T20:41:28Z

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-System.Runtime.InteropServices.JavaScript`
Milestone:	8.0.0

pavelsavara · 2023-05-15T20:41:51Z

/azp run runtime-wasm

azure-pipelines · 2023-05-15T20:42:00Z

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara · 2023-05-18T22:24:11Z

/azp run runtime-wasm

azure-pipelines · 2023-05-18T22:24:26Z

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara · 2023-05-18T22:42:14Z

/azp run runtime-wasm

azure-pipelines · 2023-05-18T22:42:22Z

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara · 2023-05-23T13:07:45Z

/azp run runtime-wasm

azure-pipelines · 2023-05-23T13:07:55Z

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara · 2023-05-23T15:50:33Z

AOT issues are #86621

pavelsavara · 2023-05-23T16:21:14Z

/azp run runtime-wasm

azure-pipelines · 2023-05-23T16:21:24Z

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara · 2023-05-23T17:07:16Z

/azp run runtime-wasm

azure-pipelines · 2023-05-23T17:07:35Z

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara · 2023-05-24T08:22:02Z

CI failures are unrelated

ilonatommy

After discussing offline: LGTM.

ilonatommy

After discussing offline: LGTM.

pavelsavara · 2023-05-24T09:11:47Z

@lambdageek or @kg please have yet another look and approve. I think the only part which needs your attention is change from invoke via mono reflection to invoke via [UnmanagedCallersOnly] with passing fn pointer. I think it would do the right thing about GC regions ?

lambdageek

Mostly LGTM. A couple of nits.

Two things to consider addressing:

The _DataIsAvailable delegate is there to avoid allocating a new delegate object each time
The 25s delay is testing the threadpool worker timeout doesn't kill a worker that has unsettled promise. Changing to 5s changes the test.

lambdageek · 2023-05-24T13:46:48Z

- [DynamicDependency("Callback")]
 [MethodImplAttribute(MethodImplOptions.InternalCall)]
-private static extern void QueueCallback();
+internal static extern unsafe void MainThreadScheduleBackgroundJob(void* callback);


nit: It would be nice to collect all of these internal calls that are called inside S.P.C into a single class

added comments added missing export of _emscripten_main_runtime_thread_id

pavelsavara · 2023-05-24T14:00:01Z

/azp run runtime-wasm

azure-pipelines · 2023-05-24T14:00:14Z

Azure Pipelines successfully started running 1 pipeline(s).

add asserts and exception handlers

pavelsavara · 2023-05-24T17:35:28Z

/azp run runtime-wasm

azure-pipelines · 2023-05-24T17:35:37Z

Azure Pipelines successfully started running 1 pipeline(s).

kg · 2023-05-24T17:43:09Z

@lambdageek or @kg please have yet another look and approve. I think the only part which needs your attention is change from invoke via mono reflection to invoke via [UnmanagedCallersOnly] with passing fn pointer. I think it would do the right thing about GC regions ?

I'm not sure that invoking via UnmanagedCallersOnly will transition from one GC mode to another correctly. @lambdageek do you know?

lambdageek · 2023-05-24T18:06:15Z

I'm not sure that invoking via UnmanagedCallersOnly will transition from one GC mode to another correctly. @lambdageek do you know?

It will work correctly (unless you use CallConvSuppressGCTransition)

# Conflicts: # src/mono/wasm/wasm.proj

pavelsavara added arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript labels May 15, 2023

pavelsavara added this to the 8.0.0 milestone May 15, 2023

pavelsavara self-assigned this May 15, 2023

build-analysisBot mentioned this pull request May 16, 2023
System.Text.Json failing some large file tests #59678
Closed

pavelsavara force-pushed the browser_threads branch from 126ce7b to 85c2276Compare May 18, 2023 22:19

threading cleanup
43f6941

pavelsavara force-pushed the browser_threads branch from eaebbbc to 43f6941Compare May 23, 2023 12:56

Merge branch 'main' into browser_threads
8fd22a4

pavelsavara changed the title ~~[draft][browser] threads experiment~~[browser] threads cleanupMay 23, 2023

pavelsavara marked this pull request as ready for review May 23, 2023 16:24

pavelsavara added 2 commits May 23, 2023 18:59

feedback
2c61c78

feedback
2eefd11

This was referenced May 23, 2023
Roslyn source generator crash on mono/linux/arm64 #81123
Closed
IOException running NuGet-Migrations during tests in dotnet CLI first run #80619
Closed
AppHost.Bundle.Tests.BundleLocalizedApp.Bundled_Localized_App_Run_Succeeds failing #85993
Closed

ilonatommy approved these changes May 24, 2023
View reviewed changes

small tweaks
a4e6961

pavelsavara changed the title ~~[browser] threads cleanup~~[browser] threads & js cleanupMay 24, 2023

lambdageek approved these changes May 24, 2023
View reviewed changes

optimized pointers to managed handlers
dcb858a
added comments added missing export of _emscripten_main_runtime_thread_id

lambdageek reviewed May 24, 2023
View reviewed changes

Comment thread...ervices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSSynchronizationContext.cs Outdated

pavelsavara added 2 commits May 24, 2023 17:29

feedback
68b57a2

fix bug in fetchhelper sample
14abe34
add asserts and exception handlers

Merge branch 'main' into browser_threads
b5b712e
# Conflicts: # src/mono/wasm/wasm.proj

pavelsavara merged commit df9ad0b into dotnet:mainMay 25, 2023

IsaMorphic mentioned this pull request May 30, 2023
[browser][mt] Running any Thread for longer than 5000 ms crashes runtime (Blazor) #86903
Closed

pavelsavara mentioned this pull request Jun 1, 2023
[browser] fix legacy interop tests #87001
Merged

ghost locked as resolved and limited conversation to collaborators Jun 24, 2023

pavelsavara deleted the browser_threads branch September 2, 2024 15:30

Uh oh!

Conversation

pavelsavara commented May 15, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TimerQueue

ThreadPool

JSSynchronizationContext

Jobs

MT Debugger

Other

Uh oh!

ghost commented May 15, 2023

Uh oh!

pavelsavara commented May 15, 2023

Uh oh!

azure-pipelinesBot commented May 15, 2023

Uh oh!

pavelsavara commented May 18, 2023

Uh oh!

azure-pipelinesBot commented May 18, 2023

Uh oh!

pavelsavara commented May 18, 2023

Uh oh!

azure-pipelinesBot commented May 18, 2023

Uh oh!

pavelsavara commented May 23, 2023

Uh oh!

azure-pipelinesBot commented May 23, 2023

Uh oh!

pavelsavara commented May 23, 2023

Uh oh!

pavelsavara commented May 23, 2023

Uh oh!

azure-pipelinesBot commented May 23, 2023

Uh oh!

pavelsavara commented May 23, 2023

Uh oh!

azure-pipelinesBot commented May 23, 2023

Uh oh!

pavelsavara commented May 24, 2023

Uh oh!

ilonatommy left a comment

Choose a reason for hiding this comment

Uh oh!

ilonatommy left a comment

Choose a reason for hiding this comment

Uh oh!

pavelsavara commented May 24, 2023

Uh oh!

lambdageek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lambdageekMay 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pavelsavara commented May 24, 2023

Uh oh!

azure-pipelinesBot commented May 24, 2023

Uh oh!

Uh oh!

pavelsavara commented May 24, 2023

Uh oh!

azure-pipelinesBot commented May 24, 2023

Uh oh!

kg commented May 24, 2023

Uh oh!

lambdageek commented May 24, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

pavelsavara commented May 15, 2023•
edited
Loading

lambdageek commented May 24, 2023•
edited
Loading