sql/catalog/descs: remove allocations from hot path#88614
sql/catalog/descs: remove allocations from hot path#88614craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
@postamar this one seems like something for you to look at. |
| // slice containing the correspondence between an index to a validation level. | ||
| type validationLevelMap struct { | ||
| m util.FastIntMap | ||
| } |
There was a problem hiding this comment.
You might want to use this in the implementation of StoredCatalog as well!
| flags tree.CommonLookupFlags, | ||
| descs []catalog.Descriptor, | ||
| ids ...descpb.ID, | ||
| ) (err error) { |
There was a problem hiding this comment.
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.
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
6ea236f to
c0b535c
Compare
ajwerner
left a comment
There was a problem hiding this comment.
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
getDescriptorsByIDandgetDescriptorsByIDWithSlicenow. Get rid of the former, take the latter and rename it to the former, and inline it everywhere. If need be define a helpergetDescriptorByIDwhich 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
StoredCatalogas 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.
|
TFTR! bors r=postamar |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
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. |
The lookup by ID path gets called constantly. This was over 1% of objects allocated in some workloads. Here's a microbenchmark:
Release note: None