colexecwindow: fix up some of the gcassert assertions #90820
Conversation
|
This change is |
|
I'm trying to upgrade |
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/colexec/colexecwindow/min_max_queue.go line 81 at r1 (raw file):
func (q *minMaxQueue) getLast() uint32 { if q.empty { colexecerror.InternalError(errors.AssertionFailedf("getting last from empty minMaxQueue"))
Why remove the assertion above but not this one? Does the method still not get inlined?
pkg/sql/colexec/colexecwindow/min_max_removable_agg_tmpl.go line 224 at r1 (raw file):
} else { // {{if not .IsBytesLike}} // gcassert:bce
I'm a little surprised at this one - maybe the accesses have to go inside PerformOperation?
This commit removes some of the `gcassert` assertions that are actually not true - here we use `// gcassert` as opposed to `//gcassert`, and since we used the old gcassert version, they weren't actually enforced. Additionally, this commit adjusts the code a bit in order to make the assertions valid in some cases. One interesting change was to move the slice accesses into `PerformOperation` which made BCEs to occur. Release note: None
yuzefovich
force-pushed
the
gcassert-colexecwindow
branch
from
a20a82e
to
8091f06
Compare
yuzefovich
changed the title
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/sql/colexec/colexecwindow/min_max_queue.go line 81 at r1 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
Why remove the assertion above but not this one? Does the method still not get inlined?
I guess I thought that get was more frequent than getLast. Removed the check and returned the inlined assertion.
pkg/sql/colexec/colexecwindow/min_max_removable_agg_tmpl.go line 224 at r1 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
I'm a little surprised at this one - maybe the accesses have to go inside
PerformOperation?
That was it, thanks.
|
Build succeeded: |
This commit removes some of the
gcassertassertions that are actuallynot true - here we use
// gcassertas opposed to//gcassert, andsince we used the old gcassert version, they weren't actually enforced.
Additionally, this commit adjusts the code a bit in order to make the
assertions valid in some cases. One interesting change was to move the
slice accesses into
PerformOperationwhich made BCEs to occur.Epic: None
Release note: None