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

refactor(sc-executor): use wasm executor builder instead of old apis #13740

Conversation

yjhmelody
Copy link
Contributor

@yjhmelody yjhmelody commented Mar 28, 2023

  • Use the builder style apis to create runtime executor
  • Deprecated the old new api for executor
  • Improve and fix inconsistency about executor config(e.g. confused max_runtime_instances with runtime_cache_size).
  • Add new_native_executor to reduce boilerplate code.

polkadot address: 15ouFh2SHpGbHtDPsJ6cXQfes9Cx1gEFnJJsJVqPGzBSTudr

@yjhmelody yjhmelody marked this pull request as ready for review March 28, 2023 20:49
@yjhmelody yjhmelody requested a review from koute as a code owner March 28, 2023 20:49
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Ty!

client/executor/src/executor.rs Outdated Show resolved Hide resolved
client/executor/src/executor.rs Outdated Show resolved Hide resolved
client/executor/src/executor.rs Outdated Show resolved Hide resolved
client/executor/src/executor.rs Outdated Show resolved Hide resolved
@yjhmelody
Copy link
Contributor Author

@bkchr Hi, anything need to be improved?

@koute koute added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Apr 4, 2023
Copy link
Contributor

@koute koute left a comment

Choose a reason for hiding this comment

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

Looks mostly fine to me.

@@ -228,6 +230,27 @@ where
Ok((client, backend, keystore_container, task_manager))
}

/// Creates a [`NativeElseWasmExecutor`] according to [`Configuration`].
pub fn new_native_executor<D: NativeExecutionDispatch>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub fn new_native_executor<D: NativeExecutionDispatch>(
pub fn new_native_or_wasm_executor<D: NativeExecutionDispatch>(

Copy link
Contributor Author

@yjhmelody yjhmelody Apr 6, 2023

Choose a reason for hiding this comment

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

Ok, though I find somewhere use this name when work with NativeElseWasmExecutor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, our current naming might not be the best. (But we're going to remove the native executor soon-ish, so I guess it doesn't really matter all that much.)

Comment on lines 247 to 248
.with_onchain_heap_alloc_strategy(strategy)
.with_onchain_heap_alloc_strategy(strategy)
Copy link
Contributor

Choose a reason for hiding this comment

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

You've copy-pasted the same thing twice. (:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be offchain

@yjhmelody
Copy link
Contributor Author

@koute Any other problem?

@bkchr bkchr added B1-note_worthy Changes should be noted in the release notes T0-node This PR/Issue is related to the topic “node”. and removed B0-silent Changes should not be mentioned in any release notes labels Apr 9, 2023
@bkchr
Copy link
Member

bkchr commented Apr 9, 2023

/tip small

@substrate-tip-bot
Copy link

@bkchr A small tip was successfully submitted for yjhmelody (15ouFh2SHpGbHtDPsJ6cXQfes9Cx1gEFnJJsJVqPGzBSTudr on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips tip

@bkchr
Copy link
Member

bkchr commented Apr 9, 2023

bot merge

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/april-updates-for-substrate-and-polkadot-devs/2764/1

nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
…aritytech#13740)

* refactor: use builder api for all executors

* improve a lot

* remove unused args

* cleanup deps

* fix inconsistency about heap alloc

* add `heap_pages` back to try-runtime

* fix

* chore: reduce duplicated code for sc-service-test

* cleanup code

* fmt

* improve test executor

* improve

* use #[deprecated]

* set runtime_cache_size: 4

* fix and improve

* refactor builder

* fix

* fix bench

* fix tests

* fix warnings

* fix warnings

* fix

* fix

* update by suggestions

* update name
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. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants