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

Jump to bottom

Introduce API for custom stack memory #7209

Merged
merged 6 commits into from
Oct 11, 2023
Merged

Introduce API for custom stack memory #7209

merged 6 commits into from
Oct 11, 2023

Conversation

rockwotj
Copy link
Contributor

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

Discussed on Zulip

@rockwotj rockwotj requested review from a team as code owners October 10, 2023 16:47
@rockwotj rockwotj requested review from alexcrichton and removed request for a team October 10, 2023 16:47
@rockwotj rockwotj force-pushed the custom-stack-memory branch 4 times, most recently from 65540ca to 4f7e6f1 Compare October 10, 2023 17:05
rockwotj
rockwotj commented Oct 10, 2023
View reviewed changes
crates/c-api/src/config.rs
);
match error {
None => {
let memory = unsafe { memory.assume_init() };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@github-actions github-actions bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. wasmtime:config Issues related to the configuration of Wasmtime labels Oct 10, 2023
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api", "wasmtime:c-api", "wasmtime:config"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api, wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@github-actions
Copy link

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

  • If you added a new Config method, you wrote extensive documentation for
    it.

    Our documentation should be of the following form:

    Short, simple summary sentence.

More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.

Is this method fallible? If so, when can it return an error?

Can this method panic? If so, when does it panic?

# Example

Optional example here.

  • If you added a new Config method, or modified an existing one, you
    ensured that this configuration is exercised by the fuzz targets.

    For example, if you expose a new strategy for allocating the next instance
    slot inside the pooling allocator, you should ensure that at least one of our
    fuzz targets exercises that new strategy.

    Often, all that is required of you is to ensure that there is a knob for this
    configuration option in wasmtime_fuzzing::Config (or one
    of its nested structs).

    Rarely, this may require authoring a new fuzz target to specifically test this
    configuration. See our docs on fuzzing for more details.

  • If you are enabling a configuration option by default, make sure that it
    has been fuzzed for at least two weeks before turning it on by default.

To modify this label's message, edit the .github/label-messager/wasmtime-config.md file.

To add new label messages or remove existing label messages, edit the
.github/label-messager.json configuration file.

Learn more.

@rockwotj
Introduce StackMemory and StackMemoryCreator
66b3072 
This allows custom implementations of stack memory to be plugged into
the async functionality for wasmtime. Currently, stacks are always
mmapped, and this custom allocator allows embedders to use any memory
they would like.

The new APIs are also exposed in the C api.

This has no effect on windows, as our hands are tied with fibers there.

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
@rockwotj
Add test for custom host memory
c4fb9db 
Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
@rockwotj
fix allocator test
4cfdfc4 
Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
@rockwotj rockwotj force-pushed the custom-stack-memory branch from b3a3730 to 4cfdfc4 Compare October 10, 2023 20:17
fitzgen
fitzgen reviewed Oct 10, 2023
View reviewed changes
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

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

crates/fiber/src/unix.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/stack.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/trampoline.rs Outdated Show resolved Hide resolved
tests/all/stack_creator.rs Outdated Show resolved Hide resolved
crates/fiber/src/unix.rs Outdated Show resolved Hide resolved
crates/fiber/src/unix.rs Outdated Show resolved Hide resolved
@rockwotj rockwotj requested a review from fitzgen October 10, 2023 21:06
@rockwotj
Address review comments
a793be8 
Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
@rockwotj rockwotj force-pushed the custom-stack-memory branch from d19ac9e to a793be8 Compare October 10, 2023 21:30
fitzgen
fitzgen approved these changes Oct 11, 2023
View reviewed changes
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

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

Thanks @rockwotj!

@fitzgen fitzgen added this pull request to the merge queue Oct 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 11, 2023
@rockwotj
Fix lint warnings
d8166c8 
prtest:full

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
@rockwotj
Copy link
Contributor Author

Thanks @fitzgen! I messed up some lint warnings when the async config is disabled. I've fixed them now.

@rockwotj
fix windows lint warning
f5a460f 
prtest:full

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
@fitzgen fitzgen added this pull request to the merge queue Oct 11, 2023
Merged via the queue into bytecodealliance:main with commit e6ff841 Oct 11, 2023
32 checks passed
@rockwotj rockwotj deleted the custom-stack-memory branch October 11, 2023 20:38
This was referenced Oct 12, 2023
Merged
Merged
@alexcrichton alexcrichton mentioned this pull request Jul 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fitzgen fitzgen fitzgen approved these changes

@alexcrichton alexcrichton Awaiting requested review from alexcrichton alexcrichton is a code owner automatically assigned from bytecodealliance/wasmtime-core-reviewers

Assignees
No one assigned
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. wasmtime:config Issues related to the configuration of Wasmtime
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rockwotj @fitzgen