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

annotate virtual/durable vrefs #6923

Merged
merged 1 commit into from
Feb 7, 2023
Merged

Conversation

warner
Copy link
Member

@warner warner commented Feb 5, 2023

fix: add 'v'/'d' virtual/durable annotations to vrefs

This changes the vref format to reveal the ephemeral/virtual/durable
status of each object to the kernel. The kernel doesn't normally care,
but I'd like to move upgrade-time responsibility for abandoning
merely-virtual objects from the vat to the kernel, which requires the
kernel be capable of distinguishing them from durable objects.

In this format, ephemeral objects exports are o+NN, merely-virtual
object exports start with o+vNN (where NN names the KindID, and
the vref always has an instance-ID suffix, so like o+v34/2), and
durable object exports use o+dNN. As before, virtual/durable vrefs
might include an additional facet-ID suffix: o+v34/2:1.

Since the kernel doesn't yet care about details of the vref, very
little code changed in packages/SwingSet: just a few tests that had
stronger opinions about vrefs than they should be having. The kernel's
copy of parseVatSlots grew the ability to distinguish virtual from
durable, but nothing uses it yet.

closes #6695

@warner warner added SwingSet package: SwingSet liveslots requires vat-upgrade to deploy changes labels Feb 5, 2023
@warner warner self-assigned this Feb 5, 2023
@warner warner changed the base branch from master to call-startvat-properly February 6, 2023 07:48
@warner warner force-pushed the 6695-annotate-durable-vrefs branch from 8398973 to 1ba4d21 Compare February 6, 2023 07:48
@warner warner changed the title 6695 annotate durable vrefs annotate virtual/durable vrefs Feb 6, 2023
@warner warner force-pushed the 6695-annotate-durable-vrefs branch from 1ba4d21 to 0ea12f8 Compare February 6, 2023 09:28
@warner warner marked this pull request as ready for review February 6, 2023 09:30
@warner warner requested review from FUDCo and mhofman February 6, 2023 09:31
@warner
Copy link
Member Author

warner commented Feb 6, 2023

cc@mhofman @dckc because we might have some other tools (slogfile visualizer? OTEL telemetry conversion?) that are sensitive to the format of vrefs. Note that krefs remain unchanged.

@mhofman
Copy link
Member

mhofman commented Feb 6, 2023

move upgrade-time responsibility for abandoning merely-virtual objects from the vat to the kernel

In the CDP world, I'd imagine this responsibility would be on the CDP, with the kernel un-aware of the differences, right? I see virtualized / durable objects as an internal CDP concern, like the vatstore is.

some other tools (slogfile visualizer? OTEL telemetry conversion?) that are sensitive to the format of vrefs

I don't believe any of these tools are sensitive to the format of the vref (in the otel case I think we only care about kref anyway)

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

LGTM

The duplication of code/changes between Swingset and swingset-liveslots is a bit unfortunate.

packages/SwingSet/src/lib/parseVatSlots.js Outdated Show resolved Hide resolved
Comment on lines +24 to +25
* virtual: BOOL, // true=>vref designates a "merely virtual" object (not durable, not ephemeral)
* durable: BOOL, // designates a durable (not merely virtual, not ephemeral)
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, is there a reason you didn't go with virtual including durable instead of making them mutually exclusive ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that, but the documentation I came up with was too confusing, which tells me that the API would have been too confusing. If "virtual" means "virtual (and maybe also durable)", then "durable" means "virtual and durable", and that's not going to be an obvious split if you're just reading some code. Figured that explicit is better than implicit.

packages/swingset-liveslots/src/parseVatSlots.js Outdated Show resolved Hide resolved
@warner
Copy link
Member Author

warner commented Feb 6, 2023

The duplication of code/changes between Swingset and swingset-liveslots is a bit unfortunate

Yeah, when I split swingset-liveslots/ out to a separate package, I thought about making a third package to hold the common support code. But 1: I don't yet know how that support code will need to be refactored, there might be other combinations of dependencies that I haven't discovered yet, needing more common packages, and 2: it makes some claims about independent updatability that aren't necessarily supported. This parseVatSlots() change is a good example: we're really defining an interface between vats and the kernel: changing the contents of the common support code doesn't cause existing vats to start behaving differently. We might augment the interface and have a kernel that supports two separate flavors of this interface, but the old version of liveslots needs to keep using the old parseVatSlots.

I figure that once the dust settles, I'll look around and see what code can be safely/usefully factored back out into shared packages.

@FUDCo
Copy link
Contributor

FUDCo commented Feb 6, 2023

In the CDP world, I'd imagine this responsibility would be on the CDP, with the kernel un-aware of the differences, right? I see virtualized / durable objects as an internal CDP concern, like the vatstore is

What is "CDP"?

@mhofman
Copy link
Member

mhofman commented Feb 6, 2023

In the CDP world, I'd imagine this responsibility would be on the CDP, with the kernel un-aware of the differences, right? I see virtualized / durable objects as an internal CDP concern, like the vatstore is

What is "CDP"?

I borrowed the name from @warner's parallelization scheme: #6447

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.

Looks straightforward. My only concern is the proliferation of boolean tests all over the place getting more complicated -- that's not an objection to this set of changes per se, but more flagging a nascent concern about creeping complexity.

@warner warner force-pushed the 6695-annotate-durable-vrefs branch from 9c265f0 to fa65192 Compare February 7, 2023 09:02
Base automatically changed from call-startvat-properly to master February 7, 2023 10:45
@warner warner force-pushed the 6695-annotate-durable-vrefs branch from fa65192 to c4eeff3 Compare February 7, 2023 16:58
@warner warner added automerge:rebase Automatically rebase updates, then merge and removed automerge:rebase Automatically rebase updates, then merge labels Feb 7, 2023
This changes the vref format to reveal the ephemeral/virtual/durable
status of each object to the kernel. The kernel doesn't normally care,
but I'd like to move upgrade-time responsibility for abandoning
merely-virtual objects from the vat to the kernel, which requires the
kernel be capable of distinguishing them from durable objects.

In this format, ephemeral objects exports are `o+NN`, merely-virtual
object exports start with `o+vNN` (where `NN` names the KindID, and
the vref always has an instance-ID suffix, so like `o+v34/2`), and
durable object exports use `o+dNN`. As before, virtual/durable vrefs
might include an additional facet-ID suffix: `o+v34/2:1`.

Since the kernel doesn't yet care about details of the vref, very
little code changed in packages/SwingSet: just a few tests that had
stronger opinions about vrefs than they should be having. The kernel's
copy of `parseVatSlots` grew the ability to distinguish virtual from
durable, but nothing uses it yet.

closes #6695

Co-authored-by: Mathieu Hofman <[email protected]>
@warner warner force-pushed the 6695-annotate-durable-vrefs branch from c4eeff3 to b859e92 Compare February 7, 2023 20:33
@warner warner added the automerge:no-update (expert!) Automatically merge without updates label Feb 7, 2023
@mergify mergify bot merged commit 1c6e79b into master Feb 7, 2023
@mergify mergify bot deleted the 6695-annotate-durable-vrefs branch February 7, 2023 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:no-update (expert!) Automatically merge without updates liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

annotate durable vrefs like o+dNN
3 participants