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

syscall sensitivity to GC of virtual collection objects and timing of organic GC #6360

Closed
warner opened this issue Sep 29, 2022 · 6 comments · Fixed by #7333
Closed

syscall sensitivity to GC of virtual collection objects and timing of organic GC #6360

warner opened this issue Sep 29, 2022 · 6 comments · Fixed by #7333
Assignees
Labels
bug Something isn't working liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet

Comments

@warner
Copy link
Member

warner commented Sep 29, 2022

While we're pretty sure that userspace code cannot sense GC activity, we were also trying to make sure that the syscalls a vat worker emits were not sensitive to GC (to preserve our ability to have different GC timing among different validators, or different heap snapshot schedules, or upgrades, or something).

I noticed a few weeks ago that one such sensitivity has snuck in. If a virtual collection Representative (e.g. the object returned by makeScalarBigMapStore()) is both held in RAM and stored as the property of a virtual object, it will have a vref, and slotToVal.get(vref) will be a populated ("full") WeakRef that points to the Represenative. We call this state "REACHABLE" in our GC design documentation. If that RAM reference is dropped (moving the Representative to the "UNREACHABLE" state), the WeakRef is still populated until a GC event occurs. After the GC event, the Representative is "COLLECTED", and the WeakRef is empty (slotToVal.get(vref).deref() === undefined). (there is also a FinalizationRegistry watching the Represenative, and when that fires it would move to "FINALIZED", but we buffer the finalization results until a dispatch.bringOutYourDead delivery, so that's not the source of the sensitivity).

GC events happen spontaneously whenever the JS engine wants a new struct slot and can't find one in the free pool. We call this "organic GC", and the timing of it depends very closely upon the code that is running: every function call or temporary JS object needs a few slots, so simply inserting a const ignored = 0; in the code would temporarily allocate a few, making organic GC happen a little bit earlier than it would have otherwise. We force GC during bringOutYourDead, but that doesn't prevent organic GC from happening before that point (although it might defer the next one until later).

If some subsequent code then reaches into the virtual object and retrieves the collection from it, liveslots must provide a Representive for the caller. The deserialization code looks at slotToVal to see if we already have one in RAM. If so, it does slotToVal.get(vref).deref() to return the old one. If not, a "reanimation" function is called to make a new Representative. This is true for all virtual objects, even user-defined ones, not just collections.

But what's different about the collection's reanimation function is that it does a vatstoreGet() to fetch the |schemata and |label keys:

function reanimateCollection(vobjID) {
const { id, subid } = parseVatSlot(vobjID);
const kindName = storeKindIDToName.get(`${id}`);
const rawSchemata = JSON.parse(
syscall.vatstoreGet(prefixc(subid, '|schemata')),
);
const [keyShape, valueShape] = unserialize(rawSchemata);
const label = syscall.vatstoreGet(prefixc(subid, '|label'));
return summonCollection(

As a result, in a vat which has performed organic GC after the UNREACHABLE transition, we'll observe a pair of syscallvatstoreGets that are missing from a vat that started out with the same userspace state but which hasn't done organic GC yet.

That sensitivity isn't a huge problem, and I'm not sure we can easily get rid of it, but it's something to keep in mind. We're no longer really convinced that we can tolerate variations in GC timing anyways, but we've avoided doing anything that obviously breaks that tolerance.

The code reads |schemata so it can unserialize the keyShape and valueShape patterns which enforce that keys/values match a particular schema. We need these before we can accept gets/sets on the collections. And it reads |label to get a string that can be added to the error messages when those shape enforcement checks fail.

At one point, I think I made the assumption that the number of collections in use would be small (they could hold an arbitrary number of entries, but in a small number of collections), and thus holding the schema in RAM would be ok, but I've realized that nobody else made this assumption: ERTP creates a new collection for every Purse, both of which are durable:

const makePurseKit = vivifyFarClassKit(
issuerBaggage,
`${name} Purse`,
PurseIKit,
() => {
const currentBalance = AmountMath.makeEmpty(brand, assetKind);
/** @type {SetStore<Payment>} */
const recoverySet = makeScalarBigSetStore('recovery set', {
durable: true,
});

and the virtual collection code correctly avoids consuming any RAM for collection Representatives that are not currently REACHABLE in RAM.

I can't think of any way to avoid doing those reads without changing the rules ("stop using lots of collections, because they're going to cost RAM even if they aren't currently REACHABLE"). But this ticket can serve as a place to collect ideas in case we think of something new in the future

@mhofman
Copy link
Member

mhofman commented Dec 13, 2022

During the meeting today discussing #6588, it struck me that liveslots should hold onto vc metadata until BOYD so that the transcript would be insensitive to organic GC of virtual collections. For that matter any virtual object in-memory state should only be forgotten on BOYD and not directly attached to the representatives (not sure if that's already the case today).

@warner
Copy link
Member Author

warner commented Dec 22, 2022

Hm, this sounds a little bit like how we do a vatstore read whenever userspace reads a virtual-object property, to ensure that syscalls are a deterministic function of userspace activity. We could move the collection |schema reads, currently performed during reanimation, into the code that does a .init or .set on the collection.

OTOH, that means a lot of schema reads, giving up an obvious caching opportunity. Probably the same problem we have with virtual objects.

@mhofman
Copy link
Member

mhofman commented Dec 22, 2022

Yeah that seems suboptimal for collections. Out of curiosity, we have BOYD and various GC related syscalls already, which determine from the point of view of the kernel when an object is collected or not. I understand these vat store values may not be exported, but I still consider BOYD to be the synchronization point where the a vat and the kernel agree on the liveness of objects. Besides extra ram usage, what is the concern with caching read values for collected store entries until a BOYD delivery?

@warner
Copy link
Member Author

warner commented Dec 23, 2022

Hm, I need to understand your proposal better. Say we do BOYD on cranks 100 and 200. On c50, someone creates a virtual collection (VC), stashes it in a virtual object, then drops the in-RAM Representative. Organic GC between c50 and c75 might happen, and if it does, the finalizer might run, but could only stash the VC's vref into possiblyDeadSet, deferring processing until the next BOYD. On c75, userspace accesses the VC briefly, causing a reanimation, but dropping it again right away. During c100 we force GC, ensuring the vref is in possiblyDeadSet, and we do the BOYD refcount checks on those vrefs. Then during c150, userspace accesses the VC briefly and we need to reanimate it. Then c200 does a BOYD again. No other cranks access the VC.

In trunk (suffering from #6360), c75 might do a vatstoreGet, or might not, depending upon whether organic GC happens between c50 and c75. c100 will definitely have the VC's vref in possiblyDeadSet (it might get added any time between c50 and c75, if organic GC happens, and then might get added again any time between c75 and c100, since c75 dropped it, and then if it wasn't added already, c100 will add it for sure, because c100 does a forced GC). Starting with c101 the Representative will definitely be gone, so c150 will definitely do a vatstoreGet.

So the externally-visible uncertainty is only in crank c75, when the vatstoreGet might or might not happen. We're uncertain about the internal state (whether the VC is in the UNREACHABLE or in the COLLECTED state) everywhere from c50 to c75, and then again uncertain from c75 to c100. But the presence of the VC vref in possiblyDeadSet is concealed from the syscall trace everwhere except for c75.

I think your proposal means maintaining a table that maps from VC vref to the metadata, populating it when we reanimate one, and depopulating it during BOYD for all VC vrefs whose Representatives have actually gone away (i.e. they were in possiblyDeadSet, and the slotToVal probe does not reveal a live WeakRef). So this table will holds vrefs whose Representatives were REACHABLE at some point since the last BOYD, even though they might later be in any state. Immediately after the BOYD, it will only hold vrefs whose Representatives are still REACHABLE.

That means in c0-c49 the table will definitely not have our VC vref, in c50-c99 it definitely will have it, in c100-c149 it will definitely not, in c150-c199 it definitely will. And because the metadata will be available in c75, we definitely will not see a vatstoreGet in c75. We'll see a vatstoreGet in c50 and c150, and then never again until after c200 sometime.

So yeah, I think that would make it work. And the memory usage would still be proportional to the number of active objects, we just stretch the definition of "active" to mean "recently active", like "active since the last BOYD".

@mhofman
Copy link
Member

mhofman commented Dec 23, 2022

I think your proposal means maintaining a table that maps from VC vref to the metadata, populating it when we reanimate one, and depopulating it during BOYD for all VC vrefs whose Representatives have actually gone away (i.e. they were in possiblyDeadSet, and the slotToVal probe does not reveal a live WeakRef). So this table will holds vrefs whose Representatives were REACHABLE at some point since the last BOYD, even though they might later be in any state. Immediately after the BOYD, it will only hold vrefs whose Representatives are still REACHABLE.

That means in c0-c49 the table will definitely not have our VC vref, in c50-c99 it definitely will have it, in c100-c149 it will definitely not, in c150-c199 it definitely will. And because the metadata will be available in c75, we definitely will not see a vatstoreGet in c75. We'll see a vatstoreGet in c50 and c150, and then never again until after c200 sometime.

So yeah, I think that would make it work. And the memory usage would still be proportional to the number of active objects, we just stretch the definition of "active" to mean "recently active", like "active since the last BOYD".

Exactly. And we could extend this approach to not only virtual collection metadata, but any vat store data that was read/written since the last BOYD. The tradeoff is in memory usage vs vatstoreGet performance.

It occurs to me an alternative may be more appropriate, where all accessed vatstore data is cached in the heap for "some time", and we force a vatstoreGet for virtual collection after cache eviction, regardless of the representative's reachability (like I understand we do for virtual objects). That might make more sense since coupling vatstore access to BOYD feels unnatural. The "some time" could be a simple LRU, maybe combined with some consideration of the number of cranks elapsed since last.

Ok so scratch my BOYD suggestion. I think an LRU cache for all vatstore access, combined with a forced read for vatstore data independent of representative's lifetime would be a better approach.

@warner warner added the liveslots requires vat-upgrade to deploy changes label Jan 24, 2023
warner added a commit that referenced this issue Mar 9, 2023
This exercises three cases in which syscalls are sensitive to GC
activity:

* reanimateDurableKindID, when regenerating a KindHandle (#7142)
* reanimate, when regenerating a plain Representative (#7142)
* reanimateCollection, when regenerating a virtual collection (#6360)
warner added a commit that referenced this issue Mar 11, 2023
This exercises three cases in which syscalls are sensitive to GC
activity:

* reanimateDurableKindID, when regenerating a KindHandle (#7142)
* reanimate, when regenerating a plain Representative (#7142)
* reanimateCollection, when regenerating a virtual collection (#6360)
warner added a commit that referenced this issue Mar 11, 2023
This exercises three cases in which syscalls are sensitive to GC
activity:

* reanimateDurableKindID, when regenerating a KindHandle (#7142)
* reanimate, when regenerating a plain Representative (#7142)
* reanimateCollection, when regenerating a virtual collection (#6360)
warner added a commit that referenced this issue Mar 17, 2023
This exercises three cases in which syscalls are sensitive to GC
activity:

* reanimateDurableKindID, when regenerating a KindHandle (#7142)
* reanimate, when regenerating a plain Representative (#7142)
* reanimateCollection, when regenerating a virtual collection (#6360)
@warner warner self-assigned this Apr 4, 2023
@warner
Copy link
Member Author

warner commented Apr 5, 2023

I ended up fixing this with a cache that holds the schema/label data from the first collection operation in a crank, until the end of that crank. That's a deterministic function of userspace behavior, plus crank boundaries, which meets our sensitivity goals. It means more syscalls than if we only flushed the cache at BOYD, but it also means less RAM usage. In the future, if we find we want to make that tradeoff the other way, we could change it to flush at BOYD without a lot of work.

warner added a commit that referenced this issue Apr 5, 2023
This changes the collectionManager to keep the per-collection
metadata (keyShape, valueShape, label) in a cache. Like the VOM's
dataCache, this `schemaCache` is populated at most once per crank, and
flushed at the end of every crank. This removes the GC-sensitive
syscalls that used to fetch the metadata when userspace caused a
collection to be reanimated. Instead, the cache is read when userspace
invokes a collection method that needs the data (most of them, to
validate keys or values), regardless of whether the collection
Representative remains in RAM or not.

closes #6360
@mergify mergify bot closed this as completed in #7333 Apr 11, 2023
mergify bot pushed a commit that referenced this issue Apr 11, 2023
This changes the collectionManager to keep the per-collection
metadata (keyShape, valueShape, label) in a cache. Like the VOM's
dataCache, this `schemaCache` is populated at most once per crank, and
flushed at the end of every crank. This removes the GC-sensitive
syscalls that used to fetch the metadata when userspace caused a
collection to be reanimated. Instead, the cache is read when userspace
invokes a collection method that needs the data (most of them, to
validate keys or values), regardless of whether the collection
Representative remains in RAM or not.

It also changes the serialized form of the schemata to be an object
like `{ keyShape, valueShape }` instead of an array like `[keyShape,
valueShape]`. If the constraints are missing, the object is empty,
which is smaller to serialize. I'm also thinking this might make it
more extensible.

closes #6360
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants