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): abandon/delete most non-durables during stopVat() #5056

Merged
merged 1 commit into from
Apr 12, 2022

Conversation

warner
Copy link
Member

@warner warner commented Apr 9, 2022

This deletes most non-durable data during upgrade. stopVat() delegates
to a new function releaseOldState(), which makes an incomplete
effort to drop everything.

The portions which are complete are:

  • find all locally-decided promises and rejects them
  • find all exported Remotables and virtual objects, and abandons them
  • simulate finalizers for all in-RAM Presences and Representatives
  • use collectionManager to delete all virtual collections
  • perform a bringOutYourDead to clean up resulting dead references

After that, deleteVirtualObjectsWithoutDecref walks the vatstore and
deletes the data from all virtual objects, without attempting to
decref the things they pointed to. This fails to release durables and
imports which were referenced by those virtual objects (e.g. cycles
that escaped the earlier purge).

Code is written, but not yet complete, to decref those objects
properly. A later update to this file will activate that (and update
the tests to confirm it works).

The new unit test constructs a large object graph and examines it
afterwards to make sure everything was deleted appropriately. The test
knows about the limitations of deleteVirtualObjectsWithoutDecref, as
well as bug #5053 which causes some other objects to be retained
incorrectly.

The collectionManager was changed to keep an in-RAM set of the vrefs
for all collections, both virtual and durable. We need the virtuals to
implement deleteAllVirtualCollections because there's no efficient
way to enumerate them from the vatstore entries, and the code is a lot
simpler if I just track all of them. We also need the Set to tolerate
duplicate deletion attempts: deleteAllVirtualCollections runs first,
but just afterwards a bringOutYourDead might notice a zero refcount
on a virtual collection and attempt to delete it a second time. We
cannot keep this Set in RAM: if we have a very large number of
collections, it violates our RAM budget, so we need to change our DB
structure to accomodate this need (#5058).

refs #1848

@warner warner added the SwingSet package: SwingSet label Apr 9, 2022
@warner warner self-assigned this Apr 9, 2022
@warner warner changed the base branch from 5044-two-gc-bugs to master April 9, 2022 03:30
@warner
Copy link
Member Author

warner commented Apr 9, 2022

Note that this introduces O(numcollections) RAM usage until we replace it with something for #5058.

@warner warner requested a review from FUDCo April 9, 2022 21:20
@warner warner marked this pull request as ready for review April 9, 2022 21:20
@warner
Copy link
Member Author

warner commented Apr 9, 2022

Oh, one significant question: how should we access things like makeKindHandle from within the swingset source tree? It's defined on globalThis.VatData, but the preferred way to get it is import { makeKindHandle } from '@agoric/vat-data' (which is a simple module that just exports pieces from globalThis.VatData). But I think @agoric/vat-data has swingset as a dependency, and that's not a circularity I'm happy with.

@FUDCo
Copy link
Contributor

FUDCo commented Apr 10, 2022

Oh, one significant question: how should we access things like makeKindHandle from within the swingset source tree? It's defined on globalThis.VatData, but the preferred way to get it is import { makeKindHandle } from '@agoric/vat-data' (which is a simple module that just exports pieces from globalThis.VatData). But I think @agoric/vat-data has swingset as a dependency, and that's not a circularity I'm happy with.

I'm not entirely sure what you mean by "within the swingset source tree". Tests that use VatData outside an actual SwingSet get it from the test scaffolding which gets created by the fake VO stuff in tools. Tests that use VatData within actual SwingSet vats get it from the global like any other vat (typically by importing @agoric/vat-data) because they run in the standard vat environment and so sort of have to. Within the virtual object manager itself (virtualObjectManager.js) they're just functions which it exports as part of the return value from makeVirtualObjectManager (so within liveslots.js they're methods on vom).

@warner
Copy link
Member Author

warner commented Apr 11, 2022

I'm not entirely sure what you mean by "within the swingset source tree".

I mean tests under packages/SwingSet/test/ that run inside a real vat, like packages/SwingSet/test/update/vat-ulrik-1.js. I want these to import @agoric/vat-data like we recommend for any vat. But that means packages/SwingSet has a dependency on packages/vat-data, and I know vat-data has some form of dependency on swingset, so there's a cycle there, and eww cycles.

Looking more closely, it appears that packages/SwingSet/package.json already declares a dependencies['@agoric/vat-data'], so that ship has left the barn. packages/vat-data/package.json declares a dev dependency on swingset, so it's not a perfect cycle (I think we can be more lenient with devDependencies). I doubt that swingset needs a full dependencies on vat-data: a devDependencies would suffice, since it should only be the swingset unit tests that attempt to import @agoric/vat-data.

I'll make these unit tests use @agoric/vat-data. And I'll experiment to see if we could weaken the full dep to a dev one.

@FUDCo
Copy link
Contributor

FUDCo commented Apr 11, 2022

I'll make these unit tests use @agoric/vat-data. And I'll experiment to see if we could weaken the full dep to a dev one.

Seems like all dependencies that tests have should be dev dependencies, or at least all dependencies that aren't already runtime dependencies of the package under test itself.

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.

A few minor nits but nothing substantive. I remain concerned about the memory footprint of all the various intermediate data structures being used to track the stuff being deleted as we're deleting it, but I think that's out of scope for this PR.

@@ -549,9 +555,13 @@ export function makeCollectionManager(
}

function deleteCollection(vobjID) {
if (!allCollectionObjIDs.has(vobjID)) {
return false; // already deleted
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what situation could cause us to get to here, but if we do I'm betting it's probably a bug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be triggered by the violence I'm doing to the refcounts: the "delete all collections" pass deletes collection-1, then a subsequent BOYD drops the last (real) reference to it, so the refmgr tries to delete it again, causing a crash when it can't find the export status.

This will be a lot better with the scheme we cooked up (#5090).

// vats to cause confusion in the new version (by referencing exports
// which the vat no longer remembers).

const rootSlot = makeVatSlot('object', true, BigInt(0));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BigInt(0) --> 0n

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aye, I was having problems with 0n killing my editor's syntax parser, but that's because it's an old version of the plugin

// and attached a .then to it), and stopVat() must not allow
// userspace to gain agency.

const rejectCapData = m.serialize('vat upgraded');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bit of OCD in me that is bothered by the 'vat upgraded' text because the vat might not yet have been upgraded when whoever's waiting on the promise receives the rejection. Kind of like those early Final Fantasy games where you saw text narrating combat in the past tense just before you were shown the animation of the combat. Maybe 'vat halted for upgrade' or something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, think of it this way: the only way for anybody to see this message is if the vat was successfully upgraded, because if something failed, all state would get rolled back, including the rejection of those promises. The anthropological argument always wins, as far as you know :).

@warner warner changed the base branch from master to swingset-vat-data-dev-dep April 12, 2022 01:57
Base automatically changed from swingset-vat-data-dev-dep to master April 12, 2022 02:03
This deletes most non-durable data during upgrade. stopVat() delegates
to a new function `releaseOldState()`, which makes an incomplete
effort to drop everything.

The portions which are complete are:

* find all locally-decided promises and rejects them
* find all exported Remotables and virtual objects, and abandons them
* simulate finalizers for all in-RAM Presences and Representatives
* use collectionManager to delete all virtual collections
* perform a bringOutYourDead to clean up resulting dead references

After that, `deleteVirtualObjectsWithoutDecref` walks the vatstore and
deletes the data from all virtual objects, without attempting to
decref the things they pointed to. This fails to release durables and
imports which were referenced by those virtual objects (e.g. cycles
that escaped the earlier purge).

Code is written, but not yet complete, to decref those objects
properly. A later update to this file will activate that (and update
the tests to confirm it works).

The new unit test constructs a large object graph and examines it
afterwards to make sure everything was deleted appropriately. The test
knows about the limitations of `deleteVirtualObjectsWithoutDecref`, as
well as bug #5053 which causes some other objects to be retained
incorrectly.

The collectionManager was changed to keep an in-RAM set of the vrefs
for all collections, both virtual and durable. We need the virtuals to
implement `deleteAllVirtualCollections` because there's no efficient
way to enumerate them from the vatstore entries, and the code is a lot
simpler if I just track all of them. We also need the Set to tolerate
duplicate deletion attempts: `deleteAllVirtualCollections` runs first,
but just afterwards a `bringOutYourDead` might notice a zero refcount
on a virtual collection and attempt to delete it a second time. We
cannot keep this Set in RAM: if we have a very large number of
collections, it violates our RAM budget, so we need to change our DB
structure to accomodate this need (#5058).

refs #1848
@warner warner added the automerge:rebase Automatically rebase updates, then merge label Apr 12, 2022
@mergify mergify bot merged commit 5d7153a into master Apr 12, 2022
@mergify mergify bot deleted the 1848-abandon-remotables branch April 12, 2022 02:45
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.

2 participants