fix(swingset): stop using a persistent kernel bundle

[warner]

Previously, the kernel sources were bundled during
initializeSwingset(), and stored in the kvStore portion of the
swingstore kernelDB. It was read from that DB each time
makeSwingsetController() launched the application process.

That ensured the kernel behavior would remain consistent (and
deterministic) even if the user's source tree changed. Using a
persistent kernel bundle also speeds up application launch: we save
about 1.8s by not re-running bundleSource on each launch.

Once upon a time I thought this consistency was important/useful, but
now I see that it causes more problems than it's worth. In particular,
upgrading a chain requires an awkward "rekernelize" action to replace
the kernel bundle in the database. And we believe we can reclaim about
1.0s per launch by moving to a not-yet-written importLocation() Endo
tool that loads module graphs from disk without serializing the
archive in the middle.

This patch changes swingset to stop using a persistent kernel
bundle. Each application launch (i.e. makeSwingsetController() call)
re-bundles the kernel sources just before importing that bundle, so
that updated source trees are automatically picked up without
additional API calls or rekernelize steps. The bundle is no longer
stored in the kvStore.

The other bundles (vats, devices, xsnap helpers) are still created
during initializeSwingset and stored in the DB, since these become
part of the deterministic history of each vat (#4376 and #5703 will
inform changes to how those bundles are managed). Only the kernel
bundle has become non-persistent.

Bundling is slow enough that many unit tests pre-bundle both the
kernel and the sources of built-in vats, devices, and xsnap helper
bundles. This provides a considerable speedup for tests that build (or
launch) multiple kernels within a single test file. These
kernelBundles are passed into initializationOptions, which then
bypassed the swingset-bundles-it-for-you code. This saves a minute or
two when running the full swingset test suite.

Unit tests can still use initializationOptions to amortize bundling
costs for the non-kernel bundles. runtimeOptions now accepts a
kernelBundle member to amortize bundling the kernel itself. Tests
which use this trick and also use makeSwingsetController() need to
be updated (else they'll run 1.8s slower). All such tests were inside
packages/SwingSet/test/ and have been updated, which would otherwise
cause the test suite to run about 50-60s slower.

The other agoric-sdk packages that do swingset-style tests are mostly
using the deprecated buildVatController(), whose options continue to
accept both kinds of bundles, and do not need changes.

closes #5679

[mhofman]

LGTM, mostly nits on option naming.

I'd like to see the slog-to-otel file updated to avoid the console err spam (arguably minimal here)

[mhofman]

Adding slog events should also result in adding a case in slog-to-otel. I'm open to suggestions on how to test this (either through types or unit tests).

[warner]

Yeah, until/unless we have slog-to-otel tests (was that sketch I wrote at all useful?), I guess types are good enough. Adding a single span with name bundle-kernel or swingset-bundle-kernel that gets recorded when the bundle-kernel-finish appears might be nice, although really I don't know how helpful it'll be to get telemetry on this, it's always going to be about the same amount of time, and happens rarely.

[mhofman]

It seems that kernelBundle (formerly kernel) is not used by initializeSwingset ? Should kernelBundles be destructured first to split the pieces? And maybe the initializationOptions property renamed to be explicit it no longer looks at the kernel option?

Aka:

  const {
    hostStorage = provideHostStorage(),
    env,
    verbose,
    kernelBundles: { kernel: kernelBundle, ...vatAndDeviceBundles } = {},
    debugPrefix,
    slogCallbacks,
    warehousePolicy,
    slogFile,
  } = runtimeOptions;

[warner]

Right, kernelBundle is no longer used by initializeSwingset. It accepts a kernelBundles (plural) that has the vat/device/lockdown/supervisor bundles (aka "bundles that the kernel needs") but not kernelBundle (aka "the bundle that contains the actual kernel"). But I figured it wasn't worth the work to remove it: initializeSwingset will just ignore the spurious property.

This buildVatController is a deprecated API that's mostly used by external unit tests, so I didn't want to change the signature significantly. And by having buildVatController accept an option object that includes both kernelBundle and the other bundles, none of those tests needs to change, and they all continue to benefit from the amortized bundling costs. But the consequence is that the name kernelBundles as used in the third argument of buildVatController (and also the return value of the exported buildKernelBundles() function) is a slightly different/larger type than the "not the kernel too" kernelBundles accepted by the third argument of initializeSwingset.

I guess your proposal would look like:

  const {
    hostStorage = provideHostStorage(),
    env,
    verbose,
    kernelBundles: { kernelBundle, ...otherBundles } = {},
    debugPrefix,
    slogCallbacks,
    warehousePolicy,
    slogFile,
  } = runtimeOptions;
  const actualRuntimeOptions = {
    env,
    verbose,
    debugPrefix,
    slogCallbacks,
    warehousePolicy,
    slogFile,
    kernelBundle,
  };
  const initializationOptions = { verbose, kernelBundles: otherBundles };

right? I'd be ok with that if it reads better to you.

And the wart is that initializationOptions.kernelBundles is a different type than actualRuntimeOptions.kernelBundles. I could change makeSwingsetController to take an option with a different name, but 1: that's a lot of churn and 2: kernelBundles is pretty good and 3: vatAndDeviceAndSupervisorAndLockdownBundles is pretty bad :).

[warner]

nope, kernelBundles: { kernelBundle, ...otherBundles } = {} means that otherBundles is set to an empty object, even if the caller didn't provide kernelBundles, which makes initializeSwingset() skips the default-value call to await buildVatAndDeviceBundles():

  const {
    kernelBundles = await buildVatAndDeviceBundles(),
    verbose,
    addVatAdmin = true,
    addComms = true,
    addVattp = true,
    addTimer = true,
  } = initializationOptions;

so they wind up empty.

I think I'd have to do something like:

  const {
    hostStorage = provideHostStorage(),
    env,
    verbose,
    kernelBundles: kernelAndOtherBundles = {},
    debugPrefix,
    slogCallbacks,
    warehousePolicy,
    slogFile,
  } = runtimeOptions;
  const { kernel: kernelBundle, ...otherBundles } = kernelAndOtherBundles;
  const kernelBundles = runtimeOptions.kernelBundles ? otherBundles : undefined;

[mhofman]

Oh yeah that'd be a problem.
Starting to be not worth it!

[mhofman]

Note to self: it previously was kernelBundles.kernel, renamed in buildKernelBundles

[mhofman]

Is there a reason not to extract this src helper out of this function and buildVatAndDeviceBundles and into top level?

[warner]

sure

[mhofman]

Any reason to rename the previously used kernel to kernelBundle ? I'm always warry of such renames with our typing info so sparse.

[warner]

When the value is passed into runtimeOptions, I wanted it to be named kernelBundle and not kernel (since at that point it's no longer living inside a container with bundles in the name). I want to keep using buildKernelBundles and buildVatController(.. initializationOptions.kernelBundles) names to avoid rewriting a bunch of other tests.

I guess one option would be to have buildKernelBundles and buildVatController create/accept .kernel, and then rename it within buildVatController to pass runtimeOptions.kernelBundle into makeSwingsetController. Lemme see how that looks.

[warner]

@mhofman New version pushed, see what you think about the changes

[mhofman]

Thanks for addressing those nits. I think you were right thought, and excluding the kernelBundle from the rest may not be worth it.

I'll address the otel stuff in my upcoming PR since we already have spam from the unknown snapshot slot events anyway.

[mhofman]

Yeah sorry that's unfortunately not as clean as I originally thought!

[warner]

Oh, sorry, I missed the fact that slog-to-otel errors everything it doesn't recognize.. I wonder if it'd be better to ignore those events, it's not like the operator can do anything about it, and it's not fatal or damaging.

[warner]

Eh, it works now, and I fixed the type problems, so I guess I'll go with the current version.