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

build durable notifier #4567

Closed
warner opened this issue Feb 16, 2022 · 10 comments · Fixed by #6502
Closed

build durable notifier #4567

warner opened this issue Feb 16, 2022 · 10 comments · Fixed by #6502
Assignees
Labels
enhancement New feature or request notifier SwingSet package: SwingSet

Comments

@warner
Copy link
Member

warner commented Feb 16, 2022

What is the Problem Being Solved?

As part of #4550 and #4383, we're faced with a challenge. Promises can be referenced by virtual objects (#2485). The Promise object remains in RAM, which doesn't meet our memory-consumption goals, but it remains sound to reference them. However they are not "durable", because they cannot be entirely ejected from RAM and then resurrected on-demand later. So durable data cannot reference virtual data, and only virtual data can reference promises.

We know that we need to virtualize Notifiers (#4513) to meet our RAM goals. We know that Zoe and contracts need to hold their long-term state in durable data, and any externally-visible objects must be Durable (e.g. contract facets must survive upgrade). So the question is how clients should get reconnected to their notifiers after an upgrade.

One option is to make durable notifiers. Then the notifier can be part of the state of the durable facet/etc. However each notifier needs to hold onto a promise (the one it will resolve upon the next update), and promises cannot be durable. If we can create virtual promises (#3787) then we aren't violating our RAM budget, but we still can't reference the merely-virtual promise from the durable notifer's state.

Another is to leave notifiers as merely virtual. But the durable facet cannot reference such an object from its state.

A trick I've been thinking about that might be useful in either approach would be to have the durable facet Kind constructor close over a merely-virtual Notifier constructor. They could share an integer index, and use it to reference a virtual table of notifiers that were created within this version of the vat. These notifiers would be lost upon upgrade, but regenerated when clients notice their getUpdateSince() promises rejecting and then ask for a new notifier.

Description of the Design

Use an ephemeral table of promises, which get rejected upon upgrade: see below.

Security Considerations

Test Plan

@warner warner added enhancement New feature or request SwingSet package: SwingSet notifier labels Feb 16, 2022
@Tartuffo Tartuffo added this to the Mainnet 1 milestone Mar 23, 2022
@warner
Copy link
Member Author

warner commented Apr 5, 2022

That trick looks like the way to go:

// create these handles only in version-1, otherwise get from baggage
const counterHandle = makeKindHandle('counter');
const notifierHandle = makeKindHandle('notifier');

// in all versions:

//  define a durable counter
const makeCounter = defineDurableKind(counterHandle, () => { c: 0 }, {
  next: ({state}) => { state.c += state.c; return state.c; },
});

const counter = makeCounter();

// define an ephemeral table, indexed by the counter:
const ephemeralPromises = new Map();
function getEphemeralPromiseKit(index) {
  if (!ephemeralPromises.has(index)) {
    ephemeralPromises.set(index, makePromiseKit());
  }
  return ephemeralPromises.get(index);
}

// each new Notifier needs a distinct index
function initNotifierState() {
  return { index: counter.next(), currentUpdateCount: undefined, currentResponse: undefined };
}


// define behavior for notifier
function getUpdateSince({ state }, updateCount = NaN) {
  const { index, currentUpdateCount, currentResponse } = state;
  const hasState = currentResponse !== undefined;
  const final = currentUpdateCount === undefined;
  if (hasState && (final || currentResponse && currentResponse.updateCount !== updateCount)) {
    assert(currentResponse !== undefined);
    return Promise.resolve(currentResponse);
  }
  // otherwise return a promise for the next state
  const pk = provideEphemeralPromiseKit(index);
  return pk.promise;
}

function updateState({ state }, value) {
  const { index, currentUpdateCount } = state;
  currentUpdateCount += 1;
  state.currentUpdateCount = currentUpdateCount
  const currentResponse = harden({ value,
                                   updateCount: currentUpdateCount });
  state.currentResponse = currentResponse;
  if (ephemeralPromises.has(index)) {
    ephemeralPromises.get(index).resolve(currentResponse);
    ephemeralPromises.delete(index);
  }
}

const makeNotifierKit = defineDurableKind(notifierHandle, initNotifierState, {
  notifier: { getUpdateSince },
  updater: { updateState },
});

There are lots of details to figure out, but the basic idea is that we remember the state, but not any promises that we've returned. The upgrade process will reject all promises made by the earlier version, which means clients of this DurableNotifier will see rejections upon upgrade. They must react to that by coming back to the Notifier (durable) and using getUpdateSince to ask for a new promise (ephemeral).

The RAM needs are O(N) in the number of notifiers for which there are any outstanding getUpdateSince() Promises. This depends upon that number being relatively low: we need the high-cardinality Notifiers to be accessed with off-chain #3756 / #4558 non-Promise-based pathways.

@mhofman
Copy link
Member

mhofman commented Apr 12, 2022

Why not use a scalar virtual weak map store, with the durable object as key, and the promise as value?

@warner
Copy link
Member Author

warner commented Apr 12, 2022

That would work too, and would probably be cleaner. But note that it doesn't save any RAM: if you put a Promise (or a Remotable) in a virtual store, the collectionManager must keep that ephemeral object alive with a refcounting table.

@mhofman
Copy link
Member

mhofman commented Apr 12, 2022

Right, the only RAM saving solution is to have virtual/durable promises, and make sure to avoid introducing native promises in the chain. While I believe that's possible, it's a discussion for another day.

@Chris-Hibbert
Copy link
Contributor

Is there a way to make the rejection distinctive so the client can tell that it's an interruption in the stream and not some other kind fo dead notifier?

@mhofman
Copy link
Member

mhofman commented May 12, 2022

While the discussion has veered wildly off-topic, I believe that was the goal of #5185 to find a reliable way.

@Tartuffo
Copy link
Contributor

Do we still need this for MN-1 on upgrade can we refresh the notifier from its source?

@warner
Copy link
Member Author

warner commented Jul 22, 2022

I think this ticket has (appropriately) mutated into "please provide a durable notifier", which internally uses the close over virtual table trick. Because the notifier is durable, it can be held by durable contracts/etc without problems. I'll update the title.

When @gibson042 and @FUDCo and I were talking about this, we figured that it would introduce a new constraint: durable notifiers can only publish durable data. The requirement would arise from a goal of "the first getUpdateSince request in version-2 should promptly report the last value that was published during version-1.

@dtribble said we don't really need that: it would be ok for the first getUpdateSince in version-2 to wait until the first post-upgrade publish event. This is basically expanding the definition of "upgrade trauma" to include not just the early promise (the last one retrieved pre-upgrade) being surprisingly rejected, but also the later promise (the first one retrieved post-upgrade) firing late. If we're all ok with that, then we don't need to introduce the "durable notifier can only publish durable data" constraint.

@warner warner changed the title how to hold virtual notifier in durable data build durable notifier Jul 22, 2022
@warner
Copy link
Member Author

warner commented Jul 22, 2022

We sketched out one possible API:

handle = provideDurableNotifierKitHandle(baggage, keyname);
const { notifier, updater } = makeDurableNotifierKit(handle, ...initialState);

or maybe:

const makeDurableNotifierKit = provideDurableNotifierKitMaker(baggage, keyname);
const { notifier, updater} = makeDurableNotifierKit(...initialState);

(but note that @FUDCo was not a fan of either)

As with other durablization work, we're seeing a pattern emerging: as a caller, if you want a library to give you durable objects, you must provide it with some durable baggage (where it can record at least the KindHandle that it uses for the durable Kind, perhaps other data).

We want to keep the number of durable Kinds to a minimum, both because each one requires some amount of RAM (the instances are virtual+durable, but the Kinds themselves are not), and because the next incarnation of the vat is obligated to re-attach behavior for each one. So it must not be the case that each call to makeDurableNotifierKit() causes the creation of a new Kind.

But given the challenge of coordinating multiple users of durable notifiers, and our appropriate reluctance to use impure module-level state (e.g. a module-level let flag to remember whether defineDurableKind had been called already or not), we figured it might be tolerable if distinct callers wound up with distinct Kinds, at long as each invocation of those callers did not create a distinct Kind. E.g.:

  • component A needs durable notifiers, so during initializeA(baggageA), it calls provideDurableNotifierKitMaker to get a makeDurableNotifierKit, creating the first Kind
    • later, each time A does some work, it uses its stored makeDurableNotifierKit to make a new one
  • component B, in the same program/vat, also needs durable notifiers, so initializeB(baggageB) does the same think, resulting in a second Kind (same behavior, different handle, different Kind)

However we should promote a pattern where provideDurableNotifierKitMaker is invoked as close to the vat entry point (buildRootObject) as possible, and then makeDurableNotifierKit is passed down into the subcomponents that need it. Where makeNotifierKit can be ambient, the need for some durable state makes makeDurableNotifierKit non-ambient. It is annoying to thread an authority like this through many layers of subroutines, but we're familiar with the process.

@warner
Copy link
Member Author

warner commented Jul 27, 2022

@mhofman asked:

Why not use a scalar virtual weak map store, with the durable object as key, and the promise as value?

Oh, I realized a better answer why we need Map (or an ephemeral Store, aka makeLegacyMap, if you like the init API): we need to store the resolve()/reject() functions, not just the Promise. (Or instead of the Promise entirely, if you're writing the flavor of notifier that's allowed to return a new Promise to each caller).

The resolve/reject functions are not methods of a Passable object, they're just bare functions, and we don't have a Far equivalent for bare functions (at least passStyleOf(harden(x => x+1)) gets me an A non-object cannot be a tagRecord: "[Function <anon>]" error, although #4932 is related).

Promises can acquire identities, so they can be serialized. But that doesn't make them durable (yet]): liveslots carefully hangs on to the ephemeral Promise object for as long as the object is refcounted by virtual data or exported to the kernel. So we can put Promises in a virtual Map, but not the resolve/reject functions, and that's what the notifier needs to retain.

It may indeed be slightly tidier to use a WeakMap (or makeLegacyWeakMap), keyed by the Notifier or Updater instance, rather than a Map keyed by an integer. The WeakMap will really be a VirtualObjectAwareWeakMap, provided by liveslots, which knows how to peek into a Representative and use the vref as a key in a real Map (and do the refcount checks to delete the entry if/when the virtual/durable object goes away). There's more overhead to using that (refcount management), but it does remove the worry that the resolve/reject functions might stick around if we accidentally forget to explicitly delete them.

While it feels a bit cleaner to use the Updater as the key, rather than the Notifier, it probably doesn't matter in practice, because I think both will be facets of the same durable-object cohort, which means they'll keep each other alive (along with the state object, context, and all the intermediate objects that might otherwise be used to build a GC sensor).

@mergify mergify bot closed this as completed in #6502 Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request notifier SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants