refactor(swingset): improve vat deliveries #4716

warner · 2022-03-02T04:56:04Z

This moves some logic out of deliverAndLogToVal and into a higher-level
dispatch function, where it can react to all failure modes in a single place.

We pull "run-queue events" off the run-queue (currently named the "acceptance
queue"). Many of these events are aimed at a specific vat, including 'notify',
but the primary one is 'send' and is aimed at a target, which is either a
kernel object or a kernel promise. For these we use routeSendEvent to
either queue the event on a promise queue, reject it if it can't be
delivered, or deliver it to a specific vat. This is the part that will change
with #3465: in that world, each vat-input-queue exists for a specific vat, so
the routing step moves earlier (into a "routing crank" that pulls events from
a vat-output-queue and possibly adds them to a vat-input-queue).

Some kinds of deliveries care about metering (or don't), and some have
opinions about what should happen if a crank gets unwound. These are now
reported as additional properties in the DeliveryStatus object that is
returned by this delivery path. processDeliveryMessage looks at these
properties, plus the delivery status (if any) to decide what to do at the end
of the crank.

I think most of the behavior should be the same as before. One change is that
the runPolicy will probably get more information about non-'send' cranks
than before (e.g. create-vat might report metering).

This refactoring should make it easier to implement #1848 vat-upgrade, as
well as #3465 queueing changes.

warner · 2022-03-02T20:09:39Z

recommend reviewing with "hide whitespace", to reduce the diff

FUDCo · 2022-03-02T20:30:30Z

packages/SwingSet/src/kernel/state/kernelKeeper.js

+        'xs-worker-no-gc',
+      ].includes(mt),
+    );
+    return undefined;


Won't a function return undefined anyway if you don't have an explicit return statement? Or is this to deal with some kind of TypeScript nonsense?

yeah, typescript

That would be the difference between a return type of void vs undefined.

let me know if there's a better way to write that assertion function, I just copied something else that works

Assertions don't require a return value in TS. This might be a case of jsdoc eslint plugin not understanding assertions ?

FUDCo

This is a very nice blood pressure reducing cleanup! I particularly like getting rid of the output named policyInput which, even though I kinda sorta understood the reasoning behind the name, has always bugged me.

mhofman

This looks great! Thanks so much for starting this refactor.

I'm not familiar enough with the termination conditions and the start vat logic to be confident enough in my review for those parts, and I trust @FUDCo on his approval there.

The requested change is for the extension in the import in the types file, in case that file ever ends up actually imported by ES code someday. Rest is mostly nits on types and consistency.

packages/SwingSet/src/kernel/kernel.js

mhofman · 2022-03-02T21:49:45Z

packages/SwingSet/src/kernel/state/kernelKeeper.js

+        'xs-worker-no-gc',
+      ].includes(mt),
+    );
+    return undefined;


Assertions don't require a return value in TS. This might be a case of jsdoc eslint plugin not understanding assertions ?

mhofman · 2022-03-02T21:51:08Z

packages/SwingSet/src/kernel/state/kernelKeeper.js

+  /**
+   *
+   * @returns { ManagerType }
+   */


FYI this should be unnecessary given that mt should be typed correctly after the assertion.

I can remove the @returns { ManagerType }, thanks. (I started from kernel.js that calls this, added the @returns so the options.managerType = kernelKeeper.getDefaultManagerType() didn't complain, and then added the assertion to make the @returns { ManagerType } shut up.. should have started with the assertion).

If I remove the return undefined; from insistManagerType(), I get this warning:

$ eslint '**/*.js' /home/warner/stuff/agoric/agoric-sdk/packages/SwingSet/src/kernel/state/kernelKeeper.js 310:3 warning JSDoc @returns declaration present but return expression not available in function jsdoc/require-returns-check

I don't know how to make that go away, so I'm inclined to leave the return undefined;, but if you know a way, I'll apply it.

Yeah sounds like a limitation of the JSDoc eslint plugin, not a problem with typescript. Besides an ugly eslint disable.

An alternative might be to return mt instead.

return mt; gets me Type 'string' is not assignable to type 'void. I don't know how to express "returns $type and also asserts that the input value is $type".

packages/SwingSet/src/types-internal.js

mhofman · 2022-03-02T21:59:02Z

packages/SwingSet/src/types-internal.js

+ * @typedef { { meterID?: MeterID } } HasMeterID
+ * @typedef { import('./types-exported.js').DynamicVatOptionsWithoutMeter } DynamicVatOptionsWithoutMeter
+ *
+ * vatKeeper.setSourceAndOptions(source, RecordedVatOptions)


This line seem out of place.

it was a reminder of where RecordedVatOptions gets used.. is there a better way to do comments within JSDoc comments? I could use * // vatKeeper.set... maybe.

packages/SwingSet/src/kernel/kernel.js

mhofman · 2022-03-03T16:34:04Z

packages/SwingSet/src/kernel/kernel.js

+  // `vatFatalSyscall`. We'd still need a way for `requestTermination` to
+  // work, though.


Note to self: could this be handled a similar way, something in the vat translation layer that remembers the exit call?

Oh, you know, we should consider making the syscall.exit handler just push an terminate-vat event onto the vat's own input-queue, and perform the teardown later. Sort of "please shut me down, eventually, when it's convenient". Then vat-manager or the translation layer doesn't need to remember anything. And if our scheduler's policy always drains the vat-output-queue before looking at the vat-input-queue, then this would automatically deliver all the previously-emitted work before shutdown, which might line up nicely with our plans around involuntary termination needing to not interfere with previous output, and might simplify the "vat is dead but queues are not" question.

I don't know enough about our use cases for this to know whether promptness is important to us. Maybe it'd be when a contract has finished transferring all its assets and obligations to a successor, and it self-destructs to free up chain capacity, and to prevent surprises when people try to talk to the old one by mistake? Seems like the vat would know that it's in this state, so it could maybe reject all attempts to start new work while waiting to be terminated. OTOH, it would be really annoying to have to add extra "am I active" checks in vat code to make up for the platform not being able to respond to a syscall.exit promptly. We might work around this by configuring the input scheduler to service terminate-vat events ahead of anything else (but still behind draining the output queue).

mhofman · 2022-03-03T16:36:06Z

packages/SwingSet/src/types-internal.js

@@ -0,0 +1,17 @@
+import './types.js';
+import './types-exported.js';


I'm surprised we still need the exported one.

mhofman · 2022-03-03T16:38:28Z

packages/SwingSet/src/types-internal.js

+ * // used by vatKeeper.setSourceAndOptions(source, RecordedVatOptions)
+ *
+ * @typedef { DynamicVatOptionsWithoutMeter & HasMeterID } InternalDynamicVatOptions
+ * @typedef { StaticVatOptions | { InternalDynamicVatOptions & HasMeterID } } RecordedVatOptions


I'm confused why this doesn't require a import('./types-exported.js').StaticVatOptions

This moves some logic out of `deliverAndLogToVal` and into a higher-level dispatch function, where it can react to all failure modes in a single place. We pull "run-queue events" off the run-queue (currently named the "acceptance queue". Many of these events are aimed at a specific vat, including 'notify', but the primary one is 'send' and is aimed at a *target* kref, which is either a kernel object or a kernel promise. For these we use `routeSendEvent` to either queue the event on a promise queue, reject it if it can't be delivered, or deliver it to a specific vat. This is the part that will change with #3465: in that world, each vat-input-queue exists for a specific vat, so the routing step moves earlier (into a "routing crank" that pulls events from a vat-output-queue and possibly adds them to a vat-input-queue). Some kinds of deliveries care about metering (or don't), and some have opinions about what should happen if a crank gets unwound. These are now reported as additional properties in the DeliveryStatus object that is returned by this delivery path. `processDeliveryMessage` looks at these properties, plus the delivery status (if any) to decide what to do at the end of the crank. I think most of the behavior should be the same as before. One change is that the `runPolicy` will probably get more information about non-'send' cranks than before (e.g. `create-vat` might report metering). This refactoring should make it easier to implement #1848 vat-upgrade, as well as #3465 queueing changes. closes #4687

Add "re-exporting" ambient file for backwards compatibility

warner added the SwingSet package: SwingSet label Mar 2, 2022

warner self-assigned this Mar 2, 2022

warner force-pushed the 1848-refactor-deliver-to-vat branch from 13aaf15 to b7fd9a6 Compare March 2, 2022 20:19

warner marked this pull request as ready for review March 2, 2022 20:19

warner requested review from FUDCo and mhofman March 2, 2022 20:19

FUDCo reviewed Mar 2, 2022

View reviewed changes

warner force-pushed the 1848-refactor-deliver-to-vat branch from b7fd9a6 to f0f5f0d Compare March 2, 2022 20:50

FUDCo approved these changes Mar 2, 2022

View reviewed changes

warner force-pushed the 1848-refactor-deliver-to-vat branch from f0f5f0d to 126acf2 Compare March 2, 2022 21:29

mhofman requested changes Mar 2, 2022

View reviewed changes

warner force-pushed the 1848-refactor-deliver-to-vat branch from 56fb177 to a946352 Compare March 3, 2022 00:43

warner requested a review from mhofman March 3, 2022 01:03

mhofman approved these changes Mar 3, 2022

View reviewed changes

warner and others added 2 commits March 3, 2022 13:35

refactor(SwingSet): Define SwingSet types as module exported

f62335a

Add "re-exporting" ambient file for backwards compatibility

warner force-pushed the 1848-refactor-deliver-to-vat branch from 996ad6b to f62335a Compare March 3, 2022 22:11

warner added the automerge:rebase Automatically rebase updates, then merge label Mar 3, 2022

mergify bot merged commit 57abfe3 into master Mar 3, 2022

mergify bot deleted the 1848-refactor-deliver-to-vat branch March 3, 2022 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(swingset): improve vat deliveries #4716

refactor(swingset): improve vat deliveries #4716

warner commented Mar 2, 2022 •

edited

Loading

warner commented Mar 2, 2022

FUDCo Mar 2, 2022

warner Mar 2, 2022

mhofman Mar 2, 2022

warner Mar 2, 2022

mhofman Mar 2, 2022

FUDCo left a comment

mhofman left a comment

mhofman Mar 2, 2022

mhofman Mar 2, 2022

warner Mar 2, 2022

mhofman Mar 2, 2022

warner Mar 3, 2022

mhofman Mar 2, 2022

warner Mar 2, 2022

mhofman Mar 3, 2022

warner Mar 3, 2022

mhofman Mar 3, 2022

mhofman Mar 3, 2022

		// `vatFatalSyscall`. We'd still need a way for `requestTermination` to
		// work, though.

		@@ -0,0 +1,17 @@
		import './types.js';
		import './types-exported.js';

refactor(swingset): improve vat deliveries #4716

refactor(swingset): improve vat deliveries #4716

Conversation

warner commented Mar 2, 2022 • edited Loading

warner commented Mar 2, 2022

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

mhofman 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

warner commented Mar 2, 2022 •

edited

Loading