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: speed up Column.VecEvalReal by using MergeNulls #22191

Merged
merged 8 commits into from
Feb 17, 2021

Conversation

Tjianke
Copy link
Contributor

@Tjianke Tjianke commented Jan 5, 2021

What problem does this PR solve?

Issue Number: close #22190

Problem Summary:

improve efficiency of Column.VecEvalReal() with Column.MergeNulls()

What is changed and how it works?

What's Changed:

expression/column.go: use Column.MergeNulls() before copying src element to result

Related changes

Tests

  • Unit test

Benchmark

use BenchmarkVectorizedBuiltinArithmeticFunc to benchmark

go test --timeout 0 -v -benchmem -bench=BenchmarkVecEvalReal -count=20 -run=BenchmarkVecEvalReal | tee old.txt
go test --timeout 0 -v -benchmem -bench=BenchmarkVecEvalReal -count=20 -run=BenchmarkVecEvalReal | tee new.txt

benchstat old.txt new.txt
name                       old time/op    new time/op    delta
VecEvalReal/VecEvalReal-2    0.39ns ± 5%    0.38ns ± 6%  -2.44%  (p=0.003 n=18+20)

name                       old alloc/op   new alloc/op   delta
VecEvalReal/VecEvalReal-2     0.00B          0.00B         ~     (all equal)

name                       old allocs/op  new allocs/op  delta
VecEvalReal/VecEvalReal-2      0.00           0.00         ~     (all equal)

benchmark func:

func BenchmarkVecEvalReal(b *testing.B){
	ctx := mock.NewContext()

	batchSize := []int{
		10,
		100,
		1000,
		100000,
	}

	for i:=0; i<len(batchSize); i++ {
		ft := types.NewFieldType(mysql.TypeFloat)
		col := &Column{RetType: ft, Index: 0}
		input := chunk.New([]*types.FieldType{ft}, batchSize[i], batchSize[i])
		for i := 0; i < batchSize[i]; i++ {
			input.AppendFloat32(0, float32(i))
		}
		result, err := newBuffer(types.ETReal, batchSize[i])
		if err != nil {
			panic(err)
		}
		b.Run(fmt.Sprintf("origin-%d", batchSize[i]), func(b *testing.B){
			col.VecEvalReal(ctx, input, result)
		})
		result, err = newBuffer(types.ETReal, batchSize[i])
	}
}

Release note

  • speed up VecEvalReal() with MergeNulls()

Reference

Another PR that utilized MergeNulls() to speed up functions #12674

@ti-srebot ti-srebot added the first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. label Jan 5, 2021
@Tjianke
Copy link
Contributor Author

Tjianke commented Jan 6, 2021

/run-all-tests

add more logging

add more logging

add more logging

add more logging

add more logging

add more logging

add more logging

add more logging
@Tjianke
Copy link
Contributor Author

Tjianke commented Jan 6, 2021

/run-all-tests

@Tjianke
Copy link
Contributor Author

Tjianke commented Jan 6, 2021

/run-all-tests

@Tjianke Tjianke marked this pull request as ready for review January 6, 2021 10:54
@Tjianke Tjianke requested a review from a team as a code owner January 6, 2021 10:54
@Tjianke Tjianke requested review from lzmhhh123 and removed request for a team January 6, 2021 10:54
@Tjianke
Copy link
Contributor Author

Tjianke commented Jan 6, 2021

Some question about tests in TiDB

Had a really difficult time to debug a randgen integration test, anyway to view the integration test or to provide more info on failed tests?

for instance, in this integration test, I could barely get anything useful except from panic info I hardcoded inside.

image

@ngaut
Copy link
Member

ngaut commented Jan 6, 2021

PTAL @zhouqiang-cl

@zhouqiang-cl
Copy link
Contributor

PTAL @zhouqiang-cl

Got, we will improve it. /cc @Tjianke

@Tjianke
Copy link
Contributor Author

Tjianke commented Jan 11, 2021

@lzmhhh123 @qw4990 PTAL, thanks~

Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 15, 2021
@Tjianke
Copy link
Contributor Author

Tjianke commented Feb 1, 2021

@qw4990 PTAL, thanks~

@b41sh
Copy link
Member

b41sh commented Feb 8, 2021

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Feb 8, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Feb 8, 2021
@lzmhhh123
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 17, 2021
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@Tjianke merge failed.

@Tjianke
Copy link
Contributor Author

Tjianke commented Feb 17, 2021

/run-all-tests

@zz-jason zz-jason merged commit c9af430 into pingcap:master Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression first-time-contributor Indicates that the PR was contributed by an external member and is a first-time 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.

expression: speed up Column.VecEvalReal by using MergeNulls
7 participants