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

[pallet-revive] pack exceeding syscall arguments into registers #7319

Merged
merged 15 commits into from
Jan 28, 2025

Conversation

xermicus
Copy link
Member

This PR changes how we call runtime API methods with more than 6 arguments: They are no longer spilled to the stack but packed into registers instead. Pointers are 32 bit wide so we can pack two of them into a single 64 bit register. Since we mostly pass pointers, this technique effectively increases the number of arguments we can pass using the available registers.

To make this work for instantiate too we now pass the code hash and the call data in the same buffer, akin to how the create family opcodes work in the EVM. The code hash is fixed in size, implying the start of the constructor call data.

@xermicus xermicus added R0-silent Changes should not be mentioned in any release notes T7-smart_contracts This PR/Issue is related to smart contracts. labels Jan 23, 2025
@xermicus xermicus requested review from pgherveou and athei January 23, 2025 18:44
@xermicus
Copy link
Member Author

bot fmt

@command-bot
Copy link

command-bot bot commented Jan 23, 2025

@xermicus https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/8080673 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 2-d43e9094-e6fe-4e8f-822a-93bd1f6a5fe2 to cancel this command or bot cancel to cancel all commands in this pull request.

Copy link
Contributor

We have migrated the command bot to GHA

Please, see the new usage instructions here or here. Soon the old commands will be disabled.

@command-bot
Copy link

command-bot bot commented Jan 23, 2025

@xermicus Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/8080673 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/8080673/artifacts/download.

@xermicus
Copy link
Member Author

/cmd prdoc --audience runtime_dev --bump major

@xermicus
Copy link
Member Author

/cmd bench --runtime dev --pallet pallet_revive

Signed-off-by: xermicus <cyrill@parity.io>
Copy link
Contributor

Command "bench --runtime dev --pallet pallet_revive" has started 🚀 See logs here

xermicus and others added 2 commits January 24, 2025 10:16
Signed-off-by: xermicus <cyrill@parity.io>
Copy link
Contributor

Command "bench --runtime dev --pallet pallet_revive" has finished ✅ See logs here

Subweight results:
File Extrinsic Old New Change [%]
cumulus/pallets/collator-selection/src/weights.rs leave_intent - - ERROR
cumulus/pallets/collator-selection/src/weights.rs new_session - - ERROR
cumulus/pallets/collator-selection/src/weights.rs register_as_candidate - - ERROR
cumulus/pallets/collator-selection/src/weights.rs set_invulnerables - - ERROR
cumulus/pallets/collator-selection/src/weights.rs take_candidate_slot - - ERROR
cumulus/pallets/collator-selection/src/weights.rs update_bond - - ERROR
cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_collator_selection.rs take_candidate_slot - - ERROR
cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_collator_selection.rs update_bond - - ERROR
cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_collator_selection.rs take_candidate_slot - - ERROR
cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_collator_selection.rs update_bond - - ERROR
cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_collator_selection.rs take_candidate_slot - - ERROR
cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_collator_selection.rs update_bond - - ERROR
cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_collator_selection.rs take_candidate_slot - - ERROR
cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_collator_selection.rs update_bond - - ERROR
cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_collator_selection.rs take_candidate_slot - - ERROR
cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_collator_selection.rs update_bond - - ERROR
cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_preimage.rs ensure_updated - - ERROR
cumulus/parachains/runtimes/people/people-rococo/src/weights/pallet_collator_selection.rs take_candidate_slot - - ERROR
cumulus/parachains/runtimes/people/people-rococo/src/weights/pallet_collator_selection.rs update_bond - - ERROR
cumulus/parachains/runtimes/people/people-westend/src/weights/pallet_collator_selection.rs take_candidate_slot - - ERROR
cumulus/parachains/runtimes/people/people-westend/src/weights/pallet_collator_selection.rs update_bond - - ERROR
polkadot/runtime/westend/src/weights/pallet_preimage.rs ensure_updated - - ERROR
substrate/frame/election-provider-support/src/weights.rs phragmen - - ERROR
substrate/frame/election-provider-support/src/weights.rs phragmms - - ERROR
substrate/frame/revive/src/weights.rs seal_call_data_load 256.00ns 365.00ns +42.58
substrate/frame/revive/src/weights.rs seal_call_data_size 267.00ns 338.00ns +26.59
substrate/frame/revive/src/weights.rs seal_ref_time_left 273.00ns 344.00ns +26.01
substrate/frame/revive/src/weights.rs seal_origin 265.00ns 329.00ns +24.15
substrate/frame/revive/src/weights.rs seal_address 271.00ns 336.00ns +23.99
substrate/frame/revive/src/weights.rs seal_caller 305.00ns 378.00ns +23.93
substrate/frame/revive/src/weights.rs seal_to_account_id 29.00us 35.09us +21.01
substrate/frame/revive/src/weights.rs seal_gas_limit 280.00ns 331.00ns +18.21
substrate/frame/revive/src/weights.rs seal_base_fee 290.00ns 341.00ns +17.59
substrate/frame/revive/src/weights.rs seal_return_data_size 267.00ns 310.00ns +16.10
substrate/frame/revive/src/weights.rs seal_block_number 274.00ns 314.00ns +14.60
substrate/frame/revive/src/weights.rs seal_minimum_balance 279.00ns 315.00ns +12.90
substrate/frame/revive/src/weights.rs seal_value_transferred 274.00ns 309.00ns +12.77
substrate/frame/revive/src/weights.rs rollback_transient_storage 1.27us 1.41us +11.04
substrate/frame/revive/src/weights.rs seal_caller_is_root 299.00ns 332.00ns +11.04
substrate/frame/revive/src/weights.rs seal_now 290.00ns 316.00ns +8.97
substrate/frame/revive/src/weights.rs seal_own_code_hash 305.00ns 328.00ns +7.54
substrate/frame/revive/src/weights.rs seal_caller_is_origin 338.00ns 357.00ns +5.62
substrate/frame/revive/src/weights.rs seal_sr25519_verify 1.39ms 1.46ms +5.29
substrate/frame/revive/src/weights.rs seal_ecdsa_recover 48.07us 50.49us +5.03
Command output:

✅ Successful benchmarks of runtimes/pallets:
-- dev: ['pallet_revive']

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12987153921
Failed job name: test-linux-stable

substrate/frame/revive/uapi/src/host/riscv64.rs Outdated Show resolved Hide resolved
Comment on lines 135 to 136
/// Helper to pack two `u32` values into a `u64` register.
pub fn pack_hi_lo(hi: u32, lo: u32) -> u64 {
Copy link
Member

Choose a reason for hiding this comment

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

Needs a little bit more explanation for what is is supposed to be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree; extended the comment.

Copy link
Member

Choose a reason for hiding this comment

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

I was hoping it reduces code size not increase it :(

But we mainly do that to reduce complexity of having two different ways of passing arguments.

@xermicus xermicus enabled auto-merge January 28, 2025 08:12
@xermicus xermicus disabled auto-merge January 28, 2025 08:12
xermicus and others added 2 commits January 28, 2025 09:16
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
@xermicus xermicus enabled auto-merge January 28, 2025 08:28
@xermicus xermicus added this pull request to the merge queue Jan 28, 2025
Merged via the queue into master with commit 4302f74 Jan 28, 2025
201 of 206 checks passed
@xermicus xermicus deleted the cl/call-arguments branch January 28, 2025 09:37
xermicus added a commit to paritytech/revive that referenced this pull request Jan 28, 2025
…stack (#174)

Companion to paritytech/polkadot-sdk#7319

Signed-off-by: xermicus <cyrill@parity.io>
ordian added a commit that referenced this pull request Jan 30, 2025
* master: (58 commits)
  [pallet-revive] pack exceeding syscall arguments into registers (#7319)
  cumulus: bump PARENT_SEARCH_DEPTH and add test for 12-core elastic scaling (#6983)
  xcm: fix for DenyThenTry Barrier (#7169)
  Migrating polkadot-runtime-common slots benchmarking to v2 (#6614)
  Add development chain-spec file for minimal/parachain templates for Omni Node compatibility (#6529)
  `Arc` definition in `TransactionPool` (#7042)
  [sync] Let new subscribers know about already connected peers (backward-compatible) (#7344)
  Removed unused dependencies (partial progress) (#7329)
  Improve debugging by using `#[track_caller]` in system `assert_last_event` and `assert_has_event` (#7142)
  `set_validation_data` register weight manually, do not use refund when the pre dispatch is zero. (#7327)
  Fix the link to the chain snapshots (#7330)
  revive: Fix compilation of `uapi` crate when `unstable-hostfn` is not set (#7318)
  [pallet-revive] eth-rpc minor fixes (#7325)
  sync-templates: enable syncing from stable release patches (#7227)
  Bridges: emulated tests small nits/improvements (#7322)
  fix(cmd bench-omni): build omni-bencher with production profile (#7299)
  Nits for collectives-westend XCM benchmarks setup (#7215)
  bench all weekly - and fix for pallet_multisig lib (#6789)
  Deprecate ParaBackingState API (#6867)
  Fix setting the image properly (#7315)
  ...
pgherveou pushed a commit that referenced this pull request Feb 5, 2025
This PR changes how we call runtime API methods with more than 6
arguments: They are no longer spilled to the stack but packed into
registers instead. Pointers are 32 bit wide so we can pack two of them
into a single 64 bit register. Since we mostly pass pointers, this
technique effectively increases the number of arguments we can pass
using the available registers.

To make this work for `instantiate` too we now pass the code hash and
the call data in the same buffer, akin to how the `create` family
opcodes work in the EVM. The code hash is fixed in size, implying the
start of the constructor call data.

---------

Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Co-authored-by: command-bot <>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
pgherveou pushed a commit that referenced this pull request Feb 5, 2025
This PR changes how we call runtime API methods with more than 6
arguments: They are no longer spilled to the stack but packed into
registers instead. Pointers are 32 bit wide so we can pack two of them
into a single 64 bit register. Since we mostly pass pointers, this
technique effectively increases the number of arguments we can pass
using the available registers.

To make this work for `instantiate` too we now pass the code hash and
the call data in the same buffer, akin to how the `create` family
opcodes work in the EVM. The code hash is fixed in size, implying the
start of the constructor call data.

---------

Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Co-authored-by: command-bot <>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T7-smart_contracts This PR/Issue is related to smart contracts.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants