Track recognizable objects used by VOM so other objects can be GC'd #3409
Conversation
| * Presence as a key into a makeWeakStore() instance or an instance of | ||
| * VirtualObjectAwareWeakMap or VirtualObjectAwareWeakSet. We currently never | ||
| * remove anything from the set, but eventually we'll use refcounts to figure | ||
| * out when the vref becomes unrecognizable, allowing the vat do send a |
There was a problem hiding this comment.
typo "do send". a masterful homage to my original typo :)
| /** @type {Set<string>} of vrefs */ | ||
| const recognizableVrefs = new Set(); | ||
|
|
||
| // XXX this function takes a value whereas its brethren (e.g., |
There was a problem hiding this comment.
Ah, right, because in the places that call this, they have a vkey but that's very much not the same thing as a vref.
Maybe we should refactor virtualObjectKey to take a vref instead? So the callers would do something like:
const vref = getSlotForVal(key);
const vkey = virtualObjectKey(vref);
if (vkey) {
addRecognizablePresence(vref);
otherstuff(vkey);
..?
There was a problem hiding this comment.
The whole point of virtualObjectKey is converting between an object reference and a key string with a vref as the intermediate state on the way there. If it were to take a vref instead you'd just be adding some logic that would have to be replicated in a bunch of places. The way it's factored now is right, I think, aside from the misleading parallel between the naming of addReachablePresence and addRecognizablePresence, so I think the proper approach is to address the naming directly. I'm thinking addReachablePresenceRef and addRecognizablePresenceValue.
| @@ -1188,12 +1188,13 @@ test('device transfer', async t => { | |||
| function getRefCounts() { | |||
| return kvStore.get(`${kref}.refCount`); // e.g. "1,1" | |||
| } | |||
| // XXX TODO: this comment should change, but I'm not 100% sure what the new text should say | |||
There was a problem hiding this comment.
"The device should hold a reachable+recognizable reference, and vat-left (which forgot about amy) does not contribute either form of refcount, making the expected count 1,1. If deviceKeeper..."
|
Oh, actually the behavior may change when we land the #3406 branch (more drops happening), so please hold off on landing this until that change has landed and we fix any tests that break when these two great changes get together for a retirement party. |
That commit (8eeec28) has landed, in PR #3416. Go ahead and test. |
FUDCo
force-pushed
the
3161-vom-recognizance
branch
from
b08e6f0 to
49e8bc7
Compare
|
Changed one of the controller tests to make it less sensitive to GC variations, at the expense of checking less precisely. Please re-review to decide if this is acceptable. |
|
Do please modify the commit comments to include a reference to the bug number, so we can track it down from the git history later. |
FUDCo
force-pushed
the
3161-vom-recognizance
branch
from
ea8b6b1 to
2b1d0d4
Compare
FUDCo
force-pushed
the
3161-vom-recognizance
branch
from
2b1d0d4 to
e59150d
Compare
FUDCo
force-pushed
the
3161-vom-recognizance
branch
from
e59150d to
9be7dfc
Compare
This was surprisingly straightforward and made the GC noticeably more aggressive, at least in the tests that it affected.
I think it needs more tests targeted at this feature specifically, but I'm not sure how to construct them, so I'm creating the PR now to see if reviewing suggests some.
Closes #3161