sql/catalog: lazily allocate hash maps in *descs.Collection

[nvanbenschoten]

We call (*descs.Collection).ResetUncommitted after each transaction from (*connExecutor).resetExtraTxnState. Previously, the method would re-allocate four different hash maps each time it was called.

This commit changes the uncommittedComments and uncommittedZoneConfigs structs to lazily initialize their hash maps, which avoids allocating the maps when they are not needed. Doing so eliminates the four map allocations per txn.

➜ benchdiff --run='BenchmarkKV/./SQL/rows=1$$' --count=25 ./pkg/sql/tests

name                     old time/op    new time/op    delta
KV/Scan/SQL/rows=1-10       118µs ± 5%     116µs ± 4%  -1.10%  (p=0.048 n=24+22)
KV/Insert/SQL/rows=1-10     162µs ± 8%     160µs ± 4%    ~     (p=0.208 n=24+22)
KV/Update/SQL/rows=1-10     270µs ± 4%     271µs ± 7%    ~     (p=0.907 n=21+23)
KV/Delete/SQL/rows=1-10     189µs ± 9%     189µs ±10%    ~     (p=0.120 n=23+23)

name                     old alloc/op   new alloc/op   delta
KV/Scan/SQL/rows=1-10      32.0kB ± 0%    31.8kB ± 0%  -0.75%  (p=0.000 n=24+22)
KV/Insert/SQL/rows=1-10    58.9kB ± 0%    58.7kB ± 1%  -0.38%  (p=0.000 n=25+24)
KV/Delete/SQL/rows=1-10    84.6kB ± 1%    84.3kB ± 1%  -0.29%  (p=0.000 n=24+22)
KV/Update/SQL/rows=1-10    70.4kB ± 1%    70.2kB ± 0%  -0.22%  (p=0.006 n=24+23)

name                     old allocs/op  new allocs/op  delta
KV/Scan/SQL/rows=1-10         365 ± 0%       361 ± 1%  -1.20%  (p=0.000 n=24+23)
KV/Delete/SQL/rows=1-10       610 ± 1%       605 ± 0%  -0.75%  (p=0.000 n=20+20)
KV/Insert/SQL/rows=1-10       493 ± 0%       489 ± 1%  -0.72%  (p=0.000 n=25+23)
KV/Update/SQL/rows=1-10       724 ± 1%       721 ± 1%  -0.44%  (p=0.000 n=24+24)

Epic: None
Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[postamar]

LGTM and I'll approve this because it's a strict improvement over the status quo. What we really should be doing here is using an nstree.MutableCatalog, but that's more work, which I'm not expecting you to do (I wouldn't mind though!). Perhaps add some commentary to the effect that that data structure should be used instead because it has all of the laziness built in? It would also put all of the uncommitted things in the same place and not spread them out in the uncommittedDescriptors/ZoneConfigs/Comments structs.

[nvanbenschoten]

TFTR! Moving to an nstree.MutableCatalog seems reasonable, though I'm not familiar with the data structures in this code, so I'll leave more invasive refactors for the experts. For now, I'll just land this smaller micro-optimization to avoid the 4 heap allocations per txn.

bors r=postamar

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)