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

Wasmer 3 🔥 #1674

Merged
merged 62 commits into from
May 24, 2023
Merged

Wasmer 3 🔥 #1674

merged 62 commits into from
May 24, 2023

Conversation

webmaster128
Copy link
Member

No description provided.

This was referenced May 2, 2023
@webmaster128 webmaster128 marked this pull request as ready for review May 23, 2023 15:41
@webmaster128
Copy link
Member Author

webmaster128 commented May 23, 2023

The CI failure will be fixed here: #1691 Done

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Partial review. Will finish with imports.rs and instance.rs next.

@@ -85,6 +85,7 @@ workflows:
- /^[0-9]+\.[0-9]+$/
# Add your branch here if benchmarking matters to your work
- fix-benchmarking
- w3
Copy link
Contributor

@maurolacy maurolacy May 24, 2023

Choose a reason for hiding this comment

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

Can now be removed perhaps?

packages/vm/examples/module_size.sh Outdated Show resolved Hide resolved
packages/vm/src/environment.rs Outdated Show resolved Hide resolved
Comment on lines 368 to 374
// Creates a MemoryView.
// This mut be short living and not be used after the memory was grown.
pub fn memory<'a>(&self, store: &'a mut impl AsStoreMut) -> MemoryView<'a> {
self.memory
.as_ref()
.expect("Memory is not set. This is a bug in the lifecycle.")
.view(store)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for performance or what? Just curious.

Copy link
Member Author

Choose a reason for hiding this comment

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

It creates a MemoryView as designed by the new Wasmer API:

/// A WebAssembly `memory` view.
///
/// A memory view is used to read and write to the linear memory.
///
/// After a memory is grown a view must not be used anymore. Views are
/// created using the Memory.grow() method.
#[derive(Debug)]
pub struct MemoryView<'a>(pub(crate) memory_view_impl::MemoryView<'a>);

It seems like the Memory itself cannot be used directly:

#[derive(Debug, Clone, PartialEq)]
pub struct Memory(pub(crate) memory_impl::Memory);

impl Memory {
    // ...

    /// Creates a view into the memory that then allows for
    /// read and write
    pub fn view<'a>(&self, store: &'a impl AsStoreRef) -> MemoryView<'a> {
        MemoryView::new(self, store)
    }

    // ...
}

Comment on lines +30 to +34
fn zero_padding_bytes(&self, _bytes: &mut [MaybeUninit<u8>]) {
// The size of Region is exactly 3x4=12 bytes with no padding.
// The `size_of::<Region>()` test below ensures that.
// So we do not need to zero any bytes here.
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A 'comment function' 🙂.

packages/vm/src/memory.rs Outdated Show resolved Hide resolved
packages/vm/src/memory.rs Outdated Show resolved Hide resolved
| Operator::F32x4Fms
| Operator::F64x2Fma
| Operator::F64x2Fms => {
| Operator::F64x2RelaxedMax => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder from where these guys come from.

Copy link
Member Author

Choose a reason for hiding this comment

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

There seems to be some new Wasm extension here:

            // Relaxed SIMD operators
            // https://github.com/WebAssembly/relaxed-simd

Comment on lines 675 to 689
// TODO: do
Operator::I8x16AvgrU => todo!(),
Operator::I16x8AvgrU => todo!(),
Operator::F32x4RelaxedFma => todo!(),
Operator::F32x4RelaxedFnma => todo!(),
Operator::F64x2RelaxedFma => todo!(),
Operator::F64x2RelaxedFnma => todo!(),
Operator::I8x16RelaxedLaneselect => todo!(),
Operator::I16x8RelaxedLaneselect => todo!(),
Operator::I32x4RelaxedLaneselect => todo!(),
Operator::I64x2RelaxedLaneselect => todo!(),
Operator::I16x8RelaxedQ15mulrS => todo!(),
Operator::I16x8DotI8x16I7x16S => todo!(),
Operator::I32x4DotI8x16I7x16AddS => todo!(),
Operator::F32x4RelaxedDotBf16x8AddF32x4 => todo!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Guess this will have to be revisited soon?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. I sorted them now.

packages/vm/src/wasm_backend/store.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

LGTM.

There are significant changes in the way memory and store are being handled in w3, with the FunctionEnv / FunctionEnvMut structs, which are above my head at this point.


let non_existent_id = 42u32;
let result = do_db_next(&env, non_existent_id);
let result = do_db_next(fe_mut.as_mut(), non_existent_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Like with clone(), seems some of these as_mut() can be removed. Not exactly sure which ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, you are right. They create short living semi-clones.

@webmaster128 webmaster128 merged commit e45401c into main May 24, 2023
3 checks passed
@webmaster128 webmaster128 deleted the w3 branch May 24, 2023 12:36
@webmaster128 webmaster128 added this to the 1.3.0 milestone May 24, 2023
@webmaster128 webmaster128 mentioned this pull request May 24, 2023
3 tasks
Copy link
Collaborator

@chipshort chipshort left a comment

Choose a reason for hiding this comment

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

LGTM

}
// We don't load from the memory cache because we had to create new store here and
// serialize/deserialize the artifact to get a full clone. Could be done but adds some code
// for a no-so-relevant use case.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// for a no-so-relevant use case.
// for a not-so-relevant use case.

Comment on lines +256 to +258
// We don't load from the memory cache because we had to create new store here and
// serialize/deserialize the artifact to get a full clone. Could be done but adds some code
// for a no-so-relevant use case.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we don't load from memory cache anymore, the function's doc comment needs to be adjusted

@@ -721,7 +730,7 @@ mod tests {
let backend5 = mock_backend(&[]);

// from file system
let _instance1 = cache
let _instance1: Instance<MockApi, MockStorage, MockQuerier> = cache
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I think this type can still be inferred.

Suggested change
let _instance1: Instance<MockApi, MockStorage, MockQuerier> = cache
let _instance1 = cache

packages/vm/src/environment.rs Outdated Show resolved Hide resolved
"Could not dereference this pointer to a Region",
)),
}
// TODO: double check error handling
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these TODOs still relevant? The error handling looks to me, like it catches the same cases as before.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I'll double check. I did not have the headspace to think about the error cases when trying to make everything compile.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we also rename the file?

Comment on lines 73 to 93
#[cfg(feature = "cranelift")]
{
let mut compiler = Cranelift::default();
for middleware in middlewares {
compiler.push_middleware(middleware.clone());
}
compiler.push_middleware(deterministic);
compiler.push_middleware(metering);
compiler.into()
}

#[cfg(not(feature = "cranelift"))]
{
let mut compiler = Singlepass::default();
for middleware in middlewares {
compiler.push_middleware(middleware.clone());
}
compiler.push_middleware(deterministic);
compiler.push_middleware(metering);
compiler.into()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Not really related to this PR, but this could be shortened like this:

Suggested change
#[cfg(feature = "cranelift")]
{
let mut compiler = Cranelift::default();
for middleware in middlewares {
compiler.push_middleware(middleware.clone());
}
compiler.push_middleware(deterministic);
compiler.push_middleware(metering);
compiler.into()
}
#[cfg(not(feature = "cranelift"))]
{
let mut compiler = Singlepass::default();
for middleware in middlewares {
compiler.push_middleware(middleware.clone());
}
compiler.push_middleware(deterministic);
compiler.push_middleware(metering);
compiler.into()
}
#[cfg(feature = "cranelift")]
let mut compiler = Cranelift::default();
#[cfg(not(feature = "cranelift"))]
let mut compiler = Singlepass::default();
for middleware in middlewares {
compiler.push_middleware(middleware.clone());
}
compiler.push_middleware(deterministic);
compiler.push_middleware(metering);
compiler.into()

and same in make_compile_time_store

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, right. This was different when the code was originally written.

@webmaster128 webmaster128 added the wasmer3/4 Changes related to the upgrade to Wasmer 3 and 4 label Jun 19, 2023
da1suk8 added a commit to da1suk8/cosmwasm that referenced this pull request Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmer3/4 Changes related to the upgrade to Wasmer 3 and 4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants