wasmtime / Issue #1785 Incorrect wasmtime::MemoryCreator ... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / Issue #1785 Incorrect wasmtime::MemoryCreator ...

Wasmtime GitHub notifications bot (May 29 2020 at 10:44):

The documentation for
fn new_memory(
    &self,
    ty: MemoryType,
    reserved_size: Option<u64>,
    guard_size: u64
) -> Result<Box<dyn LinearMemory>, String>
states:

The reserved_size value indicates the expected size of the reservation that is to be made for this memory. If this value is None than the implementation is free to allocate memory as it sees fit. If the value is Some, however, then the implementation is expected to reserve that many bytes for the memory's allocation, plus the guard size at the end

Aside: please also note the typo than.

This sounds like the intended unit for reserved_size is bytes which is also supported by its u64 type . However, the implementation passes in the size in pages:
impl RuntimeMemoryCreator for MemoryCreatorProxy {
    fn new_memory(&self, plan: &MemoryPlan) -> Result<Box<dyn RuntimeLinearMemory>, String> {
        let ty = MemoryType::new(Limits::new(plan.memory.minimum, plan.memory.maximum));
        let reserved_size = match plan.style {
            MemoryStyle::Static { bound } => Some(bound.into()),
            MemoryStyle::Dynamic => None,
        };
        self.mem_creator
            .new_memory(ty, reserved_size, plan.offset_guard_size)
            .map(|mem| Box::new(LinearMemoryProxy { mem }) as Box<dyn RuntimeLinearMemory>)
    }
}
where:
pub enum MemoryStyle {
    /// The actual memory can be resized and moved.
    Dynamic,
    /// Addresss space is allocated up front.
    Static {
        /// The number of mapped and unmapped pages.
        bound: u32,
    },
}
I'll be honest, when working with WebAssembly I am often confused as to whether the memory size is expressed in Wasm pages, OS pages, or bytes. Often it is all three at different points in time! Thus, I tend to make my variable names very explicit. May I suggest changing the function signature to:
fn new_memory(
    &self,
    ty: MemoryType,
    reserved_size_in_bytes: Option<u64>,
    guard_size_in_bytes: u64
) -> Result<Box<dyn LinearMemory>, String>
?

Or perhaps reserved_size_in_wasm_pages if that is the desired unit?

Wasmtime GitHub notifications bot (May 29 2020 at 10:44):

lostman labeled Issue #1785:

The documentation for
fn new_memory(
    &self,
    ty: MemoryType,
    reserved_size: Option<u64>,
    guard_size: u64
) -> Result<Box<dyn LinearMemory>, String>
states:

The reserved_size value indicates the expected size of the reservation that is to be made for this memory. If this value is None than the implementation is free to allocate memory as it sees fit. If the value is Some, however, then the implementation is expected to reserve that many bytes for the memory's allocation, plus the guard size at the end

Aside: please also note the typo than.

This sounds like the intended unit for reserved_size is bytes which is also supported by its u64 type . However, the implementation passes in the size in pages:
impl RuntimeMemoryCreator for MemoryCreatorProxy {
    fn new_memory(&self, plan: &MemoryPlan) -> Result<Box<dyn RuntimeLinearMemory>, String> {
        let ty = MemoryType::new(Limits::new(plan.memory.minimum, plan.memory.maximum));
        let reserved_size = match plan.style {
            MemoryStyle::Static { bound } => Some(bound.into()),
            MemoryStyle::Dynamic => None,
        };
        self.mem_creator
            .new_memory(ty, reserved_size, plan.offset_guard_size)
            .map(|mem| Box::new(LinearMemoryProxy { mem }) as Box<dyn RuntimeLinearMemory>)
    }
}
where:
pub enum MemoryStyle {
    /// The actual memory can be resized and moved.
    Dynamic,
    /// Addresss space is allocated up front.
    Static {
        /// The number of mapped and unmapped pages.
        bound: u32,
    },
}
I'll be honest, when working with WebAssembly I am often confused as to whether the memory size is expressed in Wasm pages, OS pages, or bytes. Often it is all three at different points in time! Thus, I tend to make my variable names very explicit. May I suggest changing the function signature to:
fn new_memory(
    &self,
    ty: MemoryType,
    reserved_size_in_bytes: Option<u64>,
    guard_size_in_bytes: u64
) -> Result<Box<dyn LinearMemory>, String>
?

Or perhaps reserved_size_in_wasm_pages if that is the desired unit?

Wasmtime GitHub notifications bot (May 29 2020 at 20:10):

alexcrichton commented on Issue #1785:

The intention here is that the values are all in bytes, so if it's in wasm pages that's definitely a bug! Renaming the parameter names sounds great too!

Would you be up for a PR to update this?

Wasmtime GitHub notifications bot (May 29 2020 at 20:10):

alexcrichton labeled Issue #1785:

The documentation for
fn new_memory(
    &self,
    ty: MemoryType,
    reserved_size: Option<u64>,
    guard_size: u64
) -> Result<Box<dyn LinearMemory>, String>
states:

The reserved_size value indicates the expected size of the reservation that is to be made for this memory. If this value is None than the implementation is free to allocate memory as it sees fit. If the value is Some, however, then the implementation is expected to reserve that many bytes for the memory's allocation, plus the guard size at the end

Aside: please also note the typo than.

This sounds like the intended unit for reserved_size is bytes which is also supported by its u64 type . However, the implementation passes in the size in pages:
impl RuntimeMemoryCreator for MemoryCreatorProxy {
    fn new_memory(&self, plan: &MemoryPlan) -> Result<Box<dyn RuntimeLinearMemory>, String> {
        let ty = MemoryType::new(Limits::new(plan.memory.minimum, plan.memory.maximum));
        let reserved_size = match plan.style {
            MemoryStyle::Static { bound } => Some(bound.into()),
            MemoryStyle::Dynamic => None,
        };
        self.mem_creator
            .new_memory(ty, reserved_size, plan.offset_guard_size)
            .map(|mem| Box::new(LinearMemoryProxy { mem }) as Box<dyn RuntimeLinearMemory>)
    }
}
where:
pub enum MemoryStyle {
    /// The actual memory can be resized and moved.
    Dynamic,
    /// Addresss space is allocated up front.
    Static {
        /// The number of mapped and unmapped pages.
        bound: u32,
    },
}
I'll be honest, when working with WebAssembly I am often confused as to whether the memory size is expressed in Wasm pages, OS pages, or bytes. Often it is all three at different points in time! Thus, I tend to make my variable names very explicit. May I suggest changing the function signature to:
fn new_memory(
    &self,
    ty: MemoryType,
    reserved_size_in_bytes: Option<u64>,
    guard_size_in_bytes: u64
) -> Result<Box<dyn LinearMemory>, String>
?

Or perhaps reserved_size_in_wasm_pages if that is the desired unit?

Wasmtime GitHub notifications bot (May 29 2020 at 20:24):

github-actions[bot] commented on Issue #1785:

Subscribe to Label Action

cc @peterhuene

<details>
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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Jun 04 2020 at 02:17):

lostman commented on Issue #1785:

Sure, will make a PR.

Wasmtime GitHub notifications bot (Jun 17 2020 at 04:59):

lostman closed Issue #1785:

The documentation for
fn new_memory(
    &self,
    ty: MemoryType,
    reserved_size: Option<u64>,
    guard_size: u64
) -> Result<Box<dyn LinearMemory>, String>
states:

The reserved_size value indicates the expected size of the reservation that is to be made for this memory. If this value is None than the implementation is free to allocate memory as it sees fit. If the value is Some, however, then the implementation is expected to reserve that many bytes for the memory's allocation, plus the guard size at the end

Aside: please also note the typo than.

This sounds like the intended unit for reserved_size is bytes which is also supported by its u64 type . However, the implementation passes in the size in pages:
impl RuntimeMemoryCreator for MemoryCreatorProxy {
    fn new_memory(&self, plan: &MemoryPlan) -> Result<Box<dyn RuntimeLinearMemory>, String> {
        let ty = MemoryType::new(Limits::new(plan.memory.minimum, plan.memory.maximum));
        let reserved_size = match plan.style {
            MemoryStyle::Static { bound } => Some(bound.into()),
            MemoryStyle::Dynamic => None,
        };
        self.mem_creator
            .new_memory(ty, reserved_size, plan.offset_guard_size)
            .map(|mem| Box::new(LinearMemoryProxy { mem }) as Box<dyn RuntimeLinearMemory>)
    }
}
where:
pub enum MemoryStyle {
    /// The actual memory can be resized and moved.
    Dynamic,
    /// Addresss space is allocated up front.
    Static {
        /// The number of mapped and unmapped pages.
        bound: u32,
    },
}
I'll be honest, when working with WebAssembly I am often confused as to whether the memory size is expressed in Wasm pages, OS pages, or bytes. Often it is all three at different points in time! Thus, I tend to make my variable names very explicit. May I suggest changing the function signature to:
fn new_memory(
    &self,
    ty: MemoryType,
    reserved_size_in_bytes: Option<u64>,
    guard_size_in_bytes: u64
) -> Result<Box<dyn LinearMemory>, String>
?

Or perhaps reserved_size_in_wasm_pages if that is the desired unit?

Last updated: Nov 22 2024 at 16:03 UTC