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.

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

[wasm] Redesign of managed objects marshaling and lifecycle #56538

Merged
merged 29 commits into from
Aug 7, 2021

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Jul 29, 2021

Redesign of all managed object marshaling and lifecycle

  • file cycle of js owned C# instances is driven by FinalizationRegistry
    • got rid of Runtime._weakDelegateTable and Runtime._rawToJS
    • got rid of JSObject.WeakRawObject and JSObject.RawObject
    • GCHandle instead of JSObject double proxy for plain managed ref types
    • GCHandle instead of int sequence for delegates + redesign of invocation
    • GCHandle for task + redesign of invocation
    • improved in-flight retention of thenable/promise and Task
  • explicitly delegate type of parameter for EventListener
  • moved and renamed some binding functions
  • renamed all handles to jsHandle or gcHandle as appropriate
  • removed jsHandle math
  • cleanup of unused functions
  • improved error messages for invalid handles
  • more unit tests

Fixes #55735 delegate GC
Fixes #55679 delegate GC
Fixes #53614 task GC

@ghost
Copy link

ghost commented Jul 29, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details
  • explicitly single parameter for EventListener delegate
  • GCHandle instead of int sequence
  • generalization of delegate invocation to any parameters
Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript

Milestone: -

@pavelsavara pavelsavara requested review from lewing and kg July 29, 2021 12:11
@pavelsavara pavelsavara force-pushed the wasm_delegates branch 2 times, most recently from 6fd322e to a644cb3 Compare August 3, 2021 12:01
@pavelsavara
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara changed the title [wasm] Generalize delegates interop [wasm] Redesign of managed objects marshaling and lifecycle Aug 4, 2021
@pavelsavara pavelsavara marked this pull request as ready for review August 5, 2021 18:27
src/mono/wasm/runtime/binding_support.js Outdated Show resolved Hide resolved
src/mono/wasm/runtime/binding_support.js Outdated Show resolved Hide resolved
src/mono/wasm/runtime/binding_support.js Outdated Show resolved Hide resolved
src/mono/wasm/runtime/binding_support.js Outdated Show resolved Hide resolved
src/mono/wasm/runtime/binding_support.js Outdated Show resolved Hide resolved
src/mono/wasm/runtime/binding_support.js Outdated Show resolved Hide resolved
src/mono/wasm/runtime/binding_support.js Show resolved Hide resolved
@pavelsavara pavelsavara requested a review from kg August 6, 2021 12:20
@kg
Copy link
Contributor

kg commented Aug 6, 2021

I think anything else that needs to be done to this code can be done in later PRs, this looks more than good enough

@lewing
Copy link
Member

lewing commented Aug 6, 2021

The test failures are all unrelated to this pr but the runtime (Libraries Test Run release coreclr Linux x64 Debug) is odd

  Starting:    System.Runtime.Extensions.Tests (parallel test collections = on, max threads = 2)
    System.Tests.RandomTests.NextInt_AllValuesAreWithinSpecifiedRange(derived: True, seeded: False) [FAIL]
      Assert.True() Failure
      Expected: True
      Actual:   False
      Stack Trace:
        /_/src/libraries/System.Runtime.Extensions/tests/System/Random.cs(75,0): at System.Tests.RandomTests.NextInt_AllValuesAreWithinSpecifiedRange(Boolean derived, Boolean seeded)

@lewing
Copy link
Member

lewing commented Aug 7, 2021

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara merged commit e49b9bd into dotnet:main Aug 7, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2021
@pavelsavara pavelsavara deleted the wasm_delegates branch November 20, 2021 19:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants