refactor dispatch to be a function, not an object #2674

warner · 2021-03-18T07:54:13Z

Previously, the low-level vat interface was defined by a dispatch object,
with methods like .deliver and .notify. Now, it is defined by a
dispatch() function, which takes a vatDeliveryObject that has the exact
same form as the data sent in the kernel-vat protocol.

This reduces the size of the manager/supervisor code and reduces the
redundancy somewhat.

refs #2671

warner · 2021-03-18T08:15:27Z

Can anyone help me with the type annotations? https://github.com/Agoric/agoric-sdk/pull/2674/checks?check_run_id=2137783731#step:7:98 says that

agoric-sdk/packages/SwingSet/src/kernel/vatManager/manager-subprocess-xsnap.js

Line 80 in aa507e5

return handleSyscall(vatSyscallObject);

is unhappy because Argument of type 'unknown' is not assignable to parameter of type 'Tagged'. I see that Tagged is defined as a 1+-length array of unknowns; in this case the type is always a string, and if that string is syscall then the second (and only) other element is a vatSyscallObject, which is again a 1+-length array where the first is a string and the rest depends upon the first. I don't know typescript well enough to know to cast the const vatSyscallObject = into something with more information than the unknown that it gets from args.

packages/SwingSet/src/kernel/vatManager/manager-subprocess-xsnap.js

warner · 2021-03-21T18:25:25Z

This does a number of other cleanups and refactorings to align the four different worker types. The overall diff is kind of big, but each individual commit is pretty small and self-contained, so I'd recommend reviewing each commit separately and sequentially.

dckc

Looks good!

My eyes did glaze over some parts... especially the non-XS vat managers and the tests in 9214adf . So it may be best to keep @FUDCo 's review in the critical path.

dckc · 2021-03-22T14:18:52Z

packages/SwingSet/src/types.js

+ *
+ * @typedef { [tag: 'message', target: string, msg: Message]} VatDeliveryMessage
+ * @typedef { [tag: 'notify', resolutions: string[] ]} VatDeliveryNotify
+ * @typedef { [tag: 'dropExports', vrefs: string[] ]} VatDeliveryDropExports


Yay! types! but...

Names such as VatDeliveryDropExports don't seem to get reused anywhere. I suspect the VatDeliveryObject and VatSyscallObject types would be more clear if the options were inlined.

minor style issue. not critical.

Ah, I didn't know how to do that. Would it be something like:

/** * @typedef { [tag: 'message', target: string, msg: Message] | * [tag: 'notify', resolutions: string[]] | * [tag: 'dropExports', vrefs: string[]] } VatDeliveryObject */

?

(note to self/chip, that notify signature isn't correct, I'll fix that)

At some point soon, we're going to have a function in liveslots which switches on tag and then calls one of three different functions. Would the typedefs on those three functions be a reason to retain the VatDeliveryMessage/etc subtypes?

Would it be something like: ... ?

yes.

Would the typedefs on those three functions be a reason to retain the VatDeliveryMessage/etc subtypes?

yes.

packages/SwingSet/src/kernel/vatManager/manager-nodeworker.js

packages/SwingSet/src/kernel/liveSlots.js

dckc · 2021-03-22T14:30:19Z

packages/SwingSet/src/kernel/vatManager/deliver.js

-import { assert, details as X } from '@agoric/assert';
-import { insistMessage } from '../../message';
+/** @type { (delivery: VatDeliveryObject, prefix: string) => string } */
+function summarizeDelivery(vatDeliveryObject, prefix = 'vat') {


strange name for something that always returns a ... failed message.

it's really a "make a message to log just in case it fails" function, but that name seemed expedient.

failureMessage?

summarizeDeliveryFailure maybe?

dckc · 2021-03-22T14:31:48Z

packages/SwingSet/src/kernel/vatManager/deliver.js

-    const dispatchArgs = dispatchRecord.slice(1);
-    transcriptManager.startDispatch(dispatchRecord);
-    await runAndWait(() => dispatch[dispatchOp](...dispatchArgs), errmsg);
+  // vatDeliveryObject is one of:


Did you mean to keep this comment here? It duplicates a lot of info that's now in types; move the prose to the type declaration? Also: cite the .md doc?

I disagree. Keep the prose here. It's more informative and more legible than the type declaration.

dckc · 2021-03-22T14:32:37Z

packages/SwingSet/src/kernel/vatManager/deliver.js

-    return doProcess(['dropExports', vrefs], errmsg);
-  }
-
-  // vatDeliverObject is:


likewise... move prose to type declaration and remove?

dckc · 2021-03-22T14:34:22Z

packages/SwingSet/src/kernel/vatManager/supervisor-subprocess-xsnap.js

    workerLog(`doProcess done`);
    /** @type { Tagged } */
    const vatDeliveryResults = harden(['ok']);
    return vatDeliveryResults;
  }

-  /** @type { (ts: unknown, msg: any) => Promise<Tagged> } */


yay! deleting code!

dckc · 2021-03-22T14:35:47Z

packages/SwingSet/test/files-devices/bootstrap-4.js

-        const slot = args.slots[qnode.index];
-        insistVatType('device', slot);
+  function dispatch(vatDeliverObject) {
+    const { method, args } = extractMessage(vatDeliverObject);


reads nicely.

dckc · 2021-03-22T14:37:49Z

packages/SwingSet/test/test-liveslots.js

-  return [[promiseID, rejected, data]];
-}
-
-function buildSyscall() {


ah... so we had already written buildSyscall() but not exported it. Cool.

yeah there are a bunch of little helpers in various tests that finally hit my threshold for factoring them out into something common

FUDCo

Looks fine. Getting rid of some of the extraneous, additional, and unnecessary redundancy and duplication is especially good.

You can probably expect some minor complications merging test-comms.js.

FUDCo · 2021-03-23T21:31:16Z

packages/SwingSet/src/kernel/vatManager/deliver.js

-    const dispatchArgs = dispatchRecord.slice(1);
-    transcriptManager.startDispatch(dispatchRecord);
-    await runAndWait(() => dispatch[dispatchOp](...dispatchArgs), errmsg);
+  // vatDeliveryObject is one of:


I disagree. Keep the prose here. It's more informative and more legible than the type declaration.

FUDCo · 2021-03-23T21:31:53Z

packages/SwingSet/src/kernel/vatManager/deliver.js

-import { assert, details as X } from '@agoric/assert';
-import { insistMessage } from '../../message';
+/** @type { (delivery: VatDeliveryObject, prefix: string) => string } */
+function summarizeDelivery(vatDeliveryObject, prefix = 'vat') {


summarizeDeliveryFailure maybe?

FUDCo · 2021-03-23T21:34:29Z

packages/SwingSet/src/kernel/vatManager/manager-local.js

-      assert(
-        dispatch && dispatch.deliver,
-        `vat failed to return a 'dispatch' with .deliver: ${dispatch}`,
+      assert.equal(


Is there a reason here not to use assert.typeof ?

This was a false start on the GC task. Previously we thought vat-side finalizers would call a vat-specific `decref` function at spontaneous times, and the kernel would accumulate the dropped references for later processing. Now we have liveslots collect the finalizer results and perform a non-transcript-enforced syscall.dropImports at the end of crank. This removes `testTrackDecref` from `kernelOptions`.

This changes the protocol used to speak to the three remote vat workers (nodeworker, subprocess-node, subprocess-xsnap) slightly. Previously we unpacked the VatSyscallObject (a tagged array of `[syscallTag, ...syscallArgs]`) when constructing a remote message (which has its own tag), giving us something like `['syscall', syscallTag, ...syscallArgs]`. Now we leave the VatSyscallObject intact, giving us `['syscall', [syscallTag, ...syscallArgs]]`. This should be a bit simpler. It changes the protocol in an incompatible way, but the child thread/process never outlives the kernel process, so the incompatibility shouldn't be observable.

message.js acquired insistVatDeliveryObject() and insistVatSyscallObject(), which are type guard functions that match. Also rename CapDataS to SwingSetCapData.

This changes the protocol used to speak to the three remote vat workers (nodeworker, subprocess-node, subprocess-xsnap) slightly, by leaving the VatDeliveryObject intact (rather than merging it with the outer frame). This object starts each crank by either delivering a 'message', a 'notify', or a 'dropExports' operation.

This changes the protocol used to speak to two of the remote vat workers (nodeworker, subprocess-node) slightly, by leaving the VatDeliveryResults intact (rather than merging it with the outer frame). The subprocess-xsnap protocol has its own return-value framing and wasn't merging this object into it.

These two worker types were lagging behind the others, and weren't recording dispatches into their transcripts. Any vats using them would fail to replay after a host restart, because their transcripts would be empty.

Previously, the lowest level of a vat was defined by an object named `dispatch`, which had three methods: `{ deliver, notify, dropExports }`. Now we define `dispatch()` to be a *function*, which takes a `VatDeliveryObject` (of which there are three flavors). This simplifies the supervisor code and removes a lot of redundant boilerplate, and prepares for later phase where `dispatch()` becomes `async dispatch()`. refs #2671

dckc · 2021-03-31T17:23:59Z

@warner are you holding off on this for any particular reason? It would be nice to have in the vatWarehouse work #2277 .

warner · 2021-03-31T17:54:54Z

Alas this is based upon PR #2667 (the 2660-add-weakref branch) which has been abandoned because of our late discovery of a design flaw (#2724 has details), which would have introduced non-determinism into any Zoe/ERTP pattern that uses a Presence in a WeakMap. I saw intermittent test failures locally when it was fully enabled.

I'm going to have to re-do the refactorings without the WeakRef tracking. It will be shaped a lot like this branch, but it won't be this branch, and it won't target 2660-add-weakref, so I'm going to close this PR without landing.

warner self-assigned this Mar 18, 2021

warner added the SwingSet package: SwingSet label Mar 18, 2021

warner changed the base branch from master to 2660-add-weakref March 18, 2021 07:54

dckc reviewed Mar 18, 2021

View reviewed changes

packages/SwingSet/src/kernel/vatManager/manager-subprocess-xsnap.js Outdated Show resolved Hide resolved

warner force-pushed the 2671-refactor-dispatch branch 2 times, most recently from 87fb514 to 89e1324 Compare March 21, 2021 09:21

warner force-pushed the 2660-add-weakref branch from 64a967b to 93e7e7f Compare March 21, 2021 18:05

warner force-pushed the 2671-refactor-dispatch branch from 89e1324 to 9214adf Compare March 21, 2021 18:05

warner marked this pull request as ready for review March 21, 2021 18:23

warner requested review from FUDCo and dckc March 21, 2021 18:23

dckc approved these changes Mar 22, 2021

View reviewed changes

warner force-pushed the 2660-add-weakref branch from 93e7e7f to bac7b20 Compare March 22, 2021 23:10

warner force-pushed the 2671-refactor-dispatch branch 2 times, most recently from 35c67dc to b96c1f7 Compare March 22, 2021 23:41

warner force-pushed the 2660-add-weakref branch from 9ba29cb to aefc80d Compare March 22, 2021 23:59

warner force-pushed the 2671-refactor-dispatch branch from b96c1f7 to 4eeea9a Compare March 22, 2021 23:59

FUDCo approved these changes Mar 23, 2021

View reviewed changes

warner force-pushed the 2660-add-weakref branch from 0a60719 to 8fe8077 Compare March 24, 2021 05:39

warner force-pushed the 2671-refactor-dispatch branch from 4eeea9a to 2c97e63 Compare March 24, 2021 05:39

warner added 7 commits March 24, 2021 11:27

chore(swingset): define VatDeliveryObject and VatSyscallObject types

e6257b8

message.js acquired insistVatDeliveryObject() and insistVatSyscallObject(), which are type guard functions that match. Also rename CapDataS to SwingSetCapData.

fix(swingset): record transcripts for nodeworker and subprocess-node

072a4ce

These two worker types were lagging behind the others, and weren't recording dispatches into their transcripts. Any vats using them would fail to replay after a host restart, because their transcripts would be empty.

warner force-pushed the 2671-refactor-dispatch branch from 2c97e63 to 692c7d6 Compare March 24, 2021 18:28

warner closed this Mar 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor dispatch to be a function, not an object #2674

refactor dispatch to be a function, not an object #2674

warner commented Mar 18, 2021

warner commented Mar 18, 2021

warner commented Mar 21, 2021 •

edited

Loading

dckc left a comment

dckc Mar 22, 2021

warner Mar 22, 2021

dckc Mar 22, 2021

dckc Mar 22, 2021

warner Mar 22, 2021

dckc Mar 22, 2021

FUDCo Mar 23, 2021

dckc Mar 22, 2021

FUDCo Mar 23, 2021

dckc Mar 22, 2021

dckc Mar 22, 2021

dckc Mar 22, 2021

dckc Mar 22, 2021

warner Mar 22, 2021

FUDCo left a comment

FUDCo Mar 23, 2021

FUDCo Mar 23, 2021

FUDCo Mar 23, 2021

dckc commented Mar 31, 2021

warner commented Mar 31, 2021

refactor dispatch to be a function, not an object #2674

refactor dispatch to be a function, not an object #2674

Conversation

warner commented Mar 18, 2021

warner commented Mar 18, 2021

warner commented Mar 21, 2021 • edited Loading

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FUDCo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc commented Mar 31, 2021

warner commented Mar 31, 2021

warner commented Mar 21, 2021 •

edited

Loading