Skip to content

expression: implement vectorized evaluation for builtinRepeatSig #12014

Merged
qw4990 merged 12 commits into
pingcap:masterfrom
qw4990:vecexpr-builtinRepeatSig
Sep 4, 2019
Merged

expression: implement vectorized evaluation for builtinRepeatSig #12014
qw4990 merged 12 commits into
pingcap:masterfrom
qw4990:vecexpr-builtinRepeatSig

Conversation

@qw4990
Copy link
Copy Markdown
Contributor

@qw4990 qw4990 commented Sep 3, 2019

What problem does this PR solve?

Implement vectorized evaluation for builtinRepeatSig.

What is changed and how it works?

Almost 1.5s faster than before:

BenchmarkVectorizedBuiltinFunc/builtinRepeatSig-VecBuiltinFunc-12                          50000             29567 ns/op
BenchmarkVectorizedBuiltinFunc/builtinRepeatSig-NonVecBuiltinFunc-12                       30000             45092 ns/op

Check List

Tests

  • Unit test

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 3, 2019

Codecov Report

Merging #12014 into master will decrease coverage by 0.0389%.
The diff coverage is 64.1025%.

@@               Coverage Diff               @@
##             master     #12014       +/-   ##
===============================================
- Coverage   81.3794%   81.3405%   -0.039%     
===============================================
  Files           446        447        +1     
  Lines         95658      95646       -12     
===============================================
- Hits          77846      77799       -47     
- Misses        12287      12310       +23     
- Partials       5525       5537       +12

Comment thread expression/bench_test.go Outdated
Comment thread expression/bench_test.go
Comment thread expression/bench_test.go
Comment thread expression/bench_test.go Outdated
Comment thread expression/bench_test.go
Comment thread expression/bench_test.go Outdated
Comment thread expression/bench_test.go Outdated
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

Comment thread expression/builtin_string_vec.go
Comment thread expression/bench_test.go
Comment thread expression/builtin_string_vec.go Outdated
Comment thread expression/builtin_string_vec.go
@qw4990 qw4990 added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 4, 2019
Copy link
Copy Markdown
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

LGTM

@SunRunAway SunRunAway added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 4, 2019
@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Sep 4, 2019

/run-all-tests

@qw4990 qw4990 merged commit ba8461d into pingcap:master Sep 4, 2019
@zz-jason
Copy link
Copy Markdown
Member

zz-jason commented Sep 9, 2019

to #12058

@js00070
Copy link
Copy Markdown
Contributor

js00070 commented Oct 12, 2019

i find a TODO at line 34 in expression/builtin_string_vec.go

// TODO: introduce vectorized null-bitmap to speed it up.
if buf.IsNull(i) || buf2.IsNull(i) {
    result.AppendNull()
    continue
}

I'm curious about how this should be done.

Is it right to use function MergeNulls directly? like this

buf.MergeNulls(buf2) // before the for loop
for i := 0; i < n; i++ {
	if buf.IsNull(i) { // reduce a IsNull call here
		result.AppendNull()
		continue
	}

I tried to run benchmark test but didn't find a significant performance improvement.

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

Labels

component/expression 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.

6 participants