[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.

...ervices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSSynchronizationContext.cs

src/mono/System.Private.CoreLib/src/System/Threading/ThreadPool.Browser.Mono.cs

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

src/mono/System.Private.CoreLib/src/System/Threading/ThreadPool.Browser.Mono.cs

-        [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

src/mono/System.Private.CoreLib/src/System/Threading/TimerQueue.Browser.Mono.cs

src/mono/sample/wasm/browser-threads-minimal/fetchhelper.js

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).

...ervices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSSynchronizationContext.cs

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-analysis bot 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 85c2276 Compare May 18, 2023 22:19

threading cleanup

43f6941

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

Merge branch 'main' into browser_threads

8fd22a4

pavelsavara changed the title ~~[draft][browser] threads experiment~~ [browser] threads cleanup May 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 cleanup May 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

...ervices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSSynchronizationContext.cs Outdated Show resolved Hide resolved

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:main May 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

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-pipelines bot commented May 15, 2023

Uh oh!

pavelsavara commented May 18, 2023

Uh oh!

azure-pipelines bot commented May 18, 2023

Uh oh!

pavelsavara commented May 18, 2023

Uh oh!

azure-pipelines bot commented May 18, 2023

Uh oh!

pavelsavara commented May 23, 2023

Uh oh!

azure-pipelines bot commented May 23, 2023

Uh oh!

pavelsavara commented May 23, 2023

Uh oh!

pavelsavara commented May 23, 2023

Uh oh!

azure-pipelines bot commented May 23, 2023

Uh oh!

pavelsavara commented May 23, 2023

Uh oh!

azure-pipelines bot 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!

lambdageek May 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-pipelines bot commented May 24, 2023

Uh oh!

Uh oh!

pavelsavara commented May 24, 2023

Uh oh!

azure-pipelines bot 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

Projects

pavelsavara commented May 15, 2023 •

edited

Loading

lambdageek commented May 24, 2023 •

edited

Loading