-
Notifications
You must be signed in to change notification settings - Fork 122
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
Update Benchmarks #264
Update Benchmarks #264
Conversation
Need to run this again on a machine with an NvEME disk and a CPU with better single-thread performance. [-] Failed the machine benchmark:\n2024-01-02 18:40:21 Running machine benchmarks... |
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
Few question. The benchmark runs, should we do that in CI machines? (and thus make this CI fail depending on the type of machine we get in github?) or is there an option to run it specifically on some setup with docker, so we can ask output of this command to be sent to us before starting a validator node ?
pallets/schema/src/weights.rs
Outdated
// Measured: `661` | ||
// Estimated: `19007` | ||
// Minimum execution time: 55_169_000 picoseconds. | ||
Weight::from_parts(57_651_421, 19007) |
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.
This is the one place where we are passing variable length argument. Should the weight change based on that?
@@ -766,7 +766,7 @@ pub mod pallet { | |||
/// the number of entries | |||
/// removed. | |||
#[pallet::call_index(4)] | |||
#[pallet::weight(<T as pallet::Config>::WeightInfo::remove( ))] | |||
#[pallet::weight(<T as pallet::Config>::WeightInfo::remove(T::MaxRemoveEntries::get() as u32))] |
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 this require a version change?
/// Average: 223_580 | ||
/// Median: 221_485 | ||
/// Std-Dev: 5572.62 | ||
/// Min, Max: 280_638, 577_472 |
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.
Compared to earlier a lot of diff between min and max. Is this alright?
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.
Average has not moved up much. Couple of reasons, increased blocktime and WASM
For those who are referring, the hardware tests are not part of this PR, but will be added back as a separate PR, so we can discuss more on it with focus. |
No description provided.