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

fix(swingset): remove unused worker types: node-subprocess, nodeworker #6890

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

warner
Copy link
Member

@warner warner commented Jan 30, 2023

Most of our vat workers use xs-worker, which runs the code in an xsnap subprocess. A handful of vats (maybe just comms) run in a local worker, which is co-resident with the kernel.

We have two additional worker types, nodeWorker (which uses threads, known as "workers" in Node.js), and node-subprocess (which runs a second copy of Node.js in a subprocess). I added these for completeness, and to keep us honest about handling different worker types. But they are incomplete (node-subprocess could block for a syscall, but I didn't implement that),
underfeatured (nodeWorker cannot block for a syscall without heroics involving ShareArrayBuffers and Atomics), undertested (the previous lacking features prompted us to test.skip them), and unused by anything outside of our test suite.

I have refactoring work in the pipeline that will be easier if there is less code to move around. So it's time for these to go. I might try to bring these back some day, but only when we have some interesting use case for them (which is more likely to be inside a browser, than under Node.js).

closes #4884

@warner warner added the SwingSet package: SwingSet label Jan 30, 2023
@warner warner requested a review from FUDCo January 30, 2023 19:26
@warner warner self-assigned this Jan 30, 2023
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.

Yes, please.

@warner warner added the automerge:rebase Automatically rebase updates, then merge label Jan 30, 2023
Most of our vat workers use `xs-worker`, which runs the code in an
`xsnap` subprocess. A handful of vats (maybe just comms) run in a
`local` worker, which is co-resident with the kernel.

We have two additional worker types, `nodeWorker` (which uses threads,
known as "workers" in Node.js), and `node-subprocess` (which runs a
second copy of Node.js in a subprocess). I added these for
completeness, and to keep us honest about handling different worker
types. But they are incomplete (node-subprocess *could* block for a
syscall, but I didn't implement that),
underfeatured (nodeWorker *cannot* block for a syscall without heroics
involving ShareArrayBuffers and Atomics), undertested (the previous
lacking features prompted us to `test.skip` them), and unused by
anything outside of our test suite.

I have refactoring work in the pipeline that will be easier if there
is less code to move around. So it's time for these to go. I might try
to bring these back some day, but only when we have some interesting
use case for them (which is more likely to be inside a browser, than
under Node.js).

closes #4884
@warner warner force-pushed the 4884-remove-unused-workers branch from dd39390 to bc0722f Compare January 30, 2023 22:19
@mergify mergify bot merged commit 64a3b63 into master Jan 30, 2023
@mergify mergify bot deleted the 4884-remove-unused-workers branch January 30, 2023 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node-worker/subprocess-node workers cannot do vatstore or baggage: remove?
2 participants