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

PVF: Consider removing Executor struct #622

Closed
mrcnski opened this issue May 30, 2023 · 4 comments · Fixed by #1855
Closed

PVF: Consider removing Executor struct #622

mrcnski opened this issue May 30, 2023 · 4 comments · Fixed by #1855
Labels
I4-refactor Code needs refactoring.

Comments

@mrcnski
Copy link
Contributor

mrcnski commented May 30, 2023

ISSUE

Overview

This struct doesn't seem that useful now that the rayon threadpool has been removed. It used to look like this. Without that, its methods can be standalone functions that take a Config. This would slightly simplify some of the code. I won't remove it just yet in case it could be useful again later.

From paritytech/polkadot#7246 (comment)

@eagr
Copy link
Contributor

eagr commented Oct 10, 2023

validate_using_artifact(
&compiled_artifact_blob,
&params,
executor_2,
cpu_time_start,
)

Does this call construct a runtime every time? If so, this may be worth doing. Or am I missing anything?

@mrcnski
Copy link
Contributor Author

mrcnski commented Oct 11, 2023

Not sure I'm following @eagr. But this issue would be a nice simplification if you wanna take it on. :)

@eagr
Copy link
Contributor

eagr commented Oct 11, 2023

validate_using_artifact() is being invoked in a loop. It in turn calls Executor::execute() which constructs a WasmtimeRuntime every time. It seems to me that the WasmtimeRuntime should be reused to spawn instances rather than being constructed on each iteration, right?

@mrcnski
Copy link
Contributor Author

mrcnski commented Oct 11, 2023

Interesting idea @eagr. The runtime construction depends on the artifact, so we can't re-use the same runtime for every job. But there's maybe an idea here of keeping around all runtimes in memory. If they use 30mb of RAM each, a validator could keep 30 (or however many to cover all parachains). However I think the runtime construction is a very small part of execution, and this cache would probably be less useful when pay-as-you-go arrives. @eskimor

@eagr eagr mentioned this issue Oct 11, 2023
bkchr added a commit that referenced this issue Oct 14, 2023
closes #622 

Pros:
* simpler interface, just functions:
`create_runtime_from_artifact_bytes()` and `execute_artifact()`

Cons:
* extra overhead of constructing executor semantics each time

I could make it a combination of
* `create_runtime_config(params)` (such that we could clone the
constructed semantics)
* `create_runtime(blob, config)`
* `execute_artifact(blob, config, params)`

Not sure if it's worth it though.

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this issue Mar 25, 2024
closes paritytech#622 

Pros:
* simpler interface, just functions:
`create_runtime_from_artifact_bytes()` and `execute_artifact()`

Cons:
* extra overhead of constructing executor semantics each time

I could make it a combination of
* `create_runtime_config(params)` (such that we could clone the
constructed semantics)
* `create_runtime(blob, config)`
* `execute_artifact(blob, config, params)`

Not sure if it's worth it though.

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I4-refactor Code needs refactoring.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants