sql/kv: avoid expensive heap allocations with inverted joins

[nvanbenschoten]

This PR contains three optimizations that reduce heap allocations by
inverted join operations. These were found by profiling the following
geospatial query:

SELECT Count(census.blkid), Count(subways.name)
FROM nyc_census_blocks AS census
JOIN nyc_subway_stations AS subways
ON ST_Intersects(subways.geom, census.geom);

Combined, these three commits speed up the query by a little over 3%.

name                                old ms    new ms    delta
Test/postgis_geometry_tutorial/nyc  139 ±12%  135 ±11%  -3.04%  (p=0.000 n=241+243)

sql/rowexec: avoid heap allocations in batchedInvertedExprEvaluator

This commit restructures the slice manipulation performed in
(*batchedInvertedExprEvaluator).init to avoid heap allocations. Before
the change, this method used to contain the first and fifth most
expensive memory allocations by total allocation size (alloc_space)
in a geospatial query of interest:

----------------------------------------------------------+-------------
                                         3994.06MB   100% |   github.com/cockroachdb/cockroach/pkg/sql/rowexec.(*invertedJoiner).readInput /go/src/github.com/cockroachdb/cockroach/pkg/sql/rowexec/inverted_joiner.go:418
 3994.06MB 10.70% 10.70%  3994.06MB 10.70%                | github.com/cockroachdb/cockroach/pkg/sql/rowexec.(*batchedInvertedExprEvaluator).init /go/src/github.com/cockroachdb/cockroach/pkg/sql/rowexec/inverted_expr_evaluator.go:366
----------------------------------------------------------+-------------

----------------------------------------------------------+-------------
                                          954.07MB   100% |   github.com/cockroachdb/cockroach/pkg/sql/rowexec.(*invertedJoiner).readInput /go/src/github.com/cockroachdb/cockroach/pkg/sql/rowexec/inverted_joiner.go:418
  954.07MB  4.61% 41.71%   954.07MB  4.61%                | github.com/cockroachdb/cockroach/pkg/sql/rowexec.(*batchedInvertedExprEvaluator).init /go/src/github.com/cockroachdb/cockroach/pkg/sql/rowexec/inverted_expr_evaluator.go:409
----------------------------------------------------------+-------------

The largest offender here was the routingSpans slice, which made no
attempt to recycle memory. The other offender was the pendingSpans
slice. We made an attempt to recycle the memory in this slice, except
we truncated it from the front instead of the back, so we ended us
not actually recycling anything.

This commit addresses this in two ways. First, it updates the pendingSpans
slice to point into the routingSpans slice so the two can share memory.
pendingSpans becomes a mutable window into routingSpans, so it no
longer requires its own heap allocation. Next, the commit updates the
memory recycling to recycle routingSpans instead of pendingSpans.
Since we never truncate routingSpans from the front, the recycling
now works.

With these two changes, the two heap allocations, which used to account
for 15.31% of total allocated space combined drops to a single heap
allocation that accounts for only 2.43% of total allocated space on
the geospatial query.

----------------------------------------------------------+-------------
                                          182.62MB   100% |   github.com/cockroachdb/cockroach/pkg/sql/rowexec.(*invertedJoiner).readInput /go/src/github.com/cockroachdb/cockroach/pkg/sql/rowexec/inverted_joiner.go:418
  182.62MB  2.43% 58.36%   182.62MB  2.43%                | github.com/cockroachdb/cockroach/pkg/sql/rowexec.(*batchedInvertedExprEvaluator).init /go/src/github.com/cockroachdb/cockroach/pkg/sql/rowexec/inverted_expr_evaluator.go:366
----------------------------------------------------------+-------------

sql: properly copy span slices

This commit fixes two locations where we not optimally performing copies
from one slice to another.

In invertedJoiner.generateSpans, we were not pre-allocating the
destination slice and were using append instead of direct assignment,
which is measurably slower due to the extra writes to the slice header.

In getProtoSpans, we again were appending to a slice with zero length
and sufficient capacity instead of assigning to a slice with sufficient
length.

kv: check for expensive logging before VEventf in appendRefreshSpans

Before this change, we called VEventf directly with the refresh span
corresponding to each request in a batch. This caused each span object
to allocate while passing through an interface{}. This could be seen
in heap profiles, where it accounted for 1.89% of total allocated
memory (alloc_space) in a geospatial query of interest:

----------------------------------------------------------+-------------
                                          142.01MB   100% |   github.com/cockroachdb/cockroach/pkg/roachpb.(*BatchRequest).RefreshSpanIterate /go/src/github.com/cockroachdb/cockroach/pkg/roachpb/batch.go:407
  142.01MB  1.89% 68.71%   142.01MB  1.89%                | github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord.(*txnSpanRefresher).appendRefreshSpans.func1 /go/src/github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go:510
----------------------------------------------------------+-------------

This commit adds a guard around the VEventf to avoid the allocation.

[cockroach-teamcity]

This change is 

[sumeerbhola]

This caused each span object to allocate while passing through an interface{}.

Is this an allocation of the interface instance or the roachpb.Span? Do you know why it needs to allocate on the heap -- the parameter doesn't seem like it should escape?

Reviewed 1 of 1 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @sumeerbhola)

---

pkg/sql/rowexec/inverted_expr_evaluator.go, line 397 at r1 (raw file):

	pendingSpans := b.routingSpans[:1]
	// This loop does both the union of the routingSpans and fragments the
	// routingSpans.

nice!
Can you add the following to this comment
// The pendingSpans slice contains a subsequence of the routingSpans slice, that
// when passed to fragmentPendingSpans will be mutated by it.

[nvanbenschoten]

TFTR!

Is this an allocation of the interface instance or the roachpb.Span? Do you know why it needs to allocate on the heap -- the parameter doesn't seem like it should escape?

An allocation of the roachpb.Span. Passing a value through an interface almost always causes that value to escape to the heap. Escape analysis is usually pretty useless in these cases, except when the entire thing can be inlined away. We can confirm that it's the roachpb.Span itself that's escaping in the heap profile because the allocation has a size of 48 bytes:

Also, we see the following in the debug compilation output:

txn_interceptor_span_refresher.go:565:23: span escapes to heap

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)

---

pkg/sql/rowexec/inverted_expr_evaluator.go, line 397 at r1 (raw file):

Previously, sumeerbhola wrote…

nice!
Can you add the following to this comment
// The pendingSpans slice contains a subsequence of the routingSpans slice, that
// when passed to fragmentPendingSpans will be mutated by it.

Done.

[sumeerbhola]

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

[nvanbenschoten]

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[craig]

Build succeeded:

• GitHub CI (Cockroach)