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

WIP: Store compiled modules #44

Closed
wants to merge 1 commit into from
Closed

Conversation

cpuguy83
Copy link
Member

Modules are compiled and stored in a content addressable store. Two runs of the same module are significantly faster since we only need to digest the wasm module being loaded and do not need to compile it again.

This is still a work in progress.
The store is not really concurrency safe yet (it mostly works, but multiple threads/processes could hit issues).

Modules are compiled and stored in a content addressable store.
Two runs of the same module are significantly faster since we only need
to digest the wasm module being loaded and do not need to compile it
again.

This is still a work in progress.
The store is not really concurrency safe yet (it mostly works, but multiple
threads/processes could hit issues).

Signed-off-by: Brian Goff <[email protected]>
@cpuguy83 cpuguy83 requested a review from devigned December 20, 2022 00:17
@cpuguy83
Copy link
Member Author

@rumpl

Comment on lines +99 to +104
let mut file = File::open(path).context("could not open metadata file")?;
let mut buf = String::new();
file.read_to_string(&mut buf)
.context("could not read metadata file")?;

Ok(serde_json::from_str(&buf).context("could not parse metadata file")?)
Copy link
Member

Choose a reason for hiding this comment

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

This could be written like

let data = fs::read_to_string(path).context("could not read metadata file");
Ok(serde_json::from_str(&data).context("could not parse metadata file")?)

fs::read_to_string does the exact same thing your code does


impl ContentWriter {
pub fn write(&mut self, data: &[u8]) -> Result<usize> {
self.f.try_clone()?.write(data).context("write")
Copy link
Member

Choose a reason for hiding this comment

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

Curious why do you try_clone here? Can't we directly write self.f.write(data).context("write")?

Copy link
Member Author

Choose a reason for hiding this comment

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

Lots of issues with borrowing if I recall correctly. Could also just be a remnant of me trying to get a different implementation working with file locking.

@squillace
Copy link

@cpuguy83 does this make any allowance for precise understanding of the module to check whether it's been cached? That is, it seems like it checks a sha.... which I'd be assuming is a digest. Am I right? Just want to make sure it's easy to move from a quick, possibly unsafe check to a more formal one down the line.... looks GREAT though.

@cpuguy83
Copy link
Member Author

cpuguy83 commented Jan 2, 2023

@squillace This is based on the digest of the module from the image.
There does seemt o be some extra namespacing we need to do at least for wasmtime since it (IIRC) will reject modules compiled for a different version of wasmtime.

@squillace
Copy link

I'm wondering whether that's really just a load problem: if you pull an arm image on an amd node, you'll get the same "I just can't today" error. Though if there's a coherent way to determine and handle it in the future.....

@jsturtevant
Copy link
Contributor

With the wasm oci work, this becomes possible and I've been working a bit on testing it out. I'll open a separate that does something similiar.

@jsturtevant
Copy link
Contributor

going to close this in favor of #405. Thanks @cpuguy83 for the inspiration!

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.

4 participants