Execution layer reports timings of each executed command #20923
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
mystenmark
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
| Success(Duration), | ||
| Abort(Duration), |
There was a problem hiding this comment.
does Success and Abort not already duplicate information contained in effects? maybe this is the most expedient way, but at least wanted to ask if there's a way to not duplicate that here
There was a problem hiding this comment.
I don't think it is easy to figure out which command aborted by looking at the effects, although it may be possible.
| metrics, | ||
| enable_expensive_checks, | ||
| certificate_deny_set, | ||
| ); |
There was a problem hiding this comment.
maybe add a short comment on these explaining that we're deliberately leaving timings empty for old versions?
crates/sui-types/src/execution.rs
Outdated
| Success(Duration), | ||
| Abort(Duration), | ||
| } | ||
| pub type TimingResult<R, E> = Result<(R, Vec<ExecutionTiming>), (E, Vec<ExecutionTiming>)>; |
There was a problem hiding this comment.
nit - this name makes it sounds like the main output of the result is the timing, maybe ResultWithTiming?
| @@ -281,6 +284,7 @@ mod checked { | |||
| ) -> ( | |||
| GasCostSummary, | |||
| Result<Mode::ExecutionResults, ExecutionError>, | |||
There was a problem hiding this comment.
any plan to change this to TimingResult (or ResultWithTiming) eventually? or if not why not?
There was a problem hiding this comment.
They have to be split apart at some point, and this seemed like the most convenient point. I originally tried to keep them separate throughout the stack, but changing anything that returns Result to (Result, T) is very ugly - ? stops working. Since this function already doesn't return a naked result it was easy to do the split here.
mystenmark
force-pushed
the
mlogan-cc-estimator
branch
from
c1e8493 to
23ef2af
Compare
mystenmark
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
mystenmark
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
mystenmark
force-pushed
the
mlogan-cc-estimator
branch
from
e8df0c5 to
1cb1fa7
Compare
mystenmark
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
The timings are unused for now - they are intended for the congestion control execution time estimator.