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

feat: add static buffer as an alternative to allocating memory #939

Merged
merged 2 commits into from
Mar 14, 2024

Conversation

Ddystopia
Copy link
Contributor

@Ddystopia Ddystopia commented Feb 22, 2024

Draft for static buffer as an alternative to allocating memory. Closes #878.

User can give fn() -> Option<&'static mut [u8]>, which is a factory for static buffers. I personally only need one, but there are problems if we want user to provide &'static mut [u8]. It is not Copy or Clone for obvious reasons, but Config is. It also cannot be wrapped in RefCell to be taken from config by shared reference, because there is a Sync assert.

In my code, this feature could used in 100% safe way:

fn static_buffer_factory() -> Option<&'static mut [u8]> {
    #[repr(align(32))]
    struct Memory([u8; 64 * 1024]);

    // Now the linker will refuse to create the final binary
    // if there is not enough space in the target RAM
    static WASMI_LINEAR_MEMORY: StaticCell<Memory> = StaticCell::new();

    // Will only succeed the first time. Uses atomic CAS operations under the hood
    WASMI_LINEAR_MEMORY
        .try_init_with(|| Memory([0; 64 * 1024]))
        .map(|x| &mut x.0[..])
}

let mut config = Config::default();
let config = config.set_static_buffer_factory(static_buffer_factory);
let engine = Engine::new(config);

As can be seen from godbolt, rustc optimizes out enum access in the hot methods data, data_mut and len, so there is no need for unsafe code and enum is sufficient.

wasmi_godbolt

Unresolved questions

  • Are you okay with that kind of design?
  • Should we strengthen/weaken the requirements? For example, add an alignment check?

@Ddystopia

This comment was marked as duplicate.

@Robbepop
Copy link
Member

Robbepop commented Feb 26, 2024

@Ddystopia Thank you a lot for the PR!

I have to make sure (at least locally) that these changes indeed won't have an effect on the performance of Wasmi for inputs that should have a large impact on linear memory accesses. Unfortunately I have observed far too often that effects shown via Compiler Explorer are not transferable to real world execution since Compiler Explorer views code snippets in isolation where real code always run within some context where a compiler might miss certain optimization opportunities.

I am not yet sold on the config addition but it seems to be necessary to allow for non-imported linear memory backed by static buffers. Seems like a non-ideal API surface to me. Is this a critical feature to you? Could we maybe separate this out to make this feature a bit smaller and only support imported linear memory for now?

@Ddystopia
Copy link
Contributor Author

Ddystopia commented Feb 27, 2024

Thank you for the response! I don't actually know, would imported memory be sufficient or not. Is there a way to compile Rust to wasm so that it would only import memory and not export any, so that wasmi would not create them on it's own?

@Robbepop
Copy link
Member

Robbepop commented Feb 27, 2024

I am not sure but what I can say is that there are a tons of tools that you could use to simply post process the resulting Wasm file. This is to be recommended anyways imo since LLVMs own Wasm codegen is suboptimal.
At Parity we post process all Wasm files before handing them off to Wasmi. And from what I know we use imported linear memory since this provides us with more control on the host side.

@Ddystopia
Copy link
Contributor Author

Ddystopia commented Feb 27, 2024

Force-pushed to remove config changes. This is now only available for imported memories.

@Ddystopia
Copy link
Contributor Author

Ddystopia commented Feb 28, 2024

Force pushed to fix formatting and clippy jobs

@Ddystopia
Copy link
Contributor Author

I'm not sure what should I do with that clippy job - it would not compile if I remove those 2 imports. They're probably unused under some combination of features. Should I gate those imports?

@Robbepop
Copy link
Member

I'm not sure what should I do with that clippy job - it would not compile if I remove those 2 imports. They're probably unused under some combination of features. Should I gate those imports?

I think it is a clippy or even Rust compiler bug. Nothing to do here for us at the moment.

crates/wasmi/src/memory/error.rs Outdated Show resolved Hide resolved
crates/wasmi/src/memory/buffer.rs Outdated Show resolved Hide resolved
crates/wasmi/src/memory/mod.rs Show resolved Hide resolved
@Ddystopia
Copy link
Contributor Author

Unfortunately, compiler did not manage to optimize that time when I've added a len to the static buffer, so I went with your initial idea to use vec's raw parts. There are also some tests that I tried with miri and they do pass.

Additionally rebased to main.

crates/wasmi/src/memory/buffer.rs Outdated Show resolved Hide resolved
crates/wasmi/src/memory/buffer.rs Outdated Show resolved Hide resolved
crates/wasmi/src/memory/buffer.rs Outdated Show resolved Hide resolved
crates/wasmi/src/memory/buffer.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 13, 2024

Codecov Report

Attention: Patch coverage is 72.09302% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 81.57%. Comparing base (0f4e8b5) to head (3230813).

Files Patch % Lines
crates/wasmi/src/memory/mod.rs 0.00% 19 Missing ⚠️
crates/wasmi/src/memory/buffer.rs 95.38% 3 Missing ⚠️
crates/wasmi/src/memory/error.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #939      +/-   ##
==========================================
- Coverage   81.60%   81.57%   -0.03%     
==========================================
  Files         258      258              
  Lines       23601    23682      +81     
==========================================
+ Hits        19259    19318      +59     
- Misses       4342     4364      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

From what I can see now only the proper zeroing of bytes on new_static and grow is missing. We want to zero only the initial_len for new_static and only the new parts in grow to limit the amount of work needed per operation to the minimum.

Also please do not rebase this PR since this kinda destroys my previous reviews and makes it really hard for me to keep track of what happened.

crates/wasmi/src/memory/buffer.rs Outdated Show resolved Hide resolved
crates/wasmi/src/memory/buffer.rs Show resolved Hide resolved
@Ddystopia
Copy link
Contributor Author

Okay, I'll implement it tomorrow as a separate commit.

Copy link
Member

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

Looks good to me now! :)
The new tests pass and the CI is happy.

Thanks a lot for the PR and the new feature with static buffer backed linear memories in Wasmi. I am sure this feature is going to be very helpful for the embedded community. :)

🚀

@Robbepop Robbepop merged commit 01d0881 into wasmi-labs:master Mar 14, 2024
15 of 17 checks passed
dubbelosix pushed a commit to dubbelosix/wasmi that referenced this pull request Nov 21, 2024
…-labs#939)

* feat: add static buffer as an alternative to allocating memory

* fix: zeroing static buffer gradually
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Alternative Memory Backing Options
2 participants