Skip to content

[browser] streamline Task/Promise marshalling - #93010

Merged
pavelsavara merged 1 commit into
dotnet:mainfrom
pavelsavara:browser_one_direction_tcs
Oct 16, 2023
Merged

[browser] streamline Task/Promise marshalling#93010
pavelsavara merged 1 commit into
dotnet:mainfrom
pavelsavara:browser_one_direction_tcs

Conversation

@pavelsavara

@pavelsavarapavelsavara commented Oct 4, 2023

Copy link
Copy Markdown
Member

This streamlines passing Promise/Task from two cross-boundary calls per promise argument to one.

  • none when it's passed
  • one when it's resolved, rejected or garbage collected

Changes

  • drop create_task_callback
  • simplify complete_task
  • simplify mono_wasm_marshal_promise and rename it to mono_wasm_resolve_or_reject_promise
  • introduced GCVHandle
    • it's like GCHandle in that it keeps proxy of C# object alive while JS owns it.
    • but it's allocated ahead of time on JS side
  • introduced JSVHandle
    • it's like JSHandle in that it keeps proxy of JS object alive while C# owns it.
    • but it's allocated ahead of time on C# side
  • improved GetTaskResultDynamic to handle void Task
  • introduced internal JSObject.DisposeLocal
    • we can dispose both sides during resolve/reject call without extra dispose call

Interop "stack frame"

  • fixed IntPtr and handles to be I32 rather than U32
  • changed MarshalerType from I32 to U8 size
  • added MarshalerType.TaskResolved and MarshalerType.TaskRejected
    • this allows us to pass better metadata in the same JSMarshalerArgument slot
  • moved JSMarshalerArgument.ElementType to different offset
    • to not clash with handles
  • JSMarshalerArgument.ElementType now contains type of the promise's result
    • this is necessary for marshaling with dynamic type
  • changed JSBindingType layout accordingly.
    • In future this could also help with nested generic types.
  • bumped schema version to 2

Other

  • better type for WeakRefInternal<T>
  • interop unit test no longer ForceDisposeProxies in the middle of the program because it can't balance C# side of handles

@pavelsavarapavelsavara added arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm labels Oct 4, 2023
@pavelsavarapavelsavara added this to the 9.0.0 milestone Oct 4, 2023
@pavelsavara
pavelsavara requested a review from marafOctober 4, 2023 14:55
@pavelsavarapavelsavara self-assigned this Oct 4, 2023
@ghost

ghost commented Oct 4, 2023

Copy link
Copy Markdown

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, os-browser

Milestone:9.0.0

@pavelsavara

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@marafmaraf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! 👍

Comment threadsrc/mono/wasm/runtime/loader/logging.ts Outdated
@pavelsavara

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@radicalradical closed this Oct 12, 2023
@radicalradical reopened this Oct 12, 2023
@radical

Copy link
Copy Markdown
Member

Re-triggered the builds to pick up the build fixes from #93412 .

@radical

Copy link
Copy Markdown
Member

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
pavelsavaraforce-pushed the browser_one_direction_tcs branch from 0706b84 to a55eda9CompareOctober 13, 2023 13:18
@pavelsavara

Copy link
Copy Markdown
MemberAuthor

I moved logging changes to #93472

@pavelsavara
pavelsavara merged commit 018efc5 into dotnet:mainOct 16, 2023
@pavelsavara
pavelsavara deleted the browser_one_direction_tcs branch October 16, 2023 10:10
@radical

Copy link
Copy Markdown
Member

Please avoid merging on red when the failure is not known, and especially when the failures are in wasm jobs. If merging any way, then please add an explanation for that.
#93583

pavelsavara added a commit to pavelsavara/runtime that referenced this pull request Oct 17, 2023
radical pushed a commit that referenced this pull request Oct 17, 2023
pavelsavara added a commit to pavelsavara/runtime that referenced this pull request Oct 18, 2023
@ghostghost locked as resolved and limited conversation to collaborators Nov 16, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasmWebAssembly architecturearea-System.Runtime.InteropServices.JavaScriptos-browserBrowser variant of arch-wasm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@pavelsavara@radical@maraf