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(swing-store): faster import of swing-store #8522

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

mhofman
Copy link
Member

@mhofman mhofman commented Nov 9, 2023

closes: #8521

Description

Disable journaling and full synchronous pragma modes during import, resulting in an import time for mainnet state of 9 minutes vs 33 minutes.

2023-11-09T20:59:28.218650751Z Restoring SwingSet state from snapshot at block height 12416000 with options {"exportDir":"/tmp/agd-swing-store-restore-12416000-727241191","artifactMode":"replay","exportDataMode":"all"}
2023-11-09T21:08:27.095272275Z 9:08PM INF restored swing-store export exportDir=/tmp/agd-swing-store-restore-12416000-727241191 height=12416000 module=x/swingset submodule=SwingStoreExportsHandler

Adds a check that the journaling pragma has been successfully applied to prevent running in unsafe conditions.

To avoid confusions and mis-usage, importSwingStore no longer returns the kernelStorage facet of the SwingStore, to make it clear it cannot be used for execution.

Security Considerations

This change could technically result in a corrupted DB if interrupted, however the import process in cosmic-swingset writes the expected block height just before commiting. If that block height is not correct on start, the node will refuse to start thanks to hangover checks.

Scaling Considerations

Cuts disk usage in half during the swing-store import

Documentation Considerations

None

Testing Considerations

Manually tested on a patched follower, and modified the state-sync test to reflect the open close of connection.

Upgrade Considerations

None

@mhofman mhofman requested review from warner and FUDCo November 9, 2023 21:36
@mhofman mhofman added the automerge:rebase Automatically rebase updates, then merge label Nov 9, 2023
@@ -194,6 +194,21 @@ export function makeSwingStore(dirPath, forceReset, options = {}) {
// { verbose: console.log },
);

function setUnsafeFastMode(enabled) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's probably a better name for this, but the functionality seems good to me.

@@ -9,6 +9,7 @@ import { Fail, q } from '@agoric/assert';
* transcriptStore: TranscriptStoreInternal,
* snapStore: SnapStoreInternal,
* bundleStore: BundleStoreInternal,
* setUnsafeFastMode: (enabled: boolean) => void,
Copy link
Member

@gibson042 gibson042 Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extension makes yarn lint complain about the first argument to assertComplete no longer satisfying type SwingStoreInternal at

const internal = { snapStore, bundleStore, transcriptStore };
assertComplete(internal, artifactMode);

Maybe modification of SQLite modes should just remain internal to makeSwingStore?

Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I agree with @gibson042 about the API maybe being better but I think this is clear enough for now. Approved but see if you can add the two comments below.

@@ -106,7 +109,10 @@ test('state-sync reload', async t => {
getArtifact: name => artifacts.get(name),
close: () => 0,
};
const ss2 = await importSwingStore(datasetExporter);
const ssi = await importSwingStore(datasetExporter, importDbDir);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means the SwingStore you get back from importSwingStore is no longer suitable for general use, because it's got all the commit-safety modes turned off, yeah?

Please update the docs in docs/data-export.md to mention this fact, around line 170-ish where the importSwingSstore is shown.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, it is not. I have now removed the kernelStorage facet from the return value of importSwingStore.

packages/swing-store/src/swingStore.js Show resolved Hide resolved
@mhofman mhofman removed the automerge:rebase Automatically rebase updates, then merge label Nov 9, 2023
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.

I'd second Richard & Brian's nits, but seems good overall.

@mhofman mhofman requested review from gibson042 and warner November 10, 2023 00:11
@mhofman mhofman added the automerge:rebase Automatically rebase updates, then merge label Nov 10, 2023
@mhofman mhofman force-pushed the mhofman/import-swing-store-fast branch from c953c85 to 5401d58 Compare November 10, 2023 00:16
@mhofman mhofman force-pushed the mhofman/import-swing-store-fast branch from 5401d58 to 6a833eb Compare November 10, 2023 01:05
@mergify mergify bot merged commit 7244c71 into master Nov 10, 2023
68 checks passed
@mergify mergify bot deleted the mhofman/import-swing-store-fast branch November 10, 2023 01:38
mhofman pushed a commit that referenced this pull request Nov 10, 2023
feat(swing-store): faster import of swing-store

So, to avoid pruning current-incarnation historical transcript spans when exporting from one swingstore to another, you must set (or avoid overriding) the following options along the way:

* the original swingstore must not be opened with `{ keepTranscripts: false }`, otherwise the old spans will be pruned immediately
* the export must use `makeSwingStoreExporter(dirpath, { artifactMode: 'replay'})`, otherwise the export will omit the old spans
* the import must use `importSwingStore(exporter, dirPath, { artifactMode: 'replay'})`, otherwise the import will ignore the old spans
* the `importSwingStore` call (and all subsequent `openSwingStore` calls) must not use `keepTranscripts: false`, otherwise the new swingstore will prune historical spans as new ones are created (during `rolloverSpan`).
* subsequent `openSwingStore` calls must not use `keepTranscripts: false`, otherwise the new swingstore will prune historical spans as new ones are created (during `rolloverSpan`).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

retrospective nit: it might be a good idea to retain the admonition against having keepTranscripts: false in the importSwingStore options bag (maybe as a child-bullet of the previous line).. that line was mainly about the importance of including artifactMode: 'replay', but it's also important to not prune transcripts during the import itself

@@ -11,14 +11,16 @@ import { assertComplete } from './assertComplete.js';
*/

/**
* Function used to create a new swingStore from an object implementing the
* Function used to populate a swingStore from an object implementing the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It really is about creating a new swingStore. Changing it to say "populate a swingStore" kind of makes it sound like this could be used to fill a previously-created (perhaps empty, perhaps not) database, which suggests some sort of weird merge operation between the previous contents and the import dataset.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Swing-store import is slow for mainnet
5 participants