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

chore: provide GC tools (WeakRef/FinalizationRegistry) to makeLiveSlots #1924

Merged
merged 3 commits into from
Oct 27, 2020

Conversation

warner
Copy link
Member

@warner warner commented Oct 26, 2020

These two authorities are not part of SES, so they must be pulled from the
globals of the Start Compartment and ferried through the kernel to the vat
manager factory that calls makeLiveSlots.

This gives the outer layer of the vat (liveslots) access to nondeterminism.
We rely upon liveslots to not share this power with the user-level ocap-style
"vat code". Liveslots must never allow user-level code to observe behavior
that depends upon GC activity, because that activity is not part of the
specified input to the vat.

These tools are not used by liveslots yet, that will come in a later PR.

refs #1872

@warner warner added the SwingSet package: SwingSet label Oct 26, 2020
@warner warner requested a review from FUDCo October 26, 2020 16:39
@warner warner self-assigned this Oct 26, 2020
@warner
Copy link
Member Author

warner commented Oct 26, 2020

Hm, it looks like the test failures are because Node 12.x does not expose WeakRef by default, but Node 14.x does. This might prompt us to increase our required Node.js version to 14, rather than instruct users to add whatever command-line option might enable weakrefs in 12 (if there even is such a thing.. I see --harmony-weak-refs, but it's labelled as "in progress").

@warner
Copy link
Member Author

warner commented Oct 26, 2020

Based on our discussion, we're going to provide backwards compatibility with Node v12, which means the liveslots code will either get the real WeakRef, or it will create a strong-reference-holding stub.

@warner warner marked this pull request as draft October 26, 2020 19:31
@warner warner removed the request for review from FUDCo October 26, 2020 19:32
@warner
Copy link
Member Author

warner commented Oct 26, 2020

I'm going to change this to provide a no-op implementation under Node v12, I'll re-submit for review once that's done.

The upcoming GC functionality will require `WeakRef` and
`FinalizationRegistry`. Node.js v14 provides these as globals, but v12 does
not (there might be a command-line flag to enable it, but I think it's marked
as experimental). Rather than require all users upgrade to v14, we elect to
disable GC when running on v12.

This adds a local `weakref.js` module which attempts to pull `WeakRef` and
`FinalizationRegistry` from the global, and exports either the real
constructors or no-op stubs.

refs #1872
refs #1925
These two authorities are not part of SES, so they must be pulled from the
globals of the Start Compartment and ferried through the kernel to the vat
manager factory that calls makeLiveSlots.

This gives the outer layer of the vat (liveslots) access to nondeterminism.
We rely upon liveslots to not share this power with the user-level ocap-style
"vat code". Liveslots must never allow user-level code to observe behavior
that depends upon GC activity, because that activity is not part of the
specified input to the vat.

refs #1872
@warner warner marked this pull request as ready for review October 26, 2020 20:37
@warner warner requested review from FUDCo and erights October 26, 2020 20:37
@warner
Copy link
Member Author

warner commented Oct 26, 2020

@erights am I taming the generated stub object correctly? I don't know that I'm deleting the right .constructor property.

Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

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

Looks fine, though I think we're going to want to do something about the proliferation of parameters to makeLiveSlots

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

DO NOT MERGE THIS IN ITS CURRENT STATE. My review coming soon.

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

Aside from my suggestions at #1931 , this LGTM

* fix: WeakRef taming follows taming pattern

* fix: error message

* fix: minor
@warner warner requested a review from erights October 27, 2020 06:34
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

LGTM

@warner warner merged commit b765066 into master Oct 27, 2020
@warner warner deleted the 1872-gctools branch October 27, 2020 17:02
@warner warner mentioned this pull request Oct 28, 2020
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants