grunning: add grunning.Elapsed() #95536
Conversation
Elapsed returns the running time spent doing some piece of work, with grunning.Time() measurements from the start and end. NB: This only exists due to grunning.Time()'s non-monotonicity, a bug in our runtime patch: cockroachdb#95529. We can get rid of this, keeping just grunning.Difference(), if that bug is fixed. 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. Release note: None
|
This change is |
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)
There was a problem hiding this comment.
Reviewed 3 of 4 files at r1.
Reviewable status:
pkg/util/admission/elastic_cpu_work_handle.go line 63 at r1 (raw file):
return override() } return grunning.Difference(grunning.Time(), h.cpuStart)
was this ordering change a bug in the old code?
pkg/util/grunning/grunning.go line 38 at r1 (raw file):
// // NB: This only exists due to grunning.Time()'s non-monotonicity, a bug in our // runtime patch: https://github.com/cockroachdb/cockroach/issues/95529. We can
Can we not fix the patch and backport it, instead of working around it?
There was a problem hiding this comment.
Reviewable status:
pkg/util/admission/elastic_cpu_work_handle.go line 63 at r1 (raw file):
Previously, sumeerbhola wrote…
was this ordering change a bug in the old code?
I also thought so but Difference takes the abs so the order didn't matter previously.
There was a problem hiding this comment.
Reviewable status:
pkg/util/admission/elastic_cpu_work_handle.go line 63 at r1 (raw file):
Previously, kvoli (Austen) wrote…
I also thought so but Difference takes the abs so the order didn't matter previously.
It's no changing the order, it's changing it from .Difference() to .Elapsed(). The latter takes in the start time as the first parameter. Using .Difference() doesn't make sense now that we know of the non-monotonicity, since it'll give you a value for how far back grunning.Time() jumped. Instead of 0-ing it out since it was non-monotonic.
pkg/util/grunning/grunning.go line 38 at r1 (raw file):
Previously, sumeerbhola wrote…
Can we not fix the patch and backport it, instead of working around it?
We can, but the last time we patched the runtime I ran a slew of {micro,macro}benchmarks which took time. I don't think there's a need to backport these, the errors are minor and we're not relying on monotonicity. So it was a preference to work around it for now and make time for the go patch soon afterwards.
There was a problem hiding this comment.
Reviewed 1 of 4 files at r1, all commit messages.
Reviewable status:
|
bors r+ |
|
Build succeeded: |
Elapsed returns the running time spent doing some piece of work, with grunning.Time() measurements from the start and end. This only exists due to grunning.Time()'s non-monotonicity, a bug in our runtime patch: #95529. We can get rid of this, keeping just grunning.Difference(), if that bug is fixed. 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.
Release note: None