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

VOM virtual object and vref weak key GC #3649

Merged
merged 11 commits into from
Oct 14, 2021
Merged

VOM virtual object and vref weak key GC #3649

merged 11 commits into from
Oct 14, 2021

Conversation

FUDCo
Copy link
Contributor

@FUDCo FUDCo commented Aug 10, 2021

No description provided.

@FUDCo FUDCo added enhancement New feature or request SwingSet package: SwingSet labels Aug 10, 2021
@FUDCo FUDCo requested a review from warner August 10, 2021 18:12
@FUDCo FUDCo self-assigned this Aug 10, 2021
@FUDCo FUDCo marked this pull request as draft August 10, 2021 18:13
@FUDCo FUDCo changed the title feat: VOM weak key GC VOM virtual object and vref weak key GC Aug 14, 2021
@FUDCo FUDCo marked this pull request as ready for review August 14, 2021 08:55
@FUDCo FUDCo force-pushed the 3547-vom-weak-key-gc branch 4 times, most recently from 149e3a9 to d642528 Compare August 20, 2021 05:34
Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

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

Good work! Some minor changes to make, as well as bringing it up-to-date with current trunk.

@@ -107,6 +107,8 @@ function build(
const pendingPromises = new Set(); // Promises
const importedDevices = new Set(); // device nodes
const deadSet = new Set(); // vrefs that are finalized but not yet reported
const possiblyDeadSet = new Set(); // vrefs that might need to be rechecked for being dead
Copy link
Member

Choose a reason for hiding this comment

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

The deadSet is managed both "up" and "down": we add things to deadSet when the finalizer runs, and we actively remove them (deadSet.delete) when creating a new Presence in convertValToSlot.

possiblyDeadSet is currently only managed "up": we add vrefs to it when the VOM sees one of the three pillars being removed, but we don't remove things from it when a pillar is reestablished (e.g. convertValToSlot, or VOM's setRefCount).

We don't necessarily need to do it now, but I'm thinking that, in general, deadSet should be for tracking things whose Object references (Representatives, for VOs) have been removed, and all the other processing for VOs should be done in the VOM.

The change would be to merge possiblyDeadSet and deadSet, actively .delete() vrefs when re-instantiating their Objects (actually we already do this, for both Presences and Representatives), and remove the now-redundant if (!getValForSlot(vref)) check in processDeadSet.

However, this probably interacts with what happens when the other two pillars are dropped. On one hand, if the only pillar keeping a VO alive is virtualized data, and userspace explicltly deletes it, that should allow the syscall.retireExport to happen immediately, rather than waiting for end-of-crank or bringOutYourDead. On the other hand, the probe to see whether the Presence pillar is up or down must query the WeakRef, and those results are not safe to use until bringOutYourDead (because organic GC could cause variance). So maybe the safest thing to do is to only allow VOs to be deleted during processDeadSet, even if it wasn't a Representative being deleted that enabled it, which means sticking with the pattern you established in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

On one hand, if the only pillar keeping a VO alive is virtualized data, and userspace explicltly deletes it, that should allow the syscall.retireExport to happen immediately, rather than waiting for end-of-crank or bringOutYourDead. On the other hand, the probe to see whether the Presence pillar is up or down must query the WeakRef, and those results are not safe to use until bringOutYourDead (because organic GC could cause variance). So maybe the safest thing to do is to only allow VOs to be deleted during processDeadSet, even if it wasn't a Representative being deleted that enabled it, which means sticking with the pattern you established in this PR.

I'm probably out of my depth here, but why can't the check for the Presence pillar rely on a flag "there is a known Presence, it may be dead, but since we haven't reaped it yet, we don't know", without checking for the WeakRef directly.
So there's be 3 states for presences:

  • Live presence (or unreachable presence that hasn't been actively collected by the engine yet)
  • Dead presence, but not yet reaped. The finalizer ran, and added it to the dead set
  • Dead presence, reaped. BOYD was called.

If userspace deletes virtualized data, it would only immediately retire the export if the presence was reaped. If it wasn't reaped, the export retire would happen when BOYD is called.

packages/SwingSet/src/kernel/liveSlots.js Outdated Show resolved Hide resolved
packages/SwingSet/src/kernel/liveSlots.js Outdated Show resolved Hide resolved
packages/SwingSet/src/kernel/liveSlots.js Show resolved Hide resolved
packages/SwingSet/src/kernel/virtualObjectManager.js Outdated Show resolved Hide resolved
packages/SwingSet/src/kernel/virtualObjectManager.js Outdated Show resolved Hide resolved
packages/SwingSet/src/kernel/virtualObjectManager.js Outdated Show resolved Hide resolved
@warner
Copy link
Member

warner commented Oct 13, 2021

I've reviewed the tree that results from (this PR, plus #3732, plus #3968), and it looks good. It'll need rebasing, of course. Please use a merge commit when you're done, to preserve the stack of changes we've made (in this case, I think squashing would discard too much information).

* chore(swingset): small tweaks to GC

* rename unretiredKernelRecognizableRemotables to
  kernelRecognizableRemotables , since they're all unretired
* retireOneExport: delete from kernelRecognizableRemotables even if slotToVal
  was empty. I don't think this could happen, but they're logically distinct
  checks.
* rearrange scanForDeadObjects slightly for clarity
* update comment on possibleVirtualObjectDeath
* comment on why we add globals to vatPowers
* don't completely unpack gcTools: one-off uses can dereference directly, to
  let the unpack fit in a single line

refs #3732

* fix: revert retireOneExport handling of kernelRecognizableRemotables

I'm still uncertain that this is the right way to go, but we're going to
leave this as-is and revisit it later.
@FUDCo FUDCo added the automerge:rebase Automatically rebase updates, then merge label Oct 14, 2021
@mergify mergify bot merged commit 2c06952 into master Oct 14, 2021
@mergify mergify bot deleted the 3547-vom-weak-key-gc branch October 14, 2021 21:50
@FUDCo
Copy link
Contributor Author

FUDCo commented Nov 9, 2021

Closes #3547

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 enhancement New feature or request SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants