feat(xsnap)!: refactor xsnap wrapper and snapStore to use streams for snapshots #7531
Conversation
There was a problem hiding this comment.
I like the refactoring.. feels cleaner, and clearly paves the way for the next step.
Apart from the snapshot name quibbles, it looks good to me. Don't over-rotate on finding better names, but since the simplest possible API a newcomer could imagine would be for complete snapshot Buffers to be passed around, and this API is the second-simplest possible API (streams being passed around), we should choose API names that help that newcomer realize we're doing the streams thing instead of the blobs/buffer thing.
| * @returns {Promise<SnapshotResult>} | ||
| */ | ||
| async function saveSnapshot(vatID, snapPos, saveRaw) { | ||
| async function saveSnapshot(vatID, snapPos, snapshot) { |
There was a problem hiding this comment.
Could we call this snapshotStream or readStream or snapshotReader? The name snapshot makes me think it's an actual complete Uint8Array or a Buffer, not a stream.
|
|
||
| const { result: hash, duration: compressSeconds } = | ||
| const { duration: compressSeconds, result: compressedSnapshot } = |
There was a problem hiding this comment.
I know the variable was there in the original, but "compressSeconds".. with the tee that's happening, this isn't really measuring the time it takes to compress the snapshot, right? It's more like the time it takes for the writer to give us the snapshot, plus the time it takes us to compress it?
Eventually we should look at the timers we're using and decide if they're providing us with meaningful data.
There was a problem hiding this comment.
Correct but since I still expect the compression to be the limiting factor, I didn't feel too bad leave it at compressSeconds. A createAndCompressSeconds would be more accurate, but that's a mouthful. Open to suggestions.
| }); | ||
| const hash = hashStream.digest('hex'); |
There was a problem hiding this comment.
Let's call this snapshotID, that's the name I started using recently in the new transcript entries.
Although.. the DB schema might use hash. If so, I'd be in favor of changing the schema.
| async function* loadSnapshot(vatID) { | ||
| const loadInfo = sqlLoadSnapshot.get(vatID); | ||
| loadInfo || Fail`no snapshot available for vat ${q(vatID)}`; | ||
| const { hash, compressedSnapshot } = loadInfo; |
| gzReader.destroy(); | ||
| // eslint-disable-next-line @jessie.js/no-nested-await | ||
| await finished(gzReader); | ||
| const h = hashStream.digest('hex'); |
There was a problem hiding this comment.
.. which would open up hash to use here
packages/xsnap/src/xsnap.js
Outdated
| * @property {(request:Uint8Array) => Promise<Uint8Array>} [handleCommand] | ||
| * @property {string} [name] | ||
| * @property {boolean} [debug] | ||
| * @property {number} [netstringMaxChunkSize] in bytes (must be an integer) | ||
| * @property {number} [parserBufferSize] in kB (must be an integer) | ||
| * @property {string} [snapshot] | ||
| * @property {AsyncIterable<Uint8Array>} [snapshot] |
There was a problem hiding this comment.
Maybe change this argument name to snapshotStream? The old one should really have been snapshotFilename.
packages/xsnap/src/xsnap.js
Outdated
| } | ||
| await (snapshot && | ||
| (async () => { | ||
| const { hint = 'unknown' } = /** @type {any} */ (snapshot); |
There was a problem hiding this comment.
huh, the argument is a stream, but it might have a .hint property? is that common?
There was a problem hiding this comment.
Yeah poor attempt at plumbing through a description for the tracing feature. Will redo.
packages/xsnap/src/xsnap.js
Outdated
| vatExit.promise.catch(noop).then(() => startCleanup && startCleanup()); | ||
| } | ||
|
|
||
| const [, , , writer, reader] = |
There was a problem hiding this comment.
[, , , writer, reader] feels less readable than .. stdio[3]/etc. It'd be nice if someon reading this code could immediately tell that we're using fd3/fd4 here. Hopefully there's some way of making the types work too.
There was a problem hiding this comment.
This commit shouldn't have made it in, part of the streaming with the worker process
packages/xsnap/src/xsnap.js
Outdated
| const result = baton.then(async () => { | ||
| await messagesToXsnap.next(encoder.encode(`w${file}`)); | ||
| await runToIdle(); | ||
| async function* makeSnapshot(hint = 'unknown') { |
There was a problem hiding this comment.
maybe makeSnapshotReader or makeSnapshotReadStream? although it doesn't feel as urgent for this case, somehow
packages/xsnap/src/replay.js
Outdated
| @@ -116,7 +119,7 @@ export function recordXSnap(options, folderPath, { writeFileSync }) { | |||
| name = '_replay_', | |||
| debug = false, | |||
| parserBufferSize = undefined, | |||
| snapshot = undefined, | |||
| snapshot, | |||
There was a problem hiding this comment.
ditto, snapshot sounds like a complete buffer
mhofman
force-pushed
the
mhofman/7490-xsnap-stream-snapshot-refactor
branch
from
b411f5f to
66c5d8e
Compare
I've done the rename to I had to force-push the changes to avoid conflicts, but I kept the changes in separate commit and have not changed the original ones, so you can focus on the |
mhofman
force-pushed
the
mhofman/7490-xsnap-stream-snapshot-refactor
branch
from
9caba01 to
ed87de1
Compare
closes: #7490
Description
Refactors the xsnap.js wrapper around the xsnap-worker, and the snapStore to use streams when loading and saving snapshots. This moves the temporary file logic from the snapstore to xsnap in preparation of #6363.
Security Considerations
None
Scaling Considerations
This will ultimately allow moving away from writing raw snapshots to disk.
Documentation Considerations
We lose the ability to record information about the loaded snapshot in xsnap traces which was previously carried in the temporary filename.
Updated some markdown docs to reflect the new API
Testing Considerations
Updated all related tests.