Skip to content

[Web.JS] call mono_wasm_gc_lock in MonoHeapLock #46909

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
Merged
merged 3 commits into from
Mar 27, 2023
Merged

[Web.JS] call mono_wasm_gc_lock in MonoHeapLock #46909

merged 3 commits into from
Mar 27, 2023

Conversation

lambdageek
Copy link
Member

Take the runtime's GC lock when creating a heap lock.

Important Notes

  1. While the runtime GC lock is locked, calling any C# function or doing anything that may trigger managed allocation will block the runtime.
  2. In a threaded build, only the main browser thread is allowed to call the mono_wasm_gc_lock function
  3. The runtime lock is not recursive
  4. Background threads initially continue running, so the managed object graph may change even while the GC lock is held (although objects may not move). Background threads could mutate valuetype fields of reachable objects, even while the GC lock is held. However background threads will block as soon as they try to do any allocation.
  5. The main browser thread may assume that managed heap objects will not move while the lock is held.

Corresponding runtime change dotnet/runtime#82646

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Feb 27, 2023
@lambdageek lambdageek mentioned this pull request Feb 27, 2023
Merged
@pavelsavara
Copy link
Member

Mutating UI components from worker thread is interesting scenario which we should address I think. Otherwise LGTM

lambdageek added a commit to dotnet/runtime that referenced this pull request Mar 1, 2023
@lambdageek
[wasm] Add an internal GC lock API (#82646)
5b89020 
The goal is to provide an API that can be called from JS to block a background GC from moving objects that may be accessed unsafely from JS.  Blazor does this during rendering - it traverses a set of rendering objects from JS in order to apply changes to the DOM.

Important notes for consumers of this API:
1. While the runtime GC lock is locked, calling any C# function or doing anything that may trigger managed allocation will block the runtime.
2. In a threaded build, only the main browser thread is allowed to call the `mono_wasm_gc_lock` function
3. The runtime lock is not recursive
4. Background threads initially continue running, so the managed object graph may change even while the GC lock is held (although objects may not move).  Background threads could mutate valuetype fields of reachable objects, even while the GC lock is held. However background threads will block as soon as they try to do any allocation. 
5. The main browser thread may assume that managed heap objects will not move while the lock is held.

The corresponding aspnetcore API is dotnet/aspnetcore#46909

* add wasm entrypoints to lock the GC from JS
* move gc lock api to INTERNAL object
* enter GC Unsafe before manipulating the locks

   This is an exported API, so we're normally in GC Safe mode on entry
@lambdageek lambdageek marked this pull request as ready for review March 1, 2023 16:04
@lambdageek lambdageek requested a review from a team as a code owner March 1, 2023 16:04
@lambdageek
Copy link
Member Author

Runtime change has been merged.

This PR doesn't depend on the runtime bits (and in fact, won't do anything different until threading is enabled) - if it's paired with a runtime that doesn't have the API, it behaves as before.

This is ready for review now.

@SteveSandersonMS
Copy link
Member

Background threads initially continue running, so the managed object graph may change even while the GC lock is held (although objects may not move). Background threads could mutate valuetype fields of reachable objects, even while the GC lock is held. However background threads will block as soon as they try to do any allocation.

That's a good point - thanks for raising it. I don't think this can lead to any problems based on how rendering is implemented currently, but we are going to have to make sure we don't later add any logic to SharedMemoryRenderBatch.ts that accesses any memory that could mutate due to code isn't on the renderer sync context.

@SteveSandersonMS
Copy link
Member

Mutating UI components from worker thread is interesting scenario which we should address I think

As long as Blazor WebAssembly implements support for a renderer sync context (equivalent to the one for Blazor Server / WebView), then this should be covered automatically. Components would have to dispatch to the sync context to update their render output, but the sync context would be locked while an existing renderbatch is in process of being applied to the DOM.

SteveSandersonMS
SteveSandersonMS reviewed Mar 6, 2023
View reviewed changes
@lambdageek lambdageek added blocked The work on this issue is blocked due to some dependency and removed blocked The work on this issue is blocked due to some dependency labels Mar 6, 2023
@lambdageek lambdageek added the blocked The work on this issue is blocked due to some dependency label Mar 7, 2023
lambdageek
lambdageek commented Mar 7, 2023
View reviewed changes
@lambdageek
call mono_wasm_gc_lock in MonoHeapLock
7676282
@lambdageek
Depend on the new runtime APIs
81d6e5f 
blazorwasm and the runtime must be in lockstep. There's no supported
scenario where the API might be missing
@lambdageek lambdageek removed the blocked The work on this issue is blocked due to some dependency label Mar 7, 2023
@ghost
Copy link

ghost commented Mar 15, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 15, 2023
@lambdageek
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@lambdageek lambdageek removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 16, 2023
@ghost
Copy link

ghost commented Mar 24, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 24, 2023
@lambdageek
Copy link
Member Author

/azp run

@lambdageek lambdageek removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 24, 2023
@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@lambdageek lambdageek merged commit 62b87d7 into dotnet:main Mar 27, 2023
@ghost ghost added this to the 8.0-preview4 milestone Mar 27, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pavelsavara pavelsavara pavelsavara left review comments

@javiercn javiercn javiercn approved these changes

@SteveSandersonMS SteveSandersonMS Awaiting requested review from SteveSandersonMS

Assignees
No one assigned
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Milestone
8.0-preview4
Development

Successfully merging this pull request may close these issues.
4 participants
@lambdageek @pavelsavara @SteveSandersonMS @javiercn