Stream: git-wasmtime

Topic: wasmtime / PR #10665 Allow creation of a CacheConfig with...

Wasmtime GitHub notifications bot (Apr 24 2025 at 09:27):

benbrandt opened PR #10665 from benbrandt:expose-cache-config to bytecodealliance:main:

Addresses issue #10638

Since creating a CacheConfig also creates the worker thread, I opted for a Builder pattern, so that this worker thread, validation, and default values could be created in the build method, much like what happens when deserializing from a file.

If there is a preferred alternate approach though, let me know.

One thing that came to mind is that we may make it easier to create these worker threads, because it is easier to construct the CacheConfig, but since these threads get cleaned up once the CacheConfig is dropped, maybe this is not an issue?

I'm also not sure if there is any danger in having multiple CacheConfigs pointing to the same directory in terms of the worker threads. But this issue could have existed with multiple Config with the same default config, so I assume this is not a huge issue.

If there are any changes needed, let me know, especially around coding guidelines or best testing practices.

Thanks!
<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Apr 24 2025 at 09:27):

benbrandt requested alexcrichton for a review on PR #10665.

Wasmtime GitHub notifications bot (Apr 24 2025 at 09:27):

benbrandt requested wasmtime-core-reviewers for a review on PR #10665.

Wasmtime GitHub notifications bot (Apr 24 2025 at 09:29):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 24 2025 at 10:47):

github-actions[bot] commented on PR #10665:

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.

  <details>

  Our documentation should be of the following form:

  ```text
  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.
  ```

  </details>

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

  <details>

  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][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][fuzzing-docs] for more details.

  </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.

[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html

---

<details>

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

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

Learn more.

</details>

Wasmtime GitHub notifications bot (Apr 24 2025 at 21:26):

alexcrichton submitted PR review:

Thanks again for this! FWIW the caching system hasn't really been substantially touched since its inception in 2019 so while reading over this I've noticed some other things that I think might be worth implementing, but I also don't want to hitch you with unnecessary work you don't have time to do. In that sense I'd like to see if you're interested in some of these adjacent cleanups perhaps? If not no worries we can go ahead and land this and I can work on some cleanups later.

• All Config::cache_* methods I think can now be deleted in favor of your new cache_config method with a minor adjustment to take Option<CacheConfig> where None means "disable". We could then delegate file parsing and "load default" to methods on CacheConfig which are already there as CacheConfig::from_file
• For the worker I think ideally creating a Config wouldn't actually spawn anything. Instead I think it would be best to defer that to Engine-creation time. For example we'd have some code in the engine creation where it'd call config.cache_config.spawn() or similar.
• To mirror configuration in Wasmtime we could perhaps rename wasmtime_cache::CacheConfig to Cache and then rename wasmtime_cache::CacheConfigBuilder to CacheConfig. Basically centralizing on "Thing" is the actual thing that was made where "ThingConfig" is configuration to produce the Thing. That means the new method could perhaps be Config::cache(&mut self, cache: Option<Cache>).

Would you be up for those changes as well? I've got a few comments on this change in particular below too as well, and again I'm happy to defer the above improvements for later if you'd prefer to not take that on.

Wasmtime GitHub notifications bot (Apr 24 2025 at 21:26):

alexcrichton created PR review comment:

While we don't have hard-and-fast conventions in Wasmtime I at least personally prefer builders to take &mut self and return &mut Self (like Config methods). Would you be up to changing to that pattern?

Wasmtime GitHub notifications bot (Apr 24 2025 at 21:26):

alexcrichton created PR review comment:

Mind adding some docs to this method since it'll be public-facing in the wasmtime crate API?

Wasmtime GitHub notifications bot (Apr 24 2025 at 21:26):

alexcrichton created PR review comment:

Mind adding some documentation here? I'm thinking specifically about some error cases for example.

Additionally if you're ok with the &mut self suggestion above, this would change to either &self (ideally) or &mut self if necessary.

Wasmtime GitHub notifications bot (Apr 24 2025 at 21:26):

alexcrichton created PR review comment:

If you're ok I'd prefer to leave this not exposed, it's a bit weird to me to go through the rigamarole of creating a config just to say it's disabled. In that sense I think we should consider CacheConfigBuilder as "always create something with the enabled flag set to true".

Wasmtime GitHub notifications bot (Apr 24 2025 at 21:30):

benbrandt commented on PR #10665:

@alexcrichton thanks for the thorough review!
I think all of these changes definitely make sense, and also address some weirdness I felt in implementing this. I can give the changes a try and see how far I get!

Wasmtime GitHub notifications bot (Apr 25 2025 at 20:54):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 25 2025 at 20:54):

benbrandt submitted PR review.

Wasmtime GitHub notifications bot (Apr 25 2025 at 20:54):

benbrandt created PR review comment:

Agreed this indeed felt weird as well, I think we can go with an option on the main config and handle it that way

Wasmtime GitHub notifications bot (Apr 25 2025 at 20:55):

benbrandt submitted PR review.

Wasmtime GitHub notifications bot (Apr 25 2025 at 20:55):

benbrandt created PR review comment:

I had a feeling this one might come up, and I figured it was a 50/50 chance I got it right :)
Not a problem

Wasmtime GitHub notifications bot (Apr 25 2025 at 21:07):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 27 2025 at 14:39):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 27 2025 at 15:52):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 27 2025 at 18:59):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 28 2025 at 09:05):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 28 2025 at 09:48):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 28 2025 at 12:28):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 29 2025 at 11:06):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 29 2025 at 11:27):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 29 2025 at 11:34):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 29 2025 at 11:58):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 29 2025 at 12:03):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 29 2025 at 12:08):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 29 2025 at 12:08):

benbrandt has marked PR #10665 as ready for review.

Wasmtime GitHub notifications bot (Apr 29 2025 at 12:08):

benbrandt requested wasmtime-default-reviewers for a review on PR #10665.

Wasmtime GitHub notifications bot (Apr 29 2025 at 12:08):

benbrandt requested pchickey for a review on PR #10665.

Wasmtime GitHub notifications bot (Apr 29 2025 at 12:11):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 29 2025 at 12:15):

benbrandt commented on PR #10665:

@alexcrichton Ok I think it is ready for review again. I also think this is all a lot, lot nicer than before, as well as being more flexible.

Notes of breaking changes:

• Previous cache methods on Config have been removed in favor of a single cache method that can be set to None to disable it, or passed a constructed Cache, either from a file or manually configured
• I removed enabled from CacheConfig (more controversial)

The latter felt kind of weird to leave as an option, since now there is either a Cache or there is not, and it simplified things to not have to worry if it was enabled or not.

However, because of #[serde(deny_unknown_fields)], this will cause existing config files to not work.

If we want to support this old behavior, I am fine with doing so. Users who embed wasmtime will need to update their code regardless, so this might be acceptable.
However, CLI users would have to fix this themselves.

Wasmtime GitHub notifications bot (Apr 29 2025 at 12:19):

benbrandt edited a comment on PR #10665:

@alexcrichton Ok I think it is ready for review again. I also think this is all a lot, lot nicer than before, as well as being more flexible.

Notes of breaking changes:

• Previous cache methods on Config have been removed in favor of a single cache method that can be set to None to disable it, or passed a constructed Cache, either from a file or manually configured
• I removed enabled from CacheConfig (more controversial)

Other notable changes:

• Introduced a separate Cache struct to better separate the config from the actual thing holding the worker
• Moved default behavior to serialization time to also remove the awkward workarounds for assuming struct fields are set and not None after a certain stage

The enabled flag felt kind of weird to leave as an option, since now there is either a Cache or there is not, and it simplified things to not have to worry if it was enabled or not.

However, because of #[serde(deny_unknown_fields)], this will cause existing config files to not work.

If we want to support this old behavior, I am fine with doing so. Users who embed wasmtime will need to update their code regardless, so this might be acceptable.
However, CLI users would have to fix this themselves.

Happy to address any and all feedback, I went in circles a bit trying out different things here, so there is likely room for improvement!

Wasmtime GitHub notifications bot (Apr 29 2025 at 12:24):

benbrandt updated PR #10665.

Wasmtime GitHub notifications bot (Apr 29 2025 at 14:50):

alexcrichton submitted PR review:

Thanks again for your work on this! All the changes here look reasonable to me, and thinking about it I think it's reasonable to leave deny_unknown_fields and drop enabled = true as well. That's a pretty old setting which predated the current CLI flags and configuration system, so I think it's reasonable to take this time to bring it forward to the current era.

Wasmtime GitHub notifications bot (Apr 29 2025 at 15:12):

alexcrichton merged PR #10665.

Wasmtime GitHub notifications bot (May 29 2025 at 08:16):

dev-null-undefined commented on PR #10665:

The documentation seems outdated https://github.com/bytecodealliance/wasmtime/blob/0fabedfece020cab90124768c3dc94b8d95ca4ab/docs/cli-cache.md#setting-enabled

Wasmtime GitHub notifications bot (May 29 2025 at 14:18):

benbrandt commented on PR #10665:

@dev-null-undefined good catch! Made a follow up PR here: https://github.com/bytecodealliance/wasmtime/pull/10859

Last updated: Dec 06 2025 at 07:03 UTC