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

fix: add WeakRef tracking to liveslots #1927

Closed
wants to merge 3 commits into from
Closed

Conversation

warner
Copy link
Member

@warner warner commented Oct 26, 2020

Liveslots maintains an import count: each time a vref is mentioned in an
inbound message, liveslots increments the counter. When user-level code
forgets about the Presence object, eventually (in some future turn, probably
not during one of this vat's cranks) the finalizer callback will run, and
liveslots will send a vatDecRef() message to the kernel with the old count.

This will eventually be wired up with kernel-side code that maintains a
similar counter, which will be decremented, and when it reaches zero, we'll
remove the c-list entry.

The handling of Promises needs more thought. Currently we retain a strong
reference to imported Promises until they are retired (which happens in
certain cases of resolution).

refs #1872

@warner warner added the SwingSet package: SwingSet label Oct 26, 2020
@warner warner requested a review from FUDCo October 26, 2020 20:44
@warner warner self-assigned this Oct 26, 2020
@warner warner mentioned this pull request Oct 26, 2020
12 tasks
@warner warner force-pushed the 1872-add-weakref branch 2 times, most recently from 0bea751 to 3f598b5 Compare October 26, 2020 20:58
@warner
Copy link
Member Author

warner commented Oct 27, 2020

I think the Zoe test failure is because the decref arrived after the vat was deleted, so kernel.js decref() threw an assertion that the vatID was bad, when instead it should have just ignored the call.


LOGGED ERROR: unknown vatID v8 [Error <Object <[Object: null prototype] {}>>: unknown vatID v8

  at Object.complain (kernel/packages/assert/src/assert.js:168:19)
  Uncaught exception in test/swingsetTests/brokenContracts/test-crashingContract.js
  at fail (kernel/packages/assert/src/assert.js:195:20)

  at Object.assert (kernel/packages/assert/src/assert.js:207:11)
  Error: unknown vatID v8
  at Object.decref (kernel/packages/SwingSet/src/kernel/kernel.js:149:12)
    at Object.complain (kernel/packages/assert/src/assert.js:168:19)
  at vatDecref (kernel/packages/SwingSet/src/kernel/vatManager/localVatManager.js:110:15)
    at fail (kernel/packages/assert/src/assert.js:195:20)
  at importDropped (kernel/packages/SwingSet/src/kernel/liveSlots.js:90:5)
    at Object.assert (kernel/packages/assert/src/assert.js:207:11)
  at FinalizationRegistry.cleanupSome (<anonymous>)]
    at Object.decref (kernel/packages/SwingSet/src/kernel/kernel.js:149:12)
    at vatDecref (kernel/packages/SwingSet/src/kernel/vatManager/localVatManager.js:110:15)
    at importDropped (kernel/packages/SwingSet/src/kernel/liveSlots.js:90:5)
    at FinalizationRegistry.cleanupSome (<anonymous>)

I'll add a more specific test and a fix.

Base automatically changed from 1872-add-vatdecref to master October 27, 2020 17:03
Liveslots maintains an import count: each time a vref is mentioned in an
inbound message, liveslots increments the counter. When user-level code
forgets about the Presence object, eventually (in some future turn, probably
not during one of this vat's cranks) the finalizer callback will run, and
liveslots will send a `vatDecRef()` message to the kernel with the old count.

This will eventually be wired up with kernel-side code that maintains a
similar counter, which will be decremented, and when it reaches zero, we'll
remove the c-list entry.

The handling of Promises needs more thought. Currently we retain a strong
reference to imported Promises until they are retired (which happens in
certain cases of resolution).

refs #1872
The test is skipped if the platform lacks a `WeakRef` global (Node.js v12),
or if it lacks a `gc` global (Node.js v14 without `--expose-gc`).
Finalizer callbacks run at strange times, and can occur after the vat which
dropped the import has been terminated and cleaned up (so there are no c-list
entries left to remove). Ignore these.

The unit test is necessarily probablistic. This particular sequence appears
to exercise the previously-failing path about 50% of the time. The other 50%
of the time it does not, and the test passes without accomplishing anything
useful.
@warner
Copy link
Member Author

warner commented Jan 4, 2021

I'm deprioritizing this in favor of more deterministic forms of GC, we'll reassess it later.

@warner warner removed the request for review from FUDCo March 19, 2021 22:28
@warner
Copy link
Member Author

warner commented Mar 19, 2021

We're no longer pursuing this counter-based approach, instead we're relying upon the engine to give us deterministic GC so we can avoid the counters. See #2615 for the current plan.

@warner warner closed this Mar 19, 2021
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.

1 participant