-
Notifications
You must be signed in to change notification settings - Fork 6
feat(kernel): Support liveslots distributed garbage collection #419
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
894b13b to
536c336
Compare
536c336 to
9a651d1
Compare
| import { makeBaseStore } from './base-store.ts'; | ||
| import { makeCListStore } from './clist-store.ts'; | ||
| import { makeGCStore } from './gc-store.ts'; | ||
| import { makeIdStore } from './id-store.ts'; | ||
| import { makeObjectStore } from './object-store.ts'; | ||
| import { makePromiseStore } from './promise-store.ts'; | ||
| import { makeQueueStore } from './queue-store.ts'; | ||
| import { makeRefCountStore } from './refcount-store.ts'; |
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'm not completely wild about this refactor of kernel-store. To be clear: I think breaking it apart into more manageably sized pieces is well motivated, given that it suffered a bit too much from having fallen into the big-ball-of-mud pattern (which it acquired by way of its SwingSet heritage, which in turn got that way because it just growed). Moreover, I think the partitioning of functionality here seems mostly sensible.
However, given that its job is simply wrapping various bits of higher-level, kernel-specific functionality around a single underlying (and fairly stupid) key/value store, I'm not comfortable characterizing the various sub-groupings of function as "stores" in their own right (though doing that arguably might make sense in a world where we implement more of those bits of functionality directly in the database with their own individualized tables, schemata, and prepared statements, and maybe that's an idea we should talk about -- a refactoring like this might make sense as a precursor to moving things in that direction). While calling them "stores" might actually just be a naming quibble, reifying each of them as its own object (with accompanying factory function) feels like a pretty heavy weight approach, especially given the fairly complicated graph of dependencies between them -- I'm betting that getting the order-of-initialization logic right for them all was a bit of work. Given those cross dependencies, I'm concerned that the whole thing may be fairly brittle under maintenance. On top of this, the big-ball-of-mud API is still retained in the end because that's how remains exposed to the rest of the kernel
In short, my intuition is that if you're going to refactor the kernel store, you should go all the way (perhaps exposing the "kernel store" as an explicit bag of sub-functionality groups), otherwise I'm not sure it pays for itself.
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.
Thanks! That’s actually great to hear because the main reason I kept the factory-based approach was to avoid deviating too much from the existing pattern. I was already aware that it introduces order-of-initialization challenges and I had considered refining it further to align more closely with the overall code style. I held back from making major changes in this pass but now that it's in this state I think it’s clear that a more thorough refactor would be worthwhile.
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 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.
This refactor is still going to collide with critical bug fixes in #448. I'd be much more comfortable if the kernel store refactor was separated into its own PR so that we can land the GC stuff now.
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.
0c58cb2 to
e00a14b
Compare
b27f593 to
11778ef
Compare
b336429 to
381e30e
Compare
189658d to
045188e
Compare
e4da823 to
ca199c6
Compare
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.
OK. Let's all plug our ears and wait for the kaboom!
Closes: #329
dropImports: Vat tells kernel it no longer needs certain imported referencesretireImports: Vat acknowledges retirement of imports and promises not to use them againretireExports: Vat tells kernel it's retiring certain exported objectsabandonExports: Vat tells kernel it's abandoning exported objects (even if they are live)incrementRefCountanddecrementRefCountfunctions to manage reference counts for kernel objects and promisesdeleteClistEntryto handle reachability flags and reference countingmaybeFreeKrefsephemeral set