coldata: fix BenchmarkAppend so that it could be run with multiple count #72132
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
Previously, we could get an index out of bounds because we'd allocate larger `Bytes.data` slice than `int32` offset can support. Release note: None
|
This change is |
mgartner
reviewed
Oct 29, 2021
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)
pkg/col/coldata/vec_test.go, line 435 at r1 (raw file):
b.SetBytes(8 * int64(coldata.BatchSize())) bc.args.DestIdx = 0 dest := coldata.NewMemColumn(typ, coldata.BatchSize(), coldata.StandardColumnFactory)
I'm not sure I follow this. Does this fix the issue because coldata.BatchSize() was sometimes returning a different value outside of the b.Run callback?
yuzefovich
commented
Oct 29, 2021
There was a problem hiding this comment.
Reviewable status:
pkg/col/coldata/vec_test.go, line 435 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
I'm not sure I follow this. Does this fix the issue because
coldata.BatchSize()was sometimes returning a different value outside of theb.Runcallback?
No, the problem is that we're creating an object outside of b.Run, and we don't reset that object. So that Bytes.data slice can grow unboundedly. The fix is to make sure that a fresh vector is created for each b.Run.
I think it usually works ok for a single run of the benchmark (i.e. without specifying count), but it crashes basically every time on the second run:
yuzefovich@Yahors-MacBook-Pro cockroach % make bench PKG=./pkg/col/coldata BENCHES=BenchmarkAppend/bytes TESTFLAGS='-test.count=5'
Running make with -j16
GOPATH set to /Users/yuzefovich/go
mkdir -p lib
rm -f lib/lib{geos,geos_c}.dylib
cp -L /Users/yuzefovich/go/native/x86_64-apple-darwin20.6.0/geos/lib/lib{geos,geos_c}.dylib lib
GOFLAGS= go test -mod=vendor -tags 'make x86_64_apple_darwin20.6.0' -ldflags '-X github.com/cockroachdb/cockroach/pkg/build.typ=development -extldflags "" -X "github.com/cockroachdb/cockroach/pkg/build.tag=v21.2.0-alpha.00000000-5423-g8cc3c85da7" -X "github.com/cockroachdb/cockroach/pkg/build.rev=8cc3c85da729a4ff325f13274d3a56b8df03ec46" -X "github.com/cockroachdb/cockroach/pkg/build.cgoTargetTriple=x86_64-apple-darwin20.6.0" ' -run "-" -bench "BenchmarkAppend/bytes" -timeout 5m ./pkg/col/coldata -test.count=5
I211029 19:01:23.779662 1 (gostd) rand.go:130 [-] 1 random seed: 7984515487810536210
goos: darwin
goarch: amd64
pkg: github.com/cockroachdb/cockroach/pkg/col/coldata
cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
BenchmarkAppend/bytes/AppendSimple/NullProbability=0.0-16 panic: runtime error: slice bounds out of range [:-2147483648]
goroutine 19 [running]:
github.com/cockroachdb/cockroach/pkg/col/coldata.(*Bytes).AppendSlice(0xc0000bfa00, 0xc0000bf980, 0x10000000, 0x0, 0x400)
/Users/yuzefovich/go/src/github.com/cockroachdb/cockroach/pkg/col/coldata/bytes.go:349 +0x4ff
github.com/cockroachdb/cockroach/pkg/col/coldata.(*memColumn).Append(0xc0000bfa40, 0x6f0af50, 0xc0000bf9c0, 0x0, 0x0, 0x0, 0x10000000, 0x0, 0x400)
/Users/yuzefovich/go/src/github.com/cockroachdb/cockroach/pkg/col/coldata/vec.eg.go:68 +0x227a
github.com/cockroachdb/cockroach/pkg/col/coldata_test.BenchmarkAppend.func1(0xc000a6afc0)
/Users/yuzefovich/go/src/github.com/cockroachdb/cockroach/pkg/col/coldata/vec_test.go:437 +0xda
testing.(*B).runN(0xc000a6afc0, 0x46d06)
/usr/local/opt/go/libexec/src/testing/benchmark.go:192 +0xeb
testing.(*B).launch(0xc000a6afc0)
/usr/local/opt/go/libexec/src/testing/benchmark.go:325 +0xea
created by testing.(*B).doBench
/usr/local/opt/go/libexec/src/testing/benchmark.go:280 +0x55
exit status 2
FAIL github.com/cockroachdb/cockroach/pkg/col/coldata 9.165s
FAIL
make: *** [Makefile:1073: bench] Error 1
yuzefovich@Yahors-MacBook-Pro cockroach % git checkout bytes-append
Switched to branch 'bytes-append'
Your branch and 'origin/bytes-append' have diverged,
and have 43 and 1 different commits each, respectively.
(use "git pull" to merge the remote branch into yours)
yuzefovich@Yahors-MacBook-Pro cockroach % make bench PKG=./pkg/col/coldata BENCHES=BenchmarkAppend/bytes TESTFLAGS='-test.count=5'
Running make with -j16
GOPATH set to /Users/yuzefovich/go
mkdir -p lib
rm -f lib/lib{geos,geos_c}.dylib
cp -L /Users/yuzefovich/go/native/x86_64-apple-darwin20.6.0/geos/lib/lib{geos,geos_c}.dylib lib
GOFLAGS= go test -mod=vendor -tags 'make x86_64_apple_darwin20.6.0' -ldflags '-X github.com/cockroachdb/cockroach/pkg/build.typ=development -extldflags "" -X "github.com/cockroachdb/cockroach/pkg/build.tag=v21.2.0-alpha.00000000-5424-gad566f16eb" -X "github.com/cockroachdb/cockroach/pkg/build.rev=ad566f16eb1884ff55ae805b0e9c26ee2f5d69b7" -X "github.com/cockroachdb/cockroach/pkg/build.cgoTargetTriple=x86_64-apple-darwin20.6.0" ' -run "-" -bench "BenchmarkAppend/bytes" -timeout 5m ./pkg/col/coldata -test.count=5
I211029 19:01:51.531494 1 (gostd) rand.go:130 [-] 1 random seed: 2159351605275735323
goos: darwin
goarch: amd64
pkg: github.com/cockroachdb/cockroach/pkg/col/coldata
cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
BenchmarkAppend/bytes/AppendSimple/NullProbability=0.0-16 101076 10851 ns/op 754.95 MB/s
BenchmarkAppend/bytes/AppendSimple/NullProbability=0.0-16 125724 9186 ns/op 891.81 MB/s
BenchmarkAppend/bytes/AppendSimple/NullProbability=0.0-16 159199 9780 ns/op 837.64 MB/s
BenchmarkAppend/bytes/AppendSimple/NullProbability=0.0-16 158518 7178 ns/op 1141.26 MB/s
BenchmarkAppend/bytes/AppendSimple/NullProbability=0.0-16 164301 7197 ns/op 1138.21 MB/s
BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.0-16 68745 14837 ns/op 552.14 MB/s
BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.0-16 69409 14932 ns/op 548.64 MB/s
BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.0-16 69745 14926 ns/op 548.85 MB/s
BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.0-16 67875 15068 ns/op 543.67 MB/s
BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.0-16 68520 15051 ns/op 544.30 MB/s
BenchmarkAppend/bytes/AppendSimple/NullProbability=0.2-16 163652 7092 ns/op 1155.08 MB/s
BenchmarkAppend/bytes/AppendSimple/NullProbability=0.2-16 169930 7105 ns/op 1152.92 MB/s
BenchmarkAppend/bytes/AppendSimple/NullProbability=0.2-16 163806 6960 ns/op 1177.09 MB/s
BenchmarkAppend/bytes/AppendSimple/NullProbability=0.2-16 164280 6928 ns/op 1182.46 MB/s
BenchmarkAppend/bytes/AppendSimple/NullProbability=0.2-16 163671 6987 ns/op 1172.50 MB/s
BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.2-16 62634 16607 ns/op 493.30 MB/s
BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.2-16 61861 16738 ns/op 489.41 MB/s
BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.2-16 62954 16717 ns/op 490.04 MB/s
BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.2-16 62115 16861 ns/op 485.85 MB/s
BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.2-16 62659 16855 ns/op 486.02 MB/s
PASS
ok github.com/cockroachdb/cockroach/pkg/col/coldata 35.582s
(In this case it actually crashed on the first run).
TBH I'm a bit puzzled by this behavior too, but this change seems to work. I found it when tried to run scripts/bench to see the difference between two SHAs, and there we use a count of 10. After this change is applied, I had no problems.
mgartner
approved these changes
Oct 29, 2021
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:
pkg/col/coldata/vec_test.go, line 435 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
No, the problem is that we're creating an object outside of
b.Run, and we don't reset that object. So thatBytes.dataslice can grow unboundedly. The fix is to make sure that a fresh vector is created for eachb.Run.I think it usually works ok for a single run of the benchmark (i.e. without specifying
count), but it crashes basically every time on the second run:yuzefovich@Yahors-MacBook-Pro cockroach % make bench PKG=./pkg/col/coldata BENCHES=BenchmarkAppend/bytes TESTFLAGS='-test.count=5' Running make with -j16 GOPATH set to /Users/yuzefovich/go mkdir -p lib rm -f lib/lib{geos,geos_c}.dylib cp -L /Users/yuzefovich/go/native/x86_64-apple-darwin20.6.0/geos/lib/lib{geos,geos_c}.dylib lib GOFLAGS= go test -mod=vendor -tags 'make x86_64_apple_darwin20.6.0' -ldflags '-X github.com/cockroachdb/cockroach/pkg/build.typ=development -extldflags "" -X "github.com/cockroachdb/cockroach/pkg/build.tag=v21.2.0-alpha.00000000-5423-g8cc3c85da7" -X "github.com/cockroachdb/cockroach/pkg/build.rev=8cc3c85da729a4ff325f13274d3a56b8df03ec46" -X "github.com/cockroachdb/cockroach/pkg/build.cgoTargetTriple=x86_64-apple-darwin20.6.0" ' -run "-" -bench "BenchmarkAppend/bytes" -timeout 5m ./pkg/col/coldata -test.count=5 I211029 19:01:23.779662 1 (gostd) rand.go:130 [-] 1 random seed: 7984515487810536210 goos: darwin goarch: amd64 pkg: github.com/cockroachdb/cockroach/pkg/col/coldata cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkAppend/bytes/AppendSimple/NullProbability=0.0-16 panic: runtime error: slice bounds out of range [:-2147483648] goroutine 19 [running]: github.com/cockroachdb/cockroach/pkg/col/coldata.(*Bytes).AppendSlice(0xc0000bfa00, 0xc0000bf980, 0x10000000, 0x0, 0x400) /Users/yuzefovich/go/src/github.com/cockroachdb/cockroach/pkg/col/coldata/bytes.go:349 +0x4ff github.com/cockroachdb/cockroach/pkg/col/coldata.(*memColumn).Append(0xc0000bfa40, 0x6f0af50, 0xc0000bf9c0, 0x0, 0x0, 0x0, 0x10000000, 0x0, 0x400) /Users/yuzefovich/go/src/github.com/cockroachdb/cockroach/pkg/col/coldata/vec.eg.go:68 +0x227a github.com/cockroachdb/cockroach/pkg/col/coldata_test.BenchmarkAppend.func1(0xc000a6afc0) /Users/yuzefovich/go/src/github.com/cockroachdb/cockroach/pkg/col/coldata/vec_test.go:437 +0xda testing.(*B).runN(0xc000a6afc0, 0x46d06) /usr/local/opt/go/libexec/src/testing/benchmark.go:192 +0xeb testing.(*B).launch(0xc000a6afc0) /usr/local/opt/go/libexec/src/testing/benchmark.go:325 +0xea created by testing.(*B).doBench /usr/local/opt/go/libexec/src/testing/benchmark.go:280 +0x55 exit status 2 FAIL github.com/cockroachdb/cockroach/pkg/col/coldata 9.165s FAIL make: *** [Makefile:1073: bench] Error 1 yuzefovich@Yahors-MacBook-Pro cockroach % git checkout bytes-append Switched to branch 'bytes-append' Your branch and 'origin/bytes-append' have diverged, and have 43 and 1 different commits each, respectively. (use "git pull" to merge the remote branch into yours) yuzefovich@Yahors-MacBook-Pro cockroach % make bench PKG=./pkg/col/coldata BENCHES=BenchmarkAppend/bytes TESTFLAGS='-test.count=5' Running make with -j16 GOPATH set to /Users/yuzefovich/go mkdir -p lib rm -f lib/lib{geos,geos_c}.dylib cp -L /Users/yuzefovich/go/native/x86_64-apple-darwin20.6.0/geos/lib/lib{geos,geos_c}.dylib lib GOFLAGS= go test -mod=vendor -tags 'make x86_64_apple_darwin20.6.0' -ldflags '-X github.com/cockroachdb/cockroach/pkg/build.typ=development -extldflags "" -X "github.com/cockroachdb/cockroach/pkg/build.tag=v21.2.0-alpha.00000000-5424-gad566f16eb" -X "github.com/cockroachdb/cockroach/pkg/build.rev=ad566f16eb1884ff55ae805b0e9c26ee2f5d69b7" -X "github.com/cockroachdb/cockroach/pkg/build.cgoTargetTriple=x86_64-apple-darwin20.6.0" ' -run "-" -bench "BenchmarkAppend/bytes" -timeout 5m ./pkg/col/coldata -test.count=5 I211029 19:01:51.531494 1 (gostd) rand.go:130 [-] 1 random seed: 2159351605275735323 goos: darwin goarch: amd64 pkg: github.com/cockroachdb/cockroach/pkg/col/coldata cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz BenchmarkAppend/bytes/AppendSimple/NullProbability=0.0-16 101076 10851 ns/op 754.95 MB/s BenchmarkAppend/bytes/AppendSimple/NullProbability=0.0-16 125724 9186 ns/op 891.81 MB/s BenchmarkAppend/bytes/AppendSimple/NullProbability=0.0-16 159199 9780 ns/op 837.64 MB/s BenchmarkAppend/bytes/AppendSimple/NullProbability=0.0-16 158518 7178 ns/op 1141.26 MB/s BenchmarkAppend/bytes/AppendSimple/NullProbability=0.0-16 164301 7197 ns/op 1138.21 MB/s BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.0-16 68745 14837 ns/op 552.14 MB/s BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.0-16 69409 14932 ns/op 548.64 MB/s BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.0-16 69745 14926 ns/op 548.85 MB/s BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.0-16 67875 15068 ns/op 543.67 MB/s BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.0-16 68520 15051 ns/op 544.30 MB/s BenchmarkAppend/bytes/AppendSimple/NullProbability=0.2-16 163652 7092 ns/op 1155.08 MB/s BenchmarkAppend/bytes/AppendSimple/NullProbability=0.2-16 169930 7105 ns/op 1152.92 MB/s BenchmarkAppend/bytes/AppendSimple/NullProbability=0.2-16 163806 6960 ns/op 1177.09 MB/s BenchmarkAppend/bytes/AppendSimple/NullProbability=0.2-16 164280 6928 ns/op 1182.46 MB/s BenchmarkAppend/bytes/AppendSimple/NullProbability=0.2-16 163671 6987 ns/op 1172.50 MB/s BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.2-16 62634 16607 ns/op 493.30 MB/s BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.2-16 61861 16738 ns/op 489.41 MB/s BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.2-16 62954 16717 ns/op 490.04 MB/s BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.2-16 62115 16861 ns/op 485.85 MB/s BenchmarkAppend/bytes/AppendWithSel/NullProbability=0.2-16 62659 16855 ns/op 486.02 MB/s PASS ok github.com/cockroachdb/cockroach/pkg/col/coldata 35.582s(In this case it actually crashed on the first run).
TBH I'm a bit puzzled by this behavior too, but this change seems to work. I found it when tried to run
scripts/benchto see the difference between two SHAs, and there we use a count of 10. After this change is applied, I had no problems.
I see. This makes sense, but I still don't have an explanation why it overflows at the max 32-bit integer and not the max 64-bit integer...
yuzefovich
commented
Oct 29, 2021
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/col/coldata/vec_test.go, line 435 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
I see. This makes sense, but I still don't have an explanation why it overflows at the max 32-bit integer and not the max 64-bit integer...
Oh, this part is easy - it's because Bytes.offsets is []int32, and we chose int32 instead of int64 or uint64 because that's what arrow library (for serialization) is using.
|
pkg/col/coldata/vec_test.go, line 435 at r1 (raw file): Previously, yuzefovich (Yahor Yuzefovich) wrote…
I see. And we use those |
|
Build succeeded: |
This was referenced Oct 29, 2021
Merged
craig bot
pushed a commit
that referenced
this pull request
Nov 18, 2021
70768: release-21.2: util/encoding: support deep copying when decoding bytes ascending r=yuzefovich a=blathers-crl[bot] Backport 1/1 commits from #70740 on behalf of @yuzefovich. /cc @cockroachdb/release ---- There is an optimization when decoding bytes ascending when it might return the decoded value that shares the same storage as the input buffer. However, this optimization might not be desirable in all cases: in particular, if the input buffer came from the large BatchResponse, then the decoded value might keep that whole response from being GCed. This commit adds a couple of methods to avoid using the shared storage by always performing a deep copy. It also audits all callers of the relevant methods on the KV-SQL boundary (the fetchers) to make sure they perform the deep copy when needed. Release note: None ---- Release justification: 72253: release-21.2: coldata: fix BenchmarkAppend so that it could be run with multiple count r=yuzefovich a=blathers-crl[bot] Backport 1/1 commits from #72132 on behalf of @yuzefovich. /cc @cockroachdb/release ---- Previously, we could get an index out of bounds because we'd allocate larger `Bytes.data` slice than `int32` offset can support. Release note: None ---- Release justification: Co-authored-by: Yahor Yuzefovich <[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.
Previously, we could get an index out of bounds because we'd allocate
larger
Bytes.dataslice thanint32offset can support.Release note: None