Skip to content

Implement WeakReference, Finalizer in dartdevc #47776

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

Closed
Tracked by #47772
mraleph opened this issue Nov 25, 2021 · 9 comments
Closed
Tracked by #47772

Implement WeakReference, Finalizer in dartdevc #47776

mraleph opened this issue Nov 25, 2021 · 9 comments
Assignees
Labels
area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. web-dev-compiler

Comments

@mraleph
Copy link
Member

mraleph commented Nov 25, 2021

No description provided.

@mraleph mraleph added area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. web-dev-compiler labels Nov 25, 2021
@devoncarew
Copy link
Member

@sigmundch - who's a likely owner here?

@dcharkes
Copy link
Contributor

@lrhn suggested that if detach is omitted in Finalizer.attach we use value as the detach object.

I'll update the documentation in the VM CL, but of course it should be implemented as such in dartdevc as well then.

https://dart-review.googlesource.com/c/sdk/+/229544/8/sdk/lib/core/weak.dart#185

@rileyporter
Copy link
Contributor

Sounds good, will do!

@mraleph
Copy link
Member Author

mraleph commented Jan 27, 2022

@dcharkes @lrhn I don't think we should do this. I would actually expect most finalizers to not be detachable so it does not make sense to introduce this book keeping overhead by default.

@lrhn
Copy link
Member

lrhn commented Jan 27, 2022

Unnecessary overhead was my main worry.
From a usability point of view, I think it's actually a good idea to make objects default to being detachable by themselves. The few pieces of code I wrote, just to test the API, made me want to pass the object itself as detach-object, but you can always do that explicitly. It's just much easier not to have to.

The current implementation uses a Dart-side weak-map to map detach objects to "attachment ID"s. Would you still worry about overhead if that mapping we kept in the native implementation instead, as part of the same data structure which maintain the attachments. (Or is the lookup requirement going to make it equally expensive in native code?)

@mraleph
Copy link
Member Author

mraleph commented Jan 27, 2022

@lrhn I think I would still be worried. I would like most common use-case to be very low-overhead --- meaning that there should be no additional book-keeping allocated beyond absolutely necessary one e.g. in case of NativeFinalizer you only really need a weak handle and a small piece of peer data associated with the handle. No weak-maps are involved here.

Maybe for Finalizer my concern does not apply, I need to think what is the most optimal representation there. But I would imagine that JS folks did not make it default because they had similar concerns - it is highly likely that associating detachment key costs significantly on the Web.

@mraleph
Copy link
Member Author

mraleph commented Feb 1, 2022

I just want to check if we are in agreement that we are not going to change API and keep the original (JS) semantics where you need to explicitly pass detach: object if you want to make the finalizer detachable.

@dcharkes
Copy link
Contributor

dcharkes commented Feb 1, 2022

I have updated the CL to not use value as default detach anymore.

copybara-service bot pushed a commit that referenced this issue Feb 8, 2022
Routes implementation to JavaScript WeakRef and
FinalizerRegistry APIs using JS foreign function calls. Uses
Wrapper names for the Dart library to avoid issues with DDC.

Bug: #47775, #47776
Change-Id: Iad82bd83ac10c666d08a2c042a8ed6109b8b58c8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/229180
Reviewed-by: Stephen Adams <[email protected]>
Reviewed-by: Sigmund Cherem <[email protected]>
Reviewed-by: Nicholas Shahan <[email protected]>
Commit-Queue: Riley Porter <[email protected]>
@rileyporter
Copy link
Contributor

DDC implementation landed in 278a040

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. web-dev-compiler
Projects
None yet
Development

No branches or pull requests

6 participants