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

feat(SwingSet): add "reachable" flag to clist entries #3130

Merged
merged 2 commits into from
May 21, 2021

Conversation

warner
Copy link
Member

@warner warner commented May 19, 2021

The kernel-to-vat clist entry for kref is stored in the kernelDB under a
key of ${vatID}.c.${kref} . This changes the value at that key from
${vref} to ${flag} ${vref}, where flag is either R (for "reachable
and recognizable") or _ (for "recognizable but not reachable). The "neither
reachable nor recognizable" state simply deletes the clist entry altogether.

This adds vatKeeper methods to set and clear the flag on a given kref. These
will be used by the GC syscall handlers for dropImport and friends.

It also cleans up some inconsistencies in the wrappers we put around the HostStorage API.

closes #3108

@warner warner added the SwingSet package: SwingSet label May 19, 2021
@warner warner added this to the Testnet: Stress Test Phase milestone May 19, 2021
@warner warner requested a review from FUDCo May 19, 2021 08:08
@warner warner self-assigned this May 19, 2021
@warner
Copy link
Member Author

warner commented May 19, 2021

@FUDCo I'll need to update this to work on top of #3122 once that lands. Review it for correctness, but be aware that the hostStorage/kvStore stuff will change to match.

Previously, these two wrappers didn't provide the same functionality as their
underlying HostStorage: you couldn't use empty strings for the start/end to
get more keys.

Also, add type assertions to crankBuffer to detect non-string keys/values
earlier than commitCrank.
The kernel-to-vat clist entry for `kref` is stored in the kernelDB under a
key of `${vatID}.c.${kref}` . This changes the value at that key from
`${vref}` to `${flag} ${vref}`, where `flag` is either `R` (for "reachable
and recognizable") or `_` (for "recognizable but not reachable). The "neither
reachable nor recognizable" state simply deletes the clist entry altogether.

This adds vatKeeper methods to set and clear the flag on a given kref. These
will be used by the GC syscall handlers for dropImport and friends.

closes #3108
Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

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

I think this is ok, but I find the commingling of mapping with fiddling with the reachable flag in mapVatSlotToKernelSlot and mapKernelSlotToVatSlot not 100% obvious. Which is to say, I think I follow the logic of what these functions are doing here, but I'm not deeply confident this aligns with what we want. The code in this PR implements the possibility of setting/altering/querying the flags but doesn't (yet) include anything that makes use of this possibility, so it's hard to tell if the automagic affordances provided by the mapping functions are the ones we actually want (I find it entirely plausible that they are, since this code wasn't conceived in a vacuum, but it's not immediately self evident).

@warner
Copy link
Member Author

warner commented May 21, 2021

so it's hard to tell if the automagic affordances provided by the mapping functions are the ones we actually want (I find it entirely plausible that they are, since this code wasn't conceived in a vacuum, but it's not immediately self evident).

Fair point. I think the next stage of GC work, which will invoke those functions, will teach us more about what they ought to provide.. they might need to change when we see what kernelSyscallHandler.dropImport needs to do with them. My general hunch is that it will want to call set() or clear(), and then the change to the refcount (if any) and whatever needs to be triggered by a change can be encapsulated inside set/clear instead of mapVatSlotToKernelSlot/etc. But we'll know more with usage.

@warner warner merged commit 3b0662e into master May 21, 2021
@warner warner deleted the 3108-clist-reachability branch May 21, 2021 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement clist reachability flag
2 participants