Skip to content

executor: introduce the column buffer allocator for vectorized evaluation#11743

Merged
sre-bot merged 12 commits into
pingcap:masterfrom
qw4990:vecexpr-vecbuffer
Aug 16, 2019
Merged

executor: introduce the column buffer allocator for vectorized evaluation#11743
sre-bot merged 12 commits into
pingcap:masterfrom
qw4990:vecexpr-vecbuffer

Conversation

@qw4990
Copy link
Copy Markdown
Contributor

@qw4990 qw4990 commented Aug 15, 2019

What problem does this PR solve?

In vectorized evaluation, some expressions need buffers to store their children's data.
This PR introduces an interface columnBufferAllocator to do this job.

Being compared with adding these buffers as expression's struct fields like struct someBuiltinFunc{baseBuiltinFunc, buffer1, buffer2}, this way has two advantages:

  1. keep all expressions concurrency-safe,
  2. it's convenient for us to optimize the strategy of memory management in the future.

What is changed and how it works?

  1. Introduce a new interface columnBufferAllocator.
  2. Implement this new interface with localSliceBuffer.
  3. Add some unit tests and benchmarks.

Now, the localSliceBuffer is implemented by Slice+Lock, and the slight performance drawback seems acceptable:

BenchmarkPlusIntBufAllocator/enable-12           1000000              2089 ns/op               0 B/op          0 allocs/op
BenchmarkPlusIntBufAllocator/disable-12          1000000              1993 ns/op               0 B/op          0 allocs/op

All changes are new code and unit tests, so it's easy to review.

Check List

Tests

  • Unit test

@pingcap pingcap deleted a comment from codecov Bot Aug 15, 2019
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 15, 2019

Codecov Report

Merging #11743 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11743   +/-   ##
===========================================
  Coverage   81.4601%   81.4601%           
===========================================
  Files           434        434           
  Lines         93458      93458           
===========================================
  Hits          76131      76131           
  Misses        11879      11879           
  Partials       5448       5448

Comment thread expression/builtin_vectorized.go Outdated
Comment thread expression/builtin_vectorized.go Outdated
r.head = 0
}
r.size--
return buf, nil
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we call buf.Reserve() before returning it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There are multiple ways to initialize a buffer (Resize/Reset/Reserve) and in this method, the allocator doesn't know which way is better.
Therefore it seems more appropriate to let users initialize these buffers themselves.

Comment thread expression/builtin_vectorized.go Outdated
Comment thread expression/builtin_vectorized.go
Comment thread expression/builtin_vectorized.go Outdated
Comment thread expression/builtin_vectorized.go
@qw4990
Copy link
Copy Markdown
Contributor Author

qw4990 commented Aug 16, 2019

All comments are addressed, PTAL @zz-jason @XuHuaiyu

Copy link
Copy Markdown
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 16, 2019
@zz-jason zz-jason requested a review from SunRunAway August 16, 2019 07:03
Copy link
Copy Markdown
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 16, 2019
@zz-jason zz-jason added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 16, 2019
@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 16, 2019

/run-all-tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/new-feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants