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

perf: Fix cloning of executor #4955

Merged
merged 4 commits into from
Aug 21, 2024

Conversation

dima74
Copy link
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
Copy link

github-actions bot commented Aug 8, 2024

@BAStos525

mversic
mversic previously approved these changes Aug 9, 2024
Erigara
Erigara previously approved these changes Aug 9, 2024
core/src/executor.rs Outdated Show resolved Hide resolved
@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
@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
Contributor

mversic commented Aug 9, 2024

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
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
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
Copy link
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)

core/Cargo.toml Outdated Show resolved Hide resolved
@dima74 dima74 force-pushed the diralik/fix-executor-cloning branch from 9acb399 to 23648bd Compare August 21, 2024 10:34
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
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
9 of 11 checks passed
nxsaken pushed a commit that referenced this pull request Aug 21, 2024
* 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
Contributor

Erigara 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.
It might be dangerous as well because random changes in serialized snapshot can cause memory safety issues.

EDIT: docs to deserialize.

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
Contributor

@Erigara Erigara Aug 26, 2024

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
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
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
Contributor Author

dima74 commented Aug 26, 2024

So we should change back to LoadedExecutor::raw_executor approach

@dima74
Copy link
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
Labels
config-changes Changes in configuration and start up of the Iroha
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants