executor: remove *MVMap from agg executor #7541

XuHuaiyu · 2018-08-29T12:15:44Z

What problem does this PR solve?

CPU profile to be uploaded.

What is changed and how it works?

Change groupMap of HashAggExec and HashAggFinalWorker from *MVMap to
stringSet, thus we can avoid some cost of runtime.sliceToString,
runtime.byteSliceToString, and the access to MVMap.

Check List

Tests
Exist tests

No code

Code changes

Has exported function/method change

Side effects

Related changes

winoros · 2018-08-29T12:19:11Z

executor/aggfuncs/sets.go

@@ -19,7 +19,7 @@ import (

 type decimalSet map[types.MyDecimal]struct{}
 type float64Set map[float64]struct{}
-type stringSet map[string]struct{}


Move this file to util package?

shenli · 2018-08-29T14:04:00Z

executor/aggregate.go

 	"github.com/spaolacci/murmur3"
 	"golang.org/x/net/context"
+	"sort"


Move this to the first block.

shenli · 2018-08-29T14:21:34Z

Any benchmark result?

zz-jason · 2018-08-30T06:26:55Z

executor/aggregate.go

 	for _, item := range w.groupByItems {
-		v, err := item.Eval(row)
+		v, isNull, err := item.EvalString(ctx, row)


we can not simply call EvalString here.

zz-jason · 2018-08-30T06:28:46Z

executor/aggregate.go

-	var err error
-	w.groupKey, err = codec.EncodeValue(sc, w.groupKey[:0], w.groupValDatums...)
-	return w.groupKey, errors.Trace(err)
+	return groupKey, nil


can we reuse a []byte to reduce the string object allocation?

zz-jason · 2018-08-30T06:30:49Z

executor/aggregate.go

-		}
-		partialResults := e.getPartialResults(groupKey)
+
+	sort.Strings(e.sortedGroupKey)


Why this sort is needed?

zz-jason · 2018-08-30T06:31:29Z

executor/aggregate.go

-		if item.GetType().Tp == mysql.TypeNewDecimal {
-			v.SetLength(0)
-		}
+		v, isNull, err := item.EvalString(ctx, row)


Maybe it's not safe to call EvalString?

XuHuaiyu · 2018-08-31T02:22:41Z

Some bugs remained, working on this

XuHuaiyu · 2018-08-31T02:22:48Z

/run-all-tests

XuHuaiyu · 2018-09-03T06:14:01Z

/run-all-tests

XuHuaiyu · 2018-09-03T08:50:01Z

/run-common-test tidb-test=pr/614
/run-integration-common-test tidb-test=pr/614

shenli · 2018-09-03T08:51:56Z

@XuHuaiyu Please post the benchmark result here.

coocood · 2018-09-03T14:59:51Z

The stringSet has many pointers, GC overhead would be much higher.

XuHuaiyu · 2018-09-04T02:33:19Z

There is no obvious improvement for T1 mentioned in Tune the perfomance of hash aggregate operator #7450 (comment):

master	this PR
43.0901493788s	42.8431748867s
old.txt	new.txt

Optimize points:

~~3.77% runtime.stringToByteSliceof CPU cost introduced by HashAggPartialWorker.shuffleIntermData~~
This cannot be avoid since we need to cast string as byte slice to calculate the hash value.
0.75% runtime.sliceToString of CPU cost introduced by baseHashAggWorker.getPartialResult
8.86% MVMap.Get and MVMap.Put of CPU cost introduced by access to MVMap

The performance is not improved obviously because that the access to StringSet which is used to replace MVMap seems cost as much as the access to MVMap.
But it would be more readable after using StringSet instead of MVMap.

For T2 mentioned in: Tune the perfomance of hash aggregate operator #7450 (comment)

After using a string slice to store the group keys and traverse it instead of the MVMap when fetching the final result, we got about 33.47% CPU performance improvement.

master	this PR
463.533461905s	308.472225189s
old.txt	new.txt

Optimize points:

4.5% runtime.makeslice introduced by the definition of vals in HashAggExec.getGropuKey
17.5% MVMap.Get and MVMap.Put introduced by the access to MVMap

XuHuaiyu · 2018-09-04T02:44:44Z

Seen from the profile picture, it seems the GC overhead is not much more than the master branch. @coocood

zz-jason · 2018-09-04T03:09:05Z

executor/aggregate.go

@@ -680,35 +675,34 @@ func (e *HashAggExec) execute(ctx context.Context) (err error) {
 	}
 }

-func (e *HashAggExec) getGroupKey(row chunk.Row) ([]byte, error) {
-	vals := make([]types.Datum, 0, len(e.GroupByItems))
+func (e *HashAggExec) getGroupKey(row chunk.Row) (string, error) {


This function can be further optimized. Maybe in the next PR.

#7450 (comment) @zz-jason

we can change the way to calculate the []byte according to the group by items.

this method maybe better return []byte directly and make to string lazy happen, and then modify StringSet add a Exists override method that accept []byte as argument, then use this small trick golang/go@8072f46#diff-590553aade2ea7ef319660a8151c5eb9R581

I feel @coocood want to make StringSet be map[[n]byte]struct if we have some "magic design" make groupKey be fixed length? 🐱

maybe hack.String or hack.Slice is what you want.

zz-jason · 2018-09-04T03:10:51Z

@XuHuaiyu 308.472225189s is too slow, what is the corresponding query?

zz-jason

LGTM

zz-jason · 2018-09-10T03:32:07Z

/run-all-tests

XuHuaiyu · 2018-09-11T06:27:11Z

/run-common-test tidb-test=pr/614
/run-integration-common-test tidb-test=pr/614

lysu · 2018-09-11T08:23:27Z

/run-common-test tidb-test=pr/614
/run-integration-common-test tidb-test=pr/614

lysu · 2018-09-11T08:30:47Z

/run-common-test tidb-test=pr/614

lysu

LGTM

XuHuaiyu added type/performance sig/execution SIG execution labels Aug 29, 2018

winoros reviewed Aug 29, 2018

View reviewed changes

shenli reviewed Aug 29, 2018

View reviewed changes

executor/aggregate.go Outdated

"github.com/spaolacci/murmur3"

"golang.org/x/net/context"

"sort"

Copy link

Member

shenli Aug 29, 2018

Choose a reason for hiding this comment

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

Move this to the first block.

zz-jason reviewed Aug 30, 2018

View reviewed changes

XuHuaiyu added the status/DNM label Aug 31, 2018

XuHuaiyu added 2 commits September 3, 2018 13:01

address comment

87175b2

Merge branch 'master' of https://github.com/pingcap/tidb into tune_agg

6de6fbd

XuHuaiyu force-pushed the tune_agg branch from d2ae447 to 6de6fbd Compare September 3, 2018 05:01

fix ci

de586d0

XuHuaiyu removed the status/DNM label Sep 3, 2018

tiny change

7af81a6

zz-jason reviewed Sep 4, 2018

View reviewed changes

zz-jason added type/enhancement The issue or PR belongs to an enhancement. and removed type/performance labels Sep 5, 2018

zz-jason reviewed Sep 10, 2018

View reviewed changes

zz-jason added status/LGT1 Indicates that a PR has LGTM 1. status/all tests passed labels Sep 10, 2018

Merge branch 'master' into tune_agg

916b9da

lysu approved these changes Sep 11, 2018

View reviewed changes

XuHuaiyu added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 11, 2018

Merge branch 'master' into tune_agg

a40ff1b

zz-jason merged commit 27047a0 into pingcap:master Sep 11, 2018

XuHuaiyu deleted the tune_agg branch September 12, 2018 06:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: remove *MVMap from agg executor #7541

executor: remove *MVMap from agg executor #7541

XuHuaiyu commented Aug 29, 2018 •

edited

Loading

winoros Aug 29, 2018

shenli Aug 29, 2018

shenli commented Aug 29, 2018

zz-jason Aug 30, 2018

zz-jason Aug 30, 2018

zz-jason Aug 30, 2018

zz-jason Aug 30, 2018

XuHuaiyu commented Aug 31, 2018

XuHuaiyu commented Aug 31, 2018

XuHuaiyu commented Sep 3, 2018

XuHuaiyu commented Sep 3, 2018

shenli commented Sep 3, 2018

coocood commented Sep 3, 2018

XuHuaiyu commented Sep 4, 2018

XuHuaiyu commented Sep 4, 2018

zz-jason Sep 4, 2018

XuHuaiyu Sep 4, 2018

zz-jason Sep 4, 2018

lysu Sep 4, 2018

zz-jason Sep 4, 2018

zz-jason commented Sep 4, 2018

zz-jason left a comment

zz-jason commented Sep 10, 2018

XuHuaiyu commented Sep 11, 2018

lysu commented Sep 11, 2018

lysu commented Sep 11, 2018

lysu left a comment

executor: remove *MVMap from agg executor #7541

executor: remove *MVMap from agg executor #7541

Conversation

XuHuaiyu commented Aug 29, 2018 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenli commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuHuaiyu commented Aug 31, 2018

XuHuaiyu commented Aug 31, 2018

XuHuaiyu commented Sep 3, 2018

XuHuaiyu commented Sep 3, 2018

shenli commented Sep 3, 2018

coocood commented Sep 3, 2018

XuHuaiyu commented Sep 4, 2018

XuHuaiyu commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason commented Sep 4, 2018

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Sep 10, 2018

XuHuaiyu commented Sep 11, 2018

lysu commented Sep 11, 2018

lysu commented Sep 11, 2018

lysu left a comment

Choose a reason for hiding this comment

XuHuaiyu commented Aug 29, 2018 •

edited

Loading