Skip to content
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

[browser] streamline Task/Promise marshalling #93010

Merged
merged 1 commit into from
Oct 16, 2023
Merged

[browser] streamline Task/Promise marshalling #93010

merged 1 commit into from
Oct 16, 2023

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Oct 4, 2023 •
edited
Loading

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

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

ghost commented Oct 4, 2023

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
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Oct 4, 2023
Closed
Closed
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Oct 5, 2023
Open
Open
Closed
Closed
@pavelsavara pavelsavara changed the title [browser] drop create_task_callback [browser] drop create_task_callback and mono_wasm_marshal_promise Oct 9, 2023
This was referenced Oct 9, 2023
Closed
Closed
Closed
Closed
@pavelsavara pavelsavara changed the title [browser] drop create_task_callback and mono_wasm_marshal_promise [browser] streamline Task/Promise marshalling Oct 11, 2023
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

maraf
maraf approved these changes Oct 12, 2023
View reviewed changes
Copy link
Member

@maraf maraf left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

...rvices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Interop/JavaScriptExports.cs Outdated Show resolved Hide resolved
src/mono/wasm/runtime/loader/logging.ts Outdated Show resolved Hide resolved
@build-analysis build-analysis bot mentioned this pull request Oct 12, 2023
Closed
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Oct 12, 2023
Open
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical radical closed this Oct 12, 2023
@radical radical reopened this Oct 12, 2023
@radical
Copy link
Member

radical commented Oct 12, 2023

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

@radical
Copy link
Member

radical commented Oct 12, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Oct 13, 2023
Open
@pavelsavara
Copy link
Member Author

I moved logging changes to #93472

@pavelsavara pavelsavara merged commit 018efc5 into dotnet:main Oct 16, 2023
116 of 120 checks passed
@pavelsavara pavelsavara deleted the browser_one_direction_tcs branch October 16, 2023 10:10
@radical radical mentioned this pull request Oct 17, 2023
Closed
@radical
Copy link
Member

radical commented Oct 17, 2023

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
@pavelsavara
Revert " [browser] streamline Task/Promise marshalling (dotnet#93010)"
a44f7d2 
This reverts commit 018efc5.
radical pushed a commit that referenced this pull request Oct 17, 2023
@pavelsavara
[browser] Revert - streamline Task/Promise marshalling (#93010) (#93618)
de09811 
This reverts commit 018efc5.
pavelsavara added a commit to pavelsavara/runtime that referenced this pull request Oct 18, 2023
@pavelsavara
second attempt dotnet#93010
d652302
@pavelsavara pavelsavara mentioned this pull request Oct 18, 2023
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Nov 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@maraf maraf maraf approved these changes

@lambdageek lambdageek Awaiting requested review from lambdageek

@lewing lewing Awaiting requested review from lewing lewing is a code owner

@kg kg Awaiting requested review from kg

@radical radical Awaiting requested review from radical

Assignees

@pavelsavara pavelsavara

Labels
arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@pavelsavara @radical @maraf