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

Add Engine::precompile_compatibility_hash #5826

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

lann
Copy link
Contributor

@lann lann commented Feb 17, 2023

This method returns a Hash, the output of which can be used to index precompiled binaries from one Engine instance that can be deserialized by another Engine instance.

Closes #5802

@lann lann force-pushed the precompile-compat-key branch from fb49f07 to 1163fb0 Compare February 17, 2023 21:27
crates/wasmtime/src/engine.rs Outdated Show resolved Hide resolved
@lann lann changed the title Precompile compat key Add Engine::precompile_compatibility_key Feb 17, 2023
@lann lann force-pushed the precompile-compat-key branch from 1163fb0 to 8a14477 Compare February 17, 2023 21:50
crates/wasmtime/src/engine.rs Outdated Show resolved Hide resolved
Comment on lines 242 to 247
/// Returns a value that can be used to check precompiled WebAssembly compatibility.
///
/// The outputs of [`Engine::precompile_module`] and [`Engine::precompile_component`]
/// are compatible with a different [`Engine`] instance only if the two engines use
/// compatible [`Config`]s. The value returned by this function will be the same for two
/// [`Engine`]s if a binary from one can be deserialized by the other.
Copy link
Member

Choose a reason for hiding this comment

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

I think the documentation here may want to be a bit more nuanced because if two blobs are the same then you should be able to load between engines but if they're different they may still load correctly. For example different values of generate_address_map will produce different blobs but that's not checked when loading modules into engines.

I think it may be best to reword this as something along the lines of "if two blobs are equal artifacts from one engine are guaranteed to be loadable in another" or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's what I was trying to convey but "if" is ambiguous. I'll be more explicit about it.

crates/wasmtime/src/engine.rs Show resolved Hide resolved
@lann lann force-pushed the precompile-compat-key branch from 8a14477 to babc4a9 Compare February 17, 2023 22:27
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Feb 17, 2023
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@lann
Copy link
Contributor Author

lann commented Mar 31, 2023

Finally getting back around to this, I realized that the cache system already generates a key like this, and sure enough this has exactly what I was looking for:

/// This is a helper struct used when caching to hash the state of an `Engine`
/// used for module compilation.
///
/// The hash computed for this structure is used to key the global wasmtime
/// cache and dictates whether artifacts are reused. Consequently the contents
/// of this hash dictate when artifacts are or aren't re-used.
#[cfg(all(feature = "cache", compiler))]
struct HashedEngineCompileEnv<'a>(&'a Engine);
#[cfg(all(feature = "cache", compiler))]
impl std::hash::Hash for HashedEngineCompileEnv<'_> {

Reusing this code as-is would require changing the function signature a bit, e.g.:

pub fn precompile_compatibility_hash(&self) -> impl std::hash::Hash {
    crate::module::HashedEngineCompileEnv(self)
}

@alexcrichton
Copy link
Member

Sounds reasonable to me!

This method returns a Hash, the output of which can be used to index
precompiled binaries from one Engine instance that can be deserialized
by another Engine instance.
@lann lann force-pushed the precompile-compat-key branch from babc4a9 to a5dec57 Compare April 18, 2023 21:03
@lann lann changed the title Add Engine::precompile_compatibility_key Add Engine::precompile_compatibility_hash Apr 18, 2023
@lann lann marked this pull request as ready for review April 18, 2023 21:04
@lann lann requested a review from a team as a code owner April 18, 2023 21:04
@lann lann requested review from jameysharp and removed request for a team April 18, 2023 21:04
Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

This looks sensible to me and appears to be exactly what Alex already said sounded good, so I'll go ahead and merge it. Thank you!

@jameysharp jameysharp enabled auto-merge April 18, 2023 21:15
@jameysharp jameysharp added this pull request to the merge queue Apr 18, 2023
Merged via the queue into bytecodealliance:main with commit 51ed20a Apr 18, 2023
eduardomourar pushed a commit to eduardomourar/wasmtime that referenced this pull request Apr 28, 2023
This method returns a Hash, the output of which can be used to index
precompiled binaries from one Engine instance that can be deserialized
by another Engine instance.
@lann lann deleted the precompile-compat-key branch September 23, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Engine::serialization_compatibility_hash
4 participants