Skip to content
This repository was 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

Merged
paritytech-processbot[bot] merged 29 commits intoparitytech:masterfrom
yjhmelody:refactor-use-executor-builder
Apr 9, 2023
Merged

refactor(sc-executor): use wasm executor builder instead of old apis#13740
paritytech-processbot[bot] merged 29 commits intoparitytech:masterfrom
yjhmelody:refactor-use-executor-builder

Conversation

@yjhmelody
Copy link
Contributor

@yjhmelody yjhmelody commented Mar 28, 2023 •
edited
Loading

  • 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
bkchr
bkchr reviewed Mar 29, 2023
View reviewed changes
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!

@yjhmelody yjhmelody mentioned this pull request Mar 30, 2023
Closed
@yjhmelody yjhmelody mentioned this pull request Apr 2, 2023
Merged
@yjhmelody
Copy link
Contributor Author

yjhmelody commented Apr 4, 2023

@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
koute
koute reviewed Apr 4, 2023
View reviewed changes
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.

client/service/src/builder.rs Outdated
}

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

@koute koute Apr 4, 2023

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 •
edited
Loading

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

@koute koute Apr 6, 2023

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.)

client/service/src/builder.rs Outdated
Comment on lines +247 to +248
.with_onchain_heap_alloc_strategy(strategy)
.with_onchain_heap_alloc_strategy(strategy)
Copy link
Contributor

@koute koute Apr 4, 2023

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

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

Should be offchain

@yjhmelody
Copy link
Contributor Author

yjhmelody commented Apr 7, 2023

@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

substrate-tip-bot bot commented Apr 9, 2023

@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

@paritytech-processbot paritytech-processbot bot merged commit 2e94e25 into paritytech:master Apr 9, 2023
This was referenced Apr 11, 2023
Closed
Merged
@Polkadot-Forum
Copy link

Polkadot-Forum commented May 2, 2023

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

@github-actions github-actions bot mentioned this pull request Jun 15, 2023
Closed
@nazar-pc nazar-pc mentioned this pull request Jun 16, 2023
Merged
1 task
@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@yjhmelody @nathanwhit
refactor(sc-executor): use wasm executor builder instead of old apis (p…
324bd7b 
…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
@gregdhill gregdhill mentioned this pull request Aug 2, 2023
Merged
@ukint-vs ukint-vs mentioned this pull request Oct 29, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@koute koute koute approved these changes

@bkchr bkchr bkchr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yjhmelody @bkchr @Polkadot-Forum @koute