Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

WasmExecutor takes a cache directory #8057

Merged
1 commit merged into from
Feb 9, 2021
Merged

WasmExecutor takes a cache directory #8057

1 commit merged into from
Feb 9, 2021

Conversation

pepyakin
Copy link
Contributor

@pepyakin pepyakin commented Feb 5, 2021

This PR allows to specify a cache directory when creating a WasmExecutor. If specified and wasmtime executor is used, we configure wasmtime to use it to put its compiled artifacts cache there.

This change is motivated by the polkadot validation function which has to execute many different wasm blobs as quickly as possible. Right now, the lack of caching leads to severe stalls.

polkadot companion: paritytech/polkadot#2387

That is useful for executors like wasmtime which produces compiled code
and can actually benefit from caching under some circumstances
@pepyakin pepyakin added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Feb 5, 2021
@dvdplm
Copy link
Contributor

dvdplm commented Feb 5, 2021

@insipx Potentially useful in substrate-archive too maybe?

.map_err(|err| format!("cannot write the cache config: {:?}", err))?;

config
.cache_config_load(cache_config_path)
Copy link
Contributor

@kianenigma kianenigma Feb 8, 2021

Choose a reason for hiding this comment

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

I read in the docs that this needs a config feature enabled on wasmtime, but I can't see it anywhere. Am I missing something?

In 0.19 it is not feature gated, in latest (0.21) it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I assume you meant the cache feature), it is enabled by default as well.

@@ -267,6 +283,7 @@ impl<D: NativeExecutionDispatch> NativeExecutor<D> {
default_heap_pages,
host_functions,
max_runtime_instances,
None,
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean caching is disabled when running substrate node?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I deliberately didn't put it here.

The thing is that this change is dictated by needs of polkadot. I believe it should mitigate the stalls we are seeing there right now. Therefore, it is critical to land it quickly. I figured that we should move the question about the cache out of the way.

@pepyakin
Copy link
Contributor Author

pepyakin commented Feb 9, 2021

bot merge

@ghost
Copy link

ghost commented Feb 9, 2021

Trying merge.

@ghost ghost merged commit 48e9d49 into master Feb 9, 2021
@ghost ghost deleted the ser-wasmtime-caching-hack branch February 9, 2021 16:48
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants