exec: some cleanup and small perf wins

[jordanlewis]

See individual commits for details. Just did a bunch of bounds check elimination stuff while I was on the plane. Also had an interesting observation which is that it's unnecessary to have pointer receivers for Next a lot of the time, which speeds things up by removing redundant checks from the compiler which couldn't prove in the pointer receiver case that data from the struct hadn't gotten modified by another goroutine.

And fixed a bug in hash join planning that caused us to fail to run some TPCH queries.

[cockroach-teamcity]

This change is 

[jordanlewis]

Rebased on top of @solongordon's patch.

[yuzefovich]

Nice finds!

Reviewed 3 of 3 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 2 of 2 files at r5, 1 of 1 files at r6.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis and @solongordon)

---

pkg/sql/exec/bool_vec_to_sel.go, line 68 at r6 (raw file):

		} else {
			batch.SetSelection(true)
			sel := batch.Selection()

Do we need sel = sel[:n] here as well? And in general, how do you check for this kind of things? Using something like "go.godbolt.com" to compile the code?

---

pkg/sql/exec/distinct_tmpl.go, line 211 at r6 (raw file):

		col = col[startIdx:n]
		outputCol = outputCol[startIdx:n]
		_ = outputCol[len(col)-1]

[question]: what is the goal of this instruction? (Sorry, if I asked this before.)

---

pkg/sql/exec/distinct_tmpl.go, line 244 at r6 (raw file):

// _CHECK_DISTINCT retrieves the value at the ith index of col, compares it
// to the passed in lastVal, and sets the ith value of outputCol to true if the
// compared values were distinct. It returns the value at the ith index of col.

[nit]: the method doesn't seem to return anything.

[jordanlewis]

TFTR!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @solongordon and @yuzefovich)

---

pkg/sql/exec/bool_vec_to_sel.go, line 68 at r6 (raw file):

Previously, yuzefovich wrote…

Do we need sel = sel[:n] here as well? And in general, how do you check for this kind of things? Using something like "go.godbolt.com" to compile the code?

You can ask the compiler to spit out its analysis:

go build -gcflags='-d=ssa/check_bce' ./pkg/sql/exec

In this case, sel = sel[:n] doesn't help us with anything because the compiler doesn't know how big idx in the loop below can get. We know that it's guaranteed to stay less than n, but I don't think there's any way to prove that to the compiler.

---

pkg/sql/exec/distinct_tmpl.go, line 211 at r6 (raw file):

Previously, yuzefovich wrote…

[question]: what is the goal of this instruction? (Sorry, if I asked this before.)

Even with the line above it, the compiler cannot prove that the bounds check is eliminated. This line helps the compiler realize that since it's already emitted a bounds check for outputCol at the maximum that i will be in the loop before, it does not have to emit any more bounds checks within the inner loop.

Use go build -gcflags='-d=ssa/check_bce' ./pkg/sql/exec to understand further - it will print out one line per bounds check emitted.

---

pkg/sql/exec/distinct_tmpl.go, line 244 at r6 (raw file):

Previously, yuzefovich wrote…

[nit]: the method doesn't seem to return anything.

Oops, you are right. I left this in by accident from an earlier experiment where I tried to convert this to be an ordinary function in the hopes that it would be inlined just as we do manually with templating. The result of the experiment was that the function was inlined, bounds checks were eliminated, but the code was still inexplicably not as fast as the manually templated version, so I reverted it.

[jordanlewis]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)