executor/join : use shallow copy for join. #7433

crazycs520 · 2018-08-19T15:22:42Z

What problem does this PR solve?

This is for #7420

In joiner.go, function tryToMatch is act like below:

j.chk.Reset()			
if j.outerIsRight {	
    j.makeJoinRowToChunk(j.chk, inner, outer)	
} else {	
    j.makeJoinRowToChunk(j.chk, outer, inner)	
}	
matched, err = expression.EvalBool(j.ctx, j.conditions, j.chk.GetRow(0))
...

makeJoinRowToChunk will copy inner and outer row to temporary chunk（ j.chk ）.

There's no need to do deep copy, use shadow copy will be ok, Specifically, chunk.column.data use shadow copy will be ok.

Another thing worth to say is that function call overhead in makeJoinRowToChunk is very expensive，Maybe more expensive than the actual memory cory. such as column.isNull , column.appendNullBitmap. see here pprof svg:fieldbyfield.pdf

This PR will use shadow copy for chunk.column.data, and remove column.appendNullBitmap funcation call.

Below is a benchmarck:

~/code/goread/src/github.com/pingcap/tidb/util/chunk (column-copy ✔) ᐅ go test -bench="BenchmarkCopy*" -benchtime="3s" -count=3 -run=xx
goos: darwin
goarch: amd64
pkg: github.com/pingcap/tidb/util/chunk
BenchmarkCopyFieldByField-8        50000            101054 ns/op               4 B/op          0 allocs/op
BenchmarkCopyFieldByField-8        50000            102616 ns/op               4 B/op          0 allocs/op
BenchmarkCopyFieldByField-8        50000             99987 ns/op               4 B/op          0 allocs/op
BenchmarkCopyShadow-8             100000             48771 ns/op               0 B/op          0 allocs/op
BenchmarkCopyShadow-8             100000             47126 ns/op               0 B/op          0 allocs/op
BenchmarkCopyShadow-8             100000             49391 ns/op               0 B/op          0 allocs/op
PASS
ok      github.com/pingcap/tidb/util/chunk      34.265s

create table t1 (id int, name varchar(30),addr varchar(30), course varchar(30))
create table t2 (id int, name varchar(30),addr varchar(30), course varchar(30))

# both t1, t2 data as below:
+------+----------------+-------------------+------------------+
| id   | name           | addr              | course           |
+------+----------------+-------------------+------------------+
| 0    | name_abcd_0    | address_abcd_0    | course_abcd_0    |
| 1    | name_abcd_1    | address_abcd_1    | course_abcd_1    |
| 2    | name_abcd_2    | address_abcd_2    | course_abcd_2    |
| 3    | name_abcd_3    | address_abcd_3    | course_abcd_3    |
...
10000 rows

# new
mysql root@127.0.0.1:test> select count(*) from t1,t2 where t1.id != t2.id and t1.name != t2.name and t1.addr != t2.addr and t1.course != t2.course and t1.addr > t2.course;
+----------+
| count(*) |
+----------+
| 0        |
+----------+
1 row in set
Time: 7.94s
# remove t1.addr > t2.course condition always false.
mysql root@127.0.0.1:test> select count(*) from t1,t2 where t1.id != t2.id and t1.name != t2.name and t1.addr != t2.addr and t1.course != t2.course; 
+----------+
| count(*) |
+----------+
| 99990000 |
+----------+
1 row in set
Time: 16.76s

#---------------------------------------------------------------------------------
# old
mysql root@127.0.0.1:test> select count(*) from t1,t2 where t1.id != t2.id and t1.name != t2.name and t1.addr != t2.addr and t1.course != t2.course and t1.addr > t2.course;
+----------+
| count(*) |
+----------+
| 0        |
+----------+
1 row in set
Time: 16.13s
# remove t1.addr > t2.course condition always false.
mysql root@127.0.0.1:test> select count(*) from t1,t2 where t1.id != t2.id and t1.name != t2.name and t1.addr != t2.addr and t1.course != t2.course; 
+----------+
| count(*) |
+----------+
| 99990000 |
+----------+
1 row in set
Time: 18.46s

	count(*) = 0	count(*) = 99990000
Time of new	7.94s	16.76s
Time of old	16.13s	18.46s

As you can see, if have t1.addr > t2.coursecondition always false, the result of count(*) = 0, temporary deep copy in old is spend more time.

After remove t1.addr > t2.coursecondition , the result of new is still have a little advantage than old.

create table t1 (id int);
create table t2 (id int);
# both t1, t2 data as below:
+-------+
| id    |
+-------+
| 0     |
| 1     |
| 2     |
| ...   |
| ...   |
| ...   |
| 10000 |
+-------+

# new
mysql root@127.0.0.1:test> select count(*) from t1 inner join t2 where t1.id != t2.id;
+----------+
| count(*) |
+----------+
| 99990000 |
+----------+
1 row in set
Time: 8.18s
#----------------------------------------------------------------------------------
# old
mysql root@127.0.0.1:test> select count(*) from t1 inner join t2 where t1.id != t2.id;
+----------+
| count(*) |
+----------+
| 99990000 |
+----------+
1 row in set
Time: 7.74s

	count(*) = 99990000
Time of new	8.18s
Time of old	7.74s

If both join column is int , shadow copy will have no advantage, and maybe slow than old version.

winkyao · 2018-08-19T16:08:09Z

@crazycs520 Please rebase your commit first, then push it to github.

crazycs520 · 2018-08-19T16:47:29Z

below are pprof svg of columnbycolumn and fieldbyfield.
columnbycolumn.pdf
fieldbyfield.pdf

zz-jason · 2018-08-19T17:10:35Z

Seems like the improvement from the cache locality is counteracted by the overhead of some extra cpu instructions.

crazycs520 · 2018-08-19T17:10:57Z

At present, Most of time is consume by the Iterator interface and another function call.
Maybe get the chunk from Iterator then copy between chunk is better.

Another idea is don't copy, store indexSlice of column in chunk.
When do join, Combine the two columns in inner and outer directly and rewrite indexSlice to control chunk.GetRow..... This idea needs to be considered carefully.

zhexuany · 2018-08-20T04:01:34Z

util/chunk/chunk.go

@@ -160,6 +160,77 @@ func (c *Chunk) AppendPartialRow(colIdx int, row Row) {
 }
 }

+func (c *Chunk) AppendPartialRows(colIdx int, rowIt Iterator, maxLen int) int {


For the exported method, you must comment. It is not optional.

zhexuany · 2018-08-20T04:01:45Z

util/chunk/chunk.go

+ return c.columns[colIdx+0].length - oldRowLen
+}
+
+func (c *Chunk) AppendPartialSameRows(colIdx int, row Row, rowsLen int) {


same reason.

zhexuany · 2018-08-20T04:02:14Z

util/chunk/iterator.go

@@ -32,6 +32,8 @@ type Iterator interface {
 // Next returns the next Row.
 Next() Row

+ PreRows(i int)


What is the use case for this method?

zhexuany · 2018-08-20T04:02:51Z

util/chunk/iterator.go

@@ -75,6 +77,11 @@ func (it *iterator4Slice) Next() Row {
 return row
 }

+// PreRows implements the Iterator interface.


This comment basically says nothing. You have to be more explicitly.

…copy

iamxy · 2018-08-20T08:17:17Z

/rebuild

iamxy · 2018-08-20T08:58:30Z

/run-all-tests

lysu · 2018-08-20T11:10:15Z

/run-all-tests

XuHuaiyu

change all the shadow to shallow

XuHuaiyu · 2018-08-27T02:42:54Z

executor/joiner.go

@@ -125,6 +126,7 @@ type baseJoiner struct {
 defaultInner chunk.Row
 outerIsRight bool
 chk *chunk.Chunk


Can this be removed?

No. Inner join, left out join and right out join will use chk to do deep copy. deep copy + vectorize filter + batch copy have better performance.

XuHuaiyu · 2018-08-27T02:46:54Z

executor/joiner.go

@@ -142,6 +144,16 @@ func (j *baseJoiner) makeJoinRowToChunk(chk *chunk.Chunk, lhs, rhs chunk.Row) {
 chk.AppendPartialRow(lhs.Len(), rhs)
 }

+// makeJoinRow combines inner, outer row into shadowRow.
+// combines will uses shadow copy inner and outer row data to shadowRow.


makeJoinRow shallow copies `inner` and `outer` into `shallowRow`.

XuHuaiyu

change all the shadow to shallow
rest LGTM

crazycs520 · 2018-08-27T07:21:28Z

@zz-jason PTAL

XuHuaiyu

LGTM

zz-jason · 2018-08-27T07:23:27Z

util/chunk/mutrow.go

+ for i, rowCol := range row.c.columns {
+ chkCol := chk.columns[colIdx+i]
+ if !rowCol.isNull(row.idx) {
+ chkCol.nullBitmap[0] = 1


It's better to add some comments about why we set the whole byte to 1/0

zz-jason · 2018-08-27T07:23:48Z

util/chunk/mutrow.go

+// ShallowCopyPartialRow shallow copies the data of `row` to MutRow.
+func (mr MutRow) ShallowCopyPartialRow(colIdx int, row Row) {
+ chk := mr.c
+ for i, rowCol := range row.c.columns {


how about:
s/rowCol/srcCol/
s/chkCol/dstCol/

zz-jason · 2018-08-27T07:25:58Z

util/chunk/mutrow.go

+
+// ShallowCopyPartialRow shallow copies the data of `row` to MutRow.
+func (mr MutRow) ShallowCopyPartialRow(colIdx int, row Row) {
+ chk := mr.c


chk is only used once, I think we can remove this variable and use mr.c directly.

zz-jason · 2018-08-27T07:28:37Z

executor/joiner.go

@@ -142,6 +149,15 @@ func (j *baseJoiner) makeJoinRowToChunk(chk *chunk.Chunk, lhs, rhs chunk.Row) {
 chk.AppendPartialRow(lhs.Len(), rhs)
 }

+// makeJoinRow shallow copies `inner` and `outer` into `shallowRow`.
+func (j *baseJoiner) makeJoinRow(isRightJoin bool, inner, outer chunk.Row) {


how about s/makeJoinRow/makeShallowJoinRow/

winoros · 2018-08-27T07:33:59Z

executor/joiner.go

@@ -102,18 +101,25 @@ func newJoiner(ctx sessionctx.Context, joinType plan.JoinType,
 }
 switch joinType {
 case plan.SemiJoin:
+ base.shallowRow = chunk.MutRowFromTypes(colTypes)


What not change it on line 94 directly? So we don't repeat it multiple times?

only SemiJoin, AntiSemiJoin, LeftOuterSemiJoin, AntiLeftOuterSemiJoin need shallowRow.

…copy

crazycs520 · 2018-08-28T04:21:52Z

/run-all-tests

zz-jason

LGTM

zz-jason · 2018-08-28T05:03:46Z

@XuHuaiyu @winoros PTAL

winkyao

LGTM

…copy

crazycs520 added 7 commits August 19, 2018 15:45

iterator copy init

a576cc0

fix iterator bug

79b74e7

fix nullmap index out of range and add test

d9fe98b

refine code

8fa2ad8

refine code

33ae5a0

remove iterator copy and use back to pre rows

255c64b

checkout joiner.go file

0973373

crazycs520 added the status/WIP label Aug 19, 2018

add check to bench

055a41a

zhexuany reviewed Aug 20, 2018

View reviewed changes

Merge branch 'master' of https://github.com/pingcap/tidb into column-…

c5eeff9

…copy

lysu mentioned this pull request Aug 20, 2018

executor: reduce chunk capacity #7141

Closed

crazycs520 closed this Aug 21, 2018

crazycs520 added 3 commits August 21, 2018 21:03

iterator only once

e7c6c64

add appendMultiSameNullBitmap

ce8eb4f

field by field only one line 2X

499b6f9

crazycs520 reopened this Aug 22, 2018

crazycs520 force-pushed the column-copy branch 2 times, most recently from ab33e54 to 71c54a5 Compare August 22, 2018 06:49

refine column copy

2a295a1

XuHuaiyu reviewed Aug 27, 2018

View reviewed changes

address comment

0aadbf6

crazycs520 force-pushed the column-copy branch from 9102e19 to 0aadbf6 Compare August 27, 2018 06:28

XuHuaiyu reviewed Aug 27, 2018

View reviewed changes

address comment

0de2063

crazycs520 changed the title ~~executor/join : use shadow copy for join.~~ executor/join : use shallow copy for join. Aug 27, 2018

XuHuaiyu reviewed Aug 27, 2018

View reviewed changes

zz-jason reviewed Aug 27, 2018

View reviewed changes

winoros reviewed Aug 27, 2018

View reviewed changes

crazycs520 added 2 commits August 28, 2018 12:20

address comment

c681658

Merge branch 'master' of https://github.com/pingcap/tidb into column-…

f8ccdf2

…copy

zz-jason reviewed Aug 28, 2018

View reviewed changes

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

winkyao previously approved these changes Aug 28, 2018

View reviewed changes

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 29, 2018

crazycs520 added 2 commits August 29, 2018 13:52

Merge branch 'master' of https://github.com/pingcap/tidb into column-…

c7b2301

…copy

update test after merge

1e8a9f0

crazycs520 dismissed winkyao’s stale review via 1e8a9f0 August 29, 2018 05:56

crazycs520 force-pushed the column-copy branch from 749e72b to 1e8a9f0 Compare August 29, 2018 05:56

XuHuaiyu approved these changes Aug 29, 2018

View reviewed changes

Merge branch 'master' into column-copy

b939b3b

crazycs520 merged commit 360567b into pingcap:master Aug 29, 2018

crazycs520 deleted the column-copy branch August 29, 2018 07:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor/join : use shallow copy for join. #7433

executor/join : use shallow copy for join. #7433

crazycs520 commented Aug 19, 2018 •

edited

Loading

winkyao commented Aug 19, 2018

crazycs520 commented Aug 19, 2018 •

edited

Loading

zz-jason commented Aug 19, 2018

crazycs520 commented Aug 19, 2018 •

edited

Loading

zhexuany Aug 20, 2018

zhexuany Aug 20, 2018

zhexuany Aug 20, 2018

zhexuany Aug 20, 2018

iamxy commented Aug 20, 2018

iamxy commented Aug 20, 2018

lysu commented Aug 20, 2018

XuHuaiyu left a comment

XuHuaiyu Aug 27, 2018

crazycs520 Aug 27, 2018

XuHuaiyu Aug 27, 2018

XuHuaiyu left a comment

crazycs520 commented Aug 27, 2018 •

edited

Loading

XuHuaiyu left a comment

zz-jason Aug 27, 2018

zz-jason Aug 27, 2018

zz-jason Aug 27, 2018

zz-jason Aug 27, 2018

crazycs520 Aug 28, 2018

winoros Aug 27, 2018

crazycs520 Aug 28, 2018

crazycs520 commented Aug 28, 2018

zz-jason left a comment

zz-jason commented Aug 28, 2018

winkyao left a comment

executor/join : use shallow copy for join. #7433

executor/join : use shallow copy for join. #7433

Conversation

crazycs520 commented Aug 19, 2018 • edited Loading

What problem does this PR solve?

winkyao commented Aug 19, 2018

crazycs520 commented Aug 19, 2018 • edited Loading

zz-jason commented Aug 19, 2018

crazycs520 commented Aug 19, 2018 • edited Loading

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

iamxy commented Aug 20, 2018

iamxy commented Aug 20, 2018

lysu commented Aug 20, 2018

XuHuaiyu left a 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

XuHuaiyu left a comment

Choose a reason for hiding this comment

crazycs520 commented Aug 27, 2018 • edited Loading

XuHuaiyu left a 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

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

crazycs520 commented Aug 28, 2018

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Aug 28, 2018

winkyao left a comment

Choose a reason for hiding this comment

crazycs520 commented Aug 19, 2018 •

edited

Loading

crazycs520 commented Aug 19, 2018 •

edited

Loading

crazycs520 commented Aug 19, 2018 •

edited

Loading

crazycs520 commented Aug 27, 2018 •

edited

Loading