Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql,tree: improve function resolution efficiency #89317

Merged
merged 2 commits into from
Oct 4, 2022

Conversation

ajwerner
Copy link
Contributor

@ajwerner ajwerner commented Oct 4, 2022

sql: prevent allocations by avoiding some name pointers

We don't need pointers for these names. They generally won't escape.

sql,tree: change SearchPath to avoid allocations

The closure-oriented interface was forcing the closures and the variables they
referenced to escape to the heap. This change, while not beautiful, ends up being
much more efficient.

name                                         old time/op    new time/op    delta
SQL/MultinodeCockroach/Upsert/count=1000-16    20.4ms ±11%    18.9ms ± 8%   -7.47%  (p=0.000 n=20+19)

name                                         old alloc/op   new alloc/op   delta
SQL/MultinodeCockroach/Upsert/count=1000-16    10.1MB ±29%     9.8MB ±29%     ~     (p=0.231 n=20+20)

name                                         old allocs/op  new allocs/op  delta
SQL/MultinodeCockroach/Upsert/count=1000-16     56.3k ± 7%     50.2k ±10%  -10.81%  (p=0.000 n=19+19)

Release note: None

@ajwerner ajwerner requested review from a team October 4, 2022 18:48
@ajwerner ajwerner requested a review from a team as a code owner October 4, 2022 18:48
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner
Copy link
Contributor Author

ajwerner commented Oct 4, 2022

See #85391 (comment)

@chengxiong-ruan
Copy link
Contributor

:lgtm_strong: many thanks!

@ajwerner ajwerner force-pushed the ajwerner/tune-resolve-function branch from befd56f to 0cc2d41 Compare October 4, 2022 19:03
Copy link
Contributor

@chengxiong-ruan chengxiong-ruan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

We don't need pointers for these names. They generally won't escape.

Release note: None
The closure-oriented interface was forcing the closures and the variables they
referenced to escape to the heap. This change, while not beautiful, ends up being
much more efficient.

Release note: None
@ajwerner ajwerner force-pushed the ajwerner/tune-resolve-function branch from 0cc2d41 to e99a182 Compare October 4, 2022 19:24
@ajwerner
Copy link
Contributor Author

ajwerner commented Oct 4, 2022

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 4, 2022

Build failed:

@ajwerner
Copy link
Contributor Author

ajwerner commented Oct 4, 2022

#89335

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 4, 2022

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants