wasmtime / PR #7209 Introduce API for custom stack memory · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #7209 Introduce API for custom stack memory

Wasmtime GitHub notifications bot (Oct 10 2023 at 16:47):

rockwotj opened PR #7209 from rockwotj:custom-stack-memory to bytecodealliance:main:

Introduces an API to allow embedders to allocate stacks for fibers using custom memory instead of mmap.

Discussed on Zulip

Wasmtime GitHub notifications bot (Oct 10 2023 at 16:47):

rockwotj requested wasmtime-core-reviewers for a review on PR #7209.

Wasmtime GitHub notifications bot (Oct 10 2023 at 16:47):

rockwotj requested wasmtime-default-reviewers for a review on PR #7209.

Wasmtime GitHub notifications bot (Oct 10 2023 at 16:47):

rockwotj requested alexcrichton for a review on PR #7209.

Wasmtime GitHub notifications bot (Oct 10 2023 at 16:51):

rockwotj updated PR #7209.

Wasmtime GitHub notifications bot (Oct 10 2023 at 17:03):

rockwotj updated PR #7209.

Wasmtime GitHub notifications bot (Oct 10 2023 at 17:04):

rockwotj updated PR #7209.

Wasmtime GitHub notifications bot (Oct 10 2023 at 17:05):

rockwotj updated PR #7209.

Wasmtime GitHub notifications bot (Oct 10 2023 at 17:08):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Oct 10 2023 at 17:08):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Oct 10 2023 at 17:08):

rockwotj created PR review comment:

This is a much safer API, because we don't need to assume they will sent env and finalizer to nullptr if they don't need them.

Wasmtime GitHub notifications bot (Oct 10 2023 at 18:19):

rockwotj updated PR #7209.

Wasmtime GitHub notifications bot (Oct 10 2023 at 20:17):

rockwotj updated PR #7209.

Wasmtime GitHub notifications bot (Oct 10 2023 at 20:21):

fitzgen submitted PR review:

(Going to leave the C API bits to Alex to review)

Wasmtime GitHub notifications bot (Oct 10 2023 at 20:21):

fitzgen submitted PR review:

(Going to leave the C API bits to Alex to review)

Wasmtime GitHub notifications bot (Oct 10 2023 at 20:21):

fitzgen created PR review comment:

I think I like the mmap: bool flag better than repeating everything for both Manual and Mmap.

Wasmtime GitHub notifications bot (Oct 10 2023 at 20:21):

fitzgen created PR review comment:

/// This trait is unsafe, as memory safety depends on a proper implementation

Wasmtime GitHub notifications bot (Oct 10 2023 at 20:21):

fitzgen created PR review comment:

Can remove a level of indentation via #![...] to put the attribute on the whole stack_creator module, rather than defining a submodule inside it:
#![cfg(all(not(target_os = "windows"), not(miri)))]

use anyhow::{bail, Context};
use std::{
    alloc::{GlobalAlloc, Layout, System},
    ops::Range,
    ptr::NonNull,
    sync::Arc,
};
use wasmtime::*;

Wasmtime GitHub notifications bot (Oct 10 2023 at 20:21):

fitzgen created PR review comment:

Instead of changing the constructor of the OnDemandInstanceAllocator depending on whether #[cfg(feature = "async")], how about we always leave out the stack creator, but then have a method for configuring it after construction:
impl OnDemandInstanceAllocator {
    #[cfg(feature = "async")]
    pub fn set_stack_creator(&mut self, stack_creator: Option<Arc<dyn StackCreator>>) {
        self.stack_creator = stack_creator;
    }
}
This should make construction sites a little bit simpler.

Wasmtime GitHub notifications bot (Oct 10 2023 at 20:21):

fitzgen created PR review comment:

Can we assert page alignment here?

Wasmtime GitHub notifications bot (Oct 10 2023 at 20:21):

fitzgen created PR review comment:

And here for both start and end parts of the range

Wasmtime GitHub notifications bot (Oct 10 2023 at 21:06):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Oct 10 2023 at 21:06):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Oct 10 2023 at 21:06):

rockwotj created PR review comment:

Done thanks

Wasmtime GitHub notifications bot (Oct 10 2023 at 21:06):

rockwotj created PR review comment:

Reverted that back.

Wasmtime GitHub notifications bot (Oct 10 2023 at 21:06):

rockwotj created PR review comment:

Sure!

Wasmtime GitHub notifications bot (Oct 10 2023 at 21:06):

rockwotj created PR review comment:

Done.

Wasmtime GitHub notifications bot (Oct 10 2023 at 21:06):

rockwotj created PR review comment:

Done.

Wasmtime GitHub notifications bot (Oct 10 2023 at 21:06):

rockwotj created PR review comment:

Thanks done!

Wasmtime GitHub notifications bot (Oct 10 2023 at 21:06):

rockwotj updated PR #7209.

Wasmtime GitHub notifications bot (Oct 10 2023 at 21:06):

rockwotj requested fitzgen for a review on PR #7209.

Wasmtime GitHub notifications bot (Oct 10 2023 at 21:30):

rockwotj updated PR #7209.

Wasmtime GitHub notifications bot (Oct 11 2023 at 17:47):

fitzgen submitted PR review:

Nice! I looked through the C API changes in more detail as well, and they look good as well.

Thanks @rockwotj!

Wasmtime GitHub notifications bot (Oct 11 2023 at 18:08):

rockwotj updated PR #7209.

Wasmtime GitHub notifications bot (Oct 11 2023 at 18:36):

rockwotj updated PR #7209.

Wasmtime GitHub notifications bot (Oct 11 2023 at 19:35):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 11 2023 at 20:18):

fitzgen merged PR #7209.

Last updated: Nov 22 2024 at 16:03 UTC