Skip to content

Allow creation of a CacheConfig without loading from a file #10665

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
Merged
merged 14 commits into from
Apr 29, 2025
Merged

Allow creation of a CacheConfig without loading from a file #10665

merged 14 commits into from
Apr 29, 2025

Conversation

benbrandt
Copy link
Member

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!

@benbrandt benbrandt requested a review from a team as a code owner April 24, 2025 09:27
@benbrandt benbrandt requested review from alexcrichton and removed request for a team April 24, 2025 09:27
@github-actions github-actions bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:config Issues related to the configuration of Wasmtime labels Apr 24, 2025
@github-actions 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.

alexcrichton
alexcrichton reviewed Apr 24, 2025
View reviewed changes
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

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.

@benbrandt
Copy link
Member Author

@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!

@github-actions github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label Apr 27, 2025
@benbrandt benbrandt force-pushed the expose-cache-config branch 3 times, most recently from c2d3d6f to 22020b4 Compare April 28, 2025 09:05
@benbrandt benbrandt marked this pull request as draft April 28, 2025 12:26
@benbrandt benbrandt force-pushed the expose-cache-config branch from 455e8dd to 5b8657c Compare April 29, 2025 11:34
@benbrandt benbrandt marked this pull request as ready for review April 29, 2025 12:08
@benbrandt benbrandt requested a review from a team as a code owner April 29, 2025 12:08
@benbrandt benbrandt requested review from pchickey and removed request for a team April 29, 2025 12:08
@benbrandt
Copy link
Member Author

benbrandt commented Apr 29, 2025 •
edited
Loading

@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!

benbrandt added 11 commits April 29, 2025 14:24
@benbrandt
cache: Add builder pattern for CacheConfig
4a7de19
@benbrandt
wasmtime: Add cache_config method to wasmtime::Config
de89728
@benbrandt
Refactor test_builder_default to use test_prolog helper
5c6300b
@benbrandt
Remove enabled option from CacheConfigBuilder and always set to true
ffe5ab5
@benbrandt
Change builder methods to take &mut self and return &mut Self
c7cd644
@benbrandt
Simplify cache configuration API
4b357c0 
A new `cache_config(Option<CacheConfig>)` method replaces multiple methods
for controlling module caching. Now `None` disables caching, and users can
directly provide a cache config or load one from a file.
@benbrandt
Make cache configuration optional
4e35e2a
@benbrandt
Add Cache struct to separate configuration from runtime (wip)
c28ccca
@benbrandt
Ensure default values earlier
53c917a
@benbrandt
Consolidate CacheConfig and CacheConfigBuilder
b20c619
@benbrandt
Set Cache directly on wastime::Config and make it easier to create one
b1dac37 
from a file
@benbrandt benbrandt force-pushed the expose-cache-config branch from 4c03661 to dd1f91d Compare April 29, 2025 12:24
@github-actions github-actions bot added the wasmtime:docs Issues related to Wasmtime's documentation label Apr 29, 2025
alexcrichton
alexcrichton approved these changes Apr 29, 2025
View reviewed changes
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

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.

@alexcrichton alexcrichton added this pull request to the merge queue Apr 29, 2025
Merged via the queue into bytecodealliance:main with commit 2cd52b7 Apr 29, 2025
43 checks passed
@dev-null-undefined
Copy link

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

@benbrandt
Copy link
Member Author

@dev-null-undefined good catch! Made a follow up PR here: #10859

@benbrandt benbrandt mentioned this pull request May 29, 2025
Merged
@jder jder mentioned this pull request Aug 4, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcrichton alexcrichton alexcrichton approved these changes

@pchickey pchickey Awaiting requested review from pchickey pchickey is a code owner automatically assigned from bytecodealliance/wasmtime-default-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 wasmtime:docs Issues related to Wasmtime's documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benbrandt @dev-null-undefined @alexcrichton