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

fix(swingset): initializeKindHandleKind early enough to support durables #4944

Merged
merged 1 commit into from
Mar 31, 2022

Conversation

warner
Copy link
Member

@warner warner commented Mar 28, 2022

Previously, kindIDID (the ID of the virtual object Kind that is used
to hold the KindHandles we need for durable kinds) was allocated
on-demand, the first time makeKindHandle() was called. The ID it
received (and the ID of everything allocate afterwards) thus depended
upon if/when userspace decided to use makeKindHandle().

In addition, vrm.registerKind() for the KindHandle kind was not
called until kindIDID itself was allocated. This doesn't necessarily
happen at all in the version-2 of a vat (i.e. if v2 doesn't define any
additional durable kinds), and can't be relied upon to happen before
v2 needs to deserialize the KindHandles that live in the 'baggage'.

So this commit changes liveslots to factor out the initialization and
registration into a new initializeKindHandleKind function, and
arranges to call it during startVat(). Several "fake" test harnesses
in tools/ were updated to call it as they build their stuff.

Many unit tests were updated to deal with the change in allocated IDs,
but they ought to be somewhat more stable now. Two of these updates
puzzled me:

  • In test-gc-kernel.js, I understand why doomedExport1Vref changed
    from o+9 to o+10, and I'm not surprised that doomedExport2Vref is
    now o+11, but I don't understand why doomedExport2Vref was o+9
    before instead of o+10.
  • In test-virtualObjectGC.js line 1297, why did the single- and
    multi- facet test cases start behaving the same way? Why were
    they different before?

refs #1848

@warner warner added the SwingSet package: SwingSet label Mar 28, 2022
@warner warner self-assigned this Mar 28, 2022
@warner warner marked this pull request as ready for review March 28, 2022 23:28
@warner warner requested a review from FUDCo as a code owner March 28, 2022 23:28
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.

One minor possible typo. Otherwise, seems legit.

packages/SwingSet/test/gc/test-gc-vat.js Outdated Show resolved Hide resolved
@FUDCo
Copy link
Contributor

FUDCo commented Mar 29, 2022

Oh, I mixed up this PR with another one. Which is to say, the review above is a real review & assessment, but I haven't looked into the mysterious test changes yet.

@warner warner force-pushed the 1848-init-kind-handle-kind-earlier branch from 62e0906 to 3e5b090 Compare March 29, 2022 21:33
@warner
Copy link
Member Author

warner commented Mar 29, 2022

@FUDCo figured out the first of the two questions I had: the N-1 version of test-gc-kernel.js was relying upon an edge effect of the lazy sort I was doing. I was sorting the exported vrefs lexicographically, assuming they were all single digits and thus also sorted numerically. That was fine for the N-2 version (where the values were o+1 and o+2), but the N-1 version made them o+9 and o+10, which fell into the bug gap, and the test was updated to expect o+9 even though o+10 was what it should have been looking for. Then the most recent version became o+10 and o+11, and now o+11 is the right thing to look for.

I'll add a proper numerical sort to that test.

@warner warner force-pushed the 1848-init-kind-handle-kind-earlier branch from 9e0a722 to d70dbbe Compare March 30, 2022 05:42
@warner warner changed the base branch from master to 4936-force-more-gc March 30, 2022 06:13
@warner warner force-pushed the 1848-init-kind-handle-kind-earlier branch from d70dbbe to 404ca15 Compare March 30, 2022 06:16
@warner warner force-pushed the 4936-force-more-gc branch from 4bba6ac to e897bff Compare March 30, 2022 19:58
@warner warner force-pushed the 1848-init-kind-handle-kind-earlier branch from 404ca15 to bcfd68d Compare March 30, 2022 19:58
@FUDCo
Copy link
Contributor

FUDCo commented Mar 31, 2022

I think I figured out the second weird test difference. It's driven by the same root cause (different vrefs being generated and then sorted) but via completely different paths. In the previous version of the code the difference between the single- and multi-facet tests had to do with the order of operations -- basically, when in the sequence of things the various VOs got scanned for possibly being dead. This order, however, is sensitive to the vref strings themselves. The list of vrefs that possibleVirtualObjectDeath scans is sorted before scanning to ensure that the order of processing of the list is insensitive to what order the garbage collector scavenged the objects. Unlike the other test where sorting was implicated, in this case we don't actually care where in the list something is, we just want to ensure that the order is the same from one run to the next. However, the test itself is sensitive to the actual order because the test is actually looking for particular vrefs in a particular order. Before your change it was scanning o+10 (faceted thing) followed by o+11 (holder) in the faceted case, and o+11 (holder) followed by o+9 (unfaceted thing) in the unfaceted case. Hence the difference in the order to inspection of the .rc. keys between the two cases. In the new version it's scanning o+11 (faceted thing) followed by o+12 (holder) in the faceted case, and o+10 (unfaceted thing) followed by o+12 (holder) in the unfaceted case. In the new world order, in both cases the thing precedes the holder and the conditional logic is no longer needed. In this case there's no additional correction needed to the test (beyond what you already did), since the difference is in the actual underlying behavior of the vom.

@warner warner force-pushed the 4936-force-more-gc branch from e897bff to 1a24b54 Compare March 31, 2022 17:00
Base automatically changed from 4936-force-more-gc to master March 31, 2022 17:14
@warner warner force-pushed the 1848-init-kind-handle-kind-earlier branch from bcfd68d to be91ff7 Compare March 31, 2022 17:23
@warner warner added the automerge:rebase Automatically rebase updates, then merge label Mar 31, 2022
Previously, `kindIDID` (the ID of the virtual object Kind that is used
to hold the KindHandles we need for durable kinds) was allocated
on-demand, the first time `makeKindHandle()` was called. The ID it
received (and the ID of everything allocate afterwards) thus depended
upon if/when userspace decided to use `makeKindHandle()`.

In addition, `vrm.registerKind()` for the KindHandle kind was not
called until `kindIDID` itself was allocated. This doesn't necessarily
happen at all in the version-2 of a vat (i.e. if v2 doesn't define any
additional durable kinds), and can't be relied upon to happen before
v2 needs to deserialize the KindHandles that live in the 'baggage'.

So this commit changes liveslots to factor out the initialization and
registration into a new `initializeKindHandleKind` function, and
arranges to call it during `startVat()`. Several "fake" test harnesses
in `tools/` were updated to call it as they build their stuff.

test-gc-kernel.js was updated to sort the extracted vrefs numerically,
which was allowing the previous version of this test to pass despite
having the wrong object IDs.

refs #1848
@warner warner force-pushed the 1848-init-kind-handle-kind-earlier branch from be91ff7 to ae91fff Compare March 31, 2022 18:00
@mergify mergify bot merged commit 8356c2d into master Mar 31, 2022
@mergify mergify bot deleted the 1848-init-kind-handle-kind-earlier branch March 31, 2022 18:15
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 SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants