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

refactor: Change to copy function in read_region #730

Closed
wants to merge 2 commits into from
Closed

refactor: Change to copy function in read_region #730

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bfe0c13
refactor: Change to copy function in read_region
brew0722 Jan 19, 2021
d155956
fix: Run cargo fmt
brew0722 Jan 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/vm/src/memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::ptr;
use wasmer::{Array, ValueType, WasmPtr};

use crate::conversion::to_u32;
Expand Down Expand Up @@ -39,12 +40,11 @@ pub fn read_region(memory: &wasmer::Memory, ptr: u32, max_length: usize) -> VmRe

match WasmPtr::<u8, Array>::new(region.offset).deref(memory, 0, region.length) {
Some(cells) => {
// In case you want to do some premature optimization, this shows how to cast a `&'mut [Cell<u8>]` to `&mut [u8]`:
// https://github.com/wasmerio/wasmer/blob/0.13.1/lib/wasi/src/syscalls/mod.rs#L79-L81
let raw_cells = cells as *const [_] as *const u8;
let len = region.length as usize;
let mut result = vec![0u8; len];
Copy link
Member

Choose a reason for hiding this comment

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

Does Rust allow us to create an a vector of the correct length without zeroing it? There is no point in wrinting all zeros here and then overwriting it in the next line.

Copy link
Contributor

@maurolacy maurolacy Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Seems that's the preferred, and one of the fastest ways. See rust-lang/rust#54628. For what they say, vec! has special behaviour when its first argument is zero.

Copy link
Member

Choose a reason for hiding this comment

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

It might be the best for initialized memory. But we only need uninitialized memory that just holds any data from previous memory use instead of writing zeros there.

We can do

// Allocate vector big enough for len elements.
let mut result = Vec::with_capacity(len);

// write into the vector

// Mark the first len elements of the vector as being initialized.
unsafe {
    result.set_len(len);
}

See also this example: https://doc.rust-lang.org/std/vec/struct.Vec.html#examples-18.

Then we write to this memory only once instead of twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

I was thinking, on a related note, do we want to do de the same for write_region?

Copy link
Member

@webmaster128 webmaster128 Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should do both in parallel, in one PR. Feel free to cherry pick the work from here to give credit to the author and open a new PR including both.

Copy link
Contributor

@maurolacy maurolacy Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

OK, I can do that, and in fact already have the branch created for the benchmarks.

Another option would be for @slave5vw to add these changes, along with the requested ones above, and so he gets the credits directly.

@slave5vw, what do you think?

Copy link
Contributor

@maurolacy maurolacy Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

In any case, I've already implemented these, in the read_region-copy branch, in order to benchmark it.

for i in 0..len {
result[i] = cells[i].get();
unsafe{
ptr::copy(raw_cells, result.as_mut_ptr(), len);
}
Ok(result)
}
Expand Down