rewrite scanForDeadObjects to avoid retire-without-drop #9942
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
warner
added
SwingSet
Deploying agoric-sdk with
|
| Latest commit: |
c045163
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://761d9c6a.agoric-sdk.pages.dev |
| Branch Preview URL: | https://warner-9939-retire-without-d.agoric-sdk.pages.dev |
warner
force-pushed
the
warner/9939-retire-without-drop
branch
3 times, most recently
from
479652d
to
310c064
Compare
warner
changed the title
|
note: revert the test.skip from 064ff1a when this lands |
mhofman
reviewed
Aug 28, 2024
mhofman
reviewed
Aug 29, 2024
There was a problem hiding this comment.
Partial review of comments in commit titled fix(liveslots): rewrite scanForDeadObjects to avoid retire-without-drop. I am still looking through the code changes, but wanted to provide early feedback on what I believe may be an important conceptual mistake regarding recognizable only imported presence.
warner
force-pushed
the
warner/9939-retire-without-drop
branch
2 times, most recently
from
568549b
to
a47fa2c
Compare
mhofman
approved these changes
Aug 30, 2024
no behavior changes
This rewrites scanForDeadObjects(), which is called during dispatch.bringOutYourDead to process possiblyDeadSet and possiblyRetiredSet. The new flow should be easier to review and understand. The main behavioral difference is to fix a bug (#9939) in which a vref that appears in possiblyRetiredSet (because e.g. a weak collection was deleted, which was using that vref as a key), but which 1: lacks a RAM pillar (Presence object) and 2: was not dropped in this BOYD (e.g. it has a vdata pillar), used to be sent to the kernel in a bogus `syscall.retireImports()` call. Because this vref was not previously dropped by the vat (syscall.dropImports()), this was a vat-fatal error. The new code will only retire such a Presence vref if it was not reachable by the vat. The new tests are marked as expected to pass again. thanks @mhofman and @gibson042 for recommendations fixes #9939
This reverts commit 064ff1a. Now that the underlying issue is fixed, we can re-enable this formerly-flaky test. Thanks @michaelfig for your patience.
warner
force-pushed
the
warner/9939-retire-without-drop
branch
from
a47fa2c
to
c045163
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rewrite scanForDeadObjects(), which is called during
dispatch.bringOutYourDead to process possiblyDeadSet and
possiblyRetiredSet. The new flow should be easier to review and
understand.
The main behavioral difference is to fix a bug (#9939) in which a vref
that appears in possiblyRetiredSet (because e.g. a weak collection was
deleted, which was using that vref as a key), but which 1: lacks a RAM
pillar (Presence object) and 2: was not dropped in this BOYD (e.g. it
has a vdata pillar), used to be sent to the kernel in a bogus
syscall.retireImports()call. Because this vref was not previouslydropped by the vat (syscall.dropImports()), this was a vat-fatal
error.
The new code will only retire such a Presence vref if it was not
reachable by the vat.
fixes #9939