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

[TEMP ON HOLD] refactor: Use SBOR raw value across engine for better worst case performance #1860

Draft
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

dhedey
Copy link
Contributor

@dhedey dhedey commented Jul 26, 2024

Note

This PR is massive and never quite got finished or reviewed and now has a mountain of merge conflicts. Instead, I suggest we proceed by:

  • Starting off with just the SBOR changes and get them tested a lot, and also agree names for the next steps
  • Use this PR as a refererence, and tackle areas one-by-one:
    • IndexedScryptoValue
    • Validation in the system layer
    • System APIs + Native Components
    • Events
    • Receipts
    • Database Layer
    • Manifest values (and conversion to Scrypto Values)

Summary

Looks to address #1708 and historic issues with ScryptoValue - by replacing ScryptoValue with variants of ScryptoRawValue on the critical paths in the engine.

Notably, compare these benchmarks from #1870 with after this PR:

costing::execute_transaction_creating_big_vec_substates | 706.3±12.13ms | 9.7±0.20ms
costing::execute_transaction_reading_big_vec_substates | 572.7±1.36ms | 3.5±0.02ms

This is due to quite a few places in the System layer where we were decoding as ScryptoValue as the value part of System wrappers. We've known ScryptoValue can be very slow to decode for a while; particularly Vec<u8>. And whilst we should improve that (as Bartosz recommends), using ScryptoRawValue should be faster/safer.

Instead:

  • We now use ScryptoRawValue / ScryptoRawPayload, which decodes much more quickly in the common worst case of large Vec<u8>.
  • We also make use of the new-type pattern to better articulate what something is. Instead of lots of Vec<u8> flying around, we now have concrete new types for whether things have been validated or not.
  • We now separate IndexedOwnedScryptoValue from IndexedScryptoValue<'a> where the latter doesn't necessarily own its value.

Still to come:

  • Engine:
    • Change FieldValue and KVEntry to be ScryptoUnvalidatedOwnedRawPayload and fix SystemObjectApi etc (git stash pop)
    • Change validation to operate over SborUnvalidatedOwnedRawPayload instead of SborUnvalidatedOwnedRawValue
  • SBOR:
    • Add required max depth to SborRawValue and check it when encoding
    • Comprehensive tests on SborRawValue / SborRawPayload
    • Improve vec encoder / vec decoder depth API; verify depth on vec u8 encode/decode
    • Also capture max depth optionally in SborRawPayload when encoding as sbor_raw_payload so it can be converted more performantly to SborRawValue.
    • We may wish to optimise the untyped traverser / calculate_value_tree_body_byte_length as best we can to eek out even more performance.

Details

TODO

Testing

Unit tests on encoded_wrappers.rs to come

Update Recommendations

TODO

Copy link

Phylum Report Link

Copy link

github-actions bot commented Jul 26, 2024

Benchmark for bd69b0e

Click to view benchmark
Test Base PR %
costing::bench_prepare_wasm 63.3±0.11ms 45.3±0.07ms -28.44%
costing::decode_bytes_to_manifest_raw_value 159.9±0.32ns N/A N/A
costing::decode_bytes_to_manifest_value 29.2±0.06µs N/A N/A
costing::decode_rpd_to_manifest_raw_value 12.5±0.02µs N/A N/A
costing::decode_rpd_to_manifest_value 11.0±0.02µs N/A N/A
costing::decode_sbor 10.8±0.01µs N/A N/A
costing::decode_sbor_bytes 29.1±0.05µs N/A N/A
costing::deserialize_wasm 1258.7±7.32µs 1277.0±8.85µs +1.45%
costing::execute_transaction_creating_big_vec_substates 9.7±0.20ms N/A N/A
costing::execute_transaction_reading_big_vec_substates 3.5±0.02ms N/A N/A
costing::instantiate_flash_loan 939.8±726.33µs 854.3±221.58µs -9.10%
costing::instantiate_radiswap 1572.0±6004.96µs 835.6±196.93µs -46.84%
costing::spin_loop 21.0±0.10ms 21.2±0.06ms +0.95%
costing::validate_sbor_payload 32.5±0.13µs 32.5±0.05µs 0.00%
costing::validate_sbor_payload_bytes 251.6±1.33ns 244.1±0.62ns -2.98%
costing::validate_secp256k1 76.7±0.26µs 76.7±0.08µs 0.00%
costing::validate_wasm 33.8±0.05ms 33.4±0.03ms -1.18%
decimal::add/0 8.4±0.00ns 8.4±0.00ns 0.00%
decimal::add/rust-native 9.9±0.01ns 9.9±0.00ns 0.00%
decimal::add/wasmi 222.5±0.55ns 220.4±0.21ns -0.94%
decimal::add/wasmi-call-native 2.1±0.00µs 2.1±0.00µs 0.00%
decimal::div/0 186.6±0.13ns 185.2±0.30ns -0.75%
decimal::from_string/0 156.8±0.25ns 152.9±0.18ns -2.49%
decimal::mul/0 149.1±0.12ns 147.5±0.61ns -1.07%
decimal::mul/rust-native 148.0±0.38ns 143.6±0.12ns -2.97%
decimal::mul/wasmi 12.0±0.05µs 11.9±0.02µs -0.83%
decimal::mul/wasmi-call-native 2.3±0.00µs 2.3±0.00µs 0.00%
decimal::pow/0 703.0±0.66ns 696.0±1.10ns -1.00%
decimal::pow/rust-native 665.4±1.21ns 670.8±0.36ns +0.81%
decimal::pow/wasmi 57.6±0.11µs 57.5±0.14µs -0.17%
decimal::pow/wasmi-call-native 2.5±0.01µs 2.4±0.00µs -4.00%
decimal::root/0 7.9±0.01µs 7.8±0.01µs -1.27%
decimal::sub/0 8.7±0.03ns 8.8±0.11ns +1.15%
decimal::to_string/0 440.2±0.43ns 442.6±0.41ns +0.55%
precise_decimal::add/0 9.0±0.01ns 8.8±0.04ns -2.22%
precise_decimal::add/rust-native 10.8±0.00ns 10.8±0.03ns 0.00%
precise_decimal::add/wasmi 274.7±0.43ns 273.8±0.83ns -0.33%
precise_decimal::add/wasmi-call-native 2.8±0.00µs 2.8±0.00µs 0.00%
precise_decimal::div/0 324.4±3.69ns 317.4±0.87ns -2.16%
precise_decimal::from_string/0 201.4±0.10ns 199.7±0.33ns -0.84%
precise_decimal::mul/0 361.8±0.45ns 362.0±0.56ns +0.06%
precise_decimal::mul/rust-native 322.0±1.60ns 333.1±4.73ns +3.45%
precise_decimal::mul/wasmi 35.2±0.27µs 34.1±0.07µs -3.13%
precise_decimal::mul/wasmi-call-native 3.2±0.00µs 3.2±0.01µs 0.00%
precise_decimal::pow/0 1942.1±4.08ns 1930.2±2.96ns -0.61%
precise_decimal::pow/rust-native 1545.1±8.25ns 1561.3±3.05ns +1.05%
precise_decimal::pow/wasmi 165.2±1.67µs 166.9±0.84µs +1.03%
precise_decimal::pow/wasmi-call-native 5.8±0.03µs 5.8±0.01µs 0.00%
precise_decimal::root/0 61.1±0.15µs 57.6±0.02µs -5.73%
precise_decimal::sub/0 9.2±0.04ns 9.0±0.03ns -2.17%
precise_decimal::to_string/0 707.7±0.90ns 706.6±1.99ns -0.16%
schema::validate_payload 379.1±0.54µs 379.1±0.58µs 0.00%
transaction::radiswap 5.3±0.02ms 5.2±0.02ms -1.89%
transaction::transfer 1921.0±7.32µs 1954.8±5.24µs +1.76%
transaction_processing::prepare 2.5±0.00ms 2.5±0.00ms 0.00%
transaction_processing::prepare_and_decompile 6.2±0.02ms 6.2±0.02ms 0.00%
transaction_processing::prepare_and_decompile_and_recompile 27.1±2.01ms 24.9±0.12ms -8.12%
transaction_validation::validate_manifest 42.7±0.03µs 42.6±0.03µs -0.23%
transaction_validation::verify_bls_2KB 1057.5±14.80µs 1003.5±11.10µs -5.11%
transaction_validation::verify_bls_32B 1001.8±5.61µs 1003.5±8.57µs +0.17%
transaction_validation::verify_ecdsa 74.6±0.35µs 74.6±0.06µs 0.00%
transaction_validation::verify_ed25519 55.6±0.14µs 55.4±0.21µs -0.36%

Copy link

github-actions bot commented Jul 26, 2024

Docker tags
docker.io/radixdlt/private-scrypto-builder:bd69b0e529

@dhedey dhedey force-pushed the refactor/use-sbor-raw-value-in-engine branch from 5c69067 to 21877eb Compare July 26, 2024 23:18
@dhedey dhedey force-pushed the refactor/use-sbor-raw-value-in-engine branch from 6c066ac to 5e80b55 Compare August 7, 2024 23:29
dhedey added a commit that referenced this pull request Aug 9, 2024
@dhedey dhedey changed the title refactor: Use SBOR raw value across engine for better worst case performance [TEMP ON HOLD] refactor: Use SBOR raw value across engine for better worst case performance Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant