rowcontainer,rowexec: switch joinReader to use DiskBackedNumberedRowC… #49669
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 |
|
benchstat output |
yuzefovich
reviewed
May 30, 2020
There was a problem hiding this comment.
Very nice benchmarks! The code LGTM, but I'll defer to Alfonso to stamp it.
I'm just curious whether you ran them on gceworker (I see that there is some variation, maybe it is explained by randomness) and also whether you have ideas for why in a few cases the size of total memory allocations increased by about 2x (maybe because of cache overhead?).
Reviewed 4 of 5 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @sumeerbhola, and @yuzefovich)
pkg/sql/rowcontainer/numbered_row_container.go, line 383 at r1 (raw file):
} allAccesses := make([]int, numAccesses) claimedCount := 0
nit: you could restructure the code a bit to remove "claimed count" things with something like this:
...
if elem.accesses == nil {
elem.accesses = allAccesses[:elem.numAccesses]
allAccesses = allAccesses[elem.numAccesses:]
elem.numAccesses = 0
}
...
We could take it one step further and remove the unsetting of numAccesses with
...
if elem.accesses == nil {
elem.accesses = allAccesses[0:0:elem.numAccesses]
allAccesses = allAccesses[elem.numAccesses:]
}
elem.accesses = append(elem.accesses, accessIdx)
accessIdx++
....
pkg/sql/rowcontainer/numbered_row_container_test.go, line 245 at r1 (raw file):
// This memory budget allows for some caching, but typically cannot // cache all the rows. var memoryBudget int64
nit: combine this and the next line.
pkg/sql/rowcontainer/numbered_row_container_test.go, line 316 at r1 (raw file):
continue } for i := 1; i < len(rows); i++ {
Why do we have a for loop here? I think len(rows) is always 2, so we will make only one iteration (unless skip is true).
pkg/sql/rowcontainer/numbered_row_container_test.go, line 371 at r1 (raw file):
func makeNumberedContainerUsingNRC( ctx context.Context, t require.TestingT,
This seems suspicious, how about s/require.TestingT/testing.TB/g?
pkg/sql/rowexec/joinreader_test.go, line 846 at r1 (raw file):
if memoryLimit != math.MaxInt64 { if !reqOrdering { // Smaller memory limit is not relevant when there is no ordering
nit: missing period.
pkg/sql/rowexec/joinreader_test.go, line 852 at r1 (raw file):
// The benchmark workloads are such that each right row never joins // with more than one left row. And the access pattern of right rows // accessed across all the left rows is monotonically incresing. So
s/incresing/increasing/g
asubiotto
reviewed
Jun 1, 2020
There was a problem hiding this comment.
I trust your judgement @yuzefovich . Did a skim and this LGTM
Reviewed 1 of 5 files at r1.
Reviewable status:
sumeerbhola
force-pushed
the
nrcmisc
branch
2 times, most recently
from
3364216
to
df168ba
Compare
sumeerbhola
commented
Jun 1, 2020
There was a problem hiding this comment.
I'm just curious whether you ran them on gceworker (I see that there is some variation, maybe it is explained by randomness)
No, it was on my laptop. Do we typically run on gceworker to reduce variation?
and also whether you have ideas for why in a few cases the size of total memory allocations increased by about 2x (maybe because of cache overhead?).
I looked into this -- the major ones were the map allocation, map element allocation and row copying. The last one allowed for two optimizations -- we can avoid copying when the row is not being added to the cache, and when it is being added due to an eviction the allocations can be reduced by reusing that evicted row. The latter optimization was in the indexed row container that I'd overlooked. These have all been applied and I've updated the commit text.
Memory allocation increases like
JoinReader/reqOrdering=true/matchratio=onetothirtytwo/lookuprows=256/mem=100KB-16 3.13MB ± 0% 5.98MB ± 0% +91.12% (p=0.016 n=4+5)
JoinReader/reqOrdering=true/matchratio=onetothirtytwo/lookuprows=1024/mem=100KB-16 12.2MB ± 3% 23.6MB ± 0% +93.52% (p=0.016 n=5+4)
JoinReader/reqOrdering=true/matchratio=onetothirtytwo/lookuprows=4096/mem=100KB-16 48.0MB ± 0% 94.2MB ± 1% +96.42% (p=0.016 n=4+5)
JoinReader/reqOrdering=true/matchratio=onetothirtytwo/lookuprows=8192/mem=100KB-16 95.8MB ± 0% 189.5MB ± 2% +97.73% (p=0.016 n=4+5)
JoinReader/reqOrdering=true/matchratio=onetothirtytwo/lookuprows=16384/mem=100KB-16 193MB ± 0% 376MB ± 0% +95.48% (p=0.016 n=5+4)
have now become a decrease
JoinReader/reqOrdering=true/matchratio=onetothirtytwo/lookuprows=256/mem=100KB-16 3.13MB ± 0% 2.89MB ± 2% -7.71% (p=0.016 n=4+5)
JoinReader/reqOrdering=true/matchratio=onetothirtytwo/lookuprows=1024/mem=100KB-16 12.2MB ± 3% 10.1MB ± 0% -16.86% (p=0.016 n=5+4)
JoinReader/reqOrdering=true/matchratio=onetothirtytwo/lookuprows=4096/mem=100KB-16 48.0MB ± 0% 39.4MB ± 3% -17.93% (p=0.016 n=4+5)
JoinReader/reqOrdering=true/matchratio=onetothirtytwo/lookuprows=8192/mem=100KB-16 95.8MB ± 0% 77.5MB ± 0% -19.07% (p=0.029 n=4+4)
JoinReader/reqOrdering=true/matchratio=onetothirtytwo/lookuprows=16384/mem=100KB-16 193MB ± 0% 157MB ± 2% -18.53% (p=0.008 n=5+5)
Reviewable status:
pkg/sql/rowcontainer/numbered_row_container.go, line 383 at r1 (raw file):
Previously, yuzefovich wrote…
nit: you could restructure the code a bit to remove "claimed count" things with something like this:
... if elem.accesses == nil { elem.accesses = allAccesses[:elem.numAccesses] allAccesses = allAccesses[elem.numAccesses:] elem.numAccesses = 0 } ...We could take it one step further and remove the unsetting of
numAccesseswith... if elem.accesses == nil { elem.accesses = allAccesses[0:0:elem.numAccesses] allAccesses = allAccesses[elem.numAccesses:] } elem.accesses = append(elem.accesses, accessIdx) accessIdx++ .... </blockquote></details> Done ___ *[pkg/sql/rowcontainer/numbered_row_container_test.go, line 245 at r1](https://reviewable.io/reviews/cockroachdb/cockroach/49669#-M8Xr9UGDAXh492-rhar:-M8kzRDBECQ1xMP6tLCR:bb6un1j) ([raw file](https://github.com/cockroachdb/cockroach/blob/cd8a706edecdf55025eacffad74281801adce3b2/pkg/sql/rowcontainer/numbered_row_container_test.go#L245)):* <details><summary><i>Previously, yuzefovich wrote…</i></summary><blockquote> nit: combine this and the next line. </blockquote></details> Done ___ *[pkg/sql/rowcontainer/numbered_row_container_test.go, line 316 at r1](https://reviewable.io/reviews/cockroachdb/cockroach/49669#-M8Xrt_H4Hlh-SHx3ySv:-M8kzlwF048a3XV5XPxr:bb6un1j) ([raw file](https://github.com/cockroachdb/cockroach/blob/cd8a706edecdf55025eacffad74281801adce3b2/pkg/sql/rowcontainer/numbered_row_container_test.go#L316)):* <details><summary><i>Previously, yuzefovich wrote…</i></summary><blockquote> Why do we have a `for` loop here? I think `len(rows)` is always 2, so we will make only one iteration (unless `skip` is `true`). </blockquote></details> Done ___ *[pkg/sql/rowcontainer/numbered_row_container_test.go, line 371 at r1](https://reviewable.io/reviews/cockroachdb/cockroach/49669#-M8Xs99f7Wrk4NpjkN1Q:-M8l-17B3UWLeexmDSkI:b3grltl) ([raw file](https://github.com/cockroachdb/cockroach/blob/cd8a706edecdf55025eacffad74281801adce3b2/pkg/sql/rowcontainer/numbered_row_container_test.go#L371)):* <details><summary><i>Previously, yuzefovich wrote…</i></summary><blockquote> This seems suspicious, how about `s/require.TestingT/testing.TB/g`? </blockquote></details> Done (I didn't know about `testing.TB` -- still learning Go). ___ *[pkg/sql/rowexec/joinreader_test.go, line 846 at r1](https://reviewable.io/reviews/cockroachdb/cockroach/49669#-M8XslRM72ChNbNGaWcm:-M8l-Oak3kAmZkawBiLb:bb6un1j) ([raw file](https://github.com/cockroachdb/cockroach/blob/cd8a706edecdf55025eacffad74281801adce3b2/pkg/sql/rowexec/joinreader_test.go#L846)):* <details><summary><i>Previously, yuzefovich wrote…</i></summary><blockquote> nit: missing period. </blockquote></details> Done ___ *[pkg/sql/rowexec/joinreader_test.go, line 852 at r1](https://reviewable.io/reviews/cockroachdb/cockroach/49669#-M8XsuaA5X8qjxD0E95g:-M8l-Rex0J6qH5b9A4cr:bb6un1j) ([raw file](https://github.com/cockroachdb/cockroach/blob/cd8a706edecdf55025eacffad74281801adce3b2/pkg/sql/rowexec/joinreader_test.go#L852)):* <details><summary><i>Previously, yuzefovich wrote…</i></summary><blockquote> `s/incresing/increasing/g` </blockquote></details> Done <!-- Sent from Reviewable.io -->
…ontainer
Additionally,
- added a randomized correctness test that compares the results of the indexed
and numbered containers.
- added benchmark cases to the joinReader benchmark that limit memory. None of
the workloads have repeated reads of the same right row and all access the
right rows in monotonically increasing order so the difference between the
two containers is due to the numbered container avoiding the overhead of
populating the cache.
- reduced the number of allocations in newNumberedDiskRowIterator.
- the accesses slices share the same underlying slice.
- a row copy, when there is a miss and the row is not added to the cache, is
eliminated. When a copy is needed and we have evicted a row from the cache,
the copying reuses that evicted row.
- allocations of the map, map elements are reused.
Fixes cockroachdb#48118
Release note: None
yuzefovich
approved these changes
Jun 1, 2020
There was a problem hiding this comment.
Do we typically run on gceworker to reduce variation?
Yeah, at least Macs can have noticeable variation that skews the benchmarks. In this case, however, we do see an obvious performance improvement, and I don't think it's too important to get the exact numbers.
have now become a decrease
Awesome!
Reviewed 3 of 3 files at r2.
Reviewable status:
pkg/sql/rowcontainer/numbered_row_container.go, line 413 at r2 (raw file):
elem := n.cache[rowIdx] if elem.accesses == nil { // Sub-slice that can grow up to elem.numAccesses
nit: mission period.
|
bors r+ |
Build succeeded |
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.
…ontainer
Additionally,
and numbered containers.
workloads have repeated reads of the same right row and all access the right
rows in monotonically increasing order so the difference between the two
containers is due to the numbered container avoiding the overhead of populating
the cache.
Fixes #48118
Release note: None