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

expression: implement vectorized evaluation for builtinJSONMergeSig #12916

Merged
merged 3 commits into from
Oct 30, 2019

Conversation

js00070
Copy link
Contributor

@js00070 js00070 commented Oct 24, 2019

What problem does this PR solve?

Implement vectorized evaluation for builtinJSONMergeSig mentioned in #12104

What is changed and how it works?

about 40% faster

BenchmarkVectorizedBuiltinJSONFunc/builtinJSONMergeSig-VecBuiltinFunc-4                      482           2282113 ns/op          949025 B/op      14082 allocs/op
BenchmarkVectorizedBuiltinJSONFunc/builtinJSONMergeSig-NonVecBuiltinFunc-4                   386           3197396 ns/op         1019904 B/op      14752 allocs/op

Check List

Tests

  • Unit test

@js00070 js00070 requested a review from a team as a code owner October 24, 2019 08:25
@ghost ghost requested a review from XuHuaiyu October 24, 2019 08:25
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Oct 24, 2019
@ghost ghost removed their request for review October 24, 2019 08:25
@zyxbest
Copy link
Contributor

zyxbest commented Oct 24, 2019

/run-unit-test

@codecov
Copy link

codecov bot commented Oct 24, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #12916   +/-   ##
===========================================
  Coverage   80.1577%   80.1577%           
===========================================
  Files           468        468           
  Lines        110018     110018           
===========================================
  Hits          88188      88188           
  Misses        15097      15097           
  Partials       6733       6733

@js00070
Copy link
Contributor Author

js00070 commented Oct 28, 2019

@XuHuaiyu
I did several times of benchstat, the results are unstable because of my CPU condition(the frequency is changing on different temperature), but we can see my original code is faster than the approch like you mentioned.

name                                                                old time/op    new time/op    delta
VectorizedBuiltinJSONFunc/builtinJSONMergeSig-VecBuiltinFunc-12        822µs ± 1%    1064µs ± 0%  +29.47%  (p=0.000 n=19+17)
VectorizedBuiltinJSONFunc/builtinJSONMergeSig-NonVecBuiltinFunc-12    1.08ms ± 1%    1.30ms ± 1%  +20.19%  (p=0.000 n=19+20)
name                                                                old time/op    new time/op    delta
VectorizedBuiltinJSONFunc/builtinJSONMergeSig-VecBuiltinFunc-12        906µs ± 1%    1064µs ± 0%  +17.38%  (p=0.000 n=19+17)
VectorizedBuiltinJSONFunc/builtinJSONMergeSig-NonVecBuiltinFunc-12    1.18ms ± 1%    1.30ms ± 1%   +9.60%  (p=0.000 n=19+20)
name                                                                old time/op    new time/op    delta
VectorizedBuiltinJSONFunc/builtinJSONMergeSig-VecBuiltinFunc-12       1.02ms ± 1%    1.06ms ± 0%   +4.59%  (p=0.000 n=19+17)
VectorizedBuiltinJSONFunc/builtinJSONMergeSig-NonVecBuiltinFunc-12    1.32ms ± 1%    1.30ms ± 1%   -1.87%  (p=0.000 n=19+20)

the code which produces new.txt is as follow

	result.ReserveJSON(nr)
	for i := 0; i < nr; i++ {
		nullFlag := false
		jsonVal := make([]json.BinaryJSON, 0, len(b.args))
		for j := 0; j < len(b.args); j++ {
			if argBuffers[j].IsNull(i) {
				result.AppendNull()
				nullFlag = true
				break
			}
			jsonVal = append(jsonVal, argBuffers[j].GetJSON(i))
		}
		if !nullFlag {
			result.AppendJSON(json.MergeBinary(jsonVal))
		}
	}

Copy link
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

@XuHuaiyu XuHuaiyu added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 28, 2019
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM, WELL DONE.

@qw4990 qw4990 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 Oct 30, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 30, 2019

Your auto merge job has been accepted, waiting for 13003

@sre-bot
Copy link
Contributor

sre-bot commented Oct 30, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Oct 30, 2019

/run-all-tests

@sre-bot sre-bot merged commit a13bb53 into pingcap:master Oct 30, 2019
@js00070 js00070 deleted the JSONMerge branch November 7, 2019 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression contribution This PR is from a community contributor. 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