Skip to content

feat: add PluginBuilder::with_wasmtime_config #764

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

Merged
merged 3 commits into from
Sep 19, 2024
Merged

feat: add PluginBuilder::with_wasmtime_config #764

merged 3 commits into from
Sep 19, 2024

Conversation

zshipko
Copy link
Contributor

@zshipko zshipko commented Sep 18, 2024

An alternative to #763, this PR allows an initial wasmtime::Config to be passed in when building a plugin. Some of these values may be overwritten by the Extism runtime, but it allows for things like static memory size and other low-level details to be handled directly instead of us having to wrap every option ourselves.

@nilslice
Copy link
Member

Feels like this is safe to do, since we plan to consider dropping Wasmtime for most SDKs except possibly an async-friendly Rust SDK, right?

@zshipko
Copy link
Contributor Author

zshipko commented Sep 18, 2024

Yeah, that's the goal, and we can always replace this with some other configuration options when we drop wasmtime, since there will be some refactoring needed at that point anyway - until then this gives users of the current Rust SDK some control over the low-level stuff that we don't necessarily need to care about.

@zshipko zshipko merged commit 7bf41c2 into main Sep 19, 2024
5 checks passed
@zshipko zshipko deleted the expose-config branch September 19, 2024 18:27
@SebastianHambura
Copy link
Contributor

Some of these values may be overwritten by the Extism runtime

Sorry if I'm a bit late, but I think it would be quite usefull to know which values are overwritten. I can already imagine a user changing settings and wondering why nothing is changing. From what I could quickly see, here's the spot where we overwrite some settings:

        let mut config = config.unwrap_or_default();
        config
            .async_support(false)
            .epoch_interruption(true)
            .debug_info(debug_options.debug_info)
            .coredump_on_trap(debug_options.coredump.is_some())
            .profiler(debug_options.profiling_strategy)
            .wasm_tail_call(true)
            .wasm_function_references(true)
            .wasm_gc(true);

https://github.com/extism/extism/blob/25bcbe092d94562441a2eb54a74584e023cc1e05/runtime/src/plugin.rs#L334C1-L343C28

Could we change with_wasmtime_config documentation to something like:


Configure an initial wasmtime config to be passed to the plugin

Warning: some values might be may be overwritten by the Extism runtime. In particular:

  • async_support
  • epoch_interruption
  • debug_info
  • coredump_on_trap
  • profiler
  • wasm_tail_call
  • wasm_function_references
  • wasm_gc

See the implementation details of [this function](Plugin::build_new) to check which values are overwritten.

@zshipko
Copy link
Contributor Author

zshipko commented Sep 23, 2024

Good idea, can you open a PR with this change?

zshipko pushed a commit that referenced this pull request Sep 23, 2024
PR for #764 (comment)

---------

Co-authored-by: Sebastian Hambura <sebastian.hambur@embl.de>
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.

3 participants