Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

contracts: Add host function tracing #13648

Merged
merged 19 commits into from
Mar 27, 2023
Merged

contracts: Add host function tracing #13648

merged 19 commits into from
Mar 27, 2023

Conversation

pgherveou
Copy link
Contributor

@pgherveou pgherveou commented Mar 20, 2023 •
edited
Loading

Fix: #13593

TODO:

  • Implement feature
  • Add section to README about host function tracing
  • Rebench to make sure this actually doesn't affect performance
  • Checkin the kitchensink-runtime size before and after

Compare sizes of the kitchensink runtime before and after the change

Checkin the kitchensink-runtime size before and after

ls -algh "target/release/wbuild/kitchensink-runtime/kitchensink_runtime.compact.compressed.wasm"
before after. diff
1.7M (1770993) 1786207 15214 (+ 0.85%)

@pgherveou pgherveou requested a review from athei as a code owner March 20, 2023 09:13
@pgherveou pgherveou marked this pull request as draft March 20, 2023 09:14
@pgherveou
Copy link
Contributor Author

pgherveou commented Mar 20, 2023 •
edited
Loading

this is what I have so far

Screenshot 2023-03-20 at 16 25 03

Screenshot 2023-03-20 at 16 30 19

@pgherveou
Copy link
Contributor Author

With results

Screenshot 2023-03-20 at 17 31 25

@pgherveou pgherveou added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes labels Mar 20, 2023
@pgherveou pgherveou mentioned this pull request Mar 20, 2023
Closed
4 tasks
@pgherveou
Copy link
Contributor Author

pgherveou commented Mar 20, 2023 •
edited
Loading

Note to self, this can be tested directly from substrate kitchensink node by running

cargo run --release -- --dev -l "fatal,runtime::contracts::strace=trace"

I lost quite some time updating and patching substrate-contracts-node, to run it against my local changes
https://github.com/paritytech/substrate-contracts-node/pull/174/files

athei
athei reviewed Mar 21, 2023
View reviewed changes
frame/contracts/proc-macro/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/proc-macro/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/proc-macro/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/src/wasm/runtime.rs Outdated Show resolved Hide resolved
@pgherveou pgherveou marked this pull request as ready for review March 23, 2023 13:10
@athei athei added the T1-runtime This PR/Issue is related to the topic “runtime”. label Mar 23, 2023
athei
athei reviewed Mar 23, 2023
View reviewed changes
Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

Looks good

frame/contracts/proc-macro/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/proc-macro/src/lib.rs Show resolved Hide resolved
@pgherveou pgherveou force-pushed the pg/contracts-add-host-function-tracing branch from 44970f7 to 9d04d6b Compare March 23, 2023 14:05
@pgherveou
Copy link
Contributor Author

bot bench $ pallet pallet_contracts

@command-bot
Copy link

command-bot bot commented Mar 23, 2023 •
edited
Loading

@pgherveou https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2578878 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet pallet_contracts. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 32-d454b51c-05df-4a3e-b61c-92347a8a7da4 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Mar 23, 2023

@pgherveou Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet pallet_contracts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2578878 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2578878/artifacts/download.

@pgherveou
Copy link
Contributor Author

pgherveou commented Mar 23, 2023 •
edited
Loading

@athei kitchensink-runtime size before and after

ls -alg "target/release/wbuild/kitchensink-runtime/kitchensink_runtime.compact.compressed.wasm"
before after. diff
1.7M (1770993) 1786207 15214 (+ 0.85%)

@athei
Copy link
Member

athei commented Mar 23, 2023

I think the size grow is within expectation 👍

@pgherveou
Copy link
Contributor Author

bot bench $ pallet dev pallet_contracts

@command-bot
Copy link

command-bot bot commented Mar 23, 2023 •
edited
Loading

@pgherveou https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2579580 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_contracts. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 35-36cb55f2-6b95-4c68-9324-a5a52c93da21 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Mar 23, 2023

@pgherveou Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_contracts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2579580 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2579580/artifacts/download.

@pgherveou pgherveou force-pushed the pg/contracts-add-host-function-tracing branch from 9f94b84 to 2fe9545 Compare March 24, 2023 08:40
@athei
Copy link
Member

athei commented Mar 24, 2023

Weight looks good. Just the benchmarks who are always fluctuating are fluctuating:
https://weights.tasty.limo/compare?repo=substrate&threshold=10&path_pattern=frame%2F**%2Fsrc%2Fweights.rs&method=asymptotic&ignore_errors=true&unit=time&old=master&new=pg%2Fcontracts-add-host-function-tracing

@athei athei requested a review from agryaznov March 24, 2023 11:03
@pgherveou
Copy link
Contributor Author

Weight looks good. Just the benchmarks who are always fluctuating are fluctuating: https://weights.tasty.limo/compare?repo=substrate&threshold=10&path_pattern=frame%2F**%2Fsrc%2Fweights.rs&method=asymptotic&ignore_errors=true&unit=time&old=master&new=pg%2Fcontracts-add-host-function-tracing

ah nice I was going to ask you how to visualize the diff easily, will bookmark this link!

@pgherveou
Copy link
Contributor Author

Any tips on how to fix issues with continuous-integration/gitlab-check-dependent-cumulus
I can try to set it up locally, but it looks like it fails for reason independent from this PR

@athei
Copy link
Member

athei commented Mar 24, 2023

It either happens because you changed an API that was used by cumulus or someone else did and the companion wasn't merged, yet. What you can do in the latter case (which applies here): Merge master when the companion was merged.

pgherveou
pgherveou commented Mar 24, 2023
View reviewed changes
frame/contracts/README.md Outdated Show resolved Hide resolved
xermicus
xermicus approved these changes Mar 27, 2023
View reviewed changes
frame/contracts/README.md Outdated Show resolved Hide resolved
@pgherveou @xermicus
Update frame/contracts/README.md
f493ec7 
Co-authored-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
agryaznov
agryaznov approved these changes Mar 27, 2023
View reviewed changes
Copy link
Contributor

@agryaznov agryaznov left a comment

Choose a reason for hiding this comment

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

looks good, just suggested a small DRYing

frame/contracts/proc-macro/src/lib.rs Outdated Show resolved Hide resolved
@pgherveou pgherveou merged commit f2c12fd into master Mar 27, 2023
@pgherveou pgherveou deleted the pg/contracts-add-host-function-tracing branch March 27, 2023 17:34
pgherveou added a commit that referenced this pull request Apr 4, 2023
@pgherveou
contracts: Add host function tracing (#13648)
2fd6cf3
gpestana pushed a commit that referenced this pull request Apr 23, 2023
@pgherveou @gpestana
contracts: Add host function tracing (#13648)
f94fce0
@github-actions github-actions bot mentioned this pull request Jun 15, 2023
Closed
@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@pgherveou @nathanwhit
contracts: Add host function tracing (paritytech#13648)
5ab9933
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@agryaznov agryaznov agryaznov approved these changes

@xermicus xermicus xermicus approved these changes

@athei athei athei approved these changes

Assignees

@pgherveou pgherveou

Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Smart Contracts
Status: Done ✅
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

contracts: Add host function tracing
4 participants
@pgherveou @athei @agryaznov @xermicus