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

JavaScript WeakRefs #321

Closed
3 of 5 tasks
tschneidereit opened this issue Nov 6, 2018 · 17 comments
Closed
3 of 5 tasks

JavaScript WeakRefs #321

tschneidereit opened this issue Nov 6, 2018 · 17 comments
Assignees
Labels

Comments

@tschneidereit
Copy link

tschneidereit commented Nov 6, 2018

こんにちはTAG!

I'm requesting a TAG review of:

Further details (optional):

You should also know that...

The proposal champions as well as TC39 overall, are well aware of the concerns that exposing GC timing information raises. The proposal is carefully designed to mitigate these issues by reducing the amount of information exposed, as well as the bandwidth with which it is exposed. In particular, the proposal contains mitigations against slight differences in GC timing breaking code. The champions as well as TC39 overall are confident that these mitigations are sufficient to prevent reduced interoperability and constraints on changes to engines' GC implementations.

We're particularly interested in the TAGs opinion on how the introduction of WeakRefs should influence the design of other web APIs, and what kinds of support for interop on the spec level the Ecma262 spec should provide.

Finally, note that while the crucial aspects of the proposal's semantics can be considered final, the API will change between now and January 2019, when we hope to advance it to stage 3. We don't expect these changes to impact other specifications, however.

We'd prefer the TAG provide feedback as (please select one):

  • open issues in our Github repo for each point of feedback
  • open a single issue in our Github repo for the entire review
  • leave review feedback as a comment in this issue and @-notify [github usernames]

Please preview the issue and check that the links work before submitting

For background, see our explanation of how to write a good explainer.

@travisleithead travisleithead self-assigned this Jan 22, 2019
@plinss plinss added this to the 2019-02-05-f2f milestone Jan 22, 2019
@kenchris kenchris self-assigned this Jan 22, 2019
@kenchris
Copy link

kenchris commented Feb 5, 2019

Still at stage 2

@torgo
Copy link
Member

torgo commented Feb 6, 2019

Regarding the explainer, we would really prefer something designed to be read. The slides provided appear to be designed to be talked through. We have specific guidance on what an explainer should look like.

@torgo torgo removed the extra time label Feb 6, 2019
@slightlyoff
Copy link
Member

Hey @tschneidereit:

Thanks for filing the request!

We had a chance to look through this at today's F2F in TOK and are generally supportive. In particular, we understand that WeakRefs will help reduce fracture in the platform when it comes to introducing WASM host bindings. To the extent that's true, we're excited to see this move forward.

Regarding the explainer doc itself, the PDF from Dean seems to be nearly a year old. Have there been updates to the proposal since then? The factory method seems peculiar.

As to integration with other web platform APIs, the various DOM collection types seem like they might benefit from WeakRefs, but it isn't yet clear to me if that will need to be done though new types or if compatible evolution of existing collections. In general we're excited to have this in the quiver for new feature designs.

Thanks for bringing it to our attention, and best of luck at TC39.

@slightlyoff slightlyoff added the Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review label Feb 6, 2019
@littledan
Copy link

I want to apologize for the ambiguity here. You can find the current tentative draft API in this issue comment.

This draft API does include a way to register a finalizer callback. We saw this as important, at the very least, because we don't want to encourage polling for a WeakRef to "go null".

As to integration with other web platform APIs, the various DOM collection types seem like they might benefit from WeakRefs, but it isn't yet clear to me if that will need to be done though new types or if compatible evolution of existing collections.

Could you say something more about where you see WeakRefs tying into the web platform? My understanding was that the DOM collections which make GC visible were seen as a mistake not to be repeated.

One reason why I thought TAG guidance would be useful here is that the design principles recommend against exposing GC, see w3ctag/design-principles#100 .

@dtribble
Copy link

dtribble commented Feb 6, 2019

I'll be back helping on this soon. I agree the PDF is stale wrt the proposal and needs serious update!

@alice
Copy link

alice commented Feb 26, 2019

@littledan Thanks for the pointer to the discussion of the new API.

We're wondering whether this is ready for another round of feedback on the API as outlined in that comment, or whether we should wait for that discussion to be finalised.

Thanks!

@littledan
Copy link

@alice Probably not yet; there's still ongoing work on a new explainer from @tschneidereit . Apologies for the confusion.

@alice
Copy link

alice commented Mar 12, 2019

Thanks for the update!

We're going to close this for now since there's not much we can do right now; once it's ready for another look, feel free to either

  • open another issue, linking back to this one; or
  • comment on this one and ping @plinss directly to get it re-opened.

@littledan
Copy link

The proposal has now been significantly updated, and the proposal repository reflects the rough consensus of the many people who have been contributing. I think it would benefit from further review from the TAG. @plinss , could you reopen the issue?

New explainer: https://github.com/tc39/proposal-weakrefs/

We will likely be considering this proposal for Stage 3 in the June 4-6, 2019 TC39 meeting; feedback before that meeting would be ideal, but we understand that this request is coming in a bit late.

cc @gsathya

@plinss plinss reopened this May 14, 2019
@plinss plinss removed the Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review label May 14, 2019
@kenchris kenchris added Progress: in progress Review type: later review Topic: universal JavaScript Features that work on the web and non-web (e.g. node.js) Topic: scripting ECMA, Web Assembly bindings, etc. labels May 21, 2019
@kenchris
Copy link

For the file handle example, wouldn't it be better to report/log an error instead of actually closing the file handle - especially as it is seen as a programming error?

@kenchris
Copy link

I am not a big fan of the name deref() the de sounds like it is undoing something, like creating the WeakRef itself. Maybe just call it get().

@kenchris
Copy link

Finalization sounds dangerous, but I do understand the use-cases. I am wondering whether a lot of GC might delay the event loop (which could be a problem) as all the finalizers are run right after microtasks and right before the event loop.

@dbaron
Copy link
Member

dbaron commented May 21, 2019

A few notes here:

  • The explainer doesn't seem to explain what a FinalizationGroup is. It just sort of starts talking about what you can do with it as though the reader has already heard of it. It seems like something should introduce it and say what it is first. (Enough to give somebody a mental answer to the questions: what is a finalization group? What is it a group of?) (I'd like to have another, more careful, read through this once that's clearer, because then I think I'll be able to understand the explainer better.)
  • In the note of caution about weak refs, I think there are some additional things that might (or might not) be worth saying:
    • another (common) source of variability is that other variable factors might cause an object to be retained for an unexpected amount of time (e.g., whether the user used a particular piece of functionality might affect the lifetime of an object only peripherally related to that functionality)
    • an additional problem that's distinct from sources of variability is that causing user-visible effects from a destructor causes memory management bugs to turn into user-facing bugs (e.g., saving something to disk in a destructor means that a memory leak can cause the data not to be saved). (That said, the behavior that finalizers don't run on tab close or process exit means people aren't likely to do this sort of thing.)
  • That said, I think the TAG Design Principles recommendation against exposing garbage collection is also (maybe but maybe not primarily) about not accidentally exposing garbage collection, e.g., by having a method that returns the same object until a GC happens, and then starts returning a different object, or null. In particular, I think it's more likely for authors to write code that depends on GC timing in bad ways if they're using an API that isn't obviously related to GC timing, and if such APIs are randomly scattered throughout the platform.
  • Another concern that I'd like to understand the tradeoffs of: Can finalization actually entirely prevent garbage collection in some cases? I think it could with something like Gecko's cycle collector. I'm not sure what the state of the art is these days for expressing ownership across processes -- but it seems like the use of finalization to avoid memory leaks for cross-worker proxies might fail to produce the expected results with some existing browser memory management code, e.g., if the things that the finalization does represent ownership edges that are unknown to that memory management. (This would imply that it might be better to encourage people to find other ownership models that don't have this problem.) Or are these proxy libraries careful to allow ownership edges to exist only in one direction so that this problem is avoided.
    • (Given the above, both the "releasing external resources" example and the "cross-worker proxies" examples make me pretty nervous -- but this is also based on a pretty quick skim of the explainer and not a thorough read through.)
    • (I'd also like to dig in more closely into how similar this is to the behavior of the existing DOM APIs that are mentioned in the "Iterable WeakMaps" section.)
  • The statement near the end that "To avoid leaking, cycles across isolated heaps must be explicitly broken." -- I'm curious to what extent this depends on what is or isn't an isolated heap (and how that various across browsers) -- in particular, does this risk more interop problems resulting from browsers having different ownership edges in their internal code (i.e., DOM API implementation) and different strategies (e.g., cycle collection) for dealing with those edges?

@mhofman
Copy link

mhofman commented May 21, 2019

  • The statement near the end that "To avoid leaking, cycles across isolated heaps must be explicitly broken." -- I'm curious to what extent this depends on what is or isn't an isolated heap (and how that various across browsers) -- in particular, does this risk more interop problems resulting from browsers having different ownership edges in their internal code (i.e., DOM API implementation) and different strategies (e.g., cycle collection) for dealing with those edges?

Since I wrote that statement, let me clarify: it's related to uses of WeakRefs by libraries, not internal browser implementations.
My concern is actually the same as yours above: cross-worker proxy libraries need to have a strong reference to a local object shared with a remote worker. Weak references and finalizers only help on the remote side and are not enough to allow collection of cycles that go across worker boundaries.
So by "isolated heap" I was thinking mostly about workers (which may or may not be in the same process), but it could really be anything remote, even code on the server side.

For the specific worker with postMessage use case, I believe there is a way to remove the requirement for libraries to need WeakRefs and to hold a strong reference to the local object, with an extension of the postMessage mechanism. I need to write a proposal for it.

However the cycle problem is still valid outside of postMessage.
I actually believe it's impossible to solve this problem without another mechanism allowing a library to know when it has full ownership of a currently unreachable slice of the object graph. I have an idea for a WeakRef follow-up proposal to introduce such a feature to JavaScript. You can find more details in an issue I raised, including a very early version of that idea.

@littledan
Copy link

Thanks for all of this helpful feedback. This gives us a lot to update in the explainer, consider changing in the specification, and clarify with implementers before standardizing. I just want to correct one misunderstanding:

as all the finalizers are run right after microtasks and right before the event loop.

Finalizers are run as a task, not in the microtask checkpoint. Did you see text in the proposal that suggested otherwise? See whatwg/html#4571 for details.

littledan added a commit to littledan/proposal-weakrefs that referenced this issue May 22, 2019
@littledan
Copy link

Thanks again for the timely feedback! We're discussing the name change in tc39/proposal-weakrefs#116, and I've drafted a clarified explainer at tc39/proposal-weakrefs#117 .

@kenchris
Copy link

Thanks for flying TAG! Hope the feedback was valuable!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests