cpustopwatch: s/grunning.Difference/grunning.Elapsed#95564
cpustopwatch: s/grunning.Difference/grunning.Elapsed#95564craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
grunning.Elapsed() is the API to use when measuring the running time spent doing some piece of work, with measurements from the start and end. This only exists due to grunning.Time()'s non-monotonicity, a bug in our runtime patch: cockroachdb#95529. The bug results in slight {over,under}-estimation of the running time (the latter breaking monotonicity), but is livable with our current uses of this library, including the one here in cpustopwatch. grunning.Elapsed() papers over this bug by 0-ing out when grunning.Time()stamps regress. This is unlike grunning.Difference() which would return the absolute value of the regression -- not what we want here. Release note: None
|
This change is |
yuzefovich
left a comment
There was a problem hiding this comment.
Looks reasonable to me. It seems that we might be able to introduce back some of the assertions (about having non-negative cpu time in some places) that we had at some point in #93952. I'll defer to Drew for approval and for whether we want to do that.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)
DrewKimball
left a comment
There was a problem hiding this comment.
IIUC, this change prevents underestimation but not overestimation - is that right?
WRT the assertions, the issue is that even with this change, I believe we can have situations where the CPU time measured by a child operator is greater than that of a parent - mostly when the parent calls next only a few times and the child calls it many times.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:
irfansharif
left a comment
There was a problem hiding this comment.
TFTR.
IIUC, this change prevents underestimation but not overestimation - is that right?
We're still under-estimating by zero-ing things out. Just hopefully making it more explicit in the API that this can happen until we fix the Go patch.
bors r+
Reviewable status:
|
This PR was included in a batch that timed out, it will be automatically retried |
|
Build failed: |
|
bors r+ |
|
Build succeeded: |
grunning.Elapsed()is the API to use when measuring the running time spent doing some piece of work, with measurements from the start and end. This only exists due togrunning.Time()'s non-monotonicity, a bug in our runtime patch: #95529. The bug results in slight {over,under}-estimation of the running time (the latter breaking monotonicity), but is livable with our current uses of this library, including the one here in cpustopwatch.grunning.Elapsed()papers over this bug by 0-ing out whengrunning.Time()stamps regress. This is unlikegrunning.Difference()which would return the absolute value of the regression -- not what we want here.Release note: None