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): boot xs-worker vats using snapshots from snapstore #2370

Merged
merged 3 commits into from
Apr 1, 2021

Conversation

dckc
Copy link
Member

@dckc dckc commented Feb 9, 2021

This is an initial snapstore API integrated with the xs-worker vat manager factory.
In rough measurements over the swingset tests, it saves 40% over running without snapshots (2:29 vs. 3:59).

XXX enhances test:xs-worker to run all swingset tests with SWINGSET_WORKER_TYPE=xs-worker. 3 tests seem to fail, currently:

  1. device-bridge › bridge device can return undefined
  2. metering › dynamic-vat-unmetered › unmetered dynamic vat
  3. metering › within-vat › metering within a vat - failed to setBundle: (an object)

Recall that test:xs is currently option, so these don't block merges.

ref #2273

I'd like to say "fixes..." but #2273 includes discussion of kernel DB state, taking snapshots after the initial supervisor bootstrap, etc.

@dckc dckc requested a review from warner February 9, 2021 18:58
@dckc dckc force-pushed the 2273-snapstore branch 2 times, most recently from b1bd181 to 575dfb8 Compare February 9, 2021 19:47
@dckc
Copy link
Member Author

dckc commented Feb 11, 2021

Ouch... the size of the compressed snapshot is different between nodejs 12.x and 14.x. Maybe ziplib was refined?

The sha512 of the uncompressed snapshot didn't change, though... is that close enough, @warner ?

@dckc dckc force-pushed the 2273-snapstore branch 3 times, most recently from d9f9918 to 5ac3db1 Compare February 11, 2021 16:28
@dckc
Copy link
Member Author

dckc commented Feb 11, 2021

@warner please take a look.

Note: to get all the tests to pass, I had to tolerate different size compression results that seem to be due to different node versions: 5ac3db1

@warner warner self-assigned this Feb 15, 2021
@dckc dckc enabled auto-merge (squash) February 15, 2021 19:17
@dckc dckc force-pushed the 2273-snapstore branch 2 times, most recently from b6f5f29 to b641d27 Compare February 16, 2021 02:10
@warner
Copy link
Member

warner commented Feb 16, 2021

Ouch... the size of the compressed snapshot is different between nodejs 12.x and 14.x. Maybe ziplib was refined?

The sha512 of the uncompressed snapshot didn't change, though... is that close enough, @warner ?

Yeah, let's define the snapshot ID as the hash of the uncompressed file. That's not quite as convenient as hashing the compressed version, but it's more appropriate, and would let us change the compression algorithm in the future without inadvertently changing the IDs.

@dckc
Copy link
Member Author

dckc commented Feb 16, 2021

fileHash wasn't right and fixing it seems to reveal that xsnap is not deterministic

p.s. for follow-up, see #2776

@dckc dckc disabled auto-merge February 16, 2021 17:44
@dckc dckc marked this pull request as draft February 16, 2021 17:44
@dckc dckc force-pushed the 2273-snapstore branch 3 times, most recently from 308c4ff to c9341d4 Compare April 1, 2021 15:42
@dckc dckc marked this pull request as ready for review April 1, 2021 16:47
@dckc dckc requested a review from warner April 1, 2021 16:47
@dckc
Copy link
Member Author

dckc commented Apr 1, 2021

@warner I brought this back up-to-date.

No callers of makeSwingsetController are setting the snapstorePath option yet, so it's only exercised in testing.

@dckc dckc requested a review from FUDCo April 1, 2021 16:48
@dckc dckc force-pushed the 2273-snapstore branch 2 times, most recently from 704f798 to 90d90f3 Compare April 1, 2021 19:42
@dckc
Copy link
Member Author

dckc commented Apr 1, 2021

ok, @warner @FUDCo I'm finally getting the ci results I expect. Please take a look.

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.

Very nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants