-
-
Notifications
You must be signed in to change notification settings - Fork 652
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
Record how long a process takes to run #11594
Record how long a process takes to run #11594
Conversation
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
8116445
to
6185203
Compare
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.
Looks good!
While lossy, this is much easier to get working correctly with local.rs [ci skip-build-wheels]
[ci skip-build-wheels]
metadata.execution_start_timestamp, | ||
metadata.execution_completed_timestamp, | ||
) { | ||
(Some(started), Some(completed)) => TimeSpan::from_start_and_end(&started, &completed, "") |
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 string is used for Err
, but we then throw that away with .ok()
so I left it empty.
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.
That's a weird API... ideally it would return a non-allocated error type with a Debug
/Display
impl, rather than passing in a string to format. But yea, this is fine for now.
We might consider logging at debug for negative times 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.
Thanks!
[ci skip-build-wheels]
This will allow us to answer questions like how much time caching (both local and remote) is saving vs. running the process again. For example, we can compare the cache time vs. the original recorded execution time.
To preserve the execution time, we must store the metadata both on
FallibleProcessResultWithPlatform
andActionResult
, which is what we serialize to for the local and remote caches. This introducesProcessResultMetadata
, which is a sibling to REAPI'sExecutedActionMetadata
.For remote execution, it's up to them to correctly set the
ExecutedActionMetadata
, which we then convert intoProcessResultMetadata
. But for local execution, we can ensure we set up both types correctly.The timings match what we see with the dynamic UI: