sql: avoid string to byte conversion copies on insert path #101693

cucaroach · 2023-04-17T22:38:13Z

Originated from: #91930

Incorporate those changes and add a benchmark and add some more changes based on benchmark results.

Broken into several commits:

json: avoid copying json.AsText where possible

Epic: None
Release note: None

pgwire: avoid some string to byte copies

Most parse routines don't retain the input string pointer so we can
use a pointer to the input bytes in those cases.

Release note: None
Epic: None

builtins: avoid some []byte to string copies in decode

Release note: None
Epic: None

valueside: remove string to []byte copies

Release note: None
Epic: None

colenc: use new UnsafeConvertBytesToString

Cosmetic change to use the new UnsafeConvertBytesToString.

Release note: None
Epic: None

opt: avoid allocation in DBytes interning

DBytes is a string under the covers but we usually operate on them
with []byte APIs, avoid copies in these cases.

Release note: None
Epic: None

tree: allow access to DBytes/DString/DEncodedKey raw bytes

Epic: None
Release note: None

encoding: unsafeString -> UnsafeConvertBytesToString

Make public for use elsewhere.

Release note: None
Epic: None

lex: avoid some string to byte slice copies

Facilitate some copy avoidance by using []byte instead of string.
Only copy when necessary in some cases.

Release note: None
Epic: None

bench: add a large insert benchmark

This benchmark shows the reducation in allocations by the copy avoidance
changes. Together the changes result in:

name                                     old time/op    new time/op    delta
SQL/Cockroach/InsertLarge/count=1000-10    18.7ms ± 4%    19.1ms ±26%     ~     (p=0.780 n=9+10)

name                                     old alloc/op   new alloc/op   delta
SQL/Cockroach/InsertLarge/count=1000-10    49.7MB ± 4%    39.7MB ±10%  -20.19%  (p=0.000 n=9+9)

name                                     old allocs/op  new allocs/op  delta
SQL/Cockroach/InsertLarge/count=1000-10     39.0k ± 5%     29.7k ± 8%  -23.83%  (p=0.000 n=10+9)

Release note: None
Epic: None

cockroach-teamcity · 2023-04-17T22:38:32Z

This change is

msirek

Nice reduction in allocations! Maybe this should be backported to 23.1.

Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 3 of 3 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, 2 of 2 files at r9, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rafiss)

cucaroach · 2023-04-18T11:23:46Z

Thanks @msirek! I'm gonna wait for @rafiss @otan review as well for the pgwire and lex bits.

otan

admittedly not well versed in the unsafe Go string/bytes stuff, but the changes look good to me except maybe the builtins

otan · 2023-04-24T11:53:14Z

pkg/sql/sem/builtins/builtins.go

 				if err != nil {
 					return nil, err
 				}
-				return tree.NewDBytes(tree.DBytes(res)), nil
+				return tree.NewDBytes(tree.DBytes(encoding.UnsafeConvertBytesToString(res))), nil


hmm, does this get re-used if someone uses another builtin that modifies bytes directly?

I'm DBytes are immutable based on comments here:

https://github.com/cucaroach/cockroach/blob/d463da3957254cbcd3a0f9d57f1c327c1a51948b/pkg/sql/sem/tree/datum.go#L1503

Most mutating consumers of DBytes do this:

infoBytes := []byte(tree.MustBeDBytes(row[1]))

Which will make a copy.

cucaroach · 2023-04-24T13:03:23Z

Adding @yuzefovich to double check my math on DBytes safety, my concern is that maybe the vectorized engine constructs DBytes directly from coldata.Bytes memory.

yuzefovich

Nice reduction!

to double check my math on DBytes safety, my concern is that maybe the vectorized engine constructs DBytes directly from coldata.Bytes memory.

I don't think we have any issues there. We currently always make a copy when going from vectorized coldata.Bytes to DBytes datums. For example, over in vec_to_datum_gen.go we make a copy by converting []byte (that we get from the Bytes vector) to DBytes (which is a string).

Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 3 of 3 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, 2 of 2 files at r9, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach, @nvanbenschoten, @otan, and @rafiss)

-- commits line 54 at r6:
nit: there is another place where we can do the same - in two spots in execgen/overloads_bin.go after which you'll need to run ./dev gen execgen to regenerate the code. Or perhaps leave a TODO for that.

pkg/sql/sem/builtins/builtins.go line 1171 at r9 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

I'm DBytes are immutable based on comments here:

https://github.com/cucaroach/cockroach/blob/d463da3957254cbcd3a0f9d57f1c327c1a51948b/pkg/sql/sem/tree/datum.go#L1503

Most mutating consumers of DBytes do this:
	infoBytes := []byte(tree.MustBeDBytes(row[1]))
Which will make a copy.

Yeah, this should be safe.

pkg/sql/sem/tree/datum.go line 381 at r3 (raw file):

		return nil, MakeParseError(s, types.Bytes, err)
	}
	return NewDBytes(DBytes(encoding.UnsafeConvertBytesToString(res))), nil

This function is defined in a later commit, so the build should fail on the third commit. This should be fixed. (Broken builds are bad because they make bisecting more annoying.)

pkg/bench/bench_test.go line 308 at r1 (raw file):

}

// runBenchmarkInsert benchmarks inserting count rows into a table.

nit: s/runBenchmarkInsert/runBenchmarkInsertLarge/, also mention how it differs from runBenchmarkInsert.

This benchmark shows the reducation in allocations by the copy avoidance changes. Together the changes result in: ``` name old time/op new time/op delta SQL/Cockroach/InsertLarge/count=1000-10 18.7ms ± 4% 19.1ms ±26% ~ (p=0.780 n=9+10) name old alloc/op new alloc/op delta SQL/Cockroach/InsertLarge/count=1000-10 49.7MB ± 4% 39.7MB ±10% -20.19% (p=0.000 n=9+9) name old allocs/op new allocs/op delta SQL/Cockroach/InsertLarge/count=1000-10 39.0k ± 5% 29.7k ± 8% -23.83% (p=0.000 n=10+9) ``` Release note: None Epic: None

Facilitate some copy avoidance by using []byte instead of string. Only copy when necessary in some cases. Release note: None Epic: None

Make public for use elsewhere. Release note: None Epic: None

Epic: None Release note: None

DBytes is a string under the covers but we usually operate on them with []byte APIs, avoid copies in these cases. Release note: None Epic: None

Cosmetic change to use the new UnsafeConvertBytesToString. Release note: None Epic: None

Release note: None Epic: None

Most parse routines don't retain the input string pointer so we can use a pointer to the input bytes in those cases. Release note: None Epic: None

All we do here is write the bytes to a stream so we don't need a copy. Release note: None Epic: None

We copy the DBytes into a []byte and then copy the []byte into a string. Now we get a reference to the string and return that if appropriate or iterator over string bytes directly if not. This is all predicated on DBytes being immutable. Release note: None Epic: None

Epic: None Release note: None

cucaroach

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @msirek, @nvanbenschoten, @otan, @rafiss, and @yuzefovich)

pkg/sql/sem/tree/datum.go line 381 at r3 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

This function is defined in a later commit, so the build should fail on the third commit. This should be fixed. (Broken builds are bad because they make bisecting more annoying.)

Fixed

pkg/bench/bench_test.go line 308 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: s/runBenchmarkInsert/runBenchmarkInsertLarge/, also mention how it differs from runBenchmarkInsert.

👍

cucaroach · 2023-05-03T16:55:10Z

Thanks for comments @yuzefovich, I addressed them and added a new commit to get the json.AsText callers. Will hold for @rafiss sign off.

rafiss

the changes looked safe to me!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @msirek, @nvanbenschoten, @otan, and @yuzefovich)

cucaroach · 2023-05-31T15:10:18Z

TFTRs!
bors r+

craig · 2023-05-31T15:44:12Z

Build succeeded:

Bazel Essential CI (Cockroach)

cucaroach changed the title ~~avoid copy~~ sql: avoid string to byte conversion copies on insert path Apr 17, 2023

cucaroach force-pushed the avoid-copy branch 2 times, most recently from 9d9327a to d463da3 Compare April 17, 2023 23:07

cucaroach marked this pull request as ready for review April 18, 2023 00:12

cucaroach requested a review from a team April 18, 2023 00:12

cucaroach requested review from a team as code owners April 18, 2023 00:12

cucaroach requested review from msirek and rafiss April 18, 2023 00:12

msirek approved these changes Apr 18, 2023

View reviewed changes

cucaroach requested review from otan and nvanbenschoten April 18, 2023 12:55

otan reviewed Apr 24, 2023

View reviewed changes

cucaroach requested a review from yuzefovich April 24, 2023 12:57

nvanbenschoten mentioned this pull request Apr 24, 2023

[DNM] sql: avoid memcpys on mutation data path #91930

Closed

yuzefovich reviewed Apr 26, 2023

View reviewed changes

cucaroach force-pushed the avoid-copy branch from d463da3 to f8c638f Compare May 3, 2023 16:46

blathers-crl bot requested a review from otan May 3, 2023 16:46

cucaroach force-pushed the avoid-copy branch from f8c638f to 4769f98 Compare May 3, 2023 16:47

cucaroach added 7 commits May 3, 2023 12:53

lex: avoid some string to byte slice copies

2bf2323

Facilitate some copy avoidance by using []byte instead of string. Only copy when necessary in some cases. Release note: None Epic: None

encoding: unsafeString -> UnsafeConvertBytesToString

b338d7b

Make public for use elsewhere. Release note: None Epic: None

tree: allow access to DBytes/DString/DEncodedKey raw bytes

2d83a73

Epic: None Release note: None

opt: avoid allocation in DBytes interning

2f556fc

DBytes is a string under the covers but we usually operate on them with []byte APIs, avoid copies in these cases. Release note: None Epic: None

colenc: use new UnsafeConvertBytesToString

3f225e4

Cosmetic change to use the new UnsafeConvertBytesToString. Release note: None Epic: None

valueside: remove string to []byte copies

e76872b

Release note: None Epic: None

cucaroach added 5 commits May 3, 2023 12:53

builtins: avoid some []byte to string copies in decode

81fec5a

Release note: None Epic: None

pgwire: avoid some string to byte copies

83870fc

Most parse routines don't retain the input string pointer so we can use a pointer to the input bytes in those cases. Release note: None Epic: None

sql: Avoid copy of file upload data

f3bd0bc

All we do here is write the bytes to a stream so we don't need a copy. Release note: None Epic: None

json: avoid copying json.AsText where possible

e67b8ed

Epic: None Release note: None

cucaroach force-pushed the avoid-copy branch from 4769f98 to e67b8ed Compare May 3, 2023 16:53

cucaroach commented May 3, 2023

View reviewed changes

otan removed their request for review May 4, 2023 18:53

rafiss approved these changes May 31, 2023

View reviewed changes

craig bot merged commit 774684d into cockroachdb:master May 31, 2023

cucaroach mentioned this pull request Jun 6, 2023

sqlsmith: internal error, trailing bytes in encoded value #104323

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: avoid string to byte conversion copies on insert path #101693

sql: avoid string to byte conversion copies on insert path #101693

cucaroach commented Apr 17, 2023 •

edited

Loading

cockroach-teamcity commented Apr 17, 2023

msirek left a comment

cucaroach commented Apr 18, 2023 •

edited

Loading

otan left a comment

otan Apr 24, 2023

cucaroach Apr 24, 2023

cucaroach commented Apr 24, 2023

yuzefovich left a comment

cucaroach left a comment

cucaroach commented May 3, 2023

rafiss left a comment

cucaroach commented May 31, 2023

craig bot commented May 31, 2023

sql: avoid string to byte conversion copies on insert path #101693

sql: avoid string to byte conversion copies on insert path #101693

Conversation

cucaroach commented Apr 17, 2023 • edited Loading

json: avoid copying json.AsText where possible

pgwire: avoid some string to byte copies

builtins: avoid some []byte to string copies in decode

valueside: remove string to []byte copies

colenc: use new UnsafeConvertBytesToString

opt: avoid allocation in DBytes interning

tree: allow access to DBytes/DString/DEncodedKey raw bytes

encoding: unsafeString -> UnsafeConvertBytesToString

lex: avoid some string to byte slice copies

bench: add a large insert benchmark

cockroach-teamcity commented Apr 17, 2023

msirek left a comment

Choose a reason for hiding this comment

cucaroach commented Apr 18, 2023 • edited Loading

otan left a comment

Choose a reason for hiding this comment

otan Apr 24, 2023

Choose a reason for hiding this comment

cucaroach Apr 24, 2023

Choose a reason for hiding this comment

cucaroach commented Apr 24, 2023

yuzefovich left a comment

Choose a reason for hiding this comment

cucaroach left a comment

Choose a reason for hiding this comment

cucaroach commented May 3, 2023

rafiss left a comment

Choose a reason for hiding this comment

cucaroach commented May 31, 2023

craig bot commented May 31, 2023

cucaroach commented Apr 17, 2023 •

edited

Loading

cucaroach commented Apr 18, 2023 •

edited

Loading