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.

Sign up for GitHub

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

Jump to bottom

record durable-kind stateShape, with refcounts #7365

Merged
merged 4 commits into from
Apr 13, 2023
Merged

record durable-kind stateShape, with refcounts #7365

merged 4 commits into from
Apr 13, 2023

Conversation

warner
Copy link
Member

@warner warner commented Apr 9, 2023

Save a serialized copy of the Kind's stateShape option, so future
incarnations can compare the old one against newer ones when they
re-define the Kind. Increment refcounts on any objects included in the
shape. Forbid the use of non-durable objects in the shape.

closes #7338
refs #7337

@warner warner added SwingSet package: SwingSet liveslots requires vat-upgrade to deploy changes labels Apr 9, 2023
@warner warner requested a review from mhofman April 9, 2023 03:46
@warner warner self-assigned this Apr 9, 2023
mhofman
mhofman reviewed Apr 10, 2023
View reviewed changes
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.

Partial review for now.

so future incarnations can compare the old one against newer ones when they
re-define the Kind.

To clarify, this compatibility check is not part of this PR? Any reason not to enforce an exact match for now? One way to do that would be to compare the serialized cap data of the before and after shape, and assert they're equal.

packages/swingset-liveslots/src/virtualObjectManager.js Outdated Show resolved Hide resolved
packages/swingset-liveslots/src/virtualObjectManager.js Outdated
Comment on lines 719 to 722
vrm.updateReferenceCounts(oldStateShapeSlots, newStateShapeSlots);
durableKindDescriptor.stateShapeCapData = stateShapeCapData; // replace
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we should optimize the case where we don't have (and didn't have) a stateShape by omitting it from the descriptor.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, and I did that for virtual collections' keyShape/valueShape, but Kinds are low-cardinality, so we're not saving very much space.

@warner
Copy link
Member Author

warner commented Apr 10, 2023

Correct, this doesn't assert that an upgraded stateShape matches. Partially because I wanted a test that shows old slots are decref'ed / new slots are incref'ed, and that requires a non-identical stateShape. My next PR will enforce the equality, and disable that incref/decref check (or add an option to allow non-identical shapes, just for that one unit test).

@warner warner force-pushed the 7321-refcount-keyshape-vrefs branch from 04e4d1b to 0a3c0ef Compare April 10, 2023 20:49
@warner warner force-pushed the 7338-store-state-shape branch 2 times, most recently from 5fa3b10 to ead0df2 Compare April 10, 2023 22:57
@warner warner force-pushed the 7321-refcount-keyshape-vrefs branch from c80bdb8 to a9b2fad Compare April 10, 2023 23:17
@warner warner force-pushed the 7338-store-state-shape branch from ead0df2 to 7837a4f Compare April 10, 2023 23:17
@warner warner force-pushed the 7321-refcount-keyshape-vrefs branch from a9b2fad to db996d0 Compare April 11, 2023 02:06
@warner warner force-pushed the 7338-store-state-shape branch from 7837a4f to a38e312 Compare April 11, 2023 02:06
@warner warner force-pushed the 7321-refcount-keyshape-vrefs branch from db996d0 to 5af1b31 Compare April 11, 2023 02:51
Base automatically changed from 7321-refcount-keyshape-vrefs to master April 11, 2023 03:30
@warner warner force-pushed the 7338-store-state-shape branch from a38e312 to 9252647 Compare April 12, 2023 00:54
@warner warner requested a review from mhofman April 12, 2023 00:54
@warner
Copy link
Member Author

warner commented Apr 12, 2023

@mhofman I've added the must-be-the-same check, please re-review.

mhofman
mhofman approved these changes Apr 12, 2023
View reviewed changes
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.

Besides the nit on the encoding assumptions made by the comparison, LGTM.

packages/swingset-liveslots/src/virtualObjectManager.js Outdated Show resolved Hide resolved
@warner warner force-pushed the 7338-store-state-shape branch from 9252647 to c617f76 Compare April 12, 2023 17:01
@warner warner added force:integration Force integration tests to run on PR automerge:rebase Automatically rebase updates, then merge labels Apr 12, 2023
@warner warner force-pushed the 7338-store-state-shape branch from c617f76 to ad2c0a2 Compare April 12, 2023 17:46
@turadg
Copy link
Member

turadg commented Apr 12, 2023

This passed Chain Deployment Test but it's not merging because of the new required checks. Those won't be run until #7393 is in master, but that's queued up behind this.

So I'm adding bypass to get this into master and drain the queue.

@turadg turadg added bypass:integration Prevent integration tests from running on PR and removed force:integration Force integration tests to run on PR labels Apr 12, 2023
@turadg
Copy link
Member

turadg commented Apr 12, 2023

Oh, unfortunately now that the checks are required nothing can merge without the tests being run. So we need to put this back into the queue after the CI change PR.

@Mergifyio unqueue

@turadg turadg removed the bypass:integration Prevent integration tests from running on PR label Apr 12, 2023
@turadg turadg force-pushed the 7338-store-state-shape branch from ad2c0a2 to 5d36843 Compare April 12, 2023 20:16
@warner warner removed the automerge:rebase Automatically rebase updates, then merge label Apr 12, 2023
@warner warner added automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR labels Apr 12, 2023
@warner warner force-pushed the 7338-store-state-shape branch from 5d36843 to 65a871f Compare April 12, 2023 21:14
@warner warner removed the force:integration Force integration tests to run on PR label Apr 12, 2023
@erights
Copy link
Member

erights commented Apr 12, 2023

test-vats (16.x) was cancelled due to timeout, so I took the liberty of hitting the CI "rerun failed jobs" button.

@warner warner force-pushed the 7338-store-state-shape branch from 65a871f to 3b42405 Compare April 12, 2023 22:43
warner added 4 commits April 12, 2023 17:17
@warner
factor out insistDurableCapdata
80d6f95
@warner
record durable-kind stateShape, with refcounts
84ac88f 
Save a serialized copy of the Kind's `stateShape` option, so future
incarnations can compare the old one against newer ones when they
re-define the Kind. Increment refcounts on any objects included in the
shape. Forbid the use of non-durable objects in the shape.

closes #7338
refs #7337
@warner
prohibit changes to durable-Kind stateShape during upgrade
e47f456 
When a vat upgrade provides a new definition for a pre-existing
durable Kind, it can supply a `stateShape` option that might not be
compatible with one established by the previous incarnation.

The long-term issue is how to manage "schema upgrades", as the
permissible/desired shape of the virtual-object `state` data changes
over versions. For now, our main concern is that userspace doesn't
create a situation where reading a `state` property causes an error,
because the new `stateShape` rejects values that were recorded by an
earlier version.

The short-term fix is to insist that each new incarnation defines the
Kind with exactly the same `stateShape` as its predecessor. This check
is performed by comparing the serialized/marshalled capdata of
`stateShape` against that of the earlier version, which is convenient
but overly strict (e.g. the properties must be defined in the same
order).

If violated, the new-version `buildRootObject` will throw an error as
it calls `defineDurableKind`.

refs #7337
@warner
note the limitations/assumptions of insistSameCapData
bf67ea9 
This implementation assumes that both old and new stateShapes were
marshalled with the same version (e.g. smallcaps vs pre-smallcaps). If
we introduce a third format in the future, we should change
insistSameCapData to re-serialize the old data into the current format
before attempting to compare the capdata bodies.
@warner warner force-pushed the 7338-store-state-shape branch from 3b42405 to bf67ea9 Compare April 13, 2023 00:18
@mergify mergify bot merged commit b3b0302 into master Apr 13, 2023
@mergify mergify bot deleted the 7338-store-state-shape branch April 13, 2023 01:06
@warner warner mentioned this pull request Apr 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhofman mhofman mhofman approved these changes

Assignees

@warner warner

Labels
automerge:rebase Automatically rebase updates, then merge liveslots requires vat-upgrade to deploy changes SwingSet package: SwingSet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Store stateShape in VOM metadata
4 participants
@warner @turadg @erights @mhofman