Skip to content

perf: Don't validate transactions inside WASM#4995

Merged
nxsaken merged 3 commits intohyperledger-iroha:mainfrom
dima74:diralik/remove-wasm-transaction-validation
Aug 22, 2024
Merged

perf: Don't validate transactions inside WASM#4995
nxsaken merged 3 commits intohyperledger-iroha:mainfrom
dima74:diralik/remove-wasm-transaction-validation

Conversation

@dima74
Copy link
Copy Markdown
Contributor

@dima74 dima74 commented Aug 21, 2024 •
edited
Loading

Context

I am investigating single peer tps performance (#4727), and it turns out that actual execution of wasm code takes most of the time per transaction (#3716 (comment)).

Solution

This PR removes validation of transactions at WASM side - there is no need to revalidate transaction which comes from Iroha Rust code. This gives approximately 50% tps increase (single peer). Also size of default executor is reduced from 492KB to 391KB.

Comparison of tps:

compare

Fixes #4803
Related: #4727
Related: #3716

Checklist

  • I've read CONTRIBUTING.md.
  • (optional) I've written unit tests for the code changes.
  • All review comments have been resolved.

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

github-actions bot commented Aug 21, 2024

@BAStos525

mversic
mversic reviewed Aug 21, 2024
View reviewed changes
Comment thread data_model/src/transaction.rs
mversic
mversic reviewed Aug 21, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@mversic mversic left a comment

Choose a reason for hiding this comment

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

I think you should apply this to all validate_*** methods in this crate

@dima74 dima74 requested a review from mversic August 22, 2024 11:12
@mversic
Copy link
Copy Markdown
Contributor

mversic commented Aug 22, 2024

I think you should apply this to all validate_*** methods in this crate

you still didn't apply it to Name::validate_str

@mversic
Copy link
Copy Markdown
Contributor

mversic commented Aug 22, 2024

I think you should apply this to all validate_*** methods in this crate

you still didn't apply it to Name::validate_str

also Parameter, Trigger and event_set

dima74 added 3 commits August 22, 2024 15:24
@dima74
perf: Don't validate transactions inside WASM
e1395eb 
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
@dima74
Review fixes
d5d56d0 
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
@dima74
Review fixes
3d72453 
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
@dima74 dima74 force-pushed the diralik/remove-wasm-transaction-validation branch from 72375b9 to 3d72453 Compare August 22, 2024 12:38
@dima74
Copy link
Copy Markdown
Contributor Author

dima74 commented Aug 22, 2024

  • Removed validation for Name::validate_str
  • For Parameter, the only validation is NonZeroUsize::try_from(/* NonZeroU64 */), I think it should be kept as is
  • For Trigger there is method ActionCandidate::validate which does really small match check, and also is used in ActionCandidate::new. Do you think it is worth to change it?
  • For event_set didn't find validation method, could you clarify?

@mversic
Copy link
Copy Markdown
Contributor

mversic commented Aug 22, 2024

  • Removed validation for Name::validate_str

    • For Parameter, the only validation is NonZeroUsize::try_from(/* NonZeroU64 */), I think it should be kept as is

    • For Trigger there is method ActionCandidate::validate which does really small match check, and also is used in ActionCandidate::new. Do you think it is worth to change it?

    • For event_set didn't find validation method, could you clarify?

it's fine, I only looked around for deserialize functions

Can you update the numbers in the PR description?

@dima74
Copy link
Copy Markdown
Contributor Author

dima74 commented Aug 22, 2024 •
edited
Loading

Can you update the numbers in the PR description?

Updated executor size. Tps numbers remains the same

@nxsaken nxsaken merged commit 9e8c35d into hyperledger-iroha:main Aug 22, 2024
@dima74 dima74 deleted the diralik/remove-wasm-transaction-validation branch August 22, 2024 15:24
mversic pushed a commit that referenced this pull request Aug 30, 2024
@dima74 @mversic
perf: Don't validate transactions inside WASM (#4995)
a56c440 
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
@mversic mversic mentioned this pull request Sep 5, 2024
Closed
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

@Erigara Erigara Awaiting requested review from Erigara

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

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

Assignees

@dima74 dima74

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.

Consider removing validation of transactions inside WASM

3 participants

@dima74 @mversic @nxsaken