-
Notifications
You must be signed in to change notification settings - Fork 7
refactor(kernel): Isolate Kernel Queue, Syscall, Router & Translation Components #484
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
Conversation
grypez
left a comment
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 think it is an improvement to the organization. Judging by the number of yet unimplemented syscalls, I foresee VatSyscall.ts will need to be broken up further. Perhaps a good criteria for isolating the subcomponents will be whether the syscall requires access to the KernelQueue, KernelStore, or both.
Consider labelling the PR commit with refactor(kernel): since the changes are isolated to the kernel package.
Approved!
FUDCo
left a comment
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.
Pro forma to hold merge until I have a chance to look at this. Which I'll do promptly I promise.
FUDCo
left a comment
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.
One minor nit which you can decide if you want to handle it now or later or not at all. Other than that, looks good.
| /** | ||
| * Add an item to the tail of the kernel's run queue. | ||
| * | ||
| * @param item - The item to add. | ||
| */ | ||
| enqueueRun(item: RunQueueItem): void { | ||
| this.#kernelStore.enqueueRun(item); | ||
| if (this.#kernelStore.runQueueLength() === 1 && this.#wakeUpTheRunQueue) { | ||
| const wakeUpTheRunQueue = this.#wakeUpTheRunQueue; | ||
| this.#wakeUpTheRunQueue = null; | ||
| wakeUpTheRunQueue(); | ||
| } | ||
| } |
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.
There are three enqueueXXX methods, but enqueueMessage and enqueueNotify are specific while enqueueRun is more generic (used both in the implementation of the other two and for general queue handling). So I'd suggest reordering them to put enqueueRun before of after the other two rather than in between them.
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.
ok yeah I'll do it later.
This PR modularizes our kernel code for improved organization and maintainability by:
Creating a dedicated
KernelQueueto manage the kernel’s run queue.Refactored message routing logic from Kernel into dedicated
KernelRouterclass with specialized delivery handlers for each message type, improving separation of concerns and type safety.Extracting the syscall handling logic into a new
VatSyscallclass for processing vat-related syscalls such as sending messages, resolving promises, and dropping imports.Moving translation methods from both
KernelandVatHandleinto the store object, centralizing the translation of references and messages between kernel and vat spaces.