Skip to content

perf: Fix cloning of executor#4955

Merged
nxsaken merged 4 commits intohyperledger-iroha:mainfrom
dima74:diralik/fix-executor-cloning
Aug 21, 2024
Merged

perf: Fix cloning of executor#4955
nxsaken merged 4 commits intohyperledger-iroha:mainfrom
dima74:diralik/fix-executor-cloning

Conversation

@dima74
Copy link
Copy Markdown
Contributor

@dima74 dima74 commented Aug 8, 2024

Description

I am debugging tps of a single peer (#4727), measured performance of Sumeragi::try_create_block, and noticed that executor cloning takes about 25% of try_create_block. This is because of LoadedExecutor::raw_executor which was added for snapshot creation. Could be fixed by adding Arc.

It is also interesting that other wasm-related code takes a lot of time, e.g. wasmtime::Linker::instantitate and wasmtime::Store::into_data.

image

Linked issue

Related: #4727

Benefits

Slightly optimize block creation performance

Checklist

  • I've read CONTRIBUTING.md
  • I've used the standard signed-off commit format (or will squash just before merging)
  • All applicable CI checks pass (or I promised to make them pass later)
  • (optional) I've written unit tests for the code changes
  • I replied to all comments after code review, marking all implemented changes with thumbs up

@dima74 dima74 self-assigned this Aug 8, 2024
@github-actions github-actions bot added the config-changes Changes in configuration and start up of the Iroha label Aug 8, 2024
@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 8, 2024

@BAStos525

mversic
mversic previously approved these changes Aug 9, 2024
View reviewed changes
Erigara
Erigara previously approved these changes Aug 9, 2024
View reviewed changes
dima74
dima74 commented Aug 9, 2024
View reviewed changes
Comment thread core/src/executor.rs Outdated
@dima74 dima74 dismissed stale reviews from Erigara and mversic via 871dbcf August 9, 2024 09:32
@dima74 dima74 force-pushed the diralik/fix-executor-cloning branch from fbf620e to 871dbcf Compare August 9, 2024 09:32
mversic
mversic previously approved these changes Aug 9, 2024
View reviewed changes
@dima74 dima74 requested a review from Erigara August 9, 2024 10:03
@mversic mversic force-pushed the diralik/fix-executor-cloning branch from 871dbcf to 99a3113 Compare August 9, 2024 16:04
@mversic mversic enabled auto-merge (squash) August 9, 2024 16:04
@mversic mversic self-requested a review August 9, 2024 16:09
@mversic
Copy link
Copy Markdown
Contributor

mversic commented Aug 9, 2024 •
edited
Loading

so this comment was correct until executor wasm was added to snapshot. Was using wasmtime::Module::serialize considered as a solution instead of adding LoadedExecutor::raw_executor?

@dima74
Copy link
Copy Markdown
Contributor Author

dima74 commented Aug 9, 2024

Was using wasmtime::Module::serialize considered as a solution instead of adding LoadedExecutor::raw_executor?

wasmtime::Module::serialize returns something different from what LoadedExecutor::raw_executor currently stores (I compared lengths of inner Vec<u8> and it differs)

@mversic
Copy link
Copy Markdown
Contributor

mversic commented Aug 9, 2024

Was using wasmtime::Module::serialize considered as a solution instead of adding LoadedExecutor::raw_executor?

wasmtime::Module::serialize returns something different from what LoadedExecutor::raw_executor currently stores (I compared lengths of inner Vec<u8> and it differs)

yes, I know that. What I'm saying is if you can directly serialize/deserialize the wasmtime::Module, you don't need raw_executor in LoadedExecutor

@dima74 dima74 force-pushed the diralik/fix-executor-cloning branch from 99a3113 to f2f2c5e Compare August 15, 2024 13:44
@dima74
Copy link
Copy Markdown
Contributor Author

dima74 commented Aug 15, 2024

if you can directly serialize/deserialize the wasmtime::Module, you don't need raw_executor in LoadedExecutor

Changed to serialize wasmtime::Module directly. Note that this will increase size of snapshot by 1.5MB (our default executor is 0.5MB, serialized wasmtime::Module is around 2MB)

mversic
mversic reviewed Aug 19, 2024
View reviewed changes
Comment thread core/Cargo.toml Outdated
mversic
mversic reviewed Aug 19, 2024
View reviewed changes
Comment thread core/src/smartcontracts/wasm.rs
@dima74 dima74 force-pushed the diralik/fix-executor-cloning branch from 9acb399 to 23648bd Compare August 21, 2024 10:34
dima74 added 4 commits August 21, 2024 16:50
@dima74 @nxsaken
perf: Fix cloning of executor
a5c3bb4 
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
@dima74 @nxsaken
Serialize wasmtime::Module directly for snapshots
2c6f951 
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
@dima74 @nxsaken
Review fixes
26243bd 
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
@dima74 @nxsaken
Review fixes
39befad 
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
@nxsaken nxsaken force-pushed the diralik/fix-executor-cloning branch from 23648bd to 39befad Compare August 21, 2024 12:50
@nxsaken nxsaken disabled auto-merge August 21, 2024 12:50
@nxsaken nxsaken merged commit 6fa78dd into hyperledger-iroha:main Aug 21, 2024
nxsaken pushed a commit that referenced this pull request Aug 21, 2024
@dima74 @nxsaken
perf: Fix cloning of executor (#4955)
eb19bbd 
* perf: Fix cloning of executor
* Serialize `wasmtime::Module` directly for snapshots

---------

Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
@dima74 dima74 deleted the diralik/fix-executor-cloning branch August 21, 2024 13:20
@Erigara
Copy link
Copy Markdown
Contributor

Erigara commented Aug 26, 2024 •
edited
Loading

I think changing to wasmtime::Module could cause problems because serialize/deserialize only works for the same version of wasmtime and in other case can produce errors.
It might be dangerous as well because random changes in serialized snapshot can cause memory safety issues.

EDIT: docs to deserialize.

Erigara
Erigara reviewed Aug 26, 2024
View reviewed changes
Comment thread core/src/smartcontracts/wasm.rs
Comment on lines +270 to +271
// SAFETY: `Module::deserialize` is safe when calling for bytes received from `Module::serialize`.
// We store serialization result on disk and then load it back so should be ok.
Copy link
Copy Markdown
Contributor

@Erigara Erigara Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can't guarantee that...

@mversic
Copy link
Copy Markdown
Contributor

mversic commented Aug 26, 2024

I think changing to wasmtime::Module could cause problems because serialize/deserialize only works for the same version of wasmtime and in other case can produce errors.

How can we end in this situation that we serialize with one and deserialize with another version of wasmtime?

@Erigara
Copy link
Copy Markdown
Contributor

Erigara commented Aug 26, 2024

How can we end in this situation that we serialize with one and deserialize with another version of wasmtime

@mversic update of iroha version which in theory should preserve backward compatibility for block store and snapshot.

@dima74
Copy link
Copy Markdown
Contributor Author

dima74 commented Aug 26, 2024

So we should change back to LoadedExecutor::raw_executor approach

@dima74 dima74 mentioned this pull request Aug 26, 2024
Merged
3 tasks
@dima74
Copy link
Copy Markdown
Contributor Author

dima74 commented Aug 26, 2024

Opened #5009

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mversic mversic mversic approved these changes

@s8sato s8sato Awaiting requested review from s8sato s8sato is a code owner

@SamHSmith SamHSmith Awaiting requested review from SamHSmith

@outoftardis outoftardis Awaiting requested review from outoftardis outoftardis is a code owner

@0x009922 0x009922 Awaiting requested review from 0x009922 0x009922 is a code owner

@DCNick3 DCNick3 Awaiting requested review from DCNick3 DCNick3 is a code owner

+1 more reviewer

@Erigara Erigara Erigara left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@dima74 dima74

@SamHSmith SamHSmith

@mversic mversic

Labels

config-changes Changes in configuration and start up of the Iroha

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dima74 @mversic @Erigara @SamHSmith @nxsaken