Skip to content

*: reduce object allocation by reuse chunk column#29064

Merged
ti-chi-bot merged 5 commits intopingcap:masterfrom
tiancaiamao:chunk-column
Oct 28, 2021
Merged

*: reduce object allocation by reuse chunk column#29064
ti-chi-bot merged 5 commits intopingcap:masterfrom
tiancaiamao:chunk-column

Conversation

@tiancaiamao
Copy link
Contributor

@tiancaiamao tiancaiamao commented Oct 25, 2021

What problem does this PR solve?

Reduce object allocation by reuse the chunk column.
Close #28340

Problem Summary:

For every query, we will create the chunk...
This is unnecessary, after the chunk data is drained by the MySQL client, nobody will use the chunk again, so it should be safe to reuse the chunk.

What is changed and how it works?

Many changes... maybe I should split the PR to the lib and the use of the lib?

  • Add a chunkAlloc for each client connection, allocate chunks from it, and reset for reuse after chunk sent to the client
  • Add a NewChunkFromAllocator method to the ResultSet interface
  • Update of the benchmark tests to use the chunk allocator
  • Implementation of the chunk Allocator

For the chunk Allocator implementation, I decouple the chunk object into chunk column objects for reuse, because the Chunk differs a lot, while the chunk column are fix sized.

Allocating chunk will try to take a empty Chunk object from the free list, and fill the columns from the columnAlloc,
and after reset, allocated chunks will turn into free with their columns collected into the poolColumnAllocator:

type allocator struct {
	allocated   []*Chunk
	free        []*Chunk
	columnAlloc poolColumnAllocator
}

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
cd session;
go test -bench BenchmarkPreparedPointGet -run XXX -benchtime 30s -benchmem -cpuprofile cpu.profile
BenchmarkPreparedPointGet-16    	 4350782	      8383 ns/op	    5262 B/op	      77 allocs/op
BenchmarkPreparedPointGet-16    	 4218360	      8486 ns/op	    5486 B/op	      83 allocs/op
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Reduce object allocation by reuse chunk column

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Oct 25, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • lcwangchao
  • xhebox

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 25, 2021
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 27, 2021
@lcwangchao
Copy link
Collaborator

LGTM. But I noticed we only reusing the chunks for result set. Is it necessary to reuse them for intermediate executors?

Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

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

Can not we just keep one NewChunk(alloc chunk.Allocator) api, i.e. if alloc == nil then alloc = DefaultColumnAllocator or if alloc == nil then return new(chunk...) ? Then the new API is enforced by default.

@Defined2014
Copy link
Contributor

LGTM, but I'm not reviewer. :(

@tiancaiamao
Copy link
Contributor Author

Can not we just keep one NewChunk(alloc chunk.Allocator) api, i.e. if alloc == nil then alloc = DefaultColumnAllocator or if alloc == nil then return new(chunk...) ? Then the new API is enforced by default.

Yes, we can, but it would affect another 300 lines, because there are so many place call the NewChunk() API.
I prefer use another PR, just the refactor without other logic change.
Or ... how about NewChunk(alloc ...chunk.Allocator) so the caller of NewChunk is not affected, and the change is still minimal? @xhebox

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 28, 2021
@xhebox
Copy link
Contributor

xhebox commented Oct 28, 2021

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

DetailsCommit hash: 0433c3f

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 28, 2021
@ti-chi-bot
Copy link
Member

@tiancaiamao: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot ti-chi-bot merged commit 36bc41a into pingcap:master Oct 28, 2021
@tiancaiamao tiancaiamao deleted the chunk-column branch October 28, 2021 07:11
guo-shaoge added a commit to guo-shaoge/tidb that referenced this pull request Nov 8, 2021
tiancaiamao added a commit to tiancaiamao/tidb that referenced this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Chunk column reuse

5 participants