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(swingset): fix refcounts for messages queued to a promise #3270

Merged
merged 3 commits into from
Jun 9, 2021

Conversation

warner
Copy link
Member

@warner warner commented Jun 8, 2021

As described in #3264, we were not maintaining correct reference counts when
messages get queued to an unresolved promise.

In the new approach, the lifetime of a message is:

  • all krefs (target, args, and optional result) of a message are increfed
    during doSend, as before
    • this adds a type: 'send' event to the run-queue
  • when the send event is pulled off the run-queue, all krefs are
    decrefed (as before)
  • if the event is delivered to a vat, great
  • if the event is delivered to a promise,
    kernelKeeper.addMessageToPromiseQueue increfs all krefs
    • (this is new: previously addMessageToPromiseQueue did not change the
      refcounts)
  • later, if/when the promise is resolved, the messages are transferred
    laterally from the promise queue to send events on the run-queue, without
    changing their refcounts
    • (this is new: previously the transfer was done with doSend, which
      incremented the refcounts)
    • the counts are decremented when the send event is removed from the
      run-queue, as usual

The result is the same number of increments and decrements as before, but the
increment is done earlier, so messages on a promise queue will maintain a
refcount on all their krefs (target, args, and any result). This protects
objects and promises which would otherwise have been eligible for collection
while they sat on the promise queue.

Strictly speaking we don't need to maintain a refcount on the target (which
is also kind of redundant: everything on the queue for promise kp1
obviously has target: 'kp1'), since that will always be additionally held
by the c-list of the decider (at least). But by making all promise queues do
the same thing as the main run-queue, we can laterally transfer messages from
promise- to run-queue without DB churn (decrementing then immediately
incrementing the counts).

This change moves the responsibility for the lateral transfer from
notifySubscribersAndQueue in kernel.js to
kernelKeeper.resolveKernelPromise(), deleting the former entirely and
merging the subscription loop into doResolve.

closes #3264

(reviewer note: consider examining each commit separately; they don't overlap much, but are logically separate)

@warner warner added the SwingSet package: SwingSet label Jun 8, 2021
@warner warner added this to the Testnet: Stress Test Phase milestone Jun 8, 2021
@warner warner requested a review from FUDCo June 8, 2021 20:04
@warner warner self-assigned this Jun 8, 2021
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.

Lovely.

This just adds an optional `delta=1` parameter so incstat can be used more
efficiently when incrementing multiple times.
As described in #3264, we were not maintaining correct reference counts when
messages get queued to an unresolved promise.

In the new approach, the lifetime of a message is:

* all krefs (target, args, and optional result) of a message are increfed
  during `doSend`, as before
  * this adds a `type: 'send'` event to the run-queue
* when the `send` event is pulled off the run-queue, all krefs are
  decrefed (as before)
* if the event is delivered to a vat, great
* if the event is delivered to a promise,
  `kernelKeeper.addMessageToPromiseQueue` increfs all krefs
  * (this is new: previously `addMessageToPromiseQueue` did not change the
    refcounts)
* later, if/when the promise is resolved, the messages are transferred
  laterally from the promise queue to `send` events on the run-queue, without
  changing their refcounts
  * (this is new: previously the transfer was done with `doSend`, which
    incremented the refcounts)
  * the counts are decremented when the `send` event is removed from the
    run-queue, as usual

The result is the same number of increments and decrements as before, but the
increment is done earlier, so messages on a promise queue will maintain a
refcount on all their krefs (target, args, and any result). This protects
objects and promises which would otherwise have been eligible for collection
while they sat on the promise queue.

Strictly speaking we don't need to maintain a refcount on the target (which
is also kind of redundant: everything on the queue for promise `kp1`
obviously has `target: 'kp1'`), since that will always be additionally held
by the c-list of the decider (at least). But by making all promise queues do
the same thing as the main run-queue, we can laterally transfer messages from
promise- to run-queue without DB churn (decrementing then immediately
incrementing the counts).

This change moves the responsibility for the lateral transfer from
`notifySubscribersAndQueue` in kernel.js to
`kernelKeeper.resolveKernelPromise()`, deleting the former entirely and
merging the subscription loop into `doResolve`.

closes #3264
Base automatically changed from 3264-fix-test-state to master June 9, 2021 19:14
@warner warner merged commit b613501 into master Jun 9, 2021
@warner warner deleted the 3264-refcounts branch June 9, 2021 19:14
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.

match incref/decref calls within kernel
2 participants