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-liveslots): endow passStyleOf to liveslots guest compartment #9874

Merged
merged 14 commits into from
Aug 17, 2024

Conversation

erights
Copy link
Member

@erights erights commented Aug 10, 2024

A variant of #9431 . @warner , feel free to just adopt these changes into #9431 rather than reviewing this alternate.

closes: #9781
refs: #9431 endojs/endo#2377 endojs/endo#2408

Description

The code running under liveslots, i.e., user-level vat code such as contracts, must not be able to sense gc. Thus, liveslots endows them with virtual-storage-aware WeakMap and WeakSet, which treats the virtual object as the weakly held key, whereas the builtin WeakMap and WeakSet would treat the momentary representative as the weakly held key. To achieve this, the virtual-storage-aware WeakMap and WeakSet must impose a comparative storage leak.

However, some WeakMaps and WeakSets are used purely as an encapsulated unobservable memo, in the sense that the clients of encapsulating abstraction cannot sense whether the memo hit or missed (modulo timing of course, which we can also deny). passStyleOf is such an abstraction. Measurements show that the storage leak it causes is significant. The judgement of passStyleOf is only to report the pass-style of its arguments, and all virtual objects that have representative have a pass-style of 'remotable' every time any of its representatives are tested.

To avoid this storage leak, endojs/endo#2377 (merged, released, and synced with agoric-sdk) and endojs/endo#2408 (still in review) together enable liveslots to endow the compartment it unbundles with its own efficient passStyleOf, built from the primitive WeakMap which it encapsulates.

This PR does two things:

Security Considerations

This design assumes that the endowed passStyleOf makes the memo hits vs misses unobservable, so its dependence on these does not enable the code using it to observe gc. If there is some way to trick it into exposing the difference between a hit and miss, that would be a security concern.

Scaling Considerations

The point.

With this PR, the storage leak caused by the passStyleOf memo should go away. For some vats, this should be a big improvement.

Documentation Considerations

For the normal developer, none.

Testing Considerations

Adapts the tests originally written by @warner in #9431 , which seem to demonstrate that this works both for node-based and for XS-based vats.

Upgrade Considerations

I don't believe there are any. When linked with an endo preceding even endojs/endo#2377 , the only consequence should be that the storage leak remains unfixed. Likewise, if an endo with endojs/endo#2377 and even endojs/endo#2408 is linked with an agoric-sdk prior to the PR, the only consequence should be that the storage leak remains unfixed.

erights and others added 7 commits February 23, 2024 21:22
Update the swingset test to look for it there, and compare against a
bundle-generated version (they should be different). This currently
fails because importBundle() does not respect symbol-named properties
of `vatGlobals`.

Restored the SwingSet/package.json dependency on `@endo/pass-style` so
the vat-under-test can get a bundle-generated version, for comparison.

Added a liveslots-local test of the same, which passes because it's
only looking at the `vatGlobals` given to `buildVatNamespace()`, and
doesn't use `importBundle()`.
Copy link

cloudflare-workers-and-pages bot commented Aug 10, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: d2aa6e8
Status:🚫  Build failed.

View logs

@erights erights changed the title Markm 9431 variant feat(swingset-liveslots): endow passStyleOf to liveslots guest compartment Aug 10, 2024
@erights erights marked this pull request as ready for review August 15, 2024 01:33
@erights
Copy link
Member Author

erights commented Aug 15, 2024

Ready for review!

@erights
Copy link
Member Author

erights commented Aug 15, 2024

The only CI failure is

image
https://github.com/Agoric/agoric-sdk/pull/9874/checks?check_run_id=28791874925

which I do not understand. Reviewers or anyone else, your advice would be appreciated. Thanks.

@erights erights self-assigned this Aug 17, 2024
@erights erights added the automerge:squash Automatically squash merge label Aug 17, 2024
@mergify mergify bot merged commit b6c58f2 into master Aug 17, 2024
89 of 90 checks passed
@mergify mergify bot deleted the markm-9431-variant branch August 17, 2024 03:06
erights added a commit to endojs/endo that referenced this pull request Aug 22, 2024
…2408)

Closes: #2407 
Refs: Agoric/agoric-sdk#9874
Agoric/agoric-sdk#9431
Agoric/agoric-sdk#9781 #2377

## Description

While working on Agoric/agoric-sdk#9874 , I
found that it still failed to endow `passStyleOf` via a symbol-named
property as recently agree.

Turns out there was still a lurking `Object.keys` on the compartment
endowment pathway that needs to be changed to enumerate all enumerable
own properties, whether string-named or symbol-named.

### Security Considerations

None

### Scaling Considerations

For this PR, none. But it enables liveslots to endow its `passStyleOf`
to the compartments it makes, which has the scaling benefits explained
at Agoric/agoric-sdk#9781

### Documentation Considerations

For the normal developer, none. Once endowed, `passStyleOf` will have
performance that is less surprising.

### Testing Considerations

Agoric/agoric-sdk#9874 both patches in the
equivalent of this PR, uses it to endow `passStyleOf`, and test that it
has done so, across both node and XS.

However, after this fix, Agoric/agoric-sdk#9874
works on the Node host. But it still fails on the XS host for
undiagnosed reasons, so there may be another bug like #2407 still
lurking somewhere. Until diagnosed, it isn't clear if that remaining
problem is an endo problem or an agoric-sdk problem.

### Compatibility and Upgrade Considerations

Except for code meant only to test this, `passStyleOf` should not have
any observable difference. We have not attempted to make an other use of
symbol-named global properties, so there should be no compat or upgrade
issues.
@gibson042
Copy link
Member

gibson042 commented Aug 23, 2024

Confirming that passStyleOf was able to reuse an endowment was easy, but I just now finally assessed this code in a representative liveslots environment with a real- vs. virtual-cache liveslots, albeit with some hacking (updating @endo/pass-style to export a makePassStyleOf accepting the WeakMap constructor to use and comment out the confirmedRemotables WeakSet, tricking packages/vat-data/src/vat-data-bindings.js with a Proxy, and using a mock syscall). The basic approach was to create an in-memory vat root object with a "work" method that returns a passStyleOf-checked array of ExoClass representatives of the requested size, and then measuring how long it takes to dispatch a request for size 1000, followed by BringOutYourDead and forced GC.

Results:

As hoped for (and expected), heap snapshots demonstrated that the virtual cache retains presences but the real cache does not. Further, GC duration always climbs in both V8 and XS for a yet-to-be-determined reason, but is faster with the real cache than with the virtual one (and on XS, GC with a real cache is almost twice as fast). And I also found another WeakMap subject to liveslots interference in @endo/promise-kit's Promise.race replacement, which we may want to address similarly.

implementation V8 V8 V8 V8 XS XS XS XS
cache style real real virtual virtual real real virtual virtual
GC durations (ms) 460 466 565 565 1457 1494 1974 1921
615 687 809 775 1472 1460 2148 2096
791 794 1010 988 1529 1562 2620 2486
949 1000 1252 1274 1662 1693 3365 2890
1096 1200 1522 1466 1900 1914 3383 3208
1167 1284 1760 1661 1997 1933 3591 3413
1110 1412 2063 1648 2190 2004 3888 3872
1354 1462 1944 1807 2188 2114 4132 4005
1761 1646 2098 2053 2267 2326 4589 4511
1651 1668 2356 2222 2438 2486 4833 4833
1819 1812 2326 2357 2676 2642 5052 4878
1843 2082 2552 2540 2784 2905 5004 5307
2034 2283 2762 2698 2923 2847
2176 2274 3090 2984 3270 3033
2297 2571 3224 3231 3444 3186
2368 2765 3458 3400 3239 3206
2605 2756 3636 3579 3587 3437
2759 3055 3815 3850
3914 3066 3959
3074 3154
3206 3302
3239
Experimental setup
# Run on V8 and XS, with argument N=1000, a constant 10 observations per sample,
# 40 seconds of sampling (replaced with 0 for interactive debugging),
# and JSON output to see the increasing durations.
./esbench.mjs -h 'V8,*XS' -a N:1000 -r 10 -b 20 -t json \
  -i '
    import { getGCTriggerType, forceCollectionP } from "./gc-tools.js";
    Object.assign(globalThis, { getGCTriggerType, forceCollectionP });
    forceCollectionP.then(() => print(`GC trigger by ${getGCTriggerType()}`));
  ' \
  -i '
    // Trick packages/vat-data/src/vat-data-bindings.js into accepting
    // a VatData with nothing actually in it (yet).
    globalThis.VatData = new Proxy(
      {},
      {
        get(target, k) {
          return (target[k] ||= (...args) => realVatData[k](...args));
        },
      },
    );
  ' \
  -i '
    // Define and start the vat.
    import {
      Far,
      passStyleOf as goodPassStyleOf,
      // Patched in for this testing:
      makePassStyleOf,
    } from "@endo/pass-style";
    import { kslot, kser, kunser } from "@agoric/kmarshal";
    import { makeDurableZone } from "@agoric/zone/durable.js";
    import { makeLiveSlots } from "@agoric/swingset-liveslots/src/liveslots.js";
    import { makeDummyMeterControl } from "@agoric/swingset-liveslots/test/dummyMeterControl.js";
    import {
      makeStartVat,
      makeMessage,
      makeBringOutYourDead,
    } from "@agoric/swingset-liveslots/test/util.js";
    const { stringify } = JSON;
    const { Fail } = assert;
    // cf. swingset-liveslots/test/liveslots-helpers.js `buildSyscall`
    const { syscall, settlements } = (() => {
      const settlements = new Map();
      const vatstore = new Map();
      let sortedKeys, priorKeyReturned, priorKeyIndex;
      const ensureSorted = () => {
        if (sortedKeys) return;
        sortedKeys = [...vatstore.keys()];
        sortedKeys.sort((a, b) => a.localeCompare(b));
      };
      const clearGetNextKeyCache = () => {
        priorKeyReturned = undefined;
        priorKeyIndex = -1;
      };
      const clearSorted = () => {
        sortedKeys = undefined;
        clearGetNextKeyCache();
      };
      clearSorted();
      const syscall = {
        send() {},
        subscribe() {},
        resolve(resolutions) {
          for (const [vpid, isRejection, capdata] of resolutions) {
            settlements.set(vpid, { isRejection, capdata });
          }
        },
        dropImports() {},
        retireImports() {},
        retireExports() {},
        exit() {},
        vatstoreGet: key => vatstore.get(key),
        vatstoreGetNextKey(priorKey) {
          assert.typeof(priorKey, "string");
          ensureSorted();
          const start = priorKeyReturned === priorKey ? priorKeyIndex : 0;
          let result;
          for (let i = start; i < sortedKeys.length; i += 1) {
            const key = sortedKeys[i];
            if (key > priorKey) {
              priorKeyReturned = key;
              priorKeyIndex = i;
              result = key;
              break;
            }
          }
          if (!result) clearGetNextKeyCache();
          return result;
        },
        vatstoreSet(key, value) {
          if (!vatstore.has(key)) clearSorted();
          vatstore.set(key, value);
        },
        vatstoreDelete(key) {
          if (vatstore.has(key)) clearSorted();
          vatstore.delete(key);
        },
      };
      return { syscall, settlements };
    })();
    const forceGC = () => forceCollectionP.then(fn => fn());
    const gcTools = {
      WeakRef,
      FinalizationRegistry,
      waitUntilQuiescent: forceGC,
      gcAndFinalize: forceGC,
      meterControl: makeDummyMeterControl(),
    };
    const vatPowers = {};
    const buildVatNamespace = (_vatGlobals, { WeakMap: VirtualWeakMap }) => ({
      buildRootObject: ({ VatData }, vatParameters, baggage) => {
        globalThis.VatData = globalThis.realVatData = VatData;
        const badPassStyleOf = makePassStyleOf(VirtualWeakMap);
        const zone = makeDurableZone(baggage);
        const makeRepresentative = zone.exoClass(
          "Dummy",
          undefined,
          () => ({}),
          {},
        );
        const { psoCacheType } = vatParameters;
        const passStyleOfChoices = new Map();
        passStyleOfChoices.set("real", goodPassStyleOf);
        passStyleOfChoices.set("virtual", badPassStyleOf);
        const passStyleOf = passStyleOfChoices.get(psoCacheType)
          || Fail`unknown passStyleOf cache type ${psoCacheType} in ${stringify(vatParameters)}`;
        print(`passStyleOf uses a ${psoCacheType} WeakMap cache`);
        const work = async n => {
          let result = [];
          for (let i = 0; i < n; i++) result.push(makeRepresentative());
          harden(result);
          passStyleOf(result);
          return result;
        };
        return Far("root", { work });
      },
    });
    const liveslots = makeLiveSlots(
      syscall,
      "vat-alice",
      vatPowers,
      { virtualObjectCacheSize: 0 },
      gcTools,
      console,
      buildVatNamespace,
    );
    const { dispatch } = liveslots;
    // "real" or "virtual" here:
    const psoCacheType = "real";
    const vatStartedP = dispatch(makeStartVat(kser({ psoCacheType })));
    let lastID = 1000;
    Object.assign(globalThis, {
      dispatch,
      vatRoot: "o+0",
      kslot,
      kser,
      kunser,
      makeStartVat,
      makeMessage,
      makeBringOutYourDead,
      settlements,
      vatStartedP,
      lastID,
    });
  ' \
  -s 'const forceGC = await forceCollectionP; await vatStartedP;' \
  work-and-gc:'{
    // The code to measure: work+BOYD+forceGC
    // Reserve a new result VPID.
    const vpid = `p-${++lastID}`;
    // Send the request for N new representatives.
    await dispatch(makeMessage(vatRoot, "work", [N], vpid));
    const { isRejection, capdata } = settlements.get(vpid);
    if (isRejection) throw Error(`unexpected rejection for ${vpid}: ${capdata}`);
    await dispatch(makeBringOutYourDead());
    await forceGC();
  }'

kriskowal pushed a commit that referenced this pull request Aug 27, 2024
…tment (#9874)

A variant of #9431 . @warner , feel free to just adopt these changes into #9431 rather than reviewing this alternate.

closes: #9781
refs: #9431 endojs/endo#2377 endojs/endo#2408

## Description

The code running under liveslots, i.e., user-level vat code such as contracts, must not be able to sense gc. Thus, liveslots endows them with virtual-storage-aware WeakMap and WeakSet, which treats the virtual object as the weakly held key, whereas the builtin WeakMap and WeakSet would treat the momentary representative as the weakly held key. To achieve this, the virtual-storage-aware WeakMap and WeakSet must impose a comparative storage leak.

However, some WeakMaps and WeakSets are used purely as an encapsulated unobservable memo, in the sense that the clients of encapsulating abstraction cannot sense whether the memo hit or missed (modulo timing of course, which we can also deny). `passStyleOf` is such an abstraction. Measurements show that the storage leak it causes is significant. The judgement of `passStyleOf` is only to report the pass-style of its arguments, and all virtual objects that have representative have a pass-style of `'remotable'` every time any of its representatives are tested.

To avoid this storage leak, endojs/endo#2377 (merged, released, and synced with agoric-sdk) and endojs/endo#2408 (still in review) together enable liveslots to endow the compartment it unbundles with its own efficient `passStyleOf`, built from the primitive WeakMap which it encapsulates.

This PR does two things:
- makes the change to liveslots to do this endowing, according to the conventions supported by endojs/endo#2377 and endojs/endo#2408
- because endojs/endo#2408 is not yet synced with agoric-sdk, this PR adds an "equivalent" patch, so that we can depend on it before the next endo sync.

### Security Considerations

This design *assumes* that the endowed `passStyleOf` makes the memo hits vs misses unobservable, so its dependence on these does not enable the code using it to observe gc. If there is some way to trick it into exposing the difference between a hit and miss, that would be a security concern.

### Scaling Considerations

The point.

With this PR, the storage leak caused by the `passStyleOf` memo should go away. For some vats, this should be a big improvement.

### Documentation Considerations

For the normal developer, none.

### Testing Considerations

Adapts the tests originally written by @warner in #9431 , which seem to demonstrate that this works both for node-based and for XS-based vats.

### Upgrade Considerations

I don't believe there are any. When linked with an endo preceding even endojs/endo#2377 , the only consequence should be that the storage leak remains unfixed. Likewise, if an endo with endojs/endo#2377 and even endojs/endo#2408 is linked with an agoric-sdk prior to the PR, the only consequence should be that the storage leak remains unfixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vat-environment passStyleOf cache holds strong references to objects with vrefs
4 participants