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

kv,sql,storage: avoid various allocs #137569

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

kv,sql,storage: avoid various allocs #137569

wants to merge 12 commits into from

Conversation

tbg
Copy link
Member

@tbg tbg commented Dec 16, 2024

$ benchdiff --old lastmerge  ./pkg/sql/tests -b -r 'Sysbench/SQL/3node/oltp_write_only' -d 1000x -c 20
test binaries already exist for 9354770: Merge #137344
test binaries already exist for 1b46187: server: remove alloc in setupSpanForIncomingRPC

  pkg=1/1 iter=20/20 cockroachdb/cockroach/pkg/sql/tests -

name                                   old time/op    new time/op    delta
Sysbench/SQL/3node/oltp_write_only-24    4.42ms ± 3%    4.43ms ± 3%    ~     (p=0.687 n=19+20)

name                                   old alloc/op   new alloc/op   delta
Sysbench/SQL/3node/oltp_write_only-24     956kB ± 3%     954kB ± 3%    ~     (p=0.134 n=18+19)

name                                   old allocs/op  new allocs/op  delta
Sysbench/SQL/3node/oltp_write_only-24     6.17k ± 1%     6.09k ± 1%  -1.31%  (p=0.000 n=16+16)
  • kvserver: avoid an alloc
  • kvserver: step 1 in reducing abandon() alloc
  • kvserver: step 2 in removing abandon() alloc
  • kvserver: avoid allocation due to abandon closure in hot path
  • kvserver: avoid Veventf-related allocs
  • kvserverbase: make tracing alloc in hot path conditional
  • sql: avoid ctx moving to heap in dispatchToExecutionEngine
  • sql: avoid some vevent-related allocs
  • storage: pool unlimited standalone BoundAccounts
  • mvcc: remove minor allocation in MVCCGarbageCollect
  • server: remove alloc in setupSpanForIncomingRPC

tbg added 9 commits December 16, 2024 21:48
This was around 0.5% of allocs in oltp_write_only. The account is always
populated on the "real hot path" reads, but there are also the similarly
hot but not "as real" reads for the abort span, loading the GC hint, and
so on. These are not using the kvserver-level bound account and so
benefit from this pooling.
@tbg tbg added the o-perf-efficiency Related to performance efficiency label Dec 16, 2024
@tbg tbg requested review from a team as code owners December 16, 2024 23:08
@tbg tbg requested a review from RaduBerinde December 16, 2024 23:08
Copy link

blathers-crl bot commented Dec 16, 2024

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg tbg changed the title kvserver: avoid an alloc kv,sql,storage: avoid various allocs Dec 16, 2024
tbg added 3 commits December 17, 2024 08:48
we were unconditionally heap allocationg `ba.ShallowCopy`, for 0.6% of
allocs on oltp_write_only.
I actually don't understand why this allocated.
Escape analysis said:

> func literal escapes to heap: flow: {heap} =
> &{storage for func literal}: from func literal (spill) at <the defer>

This remained true even after simplifying the code (so that `iter, err`
were local to the block) but keeping the `defer`.

This was 0.41% on oltp_write_only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
o-perf-efficiency Related to performance efficiency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants