-
Notifications
You must be signed in to change notification settings - Fork 212
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
feat(swingset): xsnap vat worker #2225
Conversation
The |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
6b065db
to
240ae15
Compare
packages/SwingSet/src/kernel/vatManager/manager-subprocess-xsnap.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
if (fs.existsSync(xsWorkerBin)) { | ||
startSubprocessWorkerXS = () => startSubprocessWorker(xsWorkerBin); | ||
} | ||
const { xsnapBundles = await buildXsBundles() } = {}; // @@@ options? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
Outdated
Show resolved
Hide resolved
}; | ||
|
||
function makeConsole(_tag) { | ||
const log = console; // TODO: anylogger(tag); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, “anylogger” must be the topical log idiom we’re using.
packages/SwingSet/src/kernel/vatManager/supervisor-subprocess-xsnap.js
Outdated
Show resolved
Hide resolved
* enableSetup: true, | ||
* }} HasSetup | ||
* | ||
* TODO: metered... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to replace this TODO with an issue link, and link the issue back to this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
packages/SwingSet/src/controller.js
Outdated
if (fs.existsSync(xsWorkerBin)) { | ||
startSubprocessWorkerXS = () => startSubprocessWorker(xsWorkerBin); | ||
} | ||
const { xsnapBundles = await buildXsBundles() } = {}; // @@@ options? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/kernel/vatManager/supervisor-subprocess-xsnap.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed incrementally. I still see some work-in-progress and will plan to do another differential review.
`(${superCode.source} | ||
)()`.trim(), |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
} | ||
|
||
function shutdown() { | ||
return worker.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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/xsnap/src/xsnap.js
Outdated
@@ -132,14 +136,14 @@ export function xsnap(options) { | |||
|
|||
/** | |||
* @param {string} code | |||
* @returns {Promise<void>} | |||
* @returns {Promise<Uint8Array | void>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be unconditionally Promise<Uint8Array>
now. The caller should be able to rely on the fully defined type.
packages/xsnap/lib/console-shim.js
Outdated
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, ... | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
- 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
@warner I sure hope this is done now. These are both done:
done.
done. |
"clean": "rm -rf build", | ||
"lint": "yarn lint:js && yarn lint:types", | ||
"lint:js": "eslint 'src/**/*.js'", | ||
"lint:js": "eslint 'src/**/*.js' 'lib/**/*.js'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What’s the deciding principle for what belongs in lib vs src?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
it passes
test-worker.js
fixes #1299
fixes #2202
fixes #45
fixes #2216
startSubprocessWorkerXS
etc. as suggested in refactor(SwingSet): unify vat-worker filenames #2204 (comment) ?xs-vat-worker
package intoxsnap
andSwingSet
?back out addition of(see also detecting an empty vat promise queue (end of "crank") #45)setImmediate
?depends on / includes: