Skip to content

release-22.2: sql/catalog/descs: remove allocations from hot path#89286

Merged
ajwerner merged 1 commit intorelease-22.2from
blathers/backport-release-22.2-88614
Oct 4, 2022
Merged

release-22.2: sql/catalog/descs: remove allocations from hot path#89286
ajwerner merged 1 commit intorelease-22.2from
blathers/backport-release-22.2-88614

Conversation

@blathers-crl
Copy link
Copy Markdown

@blathers-crl blathers-crl Bot commented Oct 4, 2022

Backport 1/1 commits from #88614 on behalf of @ajwerner.

/cc @cockroachdb/release


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


Release justification: low risk fix for performance regression

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
@blathers-crl blathers-crl Bot requested a review from a team October 4, 2022 14:46
@blathers-crl blathers-crl Bot force-pushed the blathers/backport-release-22.2-88614 branch from ec84b6a to 57bea9b Compare October 4, 2022 14:46
@blathers-crl
Copy link
Copy Markdown
Author

blathers-crl Bot commented Oct 4, 2022

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@blathers-crl blathers-crl Bot requested review from ajwerner and postamar October 4, 2022 14:46
@blathers-crl blathers-crl Bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels Oct 4, 2022
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@ajwerner ajwerner merged commit 1c36dc4 into release-22.2 Oct 4, 2022
@ajwerner ajwerner deleted the blathers/backport-release-22.2-88614 branch October 4, 2022 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants