Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

implement dispatch.retireExports for Remotables #3297

Merged
merged 2 commits into from
Jun 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 44 additions & 5 deletions packages/SwingSet/src/kernel/liveSlots.js
Original file line number Diff line number Diff line change
Expand Up @@ -839,7 +839,7 @@ function build(
assert(Array.isArray(vrefs));
vrefs.map(vref => insistVatType('object', vref));
vrefs.map(vref => assert(parseVatSlot(vref).allocatedByVat));
console.log(`-- liveslots acting upon dropExports`);
// console.log(`-- liveslots acting upon dropExports ${vrefs.join(',')}`);
for (const vref of vrefs) {
const wr = slotToVal.get(vref);
const o = wr && wr.deref();
Expand All @@ -849,18 +849,57 @@ function build(
}
}

function retireOneExport(vref) {
insistVatType('object', vref);
const { virtual, allocatedByVat, type } = parseVatSlot(vref);
assert(allocatedByVat);
assert.equal(type, 'object');
if (virtual) {
// virtual object: ignore for now, but TODO we must still not make
// syscall.retireExport for vrefs that were already retired by the
// kernel
Comment on lines +858 to +860
Copy link
Contributor

@FUDCo FUDCo Jun 12, 2021

Choose a reason for hiding this comment

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

I'm unclear why virtual objects should be treated specially in this case. Is it related to the fact that virtual objects can still point to things even while being swapped out? If so I can see why letting them just get swept might leave dangling garbage on disk, but that's a "what's the proper procedure to delete this?" problem, not a "should we delete this?" problem. Otherwise I don't see how they're different from any other Remotable. What am I not understanding?

Copy link
Member Author

Choose a reason for hiding this comment

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

Virtual object GC is still pretty incomplete. dropExports effectively ignores them (Representatives are never put in exportedRemotables, and that's the only thing dropExports touches so far), so the virtualObjectManager is not yet being told that the kernel has given up reachability. retireExports is the kernel's way of giving up recognizability too, and we can't act upon that until we've first implemented the code that acts upon the drop.

The full flowchart for GC of virtual objects is the big one in #2724 (comment) . retireExports is one signal, but the algorithm must also keep track of any Representative and whether the vref is reachable from (and/or recognizable by) virtualized data. It isn't safe to forget the virtual object merely in response to retireExports.

When we're done, we'll definitely act upon dropExports and retireExports for virtual objects: ignoring them is a temporary measure until we build up more code to implement that large flowchart. But that's currently a minority use case (the only virtual objects we use so far are Purses, and they're relatively long-lived, at least compared to Payments and Offers and everything else that gets thrown around). And we do already have enough code to safely act upon it for Remotables, which ought to save us meaningful space now. So it's a phased-implementation approach.

My plan is to either build a separate "reachability manager" that manages that flowchart, or enhance the virtualObjectManager. At that point, retireOneExport will just do reachabilityManager.kernelRetired(vref) and let the other code figure out the consequences.

// console.log(`-- liveslots ignoring retireExports ${vref}`);
} else {
// Remotable
// console.log(`-- liveslots acting on retireExports ${vref}`);
const wr = slotToVal.get(vref);
if (wr) {
const val = wr.deref();
if (val) {
// it's fine to still have a value, that just means the kernel
// (and other vats) have completely forgotten about this, but we
// still know about it

if (exportedRemotables.has(val)) {
// however this is weird: we still think the Remotable is
// reachable, otherwise we would have removed it from
// exportedRemotables. The kernel was supposed to send
// dispatch.dropExports first.
console.log(`err: kernel retired undropped ${vref}`);
// TODO: find a way to make this more severe, it's cause for
// panicing the kernel, except that vats don't have that
// authority. It's *not* cause for terminating the vat, since
// it wasn't necessarily our fault.
return;
}
valToSlot.delete(val);
droppedRegistry.unregister(val);
}
slotToVal.delete(vref);
}
}
}

function retireExports(vrefs) {
assert(Array.isArray(vrefs));
vrefs.map(vref => insistVatType('object', vref));
vrefs.map(vref => assert(parseVatSlot(vref).allocatedByVat));
console.log(`-- liveslots ignoring retireExports`);
vrefs.forEach(retireOneExport);
}

function retireImports(vrefs) {
assert(Array.isArray(vrefs));
vrefs.map(vref => insistVatType('object', vref));
vrefs.map(vref => assert(!parseVatSlot(vref).allocatedByVat));
console.log(`-- liveslots ignoring retireImports`);
// console.log(`-- liveslots ignoring retireImports ${vrefs.join(',')}`);
}

// TODO: when we add notifyForward, guard against cycles
Expand Down
67 changes: 67 additions & 0 deletions packages/SwingSet/test/test-liveslots.js
Original file line number Diff line number Diff line change
Expand Up @@ -1112,3 +1112,70 @@ test('GC dispatch.dropExports', async t => {
});
t.deepEqual(log, []);
});

test('GC dispatch.retireExports inhibits syscall.retireExports', async t => {
const { log, syscall } = buildSyscall();
let wr;
function build(_vatPowers) {
let ex1;
const root = Far('root', {
hold() {
ex1 = Far('export', {});
wr = new WeakRef(ex1);
return ex1;
},
two() {},
drop() {
// eslint-disable-next-line no-unused-vars
ex1 = undefined; // drop the last userspace strongref
},
});
return root;
}
const dispatch = makeDispatch(syscall, build, 'vatA', true);
t.deepEqual(log, []);
const rootA = 'o+0';

// rp1 = root~.hold()
// ex1 = await rp1
const rp1 = 'p-1';
await dispatch(makeMessage(rootA, 'hold', capargs([]), rp1));
const l1 = log.shift();
const ex1 = l1.resolutions[0][2].slots[0];
t.deepEqual(l1, {
type: 'resolve',
resolutions: [[rp1, false, capdataOneSlot(ex1)]],
});
t.deepEqual(log, []);

// the exported Remotable should be held in place by exportedRemotables
// until we tell the vat we don't need it any more
t.truthy(wr.deref());

// an intermediate message will trigger GC, but the presence is still held
await dispatch(makeMessage(rootA, 'two', capargs([])));
t.truthy(wr.deref());

// now tell the vat we don't need a strong reference to that export.
await dispatch(makeDropExports(ex1));

// that removes the liveslots strongref, but the vat's remains in place
t.truthy(wr.deref());

// now the kernel tells the vat we can't even recognize the export
await dispatch(makeRetireExports(ex1));

// that ought to delete the table entry, but doesn't affect the vat
// strongref
t.truthy(wr.deref());

// now tell the vat to drop its strongref
await dispatch(makeMessage(rootA, 'drop', capargs([])));

// which should let the export be collected
t.falsy(wr.deref());

// the vat should *not* emit `syscall.retireExport`, because it already
// received a dispatch.retireExport
t.deepEqual(log, []);
});