sql/rowexec: recycle coveringSpans in batchedInvertedExprEvaluator #53237
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
sumeerbhola
approved these changes
Aug 21, 2020
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)
This commit updates batchedInvertedExprEvaluator to recycle the `[]invertedSpan`
that it constructs in its init method, such that the same slice can be reused
for each batch of rows in the `invertedJoiner`.
Before this change, this heap allocations accounted for 5.51% of all heap
allocations (by size, `alloc_space`) in a geospatial query of interest:
```
----------------------------------------------------------+-------------
804MB 100% | github.com/cockroachdb/cockroach/pkg/sql/rowexec.(*invertedJoiner).readInput /go/src/github.com/cockroachdb/cockroach/pkg/sql/rowexec/inverted_joiner.go:418
804MB 5.51% 36.69% 804MB 5.51% | github.com/cockroachdb/cockroach/pkg/sql/rowexec.(*batchedInvertedExprEvaluator).init /go/src/github.com/cockroachdb/cockroach/pkg/sql/rowexec/inverted_expr_evaluator.go:407
----------------------------------------------------------+-------------
```
When applied on top of cockroachdb#53181 and cockroachdb#53215, this results in a 0.4% speedup on
the following geospatial query:
```sql
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);
```
```
name old ms new ms delta
Test/postgis_geometry_tutorial/nyc 134 ±12% 133 ±12% -0.40% (p=0.006 n=976+979)
```
nvanbenschoten
force-pushed
the
nvanbenschoten/reuseCoveringSpans
branch
from
3fd4a2c to
2601363
Compare
|
bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
craig bot
pushed a commit
that referenced
this pull request
Aug 24, 2020
52972: cli: command to deserialize descriptors r=spaskob a=spaskob We currently export the descriptors and job payloads in debug zips with hex encoding. It is often very valuable to decode these protos into JSON for inspection. Fixes #52063. Release note (cli change): The new debug command `decode-proto` reads descriptor from stdin in hex or base64 format (auto-detected) and a flag --schema=\<fully qualified name to decode\> with default value `cockroach.sql.sqlbase.Descriptor` and outputs to stdout the deserialized proto in JSON format. 53226: colexec: make hash table more dynamic and fix hash join in some cases r=yuzefovich a=yuzefovich **colexec: make hash table more dynamic** This commit replaces all fixed allocations of constant size in the hash table in favor of allocating the slices dynamically which makes the hash table more dynamic as well. This allows us to place the logic of clearing up those slices for the next iteration in a single place which simplifies the code a bit. Additionally it moves `buckets` slice that is only used by the hash joiner out of the hash table. Release note: None **colexec: fix LEFT ANTI hash join when right eq cols are key** Release note (bug fix): Previously, CockroachDB could return incorrect results when performing LEFT ANTI hash join when right equality columns form a key when it was executed via the vectorized engine, and now this has been fixed. **colexec: fix set-op hash joins and optimize them a bit** This commit fixes several problems with set-operation hash joins: 1. similar to how LEFT ANTI hash joins are handled, INTERSECT ALL and EXCEPT ALL hash joins rely on `headID` to be populated. That step is skipped if the right side is distinct (meaning right equality columns form a key). This would result in incorrect output, and we now override `rightDistinct` flag to get the desired behavior (probably sub-optimal but correct). 2. actually, before we would get an incorrect result, we would hit an out of bounds error because `hashTable.visited` would not have been created previously. That slice is used by set-op joins to track which tuples from the right side have already been "deleted" (i.e. they were used for a match). This is now also fixed. However, currently the information whether the right equality columns form a key is not propagated for set-operation joins, so the bug would not have occurred in the non-test environment. Additionally, this commit optimizes the handling of LEFT ANTI and EXCEPT ALL joins by not allocating `same` slice because those two join types have separate `collectAnti*` methods that don't use that slice. Release note: None 53248: release: fix adding libgeos files to .tar.gz and compress .zip r=jlinder a=otan * Fix issue where only the first file was added to a .tar.gz. * Compress files better when adding to zips. Release note: None 53252: sql/kv: optimize heap allocations for inverted joins and scans with many spans r=nvanbenschoten a=nvanbenschoten This PR contains a collection of optimizations that build on #53181, #53215, and #53237 to reduce the number and size of heap allocations performed by inverted joins. Combined, these changes results in a **1.46%** speedup on the following geospatial query: ```sql 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); ``` ``` name old ms new ms delta Test/postgis_geometry_tutorial/nyc 144 ±15% 142 ±16% -1.46% (p=0.000 n=972+974) ``` ---- ### kv: reserve lock spans in SpanSet ahead of time In #52610, we merged the latch and lock span declared by requests. Doing so is the long term goal in this area, but it doesn't seem like that's going to be possible for v20.2. Meanwhile, the collection of lock spans is currently less efficient than the collection of latch spans. This is because we have a heuristic to eagerly allocate lock span slices ahead of time, instead of letting them grow as they are appended to. This optimization does not exist for lock spans. We can see this by looking at an alloc_space heap profile while running a geospatial query of interest. We see that the lock spans account for **9.76%** of all allocated space while the latch spans account for only **2.65%** of all allocated space. ``` ----------------------------------------------------------+------------- 646.37MB 100% | github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval.DefaultDeclareIsolatedKeys /go/src/github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval/declare.go:90 0 0% 100% 646.37MB 9.76% | github.com/cockroachdb/cockroach/pkg/kv/kvserver/spanset.(*SpanSet).AddNonMVCC /go/src/github.com/cockroachdb/cockroach/pkg/kv/kvserver/spanset/spanset.go:127 646.37MB 100% | github.com/cockroachdb/cockroach/pkg/kv/kvserver/spanset.(*SpanSet).AddMVCC /go/src/github.com/cockroachdb/cockroach/pkg/kv/kvserver/spanset/spanset.go:139 ----------------------------------------------------------+------------- 175.37MB 100% | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).collectSpans /go/src/github.com/cockroachdb/cockroach/pkg/kv/kvserver/replica_send.go:691 175.37MB 2.65% 61.51% 175.37MB 2.65% | github.com/cockroachdb/cockroach/pkg/kv/kvserver/spanset.(*SpanSet).Reserve /go/src/github.com/cockroachdb/cockroach/pkg/kv/kvserver/spanset/spanset.go:120 ----------------------------------------------------------+------------- ``` To improve this, we can do the same ahead of time slice resizing for lock spans by reserving an estimate for the number and type of lock spans we expect a request to declare. This guess may be a little less accurate than our guess for latch spans, because there are a few request types that declare latch spans but not lock spans, but these are rare requests that are much less important to optimize than the requests that do declare a latch and a lock span. With this change, we shave off about **6.57%** of total allocated space in this workload. We no longer see allocations in `SpanSet.AddNonMVCC` and the amount of heap memory allocated by `SpanSet.Reserve` roughly doubles, as we would expect. ``` ----------------------------------------------------------+------------- 190.99MB 53.34% | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).collectSpans /go/src/github.com/cockroachdb/cockroach/pkg/kv/kvserver/replica_send.go:692 167.09MB 46.66% | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).collectSpans /go/src/github.com/cockroachdb/cockroach/pkg/kv/kvserver/replica_send.go:693 358.08MB 5.84% 36.53% 358.08MB 5.84% | github.com/cockroachdb/cockroach/pkg/kv/kvserver/spanset.(*SpanSet).Reserve /go/src/github.com/cockroachdb/cockroach/pkg/kv/kvserver/spanset/spanset.go:120 ----------------------------------------------------------+------------- ``` ### sql/row: batch allocate RequestUnion wrapper structs in txnKVFetcher.fetch A Request's approach towards representing a union type results in double indirection. For instance, a ScanRequest is represented like: ``` Request{Value: &RequestUnion_Scan{Scan: &ScanRequest{}}} ``` The code in txnKVFetcher.fetch was batch allocating the inner struct in this structure across all scans in a batch, but was not batch allocating the outer "union" struct. The effect of this is that the outer struct's heap allocation showed up as 4.78% of the alloc_objects heap profile of an interesting geospatial query: ``` ----------------------------------------------------------+------------- 2621460 100% | github.com/cockroachdb/cockroach/pkg/sql/row.(*txnKVFetcher).fetch /go/src/github.com/cockroachdb/cockroach/pkg/sql/row/kv_batch_fetcher.go:297 0 0% 100% 2621460 4.78% | github.com/cockroachdb/cockroach/pkg/roachpb.(*RequestUnion).MustSetInner /go/src/github.com/cockroachdb/cockroach/pkg/roachpb/api.go:604 2621460 100% | github.com/cockroachdb/cockroach/pkg/roachpb.(*RequestUnion).SetInner /go/src/github.com/cockroachdb/cockroach/pkg/roachpb/batch_generated.go:354 ----------------------------------------------------------+------------- ``` This commit avoids this source of per-request allocations by batch allocating these union structs across all requests in a batch just like the code was previously batch allocating the inner structs. ### sql/rowexec: avoid allocating slice header on sort in fragmentPendingSpans batchedInvertedExprEvaluator's fragmentPendingSpans performs a `sort.Slice` to sort a list of pending spans by their end key. `sort.Slice` is convenient, but it has some trade-offs. For one, it uses reflection to mimic parameterization over all slice types. Second, it accepts the slice as an interface, which causes the slice header to escape to the heap. The effect of this is that the call was showing up as 4.35% of the alloc_objects heap profile of an interesting geospatial query: ``` ----------------------------------------------------------+------------- 2387773 100% | github.com/cockroachdb/cockroach/pkg/sql/rowexec.(*batchedInvertedExprEvaluator).init /go/src/github.com/cockroachdb/cockroach/pkg/sql/rowexec/inverted_expr_evaluator.go:406 2162754 3.94% 40.58% 2387773 4.35% | github.com/cockroachdb/cockroach/pkg/sql/rowexec.(*batchedInvertedExprEvaluator).fragmentPendingSpans /go/src/github.com/cockroachdb/cockroach/pkg/sql/rowexec/inverted_expr_evaluator.go:284 225019 9.42% | sort.Slice /usr/local/go/src/sort/slice.go:15 ----------------------------------------------------------+------------- ``` This commit avoids this heap allocation by assigning the slice to a field on the batchedInvertedExprEvaluator (which was already on the heap) and using the old `sort.Sort` function. ---- @otan an area I didn't touch but wanted to talk to you about was the memory representation of `DGeography` and `DGeometry`. Specifically, I'm wondering why we're embedding pointers instead of values inside of these datums, effectively creating a double-boxing situation. I see that most of the geo operators work off pointer and that the "canonical" representation of the objects exported by `pkg/geo` is a pointer (e.g. the package exports "New..." methods instead of "Make..." methods). I don't understand why this is though. Both `Geography` and `Geometry` are only 40 bytes large, which is well within reason to pass on the stack. I ask because this makes working with these datums more expensive than strictly necessary. We make an effort to batch allocate datum objects themselves (see `DatumAlloc`), but don't do anything special with internal heap allocations. I suspect that there would be a moderate win here if we eliminated the double-boxing on any query that scans a large number of rows containing one of these data types, as I'm seeing the heap allocations in DecodeUntaggedDatum fairly prominently in profiles. Co-authored-by: Spas Bojanov <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Oliver Tan <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit updates batchedInvertedExprEvaluator to recycle the []invertedSpan
that it constructs in its init method, such that the same slice can be reused
for each batch of rows in the
invertedJoiner.Before this change, this heap allocations accounted for 5.51% of all heap
allocations (by size,
alloc_space) in a geospatial query of interest:When applied on top of #53181 and #53215, this results in a 0.4% speedup on
the following geospatial query: