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

prototype XS-based VatWorker process #1407

Merged
merged 20 commits into from
Aug 25, 2020
Merged

Conversation

dckc
Copy link
Member

@dckc dckc commented Aug 9, 2020

needs rebase and the CI step below, but I would like to start getting feedback from @warner @michaelfig etc.

ref #1299 (comment)

  • we're adding a new package to the monorepo, xs-vat-worker
  • this package will incorporate an agoric-labs/moddable tree via git submodule
  • this package will have a special yarn build-something (i.e. build:xs-lin) command that:
    • builds the XS toolset
    • builds an xs-vat-worker executable, which ...
    • this package will also export a JS library ... to provide the absolute pathname of the generated xs-vat-worker executable (or undefined if it doesn't exist yet, because yarn build:xs-lin wasn't run)
      • note: rather than use require.resolve, the pathname is generated at build time; using a post-install script in the not-built case, or by xs-lin.mk by way of yarn build:xs-lin
    • this package might instead/also have an API that encapsulates spawning the xs-vat-worker executable and running the messages-over-pipes protocol. nope; just re-used SwingSet/src/spawnSubprocessWorker.js code with slight modification to parameterize executable and args
  • SwingSet will acquire a new managerType, maybe named xs-worker. If any vats are configured to use it, but xs-vat-worker does not have an executable to offer, swingset will throw an error.
  • SwingSet unit tests will skip if the xs-vat-worker is missing.
    • A full CI run will yarn build-something first, to get coverage of the XS worker.
    • Regular developers should not (yet) have to wait for XS to build. We'll get it working manually, and then under CI, and then finally look at how long the XS build takes (and what additional tools must be installed first, maybe GIO? gcc?) before imposing the build-time burden on other developers
  • There will be a dependency cycle that's slightly awkward: SwingSet will depend on xs-vat-worker (for the library that spawns the worker executable), but xs-vat-worker will depend on the liveSlots.js that lives in swingset.
    • We might eventually choose to fix this by moving liveslots out to its own package, so both swingset and liveslots can depend upon it instead of each other.

@dckc
Copy link
Member Author

dckc commented Aug 25, 2020

re "SwingSet unit tests will skip if the xs-vat-worker is missing.":

2020-08-25T01:43:34.5983792Z XS vat worker not built; skipping

@warner warner self-requested a review August 25, 2020 04:03
Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome! minor changes to make.

For the conventional-commits check, just make all your commit comments start with chore: .

.gitignore Outdated Show resolved Hide resolved
packages/SwingSet/src/controller.js Outdated Show resolved Hide resolved
packages/SwingSet/test/workers/test-worker.js Outdated Show resolved Hide resolved
packages/xs-vat-worker/test/test-locate.js Outdated Show resolved Hide resolved
packages/xs-vat-worker/package.json Outdated Show resolved Hide resolved
packages/xs-vat-worker/xs-lin.mk Show resolved Hide resolved
packages/xs-vat-worker/xs-lin.mk Outdated Show resolved Hide resolved
packages/xs-vat-worker/tools/findmods.js Show resolved Hide resolved
packages/xs-vat-worker/start-xs.js Show resolved Hide resolved
packages/xs-vat-worker/src/vatWorker.js Show resolved Hide resolved
@dckc
Copy link
Member Author

dckc commented Aug 25, 2020

For the conventional-commits check, just make all your commit comments start with chore: .

really? Wouldn't that render chore: meaningless? Shouldn't I mark the refactor commits as such? And there's a feature in here somewhere, no? (with its own tweet :) I'd rather not change the commit comments at all than just slap chore: on them. conventional-commits isn't required yet, is it?

@warner
Copy link
Member

warner commented Aug 25, 2020

For the conventional-commits check, just make all your commit comments start with chore: .

really? Wouldn't that render chore: meaningless? Shouldn't I mark the refactor commits as such? And there's a feature in here somewhere, no? (with its own tweet :) I'd rather not change the commit comments at all than just slap chore: on them. conventional-commits isn't required yet, is it?

eh, good point. The main value of conventional-commits is to inform the correct version bump to make on the next release (breaking changes, so the major version should be bumped? new features, so the minor version should be bumped?). So there's not a lot of practical value to annotating the brand new package. For the existing package (swingset), where we're adding a new feature (the new kind of worker), we should probably label at least one of those commits with feat:.

Conventional-commits aren't required, no. I'm in the habit of making my commits appease that CI check (maybe just out of embarrassment), but it's not gating the merge.

@warner
Copy link
Member

warner commented Aug 25, 2020

BTW once the comments above are addressed, we can land this, and then add the CI run that exercises it in a later PR.

@michaelfig
Copy link
Member

really? Wouldn't that render chore: meaningless? Shouldn't I mark the refactor commits as such? And there's a feature in here somewhere, no? (with its own tweet :) I'd rather not change the commit comments at all than just slap chore: on them. conventional-commits isn't required yet, is it?

I like this approach. Just use the tags that you understand.

eh, good point. The main value of conventional-commits is to inform the correct version bump to make on the next release (breaking changes, so the major version should be bumped? new features, so the minor version should be bumped?).

I'd prefer not to reverse-engineer the release process, but to actually use the semantics that conventional-commits are supposed to have. Only use "chore" for things that are strictly mechanical changes that aren't "fix"es or "refactor"s.

Conventional-commits aren't required, no. I'm in the habit of making my commits appease that CI check (maybe just out of embarrassment),

heh. I find it more embarrassing to use "chore" where it's not correct. 😉

Right, they're only warned upon a PR branch, they won't fail for CI on the master branch itself.

vatWorker runs a vat in its own process. We have drivers for both node
and xs, though only the node driver is working as of this commit.

We also have a kernelSimulator that takes a vat transcript
(as from swingset-runner/bin/kerneldump) and simulates a swingset
kernel vat manager.

Earlier vatWorker work was done in a gist:
https://gist.github.com/dckc/f8e0b5d838079a994784d599c282cce7

This commit is based on:
2020-07-16 b17fae8
thru
2020-08-02 a32f30e simple transcript runs as integration test

The xs driver was working, to some extent, in:
2020-08-01 5b80235 agoric-sdk catch-up

See also
Agoric#1299
  - tools/findmods.js uses npm tools to walk source tree and build
    compartmap with endo-style inter-package import info
    plus intra-package info
    - for now, we check in the generated compartmap.json
  - compartmap.json also includes a "modules" key, which is
    used by reference from the XS manifest.json
  - start-xs sets up SES compartments with harden etc.
    - note harden SES issue 104
    - src/endo-load.js does endo-style compartment-per-package
      loading

solve xs compartmap constraints:

  - remove .js from "modules" section of xs manifest
  - remove .js when building compartment map from endo manifest

Note as of Jun 5 62c7f1e xs's Compartment constructor needed tweaking,
but no longer as of Aug 12 4842c4.
catching up with agoric-sdk's use of promise-kit in place of
produce-promise eliminated a large swath of dependent code.

SIGSEGV came from some interaction of Compartments and @agoric/make-hardener ?
dckc added 15 commits August 25, 2020 16:46
 - add moddable SDK git submodule
   - makefile stanza to document some git submodule usage
 - avoid much of x11 for moddable by using only libgio, not all of
   libgtk
 - ignore xs build dir
 - makefile stanza to document compartmap rebuilding
If any vats are configured to use it, but xs-vat-worker does not have
an executable to offer, swingset will throw an error. (IOU a test?)

 - Swingset package depends on xs-vat-worker
   - ISSUE: cycle. factor out liveslots?
 - add startXsWorker kernel endowment,
   which is undefined unless binary is built
 - parameterize execPath, args in startSubprocessWorker()
   (mantain previous defaults)
 - xs-vat-worker: adopt array style protocol from SwingSet (WIP)
   - based on subprocessSupervisor.js
     copied SwingSet/src/kernel/vatManager/subprocessSupervisor.js
     wholesale; applied ocap discipline to module-level mutable state;
     adapted IO interface. Compare:

Initially it would hang after the first 2 syscalls of the first
deliver:

 - [ 'subscribe', 'p-60' ]
 - [ 'subscribe', 'p-61' ]

Then I fixed handle() to return a promise in all cases to be sure to
finish delivery before next blocking read.
startSubprocessWorker was a capability provided to the kernel; adding
an execPath argument gave the kernel the ability to launch arbitrary
executables, which is well beyond the least authority it needs.

Instead, provide startSubprocessWorkerNode and startSubprocessWorkerXS
powers, each of which can launch a specific executable.

ack: warner
@dckc
Copy link
Member Author

dckc commented Aug 25, 2020

@warner 29406ad is the same as the code you reviewed earlier, 02209c6 (38f3340 with yarn.lock), modulo changes in master (and one .gitignore thing that I squashed).

The commits since then address your feedback.

Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get rid of that last tap-spec dependency, and then we're good to land.

packages/xs-vat-worker/package.json Outdated Show resolved Hide resolved
@warner warner merged commit 5ceb471 into Agoric:master Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants