Stream: git-wasmtime

Topic: wasmtime / PR #5826 Add Engine::precompile_compatibility_key

Wasmtime GitHub notifications bot (Feb 17 2023 at 21:31):

lann edited PR #5826 from precompile-compat-key to main:

Draft for #5802 for naming / impl feedback prior to writing tests.

After writing the implementation I landed on precompile_compatibility_key to show the relationship with precompile_{module,component}, but can revert to the discussed cache_key if desired.

Wasmtime GitHub notifications bot (Feb 17 2023 at 21:32):

lann edited PR #5826 from precompile-compat-key to main:

Draft for #5802 for naming / impl feedback while working on tests.

After writing the implementation I landed on precompile_compatibility_key to show the relationship with precompile_{module,component}, but can revert to the discussed cache_key if desired.

Wasmtime GitHub notifications bot (Feb 17 2023 at 21:36):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 17 2023 at 21:36):

bjorn3 created PR review comment:

Hashbrown uses ahash which doesn't guarantee stability between different systems. In fact it returns different values between systems with hardware AES instructions and those that don't. As such using ahash here will break cross-compilation.

Wasmtime GitHub notifications bot (Feb 17 2023 at 21:44):

lann submitted PR review.

Wasmtime GitHub notifications bot (Feb 17 2023 at 21:44):

lann created PR review comment:

Ah good to know, thanks.

Wasmtime GitHub notifications bot (Feb 17 2023 at 21:50):

lann updated PR #5826 from precompile-compat-key to main.

Wasmtime GitHub notifications bot (Feb 17 2023 at 21:52):

lann submitted PR review.

Wasmtime GitHub notifications bot (Feb 17 2023 at 21:52):

lann created PR review comment:

Replaced with sha2, which is probably where I should have started anyway. Its a new dep under some feature sets but it is already used by the cache system.

Wasmtime GitHub notifications bot (Feb 17 2023 at 21:53):

lann edited PR review comment.

Wasmtime GitHub notifications bot (Feb 17 2023 at 21:53):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 17 2023 at 21:53):

bjorn3 created PR review comment:

sha512 is faster on 64bit systems as it can consume more bytes at once. Not that it really matters here. This hashing is highly unlikely to ever become a bottleneck.

Wasmtime GitHub notifications bot (Feb 17 2023 at 22:20):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 17 2023 at 22:20):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 17 2023 at 22:20):

alexcrichton created PR review comment:

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.

Wasmtime GitHub notifications bot (Feb 17 2023 at 22:20):

alexcrichton created PR review comment:

I think it's fine to just return the whole blob here and let embedders hash it if they want. That avoids a dependency here and should enable a bit more flexibility for embedders as well.

Wasmtime GitHub notifications bot (Feb 17 2023 at 22:20):

alexcrichton created PR review comment:

If this compiles without the #[cfg] no need for the attribute. If the #[cfg] is required, though, then this attribute is also required for better display in documentation.

Wasmtime GitHub notifications bot (Feb 17 2023 at 22:24):

lann submitted PR review.

Wasmtime GitHub notifications bot (Feb 17 2023 at 22:24):

lann created PR review comment:

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

Wasmtime GitHub notifications bot (Feb 17 2023 at 22:27):

lann updated PR #5826 from precompile-compat-key to main.

Wasmtime GitHub notifications bot (Apr 18 2023 at 21:03):

lann updated PR #5826.

Last updated: Oct 23 2024 at 20:03 UTC