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

feat(swingset): Add Node.js Worker (thread) -based VatManager #1386

Merged
merged 1 commit into from
Aug 7, 2020

Conversation

warner
Copy link
Member

@warner warner commented Aug 7, 2020

This adds a per-vat option to run the vat code in a separate thread, sharing
the process with the main (kernel) thread, sending VatDelivery and VatSyscall
objects over the postMessage channel. This isn't particularly useful by
itself, but it establishes the protocol for running vats in a
separate process, possibly written in a different language or using a
different JS engine (like XS, in #1299).

This 'nodeWorker' managertype has several limitations. The shallow ones are:

  • vatPowers is missing transformTildot, which shouldn't be hard to add
  • vatPowers.testLog is missing, only used for unit tests so we can probably
    live without it
  • vatPowers is missing makeGetMeter/transformMetering (and will probably
    never get them, since they're only used for within-vat metering and we're
    trying to get rid of that)
  • metering is not implemented at all
  • delivery transcripts (and replay) are not yet implemented

Metering shouldn't be too hard to add, although we'll probably make it an
option, to avoid paying the instrumented-globals penalty when we aren't using
it. We also need to add proper control over vat termination (via meter
exhaustion or manually).

The deeper limitation is that nodeWorkers cannot block to wait for a
syscall (like callNow), so they cannot invoke devices.

refs #1127
closes #1384

@warner warner added the SwingSet package: SwingSet label Aug 7, 2020
@warner warner requested a review from FUDCo August 7, 2020 03:45
@warner warner self-assigned this Aug 7, 2020
@warner warner force-pushed the 1127-vat-manager-factory branch from 4b5c636 to 5cd4cde Compare August 7, 2020 03:56
@warner warner force-pushed the 1384-vatworker-thread branch from e6bebac to 0d20f8c Compare August 7, 2020 03:56
Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

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

This looks mostly fine so I'm approving it, but I'm a little concerned about having Node-specific stuff find its way into the kernel. I understand this is in service of the ultimate goal of pulling host-specific things into separable pieces with their own APIs, so we can do things like support XS too, but in the meantime I'm wondering if it's a good idea to have all this in one package. I suspect this is one of those "we'll know where to draw the cut lines once we have a couple of disparate examples to look at" things, so having it all in SwingSet is probably OK for now, but figuring out how to factor out the host-specific bits should be explicitly on our roadmap.

@warner
Copy link
Member Author

warner commented Aug 7, 2020

Yeah. On one hand, the kernel shouldn't know much about the platform, and should get its authorities to launch new workers (whether they're in shared-thread Compartments, separate threads, or separate processes) from the controller. On the other, the kernel is speaking one half of a protocol that must be matched by the worker half, and putting both sides of that protocol in the same directory is the best way to keep them aligned (and the controller shouldn't have to know the details of that protocol).

We should also consider the code that is used by the worker: liveslots, and a couple of non-kernel-specific libraries, including the waitForQuiescent function. It's a bit weird to have liveslots nominally live in the kernel but only actually get used by workers. For the main-thread ("local") worker, it makes sense to give the worker a shared function reference rather than incorporating liveSlots.js into every vat bundle, but it means liveslots sits on this border between kernel and worker in a weird way. The answer might be to move liveslots out to a separate package.

@warner warner force-pushed the 1127-vat-manager-factory branch from 5cd4cde to fb52977 Compare August 7, 2020 19:00
@warner warner force-pushed the 1384-vatworker-thread branch from 0d20f8c to 6e37904 Compare August 7, 2020 19:00
This adds a per-vat option to run the vat code in a separate thread, sharing
the process with the main (kernel) thread, sending VatDelivery and VatSyscall
objects over the postMessage channel. This isn't particularly useful by
itself, but it establishes the protocol for running vats in a
separate *process*, possibly written in a different language or using a
different JS engine (like XS, in #1299).

This 'nodeWorker' managertype has several limitations. The shallow ones are:

* vatPowers is missing transformTildot, which shouldn't be hard to add
* vatPowers.testLog is missing, only used for unit tests so we can probably
live without it
* vatPowers is missing makeGetMeter/transformMetering (and will probably
never get them, since they're only used for within-vat metering and we're
trying to get rid of that)
* metering is not implemented at all
* delivery transcripts (and replay) are not yet implemented

Metering shouldn't be too hard to add, although we'll probably make it an
option, to avoid paying the instrumented-globals penalty when we aren't using
it. We also need to add proper control over vat termination (via meter
exhaustion or manually).

The deeper limitation is that nodeWorkers cannot block to wait for a
syscall (like `callNow`), so they cannot invoke devices.

refs #1127
closes #1384
@warner warner force-pushed the 1127-vat-manager-factory branch from fb52977 to 8027720 Compare August 7, 2020 19:45
@warner warner force-pushed the 1384-vatworker-thread branch from 6e37904 to 61615a2 Compare August 7, 2020 19:45
Base automatically changed from 1127-vat-manager-factory to master August 7, 2020 20:01
@warner warner merged commit 61615a2 into master Aug 7, 2020
@warner warner deleted the 1384-vatworker-thread branch August 7, 2020 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants