fix(wasmtime):`Config` methods should be idempotent #4252

PureWhiteWu · 2022-06-09T17:21:23Z

This commit refactored Config to use a seperate CompilerConfig field instead of operating on CompilerBuilder directly to make all its methods idempotent.
Fixes #4189

PureWhiteWu · 2022-06-09T17:28:13Z

Sorry for this late fix.

r? @alexcrichton @cfallin

RELEASES.md

crates/wasmtime/src/config.rs

github-actions · 2022-06-09T17:52:33Z

Subscribe to Label Action

cc @peterhuene

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

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

peterhuene: wasmtime:api

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

Learn more.

github-actions · 2022-06-09T17:52:48Z

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.

cfallin

Thanks a bunch for this -- I think it moves the Config API in a clearer direction and eliminates some potential confusion!

I've added some thoughts below on the doc-comments, mostly, because getting these right is important for the public config interface; but I think the overall strategy is good (modulo one thought on the reftypes/bulk-memory options below).

cc @alexcrichton for thoughts on this as well, want to make sure you're also happy with this change...

crates/wasmtime/src/config.rs

cfallin · 2022-06-09T18:07:01Z

crates/wasmtime/src/config.rs

- pub unsafe fn cranelift_flag_enable(&mut self, flag: &str) -> Result<&mut Self> {
- self.compiler.enable(flag)?;
- Ok(self)
+ pub unsafe fn cranelift_flag_enable(&mut self, flag: &str) -> &mut Self {


In place of the "Errors" section in the old doc comment, we should note here that flags are validated when the engine is built, and so any errors will be deferred, I think. Likewise elsewhere where an "Errors" section is removed.

Added, except for the strategy method, because it will not return error in any situation (previously and now).

cfallin · 2022-06-09T18:10:39Z

crates/wasmtime/src/engine.rs

+ /// # Errors
+ ///
+ /// This method can fail if the `config` is invalid or some
+ /// configurations are incompatible.


Can we say a bit more here? Something about how some Wasm features require certain compiler features, and they will turn on the relevant compiler features by default, but explicitly disabling some compiler features or setting them in an incompatible way may cause errors. I think we should also note a guarantee/promise that default compiler options (i.e., if the user does not add any specific setting overrides) will always work if the underlying hardware supports what is needed.

Agreed about expanding the error documentation a bit here, but I also think it would be best to keep documentation on the original config methods as well. For example Config::cranelift_flag_set could retain some documentation indicating that if an invalid setting is specified that the error will pop out during Engine::new here.

Can we say a bit more here? Something about how some Wasm features require certain compiler features, and they will turn on the relevant compiler features by default, but explicitly disabling some compiler features or setting them in an incompatible way may cause errors.

Fixed.

I think we should also note a guarantee/promise that default compiler options (i.e., if the user does not add any specific setting overrides) will always work if the underlying hardware supports what is needed.

I don't think it necessary to add this note explicitly, because the default options should work is intuition for users, and we have enough information in the errors.

On https://docs.wasmtime.dev/c-api/config_8h.html#ada735dd95cbee6854fc6bb27dc9591c6 it says:

If the compilation strategy selected could not be enabled then an error is returned.

But the signature is:

void wasmtime_config_strategy_set(wasm_config_t *, wasmtime_strategy_t)

I've been looking everywhere in the docs to try and find what happened to config errors, I don't think it is very clear today without looking into this PR.

Also, we use wasm_engine_new_with_config to instantiate our engine with wasm.h; where is the error returned from?

Hi, thanks for your remind.
I changed the doc and the signature of the rust code, but I forgot the doc for c.
I've submitted #4550 to fix this.

@PureWhiteWu That's great! Very prompt, thank you!

A question still remains on my end but I am not sure if here's the right place for it, I'll shoot it again though:

Since wasm.h's wasm_engine_new_with_config does not return any error, where are the engine initialization errors reported when using the C API? 🤔

Sorry, I'm not familiar with the c api, @alexcrichton do you know about this?

You can take a look at the implementation and right now it looks like no error handling happens and if something otherwise would result in an error it would cause the process to abort.

This is probably something that would be good to fix, returning a null pointer there and perhaps adding a Wasmtime-specific API for the same operation but one that returns an error message.

crates/wasmtime/src/config.rs

github-actions · 2022-06-09T18:36:05Z

Subscribe to Label Action

cc @fitzgen, @peterhuene

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

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

fitzgen: fuzzing
peterhuene: wasmtime:c-api

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

Learn more.

alexcrichton

This also looks great to me, thanks for working on this!

I personally really like the idea of deferring work from Config to Engine::new as much as possible. I think there's a number of other options we could apply this to as well, but I'm happy to follow up with these in a later PR or tracking issue, there's no need to tackle them here right now unelss you're motivated:

Config::profiler could defer creation of the profiling agent to Engine-construction. This would actually also remove the Arc around it since there's no more need to clone it.
Config::cache_config_load could defer its I/O to Engine::new.
Size normalization in methods like Config::{static,dynamic}_memory_guard_size could instead happen in Engine::new instead of as a side effect of calling the configuration method.

Finally, if you're feeling even more ambitious (sorry I keep seeing these things and figure I should write them down, but don't feel like you're required to do any of them) the CompilerConfig type might actually be good to move to wasmtime_environ. The current CompilerBuilder trait could be removed entirely in favor of a construction function in the wasmtime_cranelift crate along the lines of fn build_compiler(settings: &wasmtime_environ::CompilerConfig) -> Result<Box<dyn wasmtime_environ::Compiler>> or something like that. Basically instead of mirroring hash maps in Config into hash maps stored in cranelift we'd just pass cranelift all the hash maps directly to get processed. Again though this is an orthogonal refactoring that's totally ok to leave to later.

crates/wasmtime/src/config.rs

alexcrichton · 2022-06-09T19:00:14Z

crates/wasmtime/src/engine.rs

+ /// # Errors
+ ///
+ /// This method can fail if the `config` is invalid or some
+ /// configurations are incompatible.


Agreed about expanding the error documentation a bit here, but I also think it would be best to keep documentation on the original config methods as well. For example Config::cranelift_flag_set could retain some documentation indicating that if an invalid setting is specified that the error will pop out during Engine::new here.

crates/wasmtime/src/config.rs

PureWhiteWu

Thanks for your review!

crates/wasmtime/src/config.rs

PureWhiteWu · 2022-06-09T19:02:44Z

crates/wasmtime/src/config.rs

- pub unsafe fn cranelift_flag_enable(&mut self, flag: &str) -> Result<&mut Self> {
- self.compiler.enable(flag)?;
- Ok(self)
+ pub unsafe fn cranelift_flag_enable(&mut self, flag: &str) -> &mut Self {


Added, except for the strategy method, because it will not return error in any situation (previously and now).

crates/wasmtime/src/config.rs

PureWhiteWu · 2022-06-09T19:49:49Z

crates/wasmtime/src/engine.rs

+ /// # Errors
+ ///
+ /// This method can fail if the `config` is invalid or some
+ /// configurations are incompatible.


Can we say a bit more here? Something about how some Wasm features require certain compiler features, and they will turn on the relevant compiler features by default, but explicitly disabling some compiler features or setting them in an incompatible way may cause errors.

Fixed.

I think we should also note a guarantee/promise that default compiler options (i.e., if the user does not add any specific setting overrides) will always work if the underlying hardware supports what is needed.

I don't think it necessary to add this note explicitly, because the default options should work is intuition for users, and we have enough information in the errors.

PureWhiteWu · 2022-06-09T20:08:05Z

@alexcrichton Thanks for your review and suggestions!
It's too late now for me, so I need to complete these changes tomorrow.

This commit refactored `Config` to use a seperate `CompilerConfig` field instead of operating on `CompilerBuilder` directly to make all its methods idempotent. Fixes bytecodealliance#4189

PureWhiteWu · 2022-06-10T11:52:12Z

Config::profiler could defer creation of the profiling agent to Engine-construction. This would actually also remove the Arc around it since there's no more need to clone it.

This has been done.

Config::cache_config_load could defer its I/O to Engine::new.

For this part, I think the entire CacheConfig needs to be refactored. I think a separate Cache struct should be used instead of using CacheConfig as both the config and cache itself.

This may be not be a minor change, so I will think this may need another pr. I've also submitted a tracking issue #4257 for this.

Size normalization in methods like Config::{static,dynamic}_memory_guard_size could instead happen in Engine::new instead of as a side effect of calling the configuration method.

I don't think defer these things to Engine::new can bring us gain, but will make these associated code dispersed, so I didn't change this.

And for the CompilerBuilder part I think this should also be implemented in another PR. And here's the tracking issue #4258.

alexcrichton

This looks great to me, thanks again @PureWhiteWu!

PureWhiteWu · 2022-06-13T14:51:19Z

Thank you for your patient instructions and review!

This commit fixes the .NET `Config` API to conform to the latest upstream Wasmtime's C API signatures. See bytecodealliance/wasmtime#4252 for the upstream change.

In bytecodealliance#4252 I changed the signature and doc of the rust code, but forgot to change the doc for c. Set strategy no longer returns error. This commit fixes that.

In #4252 I changed the signature and doc of the rust code, but forgot to change the doc for c. Set strategy no longer returns error. This commit fixes that.

PureWhiteWu force-pushed the fix/config_overwrite branch 2 times, most recently from 7db2afb to 4227ed2 Compare June 9, 2022 17:25

PureWhiteWu force-pushed the fix/config_overwrite branch from 4227ed2 to e212d86 Compare June 9, 2022 17:29

bjorn3 reviewed Jun 9, 2022

View reviewed changes

RELEASES.md Outdated Show resolved Hide resolved

bjorn3 reviewed Jun 9, 2022

View reviewed changes

RELEASES.md Outdated Show resolved Hide resolved

bjorn3 reviewed Jun 9, 2022

View reviewed changes

crates/wasmtime/src/config.rs Outdated Show resolved Hide resolved

github-actions bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:config Issues related to the configuration of Wasmtime wasmtime:docs Issues related to Wasmtime's documentation labels Jun 9, 2022

PureWhiteWu force-pushed the fix/config_overwrite branch from e212d86 to 3f57e76 Compare June 9, 2022 18:04

cfallin reviewed Jun 9, 2022

View reviewed changes

PureWhiteWu force-pushed the fix/config_overwrite branch from 3f57e76 to 9ea619a Compare June 9, 2022 18:22

github-actions bot added fuzzing Issues related to our fuzzing infrastructure wasmtime:c-api Issues pertaining to the C API. labels Jun 9, 2022

alexcrichton reviewed Jun 9, 2022

View reviewed changes

PureWhiteWu force-pushed the fix/config_overwrite branch from 9ea619a to fb584d1 Compare June 9, 2022 19:46

PureWhiteWu commented Jun 9, 2022

View reviewed changes

PureWhiteWu force-pushed the fix/config_overwrite branch 4 times, most recently from 7d83e3c to 0ba39e2 Compare June 9, 2022 20:04

fix(wasmtime):Config methods should be idempotent

a7b00f7

This commit refactored `Config` to use a seperate `CompilerConfig` field instead of operating on `CompilerBuilder` directly to make all its methods idempotent. Fixes bytecodealliance#4189

PureWhiteWu force-pushed the fix/config_overwrite branch from 0ba39e2 to a7b00f7 Compare June 10, 2022 11:38

PureWhiteWu mentioned this pull request Jun 10, 2022

wasmtime: CompilerConfig should be used instead of CompilerBuilder #4258

Open

PureWhiteWu requested review from alexcrichton and cfallin June 10, 2022 16:19

alexcrichton approved these changes Jun 13, 2022

View reviewed changes

alexcrichton merged commit 258dc9d into bytecodealliance:main Jun 13, 2022

PureWhiteWu deleted the fix/config_overwrite branch June 13, 2022 14:35

peterhuene mentioned this pull request Jun 14, 2022

Fix changes to Wasmtime C API. bytecodealliance/wasmtime-dotnet#119

Merged

PureWhiteWu mentioned this pull request Jul 28, 2022

doc: fix c set strategy doc #4550

Merged

cfallin pushed a commit that referenced this pull request Jul 28, 2022

doc: fix c set strategy doc (#4550)

fb7d510

In #4252 I changed the signature and doc of the rust code, but forgot to change the doc for c. Set strategy no longer returns error. This commit fixes that.

symphorien mentioned this pull request Sep 3, 2022

wasmtime: backport to release-22.05 NixOS/nixpkgs#187902

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(wasmtime):`Config` methods should be idempotent #4252

fix(wasmtime):`Config` methods should be idempotent #4252

PureWhiteWu commented Jun 9, 2022

PureWhiteWu commented Jun 9, 2022

github-actions bot commented Jun 9, 2022

github-actions bot commented Jun 9, 2022

cfallin left a comment

cfallin Jun 9, 2022

PureWhiteWu Jun 9, 2022

cfallin Jun 9, 2022

alexcrichton Jun 9, 2022

PureWhiteWu Jun 9, 2022 •

edited

Loading

thibaultcha Jul 28, 2022 •

edited

Loading

PureWhiteWu Jul 28, 2022

thibaultcha Jul 28, 2022

PureWhiteWu Jul 28, 2022

alexcrichton Jul 28, 2022

github-actions bot commented Jun 9, 2022

alexcrichton left a comment

alexcrichton Jun 9, 2022

PureWhiteWu left a comment

PureWhiteWu Jun 9, 2022

PureWhiteWu Jun 9, 2022 •

edited

Loading

PureWhiteWu commented Jun 9, 2022

PureWhiteWu commented Jun 10, 2022

alexcrichton left a comment

PureWhiteWu commented Jun 13, 2022

fix(wasmtime):Config methods should be idempotent #4252

fix(wasmtime):Config methods should be idempotent #4252

Conversation

PureWhiteWu commented Jun 9, 2022

PureWhiteWu commented Jun 9, 2022

github-actions bot commented Jun 9, 2022

Subscribe to Label Action

github-actions bot commented Jun 9, 2022

Label Messager: wasmtime:config

cfallin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PureWhiteWu Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

thibaultcha Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 9, 2022

Subscribe to Label Action

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PureWhiteWu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PureWhiteWu Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

PureWhiteWu commented Jun 9, 2022

PureWhiteWu commented Jun 10, 2022

alexcrichton left a comment

Choose a reason for hiding this comment

PureWhiteWu commented Jun 13, 2022

fix(wasmtime):`Config` methods should be idempotent #4252

fix(wasmtime):`Config` methods should be idempotent #4252

PureWhiteWu Jun 9, 2022 •

edited

Loading

thibaultcha Jul 28, 2022 •

edited

Loading

PureWhiteWu Jun 9, 2022 •

edited

Loading