Skip to content

Python snapshots: Add workerd cli options to save memory snapshots to disk - #1878

Merged
hoodmane merged 1 commit into
mainfrom
hoodmane/snapshot-to-disk
Apr 16, 2024
Merged

Python snapshots: Add workerd cli options to save memory snapshots to disk#1878
hoodmane merged 1 commit into
mainfrom
hoodmane/snapshot-to-disk

Conversation

@hoodmane

Copy link
Copy Markdown
Contributor

No description provided.

@hoodmane
hoodmane requested review from a team as code owners March 22, 2024 14:04
@hoodmane
hoodmane requested review from Warfields, dom96 and fhanau and removed request for a team March 22, 2024 14:04
@hoodmane
hoodmane force-pushed the hoodmane/reduce-snapshot-memory-usage branch from 6787e25 to 829315b Compare March 22, 2024 15:28
@hoodmane
hoodmane force-pushed the hoodmane/snapshot-to-disk branch 2 times, most recently from fac0b35 to 1761852 Compare March 22, 2024 16:19
@hoodmane
hoodmane force-pushed the hoodmane/reduce-snapshot-memory-usage branch from 829315b to 8d2fe64 Compare March 22, 2024 16:33
@hoodmane
hoodmane force-pushed the hoodmane/snapshot-to-disk branch from 1761852 to d5cd1a0 Compare March 22, 2024 16:33
@hoodmane
hoodmane force-pushed the hoodmane/reduce-snapshot-memory-usage branch from 8d2fe64 to 84687f2 Compare March 22, 2024 16:35
@hoodmane
hoodmane force-pushed the hoodmane/snapshot-to-disk branch 2 times, most recently from a0f220b to 9b718f6 Compare March 22, 2024 17:17
@kentonv

kentonv commented Mar 22, 2024

Copy link
Copy Markdown
Member

Since this doesn't affect production, is an autogate actually necessary? Autogates exist for the purpose of rolling things out to production.

Maybe this behavior should be enabled by something in workerd/server/server.c++ instead?

@hoodmane

hoodmane commented Mar 25, 2024

Copy link
Copy Markdown
Contributor Author

@kentonv This makes sense, are there other development-only features that I could look at for inspiration?

@kentonv

kentonv commented Mar 25, 2024

Copy link
Copy Markdown
Member

are there other development-only features that I could look at for inspiration?

Perhaps durableObjectStorage in workerd.capnp? Configures local disk storage for durable objects.

@hoodmane
hoodmane force-pushed the hoodmane/reduce-snapshot-memory-usage branch from 84687f2 to 34da5ba Compare March 26, 2024 14:24
@garrettgu10

garrettgu10 commented Mar 26, 2024

Copy link
Copy Markdown
Contributor

Seems like there's some overlap with #1851. Can you take a look?

Specifically, is there a possibility to reuse the DiskCache to store snapshots as well?

Base automatically changed from hoodmane/reduce-snapshot-memory-usage to main March 27, 2024 11:16
@hoodmane

hoodmane commented Mar 27, 2024

Copy link
Copy Markdown
Contributor Author

is there a possibility to reuse the DiskCache to store snapshots as well?

Yeah, this would make a lot of sense.

@jasnell jasnell added the python Issues/PRs relating to Python Workers label Apr 10, 2024
@hoodmane
hoodmane force-pushed the hoodmane/snapshot-to-disk branch from 9b718f6 to a1da8c7 Compare April 12, 2024 14:33
@hoodmane hoodmane changed the title Python snapshots: Add autogate to use snapshots from disk Python snapshots: Add workerd cli options to save memory snapshots to disk Apr 12, 2024
@hoodmane
hoodmane force-pushed the hoodmane/snapshot-to-disk branch from a1da8c7 to 9ef85a6 Compare April 12, 2024 14:52
if (ArtifactBundler.isEwValidating()) {
ArtifactBundler.storeMemorySnapshot(getMemoryToUpload());
} else if (SHOULD_SNAPSHOT_TO_DISK) {
DiskCache.put("snapshot.bin", getMemoryToUpload());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not blocking, but I think we should name the snapshot something different for each worker, since eventually we want to load the snapshot from disk cache in addition to storing it. I think it may make sense to turn the python-save-snapshot flag into a string where the string is the file name, or use some other unique identifier for the worker.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah for now I just want some way to get snapshots onto disk that I can use for tests. Note that this doesn't even implement a way to load the snapshot in workerd, only to save it. Which is probably much higher priority than giving the files distinct names...

@hoodmane
hoodmane force-pushed the hoodmane/snapshot-to-disk branch 4 times, most recently from 49ed68f to a51eff5 Compare April 16, 2024 11:03
Comment thread src/pyodide/internal/snapshot.js Outdated
Comment thread src/pyodide/python-entrypoint-helper.js
Comment thread src/workerd/server/server.h Outdated
Comment thread src/workerd/server/workerd-api.c++ Outdated
Comment thread src/workerd/server/workerd.c++ Outdated
@hoodmane
hoodmane force-pushed the hoodmane/snapshot-to-disk branch from 18eb328 to b965d89 Compare April 16, 2024 14:36
@hoodmane
hoodmane merged commit 2863624 into main Apr 16, 2024
@hoodmane
hoodmane deleted the hoodmane/snapshot-to-disk branch April 16, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Issues/PRs relating to Python Workers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants