Do Not Merge: Move kernel to dedicated worker #123

grypez · 2024-09-30T22:02:35Z

Closes: #57
Refs: #80

Todo:

FUDCo

Not done looking at this, but after making three or four comments I realized that since it's a big PR it would be better to issue these comments as they come instead of letting the pile up.

packages/extension/src/kernel-worker.ts

FUDCo

For some reason GitHub spontaneously put me into review mode when I tried to post an individual comment.

FUDCo · 2024-10-01T21:57:03Z

packages/extension/src/kernel-worker.ts

+      case CommandMethod.LaunchVat:
+        await kernel
+          .launchVat({
+            id: params,
+            port: command.ports[0],
+          })
+          .then((vat) => params === defaultVatId && resolveDefaultVat(vat));
+        break;


Further thought: my comments above notwithstanding, I don't think the port should be parameter to LaunchVat in this way at all. The kernel should never even see a port (quite possibly this is already on your roadmap). Instead (1) the communications pathway to the vat should already be whatever our general abstraction for a communications pathway is (a stream, I think? Or a stream pair? I'm still a little fuzzy on the unidirectional vs. bidirectional affordances of the stream abstraction), and (2) the kernel ought to be in the driver's seat, in that the invoker of LaunchVat should not be assumed to be the provider of the run-time container (iframe, process, whatever) that the vat will run in, but that should be separately provisioned for the kernel in response to a service call that the kernel makes; the entity that provides the kernel with this service should be provisioned at kernel startup time as an initialization parameter by whoever creates the kernel in the first place. This way the we decouple the Kernel class itself from the fact that in the base use case it runs in a web browser. In the standalone server use case, a vat process may get launched by an entirely different entity from the entity that launched the kernel itself.

I agree with both thoughts you've laid out and hope to see the kernel there soon. I think a clean worker provisioning service falls under #99, though to the best of my knowledge it will need to implement something similar to what I have here to set up both ends of the channel. To (2), in this PR the background does not have access to launchVat, rather createVat, and the provision of the worker is handled via communications between the glue and the kernel. But this should definitely go away in the future. To (1), I think our ideal path forward involves reading and writing messages over a stream, but since we cannot pass a stream over *.postMessage or a message port over a (generic) stream I am willing to be a little in the ditch in this PR as long as we're getting closer to our destination.

Yes. This PR doesn't have to be the final word. I just wanted to make sure we were pointed in the same direction, which it sounds like we are.

FUDCo · 2024-10-01T22:05:10Z

packages/extension/src/kernel-worker.ts

+    const { method, params } = systemCall;
+    console.log('received message: ', method, params);
+
+    switch (method) {


This may be a good spot to point out something that's been bothering me for a while about how we have defined the various message types: I don't think we should be dispatching to reply handlers based on method, but rather on the specific request that they are the reply to, and probably used to resolve a promise from the original message send operation that issued the request in the first place.

FUDCo · 2024-10-01T23:15:09Z

packages/extension/src/kernel-worker.ts

+    const command = port
+      ? { params: paramsArg, method: methodArg, ports: [port] }
+      : { params: paramsArg, method: methodArg };
+
+    if (!isKernelCommand(command)) {
+      console.error('invalid command', command);
      return;
    }

-    const { method, params } = event.data;
-    console.log('received message: ', method, params);
+    const { method, params } = command as KernelCommand;


Why build up the command object only to destructure it again 10 lines later? It seems like its only utility is to have a thing to pass to isKernelCommand, which is already a bit of a mess. At this point I don't think it adds value as a separate function in a separate module. RIght here the default case of the switch checks method and the various method cases should validate their args themselves so that all the logic w.r.t a given command is in just one place.

In principle the destructuring enforces that { method, params } is on the diagonal of the product type matrix, which makes the type inference work properly in the switch-case blocks.

This sounds like the tail wagging the dog. Probably worth a deeper conversation.

… throwing.

…orker.

rekmarks · 2024-10-11T05:44:41Z

All components of this have been merged.

grypez force-pushed the grypez/offscreen-doc branch 4 times, most recently from 17bf8ef to ed408be Compare October 1, 2024 19:29

grypez changed the title ~~(draft) offscreen doc~~ (draft) Move kernel to dedicated worker Oct 1, 2024

grypez force-pushed the grypez/offscreen-doc branch 2 times, most recently from c68bf18 to c5dce79 Compare October 1, 2024 20:08

FUDCo reviewed Oct 1, 2024

View reviewed changes

packages/extension/src/kernel-worker.ts Outdated Show resolved Hide resolved

packages/extension/src/kernel-worker.ts Outdated Show resolved Hide resolved

packages/extension/src/kernel-worker.ts Outdated Show resolved Hide resolved

grypez force-pushed the grypez/offscreen-doc branch from c5dce79 to 13ccd91 Compare October 1, 2024 21:14

FUDCo reviewed Oct 1, 2024

View reviewed changes

grypez force-pushed the grypez/offscreen-doc branch from 13ccd91 to 329cfd7 Compare October 1, 2024 22:57

grypez changed the base branch from main to grypez/structured-types October 1, 2024 22:57

FUDCo reviewed Oct 1, 2024

View reviewed changes

grypez force-pushed the grypez/offscreen-doc branch 3 times, most recently from faa3c1f to f2ce5e6 Compare October 3, 2024 18:24

grypez force-pushed the grypez/structured-types branch from e1256d5 to 54a0c2b Compare October 3, 2024 18:25

grypez force-pushed the grypez/offscreen-doc branch 2 times, most recently from 79d13d9 to 09469c3 Compare October 3, 2024 18:29

grypez force-pushed the grypez/structured-types branch from 54a0c2b to 0d2d329 Compare October 3, 2024 18:44

Base automatically changed from grypez/structured-types to main October 3, 2024 18:47

grypez force-pushed the grypez/offscreen-doc branch from 09469c3 to 71fcb90 Compare October 4, 2024 00:25

grypez mentioned this pull request Oct 4, 2024

feat(kernel): Differentiate cluster, kernel and vat command types. #130

Merged

grypez force-pushed the grypez/offscreen-doc branch from 71fcb90 to 01d8684 Compare October 4, 2024 07:32

rekmarks changed the title ~~(draft) Move kernel to dedicated worker~~ Do Not Merge: Move kernel to dedicated worker Oct 8, 2024

grypez added 4 commits October 9, 2024 20:06

feat(kernel): Add InitKernel command.

73a1d44

feat(extension): Use init kernel command.

ebf0413

refactor(extension): Extract kernel types from kernel-worker.

f76f58a

refactor(extension): Move kernel into kernel-worker.

ac7097c

grypez added 3 commits October 9, 2024 20:39

feat(streams): Make base stream ignore unexpected messages instead of…

ad7e5e7

… throwing.

refactor(extension): Extract kernel store functionality from kernel-w…

618db8d

…orker.

feat(kernel): Add a KernelStore interface.

d216295

grypez force-pushed the grypez/offscreen-doc branch 3 times, most recently from f79d526 to 02999f3 Compare October 10, 2024 03:17

refactor(extension,kernel): Move kernel message handling to Kernel.

cb2391c

grypez force-pushed the grypez/offscreen-doc branch from 02999f3 to cb2391c Compare October 10, 2024 03:21

grypez mentioned this pull request Oct 10, 2024

feat(streams): Generalize message channel to abstract execution contexts #140

Merged

rekmarks closed this Oct 11, 2024

rekmarks deleted the grypez/offscreen-doc branch October 11, 2024 05:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do Not Merge: Move kernel to dedicated worker #123

Do Not Merge: Move kernel to dedicated worker #123

Uh oh!

grypez commented Sep 30, 2024 •

edited by rekmarks

Loading

Uh oh!

FUDCo left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FUDCo left a comment

Uh oh!

FUDCo Oct 1, 2024

Uh oh!

grypez Oct 1, 2024

Uh oh!

FUDCo Oct 1, 2024

Uh oh!

FUDCo Oct 1, 2024

Uh oh!

FUDCo Oct 1, 2024 •

edited

Loading

Uh oh!

grypez Oct 2, 2024

Uh oh!

FUDCo Oct 2, 2024

Uh oh!

rekmarks commented Oct 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Do Not Merge: Move kernel to dedicated worker #123

Do Not Merge: Move kernel to dedicated worker #123

Uh oh!

Conversation

grypez commented Sep 30, 2024 • edited by rekmarks Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todo:

Uh oh!

FUDCo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FUDCo left a comment

Choose a reason for hiding this comment

Uh oh!

FUDCo Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

grypez Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

FUDCo Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

FUDCo Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

FUDCo Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grypez Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

FUDCo Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

rekmarks commented Oct 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

grypez commented Sep 30, 2024 •

edited by rekmarks

Loading

FUDCo Oct 1, 2024 •

edited

Loading