feat(swingset): xsnap vat worker #2225

dckc · 2021-01-21T00:06:00Z

it passes test-worker.js

fixes #1299
fixes #2202
fixes #45
fixes #2216

clean up git history
rename startSubprocessWorkerXS etc. as suggested in refactor(SwingSet): unify vat-worker filenames #2204 (comment) ?
move what's left of the xs-vat-worker package into xsnap and SwingSet?
~~back out addition of setImmediate?~~ (see also detecting an empty vat promise queue (end of "crank") #45)

depends on / includes:

dckc · 2021-01-21T00:13:01Z

The xs-vat-worker package has become largely vestigial; the part that's still used is a couple layers of rollup'd bootstrap code. Perhaps I should move that somewhere else and prune the xs-vat-worker package?

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

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

kriskowal

This is looking like great progress and has my approval for the parts relevant to where I’ve touched. My feedback is substantially nits about naming and can be safely punted or ignored. I defer to @warner for a proper review of the kernel material.

packages/SwingSet/src/controller.js

kriskowal · 2021-01-21T06:37:54Z

packages/SwingSet/src/controller.js

-  if (fs.existsSync(xsWorkerBin)) {
-    startSubprocessWorkerXS = () => startSubprocessWorker(xsWorkerBin);
-  }
+  const { xsnapBundles = await buildXsBundles() } = {}; // @@@ options?


Seems like a fine idea to thread runtimeOptions here, but I defer to @warner. Please do iron this out before landing and thanks for leaving an evocative marker!

Elsewhere we've got facilities for generating the bundles once (e.g. during a unit test which will call makeSwingsetController a dozen times), and passing them in. We should put the XS bundles into that same set, rather than forcing them to be built on every call. (bundleSource seems to take 0.5s-2.0s depending upon how much code is getting included, and I don't think it parallelized well, so it's important for tests to amortize its execution)

I'm -0 on the utility of being able to override some or all of the bundles as a runtimeOption, I think we're only likely to do that when we're developing the supervisor or lockdown code, and during such an effort we'll be re-running the whole process anyways. As long as we can amortize the bundling time, I don't currently care about passing any other options in here.

@warner I'm unclear on what you want here. How would we "pass them in"? Or is this already doing The Right Thing?

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

kriskowal · 2021-01-21T07:03:48Z

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

+};
+
+function makeConsole(_tag) {
+  const log = console; // TODO: anylogger(tag);


Ah, “anylogger” must be the topical log idiom we’re using.

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

kriskowal · 2021-01-21T07:18:11Z

packages/SwingSet/src/types.js

+ *   enableSetup: true,
+ * }} HasSetup
+ *
+ * TODO: metered...


Would be good to replace this TODO with an issue link, and link the issue back to this file.

packages/xsnap/test/test-xsnap.js

warner

Looks pretty good, once the conflicts are resolved I'm good with this landing.

The nice-to-have items (which could perhaps live in a subsequent PR) are:

move the XS lockdown/supervisor bundles into buildKernelBundles() (in src/initializeSwingset.js`), so the time spent building them is amortized in our large unit tests (I expect this to save 10s of seconds in the overall test time)
have the worker return the "result" of a delivery in the same command that started it, rather than requiring a second "getResults" command and the sequence number that glues them together

warner · 2021-01-21T21:54:45Z

packages/SwingSet/src/controller.js

-  if (fs.existsSync(xsWorkerBin)) {
-    startSubprocessWorkerXS = () => startSubprocessWorker(xsWorkerBin);
-  }
+  const { xsnapBundles = await buildXsBundles() } = {}; // @@@ options?


Elsewhere we've got facilities for generating the bundles once (e.g. during a unit test which will call makeSwingsetController a dozen times), and passing them in. We should put the XS bundles into that same set, rather than forcing them to be built on every call. (bundleSource seems to take 0.5s-2.0s depending upon how much code is getting included, and I don't think it parallelized well, so it's important for tests to amortize its execution)

I'm -0 on the utility of being able to override some or all of the bundles as a runtimeOption, I think we're only likely to do that when we're developing the supervisor or lockdown code, and during such an effort we'll be re-running the whole process anyways. As long as we can amortize the bundling time, I don't currently care about passing any other options in here.

packages/SwingSet/src/controller.js

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

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

packages/xsnap/test/test-xsnap.js

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

kriskowal

Reviewed incrementally. I still see some work-in-progress and will plan to do another differential review.

packages/SwingSet/src/controller.js

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

+        `(${superCode.source}
+            )()`.trim(),


kriskowal · 2021-01-23T05:29:27Z

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

+    }
+
+    function shutdown() {
+      return worker.close();


@warner I’m open to changing xsnapWorker.close() to xsnapWorker.shutdown() for one less name-is-different/behavior-is-same red herring lingering among names.

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

kriskowal · 2021-01-23T06:07:09Z

packages/xsnap/src/xsnap.js

@@ -132,14 +136,14 @@ export function xsnap(options) {

  /**
   * @param {string} code
-   * @returns {Promise<void>}
+   * @returns {Promise<Uint8Array | void>}


This should be unconditionally Promise<Uint8Array> now. The caller should be able to rely on the fully defined type.

packages/xsnap/src/xsnap.js

packages/xsnap/test/test-boot-lockdown.js

packages/xsnap/test/test-xsnap.js

erights · 2021-01-23T20:41:59Z

packages/xsnap/lib/console-shim.js

+const console = {
+  debug: noop,
+  log: noop,
+  info: noop,
+  warn: noop,
+  error: noop,
+  // used by SES
+  group: noop,
+  groupCollapsed: noop,
+  groupEnd: noop,
+  // others from the MDN / whatwg Console API?
+  // trace, dirxml, ...
+};


This should use the whitelists at https://github.com/Agoric/SES-shim/blob/901153147d02bc395a0f392e482814b2ccbc54a9/packages/ses/src/error/console.js#L36-L71. Move the whitelist to a more reusable place if that would help.

Also, we should use our own VirtualConsole type from https://github.com/Agoric/SES-shim/blob/901153147d02bc395a0f392e482814b2ccbc54a9/packages/ses/src/error/types.js#L235 which wraps the TypeScript Console rather than just using it directly. The reason is that console is only a de facto standard for which that are multiple somewhat inconsistent codifications. By using our VirtualConsole wrapper, we're in more control of how this type changes over time.

I added "TODO: consider other methods per SES VirtualConsole" with a pointer to #2146

I'm struggling to use SES types at all, let alone VirtualConsole.

So I'd like to leave this out of scope of this PR.

I agree. I’m working on exporting SES types. Failing that, copies of interface types are compatible.

In addition to detecting XS Machine quiescense so we can safely take snapshots, the supervisor has to detect vat queiscense so it can tell when a delivery is done. I have resorted to ad-hoc `fprintf()` at the C level for debugging enough to justify restoring print. Here we test that it's only available in the start compartment. note print() includes fflush()

fixes Agoric#2216 only tested on lin, not mac nor win

Using the .result property of a mutable object rather than the resolution of a promise is a little awkward, but it seems to work.

fixes Agoric#2202

- build xsnap bootstrap bundles - bytes to tagged array and back - setBundle, importBundle - syscall - delivery success symbol is ok, not deliverDone - Use Tagged type consistently; don't constrain tag to be string. - clean up logging: use parentLog(), trace(), ... - static typing for doProcess: capture dispatch while it's known to be not null - silence parentLog, workerLog for xsnap - no, handleSyscall doesn't return Tagged - inherit stdout, stderr in xsnap - vatid arg on doNotify is no more

- manager: prune commandResult - supervisor: factor out "transport" logic as `ManagerPort`, separate from vat-worker `makeWorker()` - ManagerPort.handler provides `{ result?: ArrayBuffer }` idiom based on Promise<Tagged> - testLog uses ManagerPort.send - clean up redundant 'ok' tag in doMessage, doNotify - refactor: tagged -> item for consistency

- prune 'starting xsnap' log msg (per code review) - handle rejection in ManagerPort.handler

- prune obsolete locate.js

move lockdown-shim.js and the rest of the SES bootstrap files from src/ to lib/ to avoid many tsc errors of the form... ``` Error: ../../node_modules/ses/src/error/assert.js(24,20): error TS2304: Cannot find name 'StringablePayload'. ```

add TODO re other console methods with pointer to Agoric#2146

dckc · 2021-01-24T03:10:57Z

Looks pretty good, once the conflicts are resolved I'm good with this landing.

@warner I sure hope this is done now. These are both done:

The nice-to-have items (which could perhaps live in a subsequent PR) are:

move the XS lockdown/supervisor bundles into buildKernelBundles() (in src/initializeSwingset.js`),

done.

have the worker return the "result" of a delivery in the same command that started it, rather than requiring a second "getResults" command and the sequence number that glues them together

done.

kriskowal · 2021-01-24T03:27:48Z

packages/xsnap/package.json

    "clean": "rm -rf build",
    "lint": "yarn lint:js && yarn lint:types",
-    "lint:js": "eslint 'src/**/*.js'",
+    "lint:js": "eslint 'src/**/*.js' 'lib/**/*.js'",


What’s the deciding principle for what belongs in lib vs src?

lib is the SES lockdown bootstrap material that we feed into rollup.

It doesn't pass lint:types (tons of errors from SES assert that I don't begin to understand) so I moved it somewhere that tsc isn't looking.

1ebf699 move lockdown-shim out of src/ to avoid tsc errors

packages/xsnap/src/xsnap.c

dckc requested review from warner and kriskowal January 21, 2021 00:06

This comment has been minimized.

Sign in to view

dckc force-pushed the xs-vat-worker branch 4 times, most recently from 6b065db to 240ae15 Compare January 21, 2021 02:05

dckc mentioned this pull request Jan 21, 2021

refactor(SwingSet): unify vat-worker filenames #2204

Closed

dckc commented Jan 21, 2021

View reviewed changes

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

dckc commented Jan 21, 2021

View reviewed changes

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

dckc commented Jan 21, 2021

View reviewed changes

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

dckc marked this pull request as ready for review January 21, 2021 06:31

kriskowal reviewed Jan 21, 2021

View reviewed changes

dckc mentioned this pull request Jan 21, 2021

test(xsnap): Test xsnap close and terminate #2227

Merged

warner approved these changes Jan 22, 2021

View reviewed changes

dckc force-pushed the xs-vat-worker branch from 240ae15 to ad5ca74 Compare January 23, 2021 03:25

dckc requested a review from warner January 23, 2021 04:03

dckc force-pushed the xs-vat-worker branch from 7e69920 to 9ac2289 Compare January 23, 2021 04:56

dckc requested a review from kriskowal January 23, 2021 04:59

kriskowal reviewed Jan 23, 2021

View reviewed changes

dckc force-pushed the xs-vat-worker branch from 9ac2289 to 358a49e Compare January 23, 2021 17:32

dckc changed the title ~~xsnap vat worker~~ feat(swingset): xsnap vat worker Jan 23, 2021

dckc force-pushed the xs-vat-worker branch from 5538150 to e4c349e Compare January 23, 2021 18:22

This was referenced Jan 23, 2021

detecting an empty vat promise queue (end of "crank") #45

Closed

console API on XS: compatibility, security impact? #2146

Closed

dckc force-pushed the xs-vat-worker branch from e4c349e to ad730d2 Compare January 23, 2021 18:46

erights reviewed Jan 23, 2021

View reviewed changes

dckc added 21 commits January 23, 2021 19:41

build(xsnap): don't set mxDebug in release builds

48d9b8b

fixes Agoric#2216 only tested on lin, not mac nor win

build(xsnap): build GOAL=debug too

6aefa2f

fix(xsnap): don't swallow error message

7caa380

feat(xsnap): return data from xsnap.evaluate()

19ad452

Using the .result property of a mutable object rather than the resolution of a promise is a little awkward, but it seems to work.

chore(xs-vat-worker): prune obsolete dependencies

7119d81

build(xs-vat-worker): moddable submodule is obsolete

98d0b2c

style(xs-vat-worker): use canonical @Agoric style

9ba6e65

feat(xs-vat-worker): TextDecoder, HandledPromise before lockdown

51a30cd

refactor(SwingSet): unify vat-worker filenames

69d08c7

fixes Agoric#2202

fix: crank 1 comment in vat-target.js

a19da62

fix: supply groupCollapsed etc. in console-shim for SES

c16d6ce

fix(xsnap): handle edge cases in sending replies to e, ?

454362d

feat(xsnap worker): pass console log messages to manager

58dfb17

- prune 'starting xsnap' log msg (per code review) - handle rejection in ManagerPort.handler

fix(xsnap): build args

7df4d3b

refactor(xsnap): fold in what's left of xs-vat-worker

f1b2000

- prune obsolete locate.js

docs(xsnap): document XS handleCommand async idiom

aab7d0d

refactor: build XS bundles with Kernel bundles

701e2f4

fix(xsnap worker): update syscall API to use .resolve()

d200428

dckc force-pushed the xs-vat-worker branch from ad730d2 to d200428 Compare January 24, 2021 02:08

chore(xsnap): provide non-trivial console in start compartment

37a658c

add TODO re other console methods with pointer to Agoric#2146

kriskowal approved these changes Jan 24, 2021

View reviewed changes

dckc merged commit 50c8548 into Agoric:master Jan 24, 2021

This was referenced Jan 25, 2021

how to get to swingset-on-XS #2107

Closed

integrate XS into SES-shim ci build / test endojs/endo#508

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(swingset): xsnap vat worker #2225

feat(swingset): xsnap vat worker #2225

dckc commented Jan 21, 2021 •

edited

Loading

dckc commented Jan 21, 2021

This comment has been minimized.

This comment has been minimized.

kriskowal left a comment

kriskowal Jan 21, 2021

warner Jan 21, 2021

dckc Jan 23, 2021

kriskowal Jan 21, 2021

kriskowal Jan 21, 2021

warner left a comment

warner Jan 21, 2021

kriskowal left a comment

This comment was marked as outdated.

kriskowal Jan 23, 2021

kriskowal Jan 23, 2021

erights Jan 23, 2021

erights Jan 23, 2021

dckc Jan 24, 2021

kriskowal Jan 24, 2021

dckc commented Jan 24, 2021

kriskowal Jan 24, 2021

dckc Jan 24, 2021

feat(swingset): xsnap vat worker #2225

feat(swingset): xsnap vat worker #2225

Conversation

dckc commented Jan 21, 2021 • edited Loading

dckc commented Jan 21, 2021

This comment has been minimized.

This comment has been minimized.

kriskowal 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

warner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

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

dckc commented Jan 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc commented Jan 21, 2021 •

edited

Loading