sql/lexbase: remove map access overhead when encoding SQL strings

[mgartner]

opt: add benchmark for simple INSERT with JSON column

Release note: None

sql/lexbase: remove map access overhead when encoding SQL strings

Map accesses have been replaced with more efficient array accesses for
determining characters to quote in EncodeSQLStringWithFlags. In a CPU
profile from a recent POC these map access accounted for a significant
portion of CPU time spent in query optimization because the optimizer's
interner converts tree.DJSON objects to strings using this encoding
function when hashing and checking equality of JSON constants.

Epic: None

Release note: None

[cockroach-teamcity]

This change is 

[mgartner]

Benchmark results of the new test case:

name                                                  old time/op    new time/op    delta
Phases/json-insert/Simple/Parse                         5.02µs ± 6%    4.79µs ± 2%     ~     (p=0.151 n=5+5)
Phases/json-insert/Simple/OptBuildNoNorm                12.9µs ± 2%    12.8µs ± 9%     ~     (p=0.548 n=5+5)
Phases/json-insert/Simple/OptBuildNorm                  13.5µs ±12%    12.4µs ± 2%   -8.42%  (p=0.008 n=5+5)
Phases/json-insert/Simple/Explore                       14.8µs ± 3%    13.9µs ± 1%   -6.14%  (p=0.008 n=5+5)
Phases/json-insert/Simple/ExecBuild                     16.1µs ± 5%    14.7µs ± 0%   -8.74%  (p=0.008 n=5+5)
Phases/json-insert/Prepared/AssignPlaceholdersNoNorm    4.12µs ± 1%    3.46µs ± 0%  -15.95%  (p=0.008 n=5+5)
Phases/json-insert/Prepared/AssignPlaceholdersNorm      4.34µs ± 6%    3.45µs ± 0%  -20.41%  (p=0.016 n=5+4)
Phases/json-insert/Prepared/Explore                     5.75µs ± 6%    4.89µs ± 1%  -14.89%  (p=0.008 n=5+5)
Phases/json-insert/Prepared/ExecBuild                   6.75µs ±10%    5.64µs ± 2%  -16.37%  (p=0.008 n=5+5)

[yuzefovich]

I have some nits to consider, but

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)

---

pkg/sql/lexbase/encode.go line 117 at r2 (raw file):

const minQuoteChar = ' ' // 0x20
const maxQuoteChar = '~' // 0x7E
var mustQuoteMap = [maxQuoteChar]bool{

nit: array needs to have maxQuoteChar + 1 elements (or maxQuoteChar needs to be 0x7F). (We could also use const minQuoteChar = byte(0x20) to be more explicit.)

---

pkg/sql/lexbase/encode.go line 157 at r2 (raw file):

		ch := byte(r)
		if r >= minQuoteChar && r <= maxQuoteChar {
			if mustQuoteMap[ch] {

nit: might be worth trying switch with 4 cases to see whether it'd be faster.

---

pkg/sql/lexbase/encode.go line 161 at r2 (raw file):

				bareStrings = false
			}
			if !stringencoding.NeedEscape(ch) && ch != '\'' {

nit: we can pull this if to be above mustQuoteMap check (we could avoid mustQuoteMap lookup if this condition is true).

[mgartner]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @rytaft and @yuzefovich)

---

pkg/sql/lexbase/encode.go line 117 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: array needs to have maxQuoteChar + 1 elements (or maxQuoteChar needs to be 0x7F). (We could also use const minQuoteChar = byte(0x20) to be more explicit.)

Oops, great catch! I switch this to use unicode.MaxASCII, let me know what you think. Using const minQuoteChar = byte(0x20) seems less obvious because you need to know that ' ' == 0x20 to make the connection to the ' ': true entry in the array.

---

pkg/sql/lexbase/encode.go line 157 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: might be worth trying switch with 4 cases to see whether it'd be faster.

I tested a simple benchmark and the array is the fastest: https://gist.github.com/mgartner/f1b0fbd7f05faf37038c8545dabc7b80

---

pkg/sql/lexbase/encode.go line 161 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: we can pull this if to be above mustQuoteMap check (we could avoid mustQuoteMap lookup if this condition is true).

I think the semantics would change if any chars satisfy both the conditional and the mustQuoteMap—in that case we wouldn't set baseStrings = false. It looks like that is impossible currently, but not guaranteed by anything. So I'm hesitant to change it.

[mgartner]

pkg/sql/lexbase/encode.go line 157 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I tested a simple benchmark and the array is the fastest: https://gist.github.com/mgartner/f1b0fbd7f05faf37038c8545dabc7b80

Hmm, maybe not the most fair test because the array likely sits in the CPU cache for the entire benchmark, which might be less likely in a production workload. 🤷

[yuzefovich]

Reviewed 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @rytaft)

---

pkg/sql/lexbase/encode.go line 157 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Hmm, maybe not the most fair test because the array likely sits in the CPU cache for the entire benchmark, which might be less likely in a production workload. 🤷

Thanks for trying (I thought I remembered reading somewhere that switch in recent Go versions became faster, perhaps not fast enough :) )

---

pkg/sql/lexbase/encode.go line 161 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I think the semantics would change if any chars satisfy both the conditional and the mustQuoteMap—in that case we wouldn't set baseStrings = false. It looks like that is impossible currently, but not guaranteed by anything. So I'm hesitant to change it.

Good point, I missed that.

[mgartner]

My previous attempts were actually broken in subtle ways. @yuzefovich do you mind taking one more look and letting me know if this still LGTY?

[yuzefovich]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @rytaft)

[mgartner]

TFTRs!

bors r+

[craig]

Build failed (retrying...):

• check_generated_code

[craig]

Build failed (retrying...):

• unit_tests

[craig]

Build succeeded:

• unit_tests
• docker_image_amd64
• acceptance
• macos_amd64_build
• linux_arm64_build
• local_roachtest
• local_roachtest_fips
• linux_amd64_fips_build
• linux_amd64_build
• lint
• check_generated_code
• macos_arm64_build
• examples_orms
• windows_build