exec: fix possible infinite loops in the benchmark #37106
Conversation
|
This change is |
There was a problem hiding this comment.
@solongordon I think you were looking at this too?
@yuzefovich it might be easier to always include an element of mintint64 as the 0th element or something?
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)
There was a problem hiding this comment.
My in-progress PR for null handling makes these benchmarks deterministic. No harm in merging this in the meantime.
It might also be nice to add a bit of logic to repeatableBatchSource which panics if Next is called some unrealistically high number of times, since I'm sure we'll hit this problem again.
Reviewable status:
yuzefovich
force-pushed
the
fix-benchmark
branch
from
ff7b196
to
0608e60
Compare
There was a problem hiding this comment.
I liked Jordan's suggestion on how to simplify the logic a bit by putting a few specific values.
Not sure about the repeatableBatchSource - what that unrealistic number would be? I think that Golang itself panics after reaching the certain depth of the stack, right? And our logic would be very similar - we'd check for number of batches returned and panic with a custom message instead of Golang's.
Reviewable status:
Release note: None
yuzefovich
force-pushed
the
fix-benchmark
branch
from
0608e60
to
56906de
Compare
There was a problem hiding this comment.
I don't think there's recursion going on so we wouldn't hit a stack depth error. But yeah, don't know off the top of my head what a good number would be. Don't worry about it for this PR, just an idea.
Reviewable status:
|
TFTRs! bors r+ |
Build succeeded |
Release note: None