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

catalog/descs: remove an allocation due to logging #89260

Closed
wants to merge 1 commit into from

Conversation

yuzefovich
Copy link
Member

This commit removes an allocation that we incurred due to a logging message - constructing the log message wasn't free in this case, so we hide it behind an "expensive logging enabled" check.

name                                         old time/op    new time/op    delta
SQL/MultinodeCockroach/Upsert/count=1000-24    25.5ms ± 4%    24.1ms ± 5%  -5.15%  (p=0.000 n=10+10)

name                                         old alloc/op   new alloc/op   delta
SQL/MultinodeCockroach/Upsert/count=1000-24    10.7MB ±13%    10.6MB ±18%    ~     (p=0.497 n=9+10)

name                                         old allocs/op  new allocs/op  delta
SQL/MultinodeCockroach/Upsert/count=1000-24     79.1k ± 6%     73.2k ± 6%  -7.50%  (p=0.007 n=10+10)

Addresses: #87685.

Release note: None

This commit removes an allocation that we incurred due to a logging
message - constructing the log message wasn't free in this case, so we
hide it behind an "expensive logging enabled" check.

```
name                                         old time/op    new time/op    delta
SQL/MultinodeCockroach/Upsert/count=1000-24    25.5ms ± 4%    24.1ms ± 5%  -5.15%  (p=0.000 n=10+10)

name                                         old alloc/op   new alloc/op   delta
SQL/MultinodeCockroach/Upsert/count=1000-24    10.7MB ±13%    10.6MB ±18%    ~     (p=0.497 n=9+10)

name                                         old allocs/op  new allocs/op  delta
SQL/MultinodeCockroach/Upsert/count=1000-24     79.1k ± 6%     73.2k ± 6%  -7.50%  (p=0.007 n=10+10)
```

Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner
Copy link
Contributor

ajwerner commented Oct 4, 2022

This will be fixed and then some by #88614 I believe. Sprucing it up now.

@yuzefovich
Copy link
Member Author

Ack, thanks.

@yuzefovich yuzefovich closed this Oct 4, 2022
@yuzefovich yuzefovich deleted the descs-allocation branch October 4, 2022 02:26
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