sql: fix erroneous benchmark regressions #117542
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rharding6373)
pkg/sql/opt/bench/fk_test.go line 54 at r1 (raw file):
setup(b, r, cfg.setupFKs) b.ResetTimer() run(b, r)
i think it could be more clear to be explicit about when the timer is stopped. e.g.:
b.ResetTimer()
run(..)
b.StopTimer()
it's a little more resilient to the code being refactored. basically, we should always call StopTimer after the for ... i < b.N ... loop. (looks like most other benchmarks seem to do it this way)
Several benchmarks incorrectly included cluster shutdown as part of the benchmark timing. This caused major regressions in benchmarks when cockroachdb#117116 was merged because it made cluster shutdown slower. The benchmarks now stop the timer before initiating cluster shutdown to more accurately measure the code in question. Fixes cockroachdb#117494 Release note: None
mgartner
force-pushed
the
117494-fix-benchmarks
branch
from
9deee4e
to
5e77ef4
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/bench/fk_test.go line 54 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i think it could be more clear to be explicit about when the timer is stopped. e.g.:
b.ResetTimer() run(..) b.StopTimer()it's a little more resilient to the code being refactored. basically, we should always call
StopTimerafter thefor ... i < b.N ...loop. (looks like most other benchmarks seem to do it this way)
Good call. Done.
There was a problem hiding this comment.
Reviewable status:
|
TFTRs! bors r+ |
|
Build succeeded: |
This commit fixes several benchmarks that incorrectly included cluster shutdown as part of the benchmark timing. This caused major regressions. See cockroachdb#117542 for more details. Release note: None
117807: sql: fix benchmark regressions r=mgartner a=mgartner This commit fixes several benchmarks that incorrectly included cluster shutdown as part of the benchmark timing. This caused major regressions. See #117542 for more details. Epic: None Release note: None Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
Several benchmarks incorrectly included cluster shutdown as part of the
benchmark timing. This caused major regressions in benchmarks
when #117116 was merged because it made cluster shutdown slower. The
benchmarks now stop the timer before initiating cluster shutdown to more
accurately measure the code in question.
Fixes #117494
Release note: None