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.

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

feat(wasmtime) expose cache_config directive #536

Closed
wants to merge 1 commit into from

Conversation

hishamhm
Copy link
Contributor

This adds support for wasmtime { flag cache_config_load <config.toml>; }, for enabling disk caching of ahead-of-time compilation of .wasm files.

This should alleviate noticeable CPU spikes when starting up workers using Wasmtime and Wasm modules larger than a few megabytes.

Implementation notes:

@hishamhm
Copy link
Contributor Author

@thibaultcha the tests for cache enabled and disabled don't actually check that the cache was actually created (I don't know how to check for the existence of files at the end of a test within the Test::Nginx DSL), but if you run each of them with -- ONLY you can see that one of them creates the cache in the temporary dir and the other one doesn't.

Copy link

codecov bot commented Apr 23, 2024

Codecov Report

Merging #536 (fa89090) into main (bb139e1) will decrease coverage by 0.00210%.
Report is 2 commits behind head on main.
The diff coverage is 89.47368%.

Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                 @@
##                main        #536         +/-   ##
===================================================
- Coverage   90.06471%   90.06261%   -0.00210%     
===================================================
  Files             47          47                 
  Lines          10045       10063         +18     
===================================================
+ Hits            9047        9063         +16     
- Misses           998        1000          +2     
Files Coverage Δ
src/wasm/ngx_wasm_core_module.c 92.64706% <ø> (ø)
src/wasm/wrt/ngx_wrt_wasmtime.c 82.56881% <89.47368%> (+0.36816%) ⬆️
Flag Coverage Δ
unit 90.12555% <86.66667%> (-0.00588%) ⬇️
valgrind 78.05435% <26.31579%> (-0.10945%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@thibaultcha
Copy link
Member

Ultimately it's fine if all we test is our directive and its calling of the underlying wasmtime function. We aren't responsible for testing the behavior of the runtime itself, even though that's a plus if we can, but well...

@hishamhm hishamhm force-pushed the feat/wasmtime-config-cache branch from b852a14 to 70e7140 Compare April 24, 2024 14:49
@hishamhm hishamhm changed the title feat(wasmtime) expose cache_config_load directive feat(wasmtime) expose cache_config directive Apr 24, 2024
@hishamhm hishamhm force-pushed the feat/wasmtime-config-cache branch from 70e7140 to 9ef2ec5 Compare April 24, 2024 15:16
@hishamhm hishamhm force-pushed the feat/wasmtime-config-cache branch from 9ef2ec5 to 852fdee Compare April 24, 2024 15:57
This adds support for `wasmtime { cache_config <config.toml>; }`,
for enabling disk caching of ahead-of-time compilation of .wasm files.

This should alleviate noticeable CPU spikes when starting up workers using
Wasmtime and Wasm modules larger than a few megabytes.
@hishamhm hishamhm force-pushed the feat/wasmtime-config-cache branch from 852fdee to fa89090 Compare April 24, 2024 16:01
#warn "\n", $file, "\n";
unlink($file);
}
});
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this in this file?

use t::TestWasmX;

our $nginxV = $t::TestWasmX::nginxV;
our $osname = $t::TestWasmX::osname;
Copy link
Member

Choose a reason for hiding this comment

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

Also looks unused

@thibaultcha thibaultcha added the pr/merge-in-progress PR: Merge in progress (do not push) label Apr 24, 2024
@thibaultcha thibaultcha deleted the feat/wasmtime-config-cache branch April 24, 2024 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/merge-in-progress PR: Merge in progress (do not push)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants