Skip to content

expression: speed up Column.VecEvalReal by using MergeNulls#22191

Merged
zz-jason merged 8 commits intopingcap:masterfrom
Tjianke:mergeNulls
Feb 17, 2021
Merged

expression: speed up Column.VecEvalReal by using MergeNulls#22191
zz-jason merged 8 commits intopingcap:masterfrom
Tjianke:mergeNulls

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