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

tweak: Move System version into type system #1919

Open
wants to merge 1 commit into
base: refactor/engine-init-improvements
Choose a base branch
from

Conversation

dhedey
Copy link
Contributor

@dhedey dhedey commented Sep 15, 2024

Summary

This PR moves the system logic version into the type system (SystemV1 = System<V1, ..> and SystemV2) which has a few benefits in my mind:

  • I think the code reads more nicely:
    • I prefer L::do_x(...) than having to read a self.logic_version.do_x()
    • It also can't cause issues with the borrow checker, unlike where the logic version was a value - where sometimes you had to rely on the logic being Copy and do let logic = self.system.versioned_logic; logic.xyz(&mut self.mutable_thing).
  • I think it's the right abstraction - in many places, we're changing the code, not just changing some minor values / configuration - so being able to call into the versioned code directly feels good.
  • We can put calls to it on a hot path (e.g. to change costing logic) without a perf hit.

Testing

Existing tests pass

Other notes

Due to mono-morphization, we effectively doubles the size of some of the compiled engine code, but it doesn't seem to be a big hit: the develop build of the workspace with cargo clean; cargo build; cargo clean is now ~11.0GiB, and it was previously ~10.6GiB - so I don't think we need to factor that in.

Copy link

Phylum Report Link

Copy link

github-actions bot commented Sep 15, 2024

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

@dhedey dhedey force-pushed the tweak/versioned-system-logic-is-generic branch from a3ef568 to 0048963 Compare September 15, 2024 23:19
Copy link

github-actions bot commented Sep 15, 2024

Benchmark for 2d7810f

Click to view benchmark
Test Base PR %
costing::bench_prepare_wasm 45.0±0.10ms 44.6±0.10ms -0.89%
costing::decode_encoded_i8_array_to_manifest_raw_value 19.5±0.12ms 19.3±0.03ms -1.03%
costing::decode_encoded_i8_array_to_manifest_value 42.3±0.05ms 41.7±0.06ms -1.42%
costing::decode_encoded_tuple_array_to_manifest_raw_value 63.4±0.12ms 63.1±0.12ms -0.47%
costing::decode_encoded_tuple_array_to_manifest_value 98.8±1.08ms 104.1±0.20ms +5.36%
costing::decode_encoded_u8_array_to_manifest_raw_value 25.8±0.21µs 26.2±0.06µs +1.55%
costing::decode_encoded_u8_array_to_manifest_value 42.1±0.07ms 41.5±0.26ms -1.43%
costing::decode_rpd_to_manifest_raw_value 12.5±0.04µs 12.5±0.05µs 0.00%
costing::decode_rpd_to_manifest_value 10.5±0.02µs 10.8±0.05µs +2.86%
costing::deserialize_wasm 1273.2±4.42µs 1265.7±2.39µs -0.59%
costing::execute_transaction_creating_big_vec_substates 690.2±6.10ms 705.6±8.71ms +2.23%
costing::execute_transaction_reading_big_vec_substates 578.3±2.20ms 606.1±3.24ms +4.81%
costing::instantiate_flash_loan 1011.3±1391.53µs 961.1±1078.70µs -4.96%
costing::instantiate_radiswap 1029.2±1496.64µs 969.2±515.19µs -5.83%
costing::spin_loop 21.3±0.11ms 21.4±0.08ms +0.47%
costing::validate_sbor_payload 32.5±0.08µs 32.4±0.10µs -0.31%
costing::validate_sbor_payload_bytes 257.2±0.99ns 254.5±1.43ns -1.05%
costing::validate_secp256k1 76.8±0.34µs 76.8±0.26µs 0.00%
costing::validate_wasm 33.6±0.05ms 33.7±0.03ms +0.30%
decimal::add/0 8.4±0.00ns 8.4±0.00ns 0.00%
decimal::add/rust-native 9.9±0.01ns 9.9±0.01ns 0.00%
decimal::add/wasmi 221.7±0.63ns 223.1±0.61ns +0.63%
decimal::add/wasmi-call-native 2.1±0.01µs 2.1±0.00µs 0.00%
decimal::div/0 186.9±0.23ns 186.4±0.09ns -0.27%
decimal::from_string/0 163.6±1.09ns 163.8±0.56ns +0.12%
decimal::mul/0 151.3±0.18ns 150.5±0.21ns -0.53%
decimal::mul/rust-native 147.8±0.16ns 147.6±0.12ns -0.14%
decimal::mul/wasmi 12.1±0.05µs 11.8±0.04µs -2.48%
decimal::mul/wasmi-call-native 2.2±0.01µs 2.3±0.00µs +4.55%
decimal::pow/0 696.4±0.77ns 702.9±0.43ns +0.93%
decimal::pow/rust-native 667.9±1.50ns 666.4±0.49ns -0.22%
decimal::pow/wasmi 57.4±0.24µs 56.9±0.38µs -0.87%
decimal::pow/wasmi-call-native 2.5±0.01µs 2.5±0.01µs 0.00%
decimal::root/0 7.9±0.02µs 8.0±0.01µs +1.27%
decimal::sub/0 8.2±0.00ns 8.2±0.01ns 0.00%
decimal::to_string/0 441.9±1.29ns 443.3±0.31ns +0.32%
precise_decimal::add/0 8.8±0.03ns 9.0±0.02ns +2.27%
precise_decimal::add/rust-native 10.7±0.02ns 10.7±0.03ns 0.00%
precise_decimal::add/wasmi 271.6±1.38ns 282.9±0.46ns +4.16%
precise_decimal::add/wasmi-call-native 2.7±0.01µs 2.9±0.01µs +7.41%
precise_decimal::div/0 318.8±1.01ns 316.5±0.93ns -0.72%
precise_decimal::from_string/0 209.8±0.57ns 210.7±1.81ns +0.43%
precise_decimal::mul/0 366.2±1.80ns 369.2±2.13ns +0.82%
precise_decimal::mul/rust-native 321.7±2.29ns 318.7±1.30ns -0.93%
precise_decimal::mul/wasmi 35.1±0.08µs 34.2±0.39µs -2.56%
precise_decimal::mul/wasmi-call-native 3.1±0.01µs 3.2±0.01µs +3.23%
precise_decimal::pow/0 1972.6±7.45ns 1947.3±3.75ns -1.28%
precise_decimal::pow/rust-native 1568.6±3.44ns 1540.6±3.91ns -1.79%
precise_decimal::pow/wasmi 164.8±1.12µs 164.9±1.59µs +0.06%
precise_decimal::pow/wasmi-call-native 5.7±0.01µs 5.8±0.01µs +1.75%
precise_decimal::root/0 58.0±0.04µs 58.5±0.08µs +0.86%
precise_decimal::sub/0 8.9±0.03ns 9.1±0.04ns +2.25%
precise_decimal::to_string/0 694.0±3.19ns 693.2±0.78ns -0.12%
schema::validate_payload 363.9±0.57µs 366.2±0.43µs +0.63%
transaction::radiswap 5.1±0.02ms 5.1±0.03ms 0.00%
transaction::transfer 1855.5±2.18µs 1873.3±3.27µs +0.96%
transaction_processing::prepare 2.7±0.00ms 2.6±0.01ms -3.70%
transaction_processing::prepare_and_decompile 7.1±0.03ms 7.0±0.04ms -1.41%
transaction_processing::prepare_and_decompile_and_recompile 25.6±0.16ms 25.3±0.09ms -1.17%
transaction_validation::validate_manifest 42.9±0.15µs 42.8±0.03µs -0.23%
transaction_validation::verify_bls_2KB 1000.9±6.16µs 1090.5±20.81µs +8.95%
transaction_validation::verify_bls_32B 1002.8±7.99µs 1009.8±17.95µs +0.70%
transaction_validation::verify_ecdsa 74.7±0.10µs 74.7±0.22µs 0.00%
transaction_validation::verify_ed25519 55.2±0.08µs 55.3±0.13µs +0.18%

@dhedey dhedey force-pushed the tweak/versioned-system-logic-is-generic branch from 0048963 to b31ba81 Compare September 20, 2024 00:55
@dhedey dhedey changed the base branch from tweak/markups-and-test-transaction-builder to develop September 20, 2024 01:00
@dhedey dhedey changed the title tweak: Make SystemLogicVersion a generic for performance tweak: Move SystemLogic version into the type system Sep 20, 2024
@dhedey dhedey changed the title tweak: Move SystemLogic version into the type system tweak: Move System Version logic into the type system Sep 20, 2024
@dhedey dhedey changed the title tweak: Move System Version logic into the type system tweak: Refactor Engine Init (+ move System version into type) Sep 23, 2024
@dhedey dhedey force-pushed the tweak/versioned-system-logic-is-generic branch from 25299b4 to dd9a02f Compare September 23, 2024 10:55
@dhedey dhedey changed the title tweak: Refactor Engine Init (+ move System version into type) tweak: Move System version into type paramater Sep 23, 2024
@dhedey dhedey changed the base branch from develop to refactor/engine-init-improvements September 23, 2024 10:55
@dhedey dhedey force-pushed the tweak/versioned-system-logic-is-generic branch from dd9a02f to 2e6b850 Compare September 23, 2024 10:58
@dhedey dhedey changed the title tweak: Move System version into type paramater tweak: Move System version into type system Sep 23, 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