executor: decrease the memory usage of hashTable in HashJoinExec #11832

SunRunAway · 2019-08-22T08:30:42Z

What problem does this PR solve?

part of #11607

What is changed and how it works?

See section 1 of #11607 (comment)

See benchmark at #11832 (comment)

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change

Side effects

Possible performance regression

Related changes

None

XuHuaiyu · 2019-08-22T08:44:31Z

Do we have any memory benchmark for this?

codecov · 2019-08-22T08:47:03Z

Codecov Report

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

@@             Coverage Diff             @@
##             master     #11832   +/-   ##
===========================================
  Coverage   81.4279%   81.4279%           
===========================================
  Files           442        442           
  Lines         95186      95186           
===========================================
  Hits          77508      77508           
  Misses        12216      12216           
  Partials       5462       5462

SunRunAway · 2019-08-22T11:52:55Z

/run-all-tests

SunRunAway · 2019-08-22T13:05:35Z

benchmark result

TLDR

This pr has little improvement with time and great improvement with memory if join keys are larger enough, while has a little performance regression if join keys are simple.

go test -run=XX -bench="BenchmarkBuildHashTableForList|BenchmarkHashJoinExec" -test.benchmem -count 10
&&
benchstat /tmp/old.txt /tmp/new.txt

name                                                                     old time/op    new time/op    delta
HashJoinExec/(rows:100000,_concurency:4,_joinKeyIdx:_[0_1])-12              917ms ± 3%     735ms ± 3%  -19.81%  (p=0.000 n=8+10)
HashJoinExec/(rows:100000,_concurency:4,_joinKeyIdx:_[0])-12                168ms ± 2%     163ms ± 1%   -3.05%  (p=0.000 n=10+10)
BuildHashTableForList/(rows:100000,_concurency:4,_joinKeyIdx:_[0_1])-12     664ms ±11%     519ms ± 2%  -21.78%  (p=0.000 n=10+10)
BuildHashTableForList/(rows:100000,_concurency:4,_joinKeyIdx:_[0])-12      15.5ms ±12%    14.0ms ±12%   -9.64%  (p=0.009 n=10+10)

name                                                                     old alloc/op   new alloc/op   delta
HashJoinExec/(rows:100000,_concurency:4,_joinKeyIdx:_[0_1])-12              853MB ± 0%     305MB ± 0%  -64.24%  (p=0.000 n=10+10)
HashJoinExec/(rows:100000,_concurency:4,_joinKeyIdx:_[0])-12                307MB ± 0%     305MB ± 0%   -0.56%  (p=0.000 n=9+10)
BuildHashTableForList/(rows:100000,_concurency:4,_joinKeyIdx:_[0_1])-12     556MB ± 0%       8MB ± 0%  -98.57%  (p=0.000 n=10+10)
BuildHashTableForList/(rows:100000,_concurency:4,_joinKeyIdx:_[0])-12      10.4MB ± 0%     8.0MB ± 0%  -23.63%  (p=0.000 n=10+10)

name                                                                     old allocs/op  new allocs/op  delta
HashJoinExec/(rows:100000,_concurency:4,_joinKeyIdx:_[0_1])-12               215k ± 0%      306k ± 0%  +42.68%  (p=0.000 n=8+9)
HashJoinExec/(rows:100000,_concurency:4,_joinKeyIdx:_[0])-12                 206k ± 0%      306k ± 0%  +48.42%  (p=0.000 n=10+10)
BuildHashTableForList/(rows:100000,_concurency:4,_joinKeyIdx:_[0_1])-12     12.4k ± 0%      4.0k ± 1%  -67.38%  (p=0.000 n=10+10)
BuildHashTableForList/(rows:100000,_concurency:4,_joinKeyIdx:_[0])-12       4.08k ± 0%     4.04k ± 0%   -0.92%  (p=0.000 n=9+10)

old.txt
new.txt

SunRunAway · 2019-08-22T14:08:49Z

/run-all-tests

SunRunAway · 2019-08-22T15:49:19Z

/run-all-tests

XuHuaiyu · 2019-08-23T03:15:27Z

executor/index_lookup_join.go
102:    lookupMap         *mvmap.MVMap
368:            lookupMap:         mvmap.NewMVMap(),

expression/aggregation/util.go
26:     existingKeys *mvmap.MVMap
35:             existingKeys: mvmap.NewMVMap(),

It seems that these two usages can be optimized, too. Maybe we can create an issue for it?

SunRunAway · 2019-08-23T03:18:13Z

@XuHuaiyu
Yes, these are the other tasks of issue #11607

qw4990 · 2019-08-23T05:59:13Z

executor/join.go

@@ -50,7 +50,7 @@ type HashJoinExec struct {

 	// concurrency is the number of partition, build and join workers.
 	concurrency     uint
-	hashTable       *mvmap.MVMap
+	hashTable       rowHashTable
 	innerFinished   chan error
 	hashJoinBuffers []*hashJoinBuffer


It seems that this field can be removed.

XuHuaiyu · 2019-08-23T05:57:55Z

executor/join.go

+}
+
+func (e *HashJoinExec) matchJoinKey(inner, outer chunk.Row) bool {
+	innerAllTypes := retTypes(e.innerExec)


For my part, innerAllTypes, outerAllTypes := xxx, innerIdx, outerIdx := xxx, innerTp, outerTp := xxx are more clearer?

XuHuaiyu · 2019-08-23T06:02:10Z

util/chunk/column.go

+		elemLen := len(c.elemBuf)
+		data = c.data[rowID*elemLen : rowID*elemLen+elemLen]
+	} else {
+		start, end := c.offsets[rowID], c.offsets[rowID]


Does it seem to be wrong that start == end, any test case covered this?

The benchmark was fake because of this bug. 😢
I'll update a new one.

qw4990 · 2019-08-23T06:29:34Z

util/codec/codec.go

-					return nil, errors.Trace(err)
+			flag := varintFlag
+			if mysql.HasUnsignedFlag(allTypes[i].Flag) {
+				if integer := row.GetInt64(i); integer < 0 {


Suggested change

if integer := row.GetInt64(i); integer < 0 {

if row.GetInt64(i) < 0 {

qw4990 · 2019-08-23T06:57:29Z

util/codec/codec.go

 		default:
-			return nil, errors.Errorf("unsupport column type for encode %d", allTypes[i].Tp)
+			_, err = row.WriteTo(i, w)


It's dangerous to skip the type interpretation and hash the raw data directly in the table building stage.
For example:

create table t1(x float); create table t2(x double); insert into t1 values (1); insert into t2 values (1); select * from t1, t2 where t1.x=t2.x;

An empty set is returned in this PR since it doesn't take data types into account when building the inner hash table.

Since that, I've no good solution except changing all encodeXXX function along with an io.Writer argument to avoid extra memory copy.
I'll revert this part code for now and try to optimize it in further PR.
Any suggestion is very welcome, @qw4990 @zz-jason @XuHuaiyu

SunRunAway · 2019-08-23T14:52:06Z

/run-all-tests

SunRunAway · 2019-08-23T14:52:55Z

@XuHuaiyu @qw4990
All comments are addressed and the benchmark is updated, PTAL again, thanks.

sre-bot · 2019-08-28T13:51:07Z

/run-all-tests

SunRunAway · 2019-08-28T14:05:45Z

@XuHuaiyu got two LGTMs, PTAL again~

XuHuaiyu · 2019-08-29T06:07:32Z

executor/hash_table.go

+}
+
+// Put puts the key/rowPtr pairs to the rowHashMap, multiple rowPtrs are stored in a list.
+func (m *rowHashMap) Put(hashKey uint64, rowPtr chunk.RowPtr) {


If the origin rows are all stored in a Chunk but not a List, we need to pass chunk.RowPtr{0, rowIdx} into this function?

SunRunAway · 2019-08-29T06:13:54Z

At least for now, but we can bench and optimize it later.

…

On Aug 29, 2019, at 2:08 PM, HuaiyuXu ***@***.***> wrote: @XuHuaiyu commented on this pull request. In executor/hash_table.go <#11832 (comment)>: > + length int +} + +// newRowHashMap creates a new rowHashMap. +func newRowHashMap() *rowHashMap { + m := new(rowHashMap) + // TODO(fengliyuan): initialize the size of map from the estimated row count for better performance. + m.hashTable = make(map[uint64]entryAddr) + m.entryStore.slices = [][]entry{make([]entry, 0, 64)} + // Reserve the first empty entry, so entryAddr{} can represent nullEntryAddr. + m.entryStore.put(entry{}) + return m +} + +// Put puts the key/rowPtr pairs to the rowHashMap, multiple rowPtrs are stored in a list. +func (m *rowHashMap) Put(hashKey uint64, rowPtr chunk.RowPtr) { If the origin rows are all stored in a Chunk but not a List, we need to pass chunk.RowPtr{0, rowIdx} into this function? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#11832?email_source=notifications&email_token=AAGTYNNX6DGJDQTERXDI4ETQG5RXDA5CNFSM4IOSJJNKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCDBVAQQ#pullrequestreview-281235522>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGTYNPU6HUPAERWWLDFAODQG5RXDANCNFSM4IOSJJNA>.

XuHuaiyu

LGTM

sre-bot · 2019-08-29T06:33:37Z

/run-all-tests

sre-bot · 2019-08-29T06:36:35Z

@SunRunAway merge failed.

SunRunAway · 2019-08-29T06:41:40Z

/run-common-test

SunRunAway · 2019-08-29T10:04:55Z

/bench

SunRunAway added sig/execution SIG execution type/enhancement type/new-feature labels Aug 22, 2019

SunRunAway mentioned this pull request Aug 22, 2019

Consider using a disk-based hash table for hash join avoiding OOM #11607

Open

14 tasks

qw4990 self-requested a review August 22, 2019 09:16

SunRunAway requested review from XuHuaiyu and zz-jason August 22, 2019 14:10

SunRunAway added the status/PTAL label Aug 22, 2019

SunRunAway force-pushed the disk-join-issue11607 branch from 9f43098 to 56f17e6 Compare August 22, 2019 15:42

SunRunAway added the status/all tests passed label Aug 22, 2019

XuHuaiyu requested a review from wshwsh12 August 23, 2019 02:49

qw4990 reviewed Aug 23, 2019

View reviewed changes

XuHuaiyu reviewed Aug 23, 2019

View reviewed changes

qw4990 reviewed Aug 23, 2019

View reviewed changes

SunRunAway force-pushed the disk-join-issue11607 branch 3 times, most recently from 4891038 to afe7881 Compare August 23, 2019 14:43

SunRunAway requested review from XuHuaiyu and qw4990 August 23, 2019 14:53

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/can-merge Indicates a PR has been approved by a committer. labels Aug 28, 2019

SunRunAway added 11 commits August 28, 2019 22:00

executor: decrease the memory usage of hashTable in HashJoinExec

c895108

benchmark

bcdb719

fix licence

60ccc83

address comments

04526e2

rewrite encodeHashChunk & hashTable

09a53e4

fixup

a54f539

fixup

2da2ebb

add more tests

60f3387

add comment for rowHashMap

aabe472

address commment

be51694

fix unit test in master branch

134780a

SunRunAway dismissed qw4990’s stale review via 134780a August 28, 2019 14:03

SunRunAway force-pushed the disk-join-issue11607 branch from 4386e72 to 134780a Compare August 28, 2019 14:03

XuHuaiyu reviewed Aug 29, 2019

View reviewed changes

XuHuaiyu approved these changes Aug 29, 2019

View reviewed changes

XuHuaiyu added the status/can-merge Indicates a PR has been approved by a committer. label Aug 29, 2019

Merge branch 'master' into disk-join-issue11607

538860d

SunRunAway merged commit bdbaeb4 into pingcap:master Aug 29, 2019

SunRunAway deleted the disk-join-issue11607 branch August 29, 2019 06:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: decrease the memory usage of hashTable in HashJoinExec #11832

executor: decrease the memory usage of hashTable in HashJoinExec #11832

SunRunAway commented Aug 22, 2019 •

edited

XuHuaiyu commented Aug 22, 2019

codecov bot commented Aug 22, 2019 •

edited

SunRunAway commented Aug 22, 2019

SunRunAway commented Aug 22, 2019 •

edited

SunRunAway commented Aug 22, 2019

SunRunAway commented Aug 22, 2019

XuHuaiyu commented Aug 23, 2019

SunRunAway commented Aug 23, 2019

qw4990 Aug 23, 2019

XuHuaiyu Aug 23, 2019

XuHuaiyu Aug 23, 2019

SunRunAway Aug 23, 2019

qw4990 Aug 23, 2019

qw4990 Aug 23, 2019 •

edited

SunRunAway Aug 23, 2019 •

edited

SunRunAway commented Aug 23, 2019

SunRunAway commented Aug 23, 2019 •

edited

sre-bot commented Aug 28, 2019

SunRunAway commented Aug 28, 2019

XuHuaiyu Aug 29, 2019

SunRunAway commented Aug 29, 2019 via email

XuHuaiyu left a comment

sre-bot commented Aug 29, 2019

sre-bot commented Aug 29, 2019

SunRunAway commented Aug 29, 2019

SunRunAway commented Aug 29, 2019

	if integer := row.GetInt64(i); integer < 0 {
	if row.GetInt64(i) < 0 {

executor: decrease the memory usage of hashTable in HashJoinExec #11832

executor: decrease the memory usage of hashTable in HashJoinExec #11832

Conversation

SunRunAway commented Aug 22, 2019 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

XuHuaiyu commented Aug 22, 2019

codecov bot commented Aug 22, 2019 • edited

Codecov Report

SunRunAway commented Aug 22, 2019

SunRunAway commented Aug 22, 2019 • edited

benchmark result

TLDR

SunRunAway commented Aug 22, 2019

SunRunAway commented Aug 22, 2019

XuHuaiyu commented Aug 23, 2019

SunRunAway commented Aug 23, 2019

qw4990 Aug 23, 2019

Choose a reason for hiding this comment

XuHuaiyu Aug 23, 2019

Choose a reason for hiding this comment

XuHuaiyu Aug 23, 2019

Choose a reason for hiding this comment

SunRunAway Aug 23, 2019

Choose a reason for hiding this comment

qw4990 Aug 23, 2019

Choose a reason for hiding this comment

qw4990 Aug 23, 2019 • edited

Choose a reason for hiding this comment

SunRunAway Aug 23, 2019 • edited

Choose a reason for hiding this comment

SunRunAway commented Aug 23, 2019

SunRunAway commented Aug 23, 2019 • edited

sre-bot commented Aug 28, 2019

SunRunAway commented Aug 28, 2019

XuHuaiyu Aug 29, 2019

Choose a reason for hiding this comment

SunRunAway commented Aug 29, 2019 via email

XuHuaiyu left a comment

Choose a reason for hiding this comment

sre-bot commented Aug 29, 2019

sre-bot commented Aug 29, 2019

SunRunAway commented Aug 29, 2019

SunRunAway commented Aug 29, 2019

SunRunAway commented Aug 22, 2019 •

edited

codecov bot commented Aug 22, 2019 •

edited

SunRunAway commented Aug 22, 2019 •

edited

qw4990 Aug 23, 2019 •

edited

SunRunAway Aug 23, 2019 •

edited

SunRunAway commented Aug 23, 2019 •

edited