-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for the prefix length maybe being relevant:
LGTM, but of course I might have missed sth because I'm not familir enough with the system.
// `set_code` was not benchmarked because it is pretty hard to come up with a real | ||
// Wasm runtime to test the upgrade with. But this is okay because we will make | ||
// `set_code` take a full block anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to benchmark (more, like, smoketest) whether you can find a WASM that would take longer than the block time to call set_code
on. Not sure how to do that, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would have to be something we test more manually or with some other setup versus using our runtime benchmarking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe open an issue, would be good to follow up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, they all have an effective weight higher than a block. Just something we have to live with. It's a root call so still basically fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR adds a crate that benchmarks the various extrinsics in
frame_system
.