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

[fix] #4082: Remove cloning for ISI and query execution in wasm #4182

Merged

Conversation

mversic
Copy link
Contributor

@mversic mversic commented Jan 4, 2024

Description

I left Visit trait to work with references and have removed redundant cloning through other means. One of the reasons why I left it is so that we can keep calling mem::forget at the end of validation but also because it's just not necessary to take them by value

Linked issue

Closes #4082

Benefits

less cloning

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

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Jan 4, 2024
@mversic mversic force-pushed the optimize_wasm_isi_execute branch 2 times, most recently from 6645096 to 328b46c Compare January 4, 2024 17:49
@mversic mversic changed the title [fix] #4082: Remove cloning for ISI execution in wasm [fix] #4082: Remove cloning for ISI and query execution in wasm Jan 4, 2024
Copy link
Contributor

@Erigara Erigara left a comment

Choose a reason for hiding this comment

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

Like this change, often cloning could be avoided when doing serialization.

configs/peer/executor.wasm Outdated Show resolved Hide resolved
data_model/src/smart_contract.rs Show resolved Hide resolved
data_model/src/isi.rs Outdated Show resolved Hide resolved
@mversic mversic force-pushed the optimize_wasm_isi_execute branch 3 times, most recently from e059f67 to c43a409 Compare January 10, 2024 08:08
@mversic mversic requested a review from Erigara January 10, 2024 08:09
@mversic mversic force-pushed the optimize_wasm_isi_execute branch 2 times, most recently from e5f062a to c09ebae Compare January 10, 2024 08:39
@DCNick3 DCNick3 self-assigned this Jan 10, 2024
config/src/path.rs Outdated Show resolved Hide resolved
@DCNick3
Copy link
Contributor

DCNick3 commented Jan 10, 2024

I kind of don't like how fragile this approach is: any mismatch between the encode_as_* function implementation and the derived scale encodings (that can be easily created by accident when modifying some instructions, for example) would lead to hard-to-diagnose encoding issues. But I don't there's a way to resolve this that doesn't rely on macros, and we already have enough of those in iroha..

If we decide to go the macro way we can either:

  • automatically derive the encode_as_* functions (keeping the approach used in this PR)
  • automatically generate the sister *BoxRef structures with owned values replaced by references (following the approach suggested by @Erigara)

I think the second one will be easier to implement, especially seeing how heterogeneous the encode methods for instructions are due to different levels of nesting..

Another far-fetched idea w/o using macros is to somehow make instructions/queries cheaper to clone (for example, by moving out all the expensive objects like strings to arenas/bump allocators/...). I don't think those are worth the complexity they bring though.

@Arjentix Arjentix self-assigned this Jan 10, 2024
core/src/smartcontracts/wasm.rs Show resolved Hide resolved
data_model/src/isi.rs Outdated Show resolved Hide resolved
smart_contract/src/lib.rs Show resolved Hide resolved
smart_contract/src/lib.rs Show resolved Hide resolved
@Arjentix
Copy link
Contributor

any mismatch between the encode_as_* function implementation and the derived scale encodings (that can be easily created by accident when modifying some instructions, for example)

I don't think that could actually happen to us.
encode_as_* fallback to self.encode_to(), so changing internal fields won't break anything.
And while we keep this enums everything should be fine. Throwing back these enums would be a big change and we certunally catch such problems with encodings (at least because of autogenerated *Type enums).

Arjentix
Arjentix previously approved these changes Feb 19, 2024
Copy link
Contributor

@Arjentix Arjentix left a comment

Choose a reason for hiding this comment

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

I like it

data_model/src/isi.rs Show resolved Hide resolved
data_model/src/isi.rs Show resolved Hide resolved
@mversic mversic force-pushed the optimize_wasm_isi_execute branch 4 times, most recently from e28c6f8 to fbf7c06 Compare February 20, 2024 11:24
@github-actions github-actions bot added the config-changes Changes in configuration and start up of the Iroha label Feb 26, 2024
Copy link

@BAStos525

Arjentix
Arjentix previously approved these changes Feb 26, 2024
data_model/derive/src/lib.rs Outdated Show resolved Hide resolved
data_model/src/query/mod.rs Show resolved Hide resolved
@s8sato s8sato removed the config-changes Changes in configuration and start up of the Iroha label Feb 26, 2024
@mversic mversic force-pushed the optimize_wasm_isi_execute branch 3 times, most recently from 2b2c6d6 to 5c1b523 Compare February 26, 2024 14:41
@github-actions github-actions bot added the api-changes Changes in the API for client libraries label Feb 26, 2024
Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
@mversic mversic merged commit fd09f10 into hyperledger-iroha:iroha2-dev Feb 28, 2024
15 checks passed
@mversic mversic deleted the optimize_wasm_isi_execute branch April 16, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes Changes in the API for client libraries iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants