Liveslots should avoid probing WeakRef through deref to check target state

[mhofman]

What is the Problem Being Solved?

Liveslots probes the state of a target using weakRef.deref(), without using the target. This is an anti-pattern in JS as it prevent gc of the target for an implementation defined amount of time. Instead it should solely rely on FinalizationRegistry callbacks to inform its accounting.

When a WeakRef is derefed, the target (if not collected) will be added to a host kept object list. Per spec that list cannot be cleared during the current synchronous execution (turn). However it's not guaranteed the list will be cleared at the end of the current turn, and indeed XS currently has a sub-optimal implementation that only clears the list after draining the promise queue.

This has 2 impact in our case:

• a WeakRef probed during delivery will have its target kept for the full crank
• a WeakRef probed during a FinalizationRegistry callback will have its target kept until a future job enqueues promises, which may depends on the supervisor implementation

This xsnap behavior probably explain workarounds like:

agoric-sdk/packages/SwingSet/src/lib-nodejs/gc-and-finalize.js

Lines 83 to 84 in 72d576e

	// on xsnap, we must do it twice for some reason
	await new Promise(setImmediate);

That said, even if the xsnap behavior was changed to clear kept objects more aggressively after each synchronous execution, there is still no reason to for liveslots to probe a weakref if it doesn't intend to use and pass the target to guest code. It prevents collection at best in the current turn where liveslots performed this probing.

I have raised this issue before, e.g. in #3732 (comment)

Description of the Design

If liveslots doesn't need the target of a weakref (the program isn't trying to get a virtual object representative, or we're not passing a presence in a delivery), then liveslots should rely on its existing understanding of the target's state as tracked by its FinalizationRegistry callbacks.

[mhofman]

Here 2 problematic defer usages:

agoric-sdk/packages/SwingSet/src/liveslots/liveslots.js

Lines 242 to 243 in d7652cb

	if (wr && !wr.deref()) {
	// we're in the COLLECTED state, or FINALIZED after a re-introduction

This could probably be solved by calling unregister when we found the first weakRef empty.

agoric-sdk/packages/SwingSet/src/liveslots/liveslots.js

Lines 285 to 287 in d7652cb

	if (!getValForSlot(vref) && !deadSet.has(vref)) {
	// Don't retire things that haven't yet made the transition to dead,
	// i.e., always drop before retiring

I'm not familiar enough with this logic, but it seems that similarly tracking collection and re-introduction better would remove the need for this probe.

[mhofman]

Reproduction of the WeakRef and FR issues in XS: https://gist.github.com/mhofman/f26cd6367d200bd13590b43131235edb

[warner]

contributes to gc sensitivity #6784, but it is maybe no higher priority than that issue

if it causes objects to be kept around a lot longer than they should (BOYD is pretty infrequent), then it might be more painful, and should be higher priority

requires changes to liveslots, which means to deploy it we'll need to upgrade all vats

[warner]

This is probably not a problem now, and is even less likely to be a problem once @mhofman lands the next batch of XS changes, so we don't need to fix it for this release.