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.

Sign up for GitHub

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

Jump to bottom

re: restore the low level wasmer2 refactor #6043

Merged
merged 6 commits into from
Jan 13, 2022
Merged

re: restore the low level wasmer2 refactor #6043

merged 6 commits into from
Jan 13, 2022

Conversation

nagisa
Copy link
Collaborator

@nagisa nagisa commented Jan 11, 2022

This reverts commit 353b4b4.

@nagisa
Copy link
Collaborator Author

nagisa commented Jan 11, 2022

Note, this is a draft, pending investigation into what exactly causes us to attempt an import of a 65536 page memory region.

@nagisa
Copy link
Collaborator Author

nagisa commented Jan 12, 2022

I believe I figured out the problem that we had. It isme forgetting to bump the WASMER2_CONFIG constant (again) so that potential changes to the artifacts are taken into account for caching purposes.

The change in this particular instance was in the Tunables implementation which now returns correctly bounded MemoryStyle. Previously even if we allocated memories with up-to 2048 pages, the MemoryStyle would still say something along the lines of “this is up to 0xFFFF pages” or some such.

Now that these are consistent, across the board, loading an artifact generated with an old version of implementation would give us a module with a MemoryStyle that's… vzery different.

@nagisa nagisa marked this pull request as ready for review January 13, 2022 11:50
@nagisa nagisa requested a review from Ekleog January 13, 2022 11:50
matklad
matklad reviewed Jan 13, 2022
View reviewed changes
runtime/near-vm-runner/src/tests/cache.rs Outdated
#[test]
fn test_wasmer2_artifact_output_stability() {
// If this test has failed, you want to adjust the necessary constants so that `cache::vm_hash`
// changes (and the only then the hashes here).
Copy link
Contributor

Choose a reason for hiding this comment

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

Something's up with grammar here? "the only then the hashes"? love the alliteration though

matklad
matklad approved these changes Jan 13, 2022
View reviewed changes
runtime/near-vm-runner/src/tests/cache.rs Outdated
let artifact = vm.compile_uncached(&prepared_code).unwrap();
let serialized = artifact.artifact().serialize().unwrap();
serialized.hash(&mut hasher);
assert_eq!(hasher.finish(), 7017891230624373240, "WASMER2_CONFIG needs version change");
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 that's a very good test, thanks for realizing we can do this. It did happen in the past that we've almost forgot to bust the caches.

@near-bulldozer near-bulldozer bot merged commit 14fb25a into master Jan 13, 2022
@near-bulldozer near-bulldozer bot deleted the nagisa/unrevert-wasmer2-ll branch January 13, 2022 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matklad matklad matklad approved these changes

@jakmeier jakmeier Awaiting requested review from jakmeier

@olonho olonho Awaiting requested review from olonho

@Ekleog Ekleog Awaiting requested review from Ekleog

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nagisa @matklad