feat: Implement refcounting and vat termination cleanup #478
Conversation
sirtimid
force-pushed
the
sirtimid/terminated-vats
branch
from
06d73c3 to
8a7fb12
Compare
sirtimid
force-pushed
the
sirtimid/terminated-vats
branch
from
8a7fb12 to
7cf9c29
Compare
sirtimid
changed the title
FUDCo
left a comment
FUDCo
left a comment
There was a problem hiding this comment.
A couple of nits to pick but nothing to hold up the train (if you want to make changes for those I promise a quick re-approval if you push an update).
| */ | ||
| async #run(): Promise<void> { | ||
| for await (const item of this.#runQueueItems()) { | ||
| this.#kernelStore.nextTerminatedVatCleanup(); |
There was a problem hiding this comment.
This seems like an expensive operation to do on every crank, since vat terminations are comparatively rare. Also, if I read the referenced function right, this will only clean up one vat worth regardless of how many vats terminated. What I'd do (not telling you how, just how I'd do it) is keep an in-memory flag that's set by vat termination and cleared by cleanup, have the cleanup be exhaustive, and check for dangling dead vats at kernel startup to handle the case of the kernel having shut down with pending terminations un-cleaned-up after.
There was a problem hiding this comment.
Thanks for the feedback! This operation isn’t expensive because, as you mentioned, vat terminations are rare. When no vats are terminated, nothing happens. And even if one is, the cleanup only handles one vat at a time.
I followed Agoric’s SwingSet approach here, where they also clean up one vat per crank. That helps avoid putting too much load on the database, especially if there are lots of references. That’s also why we log each cleanup step.
Of course, I don’t know if our kernel will grow as much as Agoric’s, and I’m happy to change this if needed. I just wanted to stay consistent with the SwingSet logic for now.
There was a problem hiding this comment.
I think the clean up of one vat per crank may postdate my involvement with the SwingSet code then. The thing that struck me as expensive was reading and parsing the terminated vats list every time. But since the list is usually empty and the record is cached then I guess the hit wouldn't be too bad.
| * @param eref - The ERef. | ||
| */ | ||
| function addClistEntry(endpointId: EndpointId, kref: KRef, eref: ERef): void { | ||
| function addCListEntry(endpointId: EndpointId, kref: KRef, eref: ERef): void { |
There was a problem hiding this comment.
Clist (as if it was a word) is kinda sort of a term of art, so I'm not sure twiddling with the capitalization like this is best. That said, I wouldn't necessarily expend effort to put it back the way it was unless you feel like it.
There was a problem hiding this comment.
Oh ok! I changed it to CList because that’s how it appears in swingset code. I can definitely revert it though.
There was a problem hiding this comment.
Really? My memory must be failing. Yeah, keep CList then. I feel dumb.
Closes #467
This PR improves vat termination by implementing a comprehensive cleanup process that ensures proper cleanup of all vat resources, reference counting and garbage collection.
Changes
markVatAsTerminatedcalls inKernel.tsfor vat terminationcleanupTerminatedVatinvat.tswith proper handling of:deleteEndpointto end of cleanup processdeleteVatto only handle vat config/store deletion, preventing premature cleanupCollect Garbageto the control panel for debugging garbage collectionIntegration
nextTerminatedVatCleanupruns at start of crank, processing terminated vat queueReference Count Operations
Creation
initKernelPromise()initKernelObject()Resolution
Imports/Exports
incrementRefCount()increases both reachable and recognizable counts for objectsGarbage Collection
decrementRefCount()reduces both reachable and recognizable countsVat Termination
Special Handling
clearReachableFlag()specifically decrements just the reachable count for object imports