-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[short][move e2e bench] Add gas metrics and more existing transaction types #15302
Conversation
⏱️ 2h 39m total CI duration on this PR
|
aptos-move/e2e-tests/src/executor.rs
Outdated
@@ -144,6 +144,13 @@ pub enum GasMeterType { | |||
UnmeteredGasMeter, | |||
} | |||
|
|||
#[derive(Clone)] | |||
pub struct TimeAndGas { | |||
pub elapsed_micros: u128, |
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.
nit: elapsed_micro_secs
?
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.
rust uses term micros, i.e. Duration::from_micros
aptos-move/e2e-tests/src/executor.rs
Outdated
@@ -144,6 +144,13 @@ pub enum GasMeterType { | |||
UnmeteredGasMeter, | |||
} | |||
|
|||
#[derive(Clone)] | |||
pub struct TimeAndGas { |
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.
nit: calling it Measurement
can be more consitent with what you call it in e2e-benchmarks
aptos-move/e2e-benchmark/src/main.rs
Outdated
); | ||
|
||
json_lines.push(json!({ | ||
"grep": "grep_json_aptos_move_vm_perf", | ||
"transaction_type": entry_point_name, | ||
"wall_time_us": elapsed_micros, | ||
"wall_time_us": measurement.elapsed_micros, | ||
"gps": (measurement.execution_gas + measurement.io_gas) as u128 / measurement.elapsed_micros, |
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.
gas/us
for better readability? Plus it is not seconds?
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.
parsing with / in logs can be more complicated - this json is for humio graphs
gas unit is 1/10^6 gas internal units, and same as micros, so this is gas unit / s
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.
I see, can you maybe put a comment for that here? gps is ok 👍
9289a13
to
538ad1f
Compare
538ad1f
to
54cc4b0
Compare
aptos-move/e2e-benchmark/src/main.rs
Outdated
InitializeVectorPicture { length: 128 } 6 0.973 1.066 75.0 | ||
VectorPicture { length: 128 } 6 0.955 1.092 22.0 | ||
VectorPictureRead { length: 128 } 6 0.952 1.047 21.0 |
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.
The timing data for InitializeVectorPicture
, VectorPicture
, and VectorPictureRead
operations appears to be using measurements from when length
was 40, but the code now tests with length
of 128. These calibration values should be regenerated with the new length to maintain benchmark accuracy. Without updated measurements, performance regressions or improvements may go undetected since the baseline values don't reflect the current workload.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
54cc4b0
to
830e5e8
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
This allows for easier understanding of gas calibration. Looking at gas/s metrics (and execution and io breakdown) makes it easy to calibrate new constants.
How Has This Been Tested?
used to calibrate vector::move_range native
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist