sql/catalog/descs: remove allocations from hot path

[ajwerner]

The lookup by ID path gets called constantly. This was over 1% of objects allocated in some workloads. Here's a microbenchmark:

name                                                                    old time/op    new time/op    delta
ResolveExistingObject/CREATE_SCHEMA_sc;CREATE_TABLE_sc.foo_()sc.foo-16    2.62µs ± 1%    2.18µs ± 1%   -16.63%  (p=0.000 n=10+8)

name                                                                    old alloc/op   new alloc/op   delta
ResolveExistingObject/CREATE_SCHEMA_sc;CREATE_TABLE_sc.foo_()sc.foo-16      150B ± 0%        4B ± 0%   -97.33%  (p=0.001 n=8+9)

name                                                                    old allocs/op  new allocs/op  delta
ResolveExistingObject/CREATE_SCHEMA_sc;CREATE_TABLE_sc.foo_()sc.foo-16      12.0 ± 0%       0.0       -100.00%  (p=0.000 n=10+10)

Release note: None

[cockroach-teamcity]

This change is 

[ajwerner]

@postamar this one seems like something for you to look at.

[postamar]

Nice, this seems like an easy win.

[postamar]

You might want to use this in the implementation of StoredCatalog as well!

[postamar]

There's no point in maintaining both getDescriptorsByID and getDescriptorsByIDWithSlice now. Get rid of the former, take the latter and rename it to the former, and inline it everywhere. If need be define a helper getDescriptorByID which returns a (desc, err) pair.

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar)

---

pkg/sql/catalog/descs/descriptor.go line 109 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

There's no point in maintaining both getDescriptorsByID and getDescriptorsByIDWithSlice now. Get rid of the former, take the latter and rename it to the former, and inline it everywhere. If need be define a helper getDescriptorByID which returns a (desc, err) pair.

Honestly, having two functions here is helpful. Not returning the slice is painful because you have to allocate the slice etc. I refactored it and it felt somewhat worse. I did take your advice on the single descriptor helper. I made it a function to clear up any confusion. I think it reads fine.

---

pkg/sql/catalog/descs/descriptor.go line 561 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

You might want to use this in the implementation of StoredCatalog as well!

I'm not that worried down there -- if we're going to issue an RPC, what's a slice allocation? We can do it when a profile asks for it.

[ajwerner]

TFTR!

bors r=postamar

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from c0b535c to blathers/backport-release-22.1-88614: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

---

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