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

executor: reorg codes for hashtable in HashJoinExec #11937

Merged
merged 17 commits into from Sep 3, 2019

Conversation

SunRunAway
Copy link
Contributor

@SunRunAway SunRunAway commented Aug 29, 2019

What problem does this PR solve?

Part of #11607
It reorgs codes for the hash table and introduces hashRowContainer. So that In further pull request, I can implement spillOutToDisk feature for hashRowContainer

What is changed and how it works?

  • reorg codes for hash table in HashJoinExec
  • make a suitable maxEntrySliceLen, it's currently too large(8k) for OLTP queries.
  • add unit test for rowHashMap.
  • try to replace FNV64 to another hash function. (abandon, FNV64 is the fastest.)
  • initialize the size of rowHashMap from the estimated row count for better performance.

Check List

Tests

  • Unit test

Code changes

  • None

Side effects

  • None

Related changes

  • None

Release note

  • None

Benchmark

name                                                                     old time/op    new time/op    delta
HashJoinExec/(rows:100000,_concurency:4,_joinKeyIdx:_[0_1])-12              760ms ± 6%     781ms ± 7%     ~     (p=0.421 n=5+5)
HashJoinExec/(rows:100000,_concurency:4,_joinKeyIdx:_[0])-12                168ms ± 5%     166ms ± 2%     ~     (p=0.841 n=5+5)
BuildHashTableForList/(rows:100000,_concurency:4,_joinKeyIdx:_[0_1])-12     512ms ± 0%     544ms ± 1%   +6.19%  (p=0.016 n=4+5)
BuildHashTableForList/(rows:100000,_concurency:4,_joinKeyIdx:_[0])-12      12.2ms ± 2%    12.5ms ± 8%     ~     (p=0.548 n=5+5)
BuildHashTableForList/(rows:10,_concurency:4,_joinKeyIdx:_[0])-12          2.46µs ± 2%    2.94µs ± 0%  +19.46%  (p=0.008 n=5+5)

name                                                                     old alloc/op   new alloc/op   delta
HashJoinExec/(rows:100000,_concurency:4,_joinKeyIdx:_[0_1])-12              305MB ± 0%     304MB ± 0%   -0.28%  (p=0.008 n=5+5)
HashJoinExec/(rows:100000,_concurency:4,_joinKeyIdx:_[0])-12                305MB ± 0%     304MB ± 0%   -0.28%  (p=0.008 n=5+5)
BuildHashTableForList/(rows:100000,_concurency:4,_joinKeyIdx:_[0_1])-12    7.97MB ± 0%    7.11MB ± 0%  -10.80%  (p=0.008 n=5+5)
BuildHashTableForList/(rows:100000,_concurency:4,_joinKeyIdx:_[0])-12      7.97MB ± 0%    7.11MB ± 0%  -10.80%  (p=0.008 n=5+5)
BuildHashTableForList/(rows:10,_concurency:4,_joinKeyIdx:_[0])-12          1.62kB ± 0%    1.96kB ± 0%  +20.74%  (p=0.008 n=5+5)

name                                                                     old allocs/op  new allocs/op  delta
HashJoinExec/(rows:100000,_concurency:4,_joinKeyIdx:_[0_1])-12               306k ± 0%      306k ± 0%   -0.08%  (p=0.008 n=5+5)
HashJoinExec/(rows:100000,_concurency:4,_joinKeyIdx:_[0])-12                 306k ± 0%      306k ± 0%   -0.09%  (p=0.008 n=5+5)
BuildHashTableForList/(rows:100000,_concurency:4,_joinKeyIdx:_[0_1])-12     4.07k ± 2%     3.80k ± 1%   -6.56%  (p=0.008 n=5+5)
BuildHashTableForList/(rows:100000,_concurency:4,_joinKeyIdx:_[0])-12       4.04k ± 0%     3.79k ± 0%   -6.41%  (p=0.008 n=5+5)
BuildHashTableForList/(rows:10,_concurency:4,_joinKeyIdx:_[0])-12            9.00 ± 0%     14.00 ± 0%  +55.56%  (p=0.008 n=5+5)

old.txt
new.txt

@SunRunAway
Copy link
Contributor Author

/run-all-tests

@codecov
Copy link

codecov bot commented Aug 29, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #11937   +/-   ##
===========================================
  Coverage   81.4498%   81.4498%           
===========================================
  Files           444        444           
  Lines         95622      95622           
===========================================
  Hits          77884      77884           
  Misses        12238      12238           
  Partials       5500       5500

@SunRunAway
Copy link
Contributor Author

/run-all-tests

type rowHashMap struct {
entryStore entryStore
hashTable map[uint64]entryAddr
length int
}

// newRowHashMap creates a new rowHashMap.
func newRowHashMap() *rowHashMap {
func newRowHashMapWithStatCount(statCount int) *rowHashMap {
Copy link
Contributor

Choose a reason for hiding this comment

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

statCount -> initCap?

Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

The current rowContainer suppose that we always use the inner table to build hash table and use outer row to probe.
Should we also handle the case that using the outer table to build the hash table?

executor/join.go Outdated

h := fnv.New64()
chkIdx := uint32(0)
statCount := int(e.innerStatsCount / statCountDivisor)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put line 518~524 into newHashRowContainer?

if err != nil {
return errors.Trace(err)
}
if hasNull {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we record the rowIdx of which hasNull is true?
If we use the outer table to build hashtable, we need to know the null-value lines.

Copy link
Contributor Author

@SunRunAway SunRunAway Sep 2, 2019

Choose a reason for hiding this comment

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

I'll adjust it in further PR to involve hashRowContainer into index join.

// in multiple goroutines while each goroutine should keep its own
// h and buf.
func (c *hashRowContainer) GetMatchedRows(probeRow chunk.Row, joinKeysTypes []*types.FieldType, keyColIdx []int, h hash.Hash64, buf []byte) (matched []chunk.Row, hasNull bool, err error) {

Copy link
Contributor

Choose a reason for hiding this comment

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

useless line

}
innerPtrs := c.hashTable.Get(key)
if len(innerPtrs) == 0 {
hasNull = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we set hashNull to true?

Copy link
Contributor Author

@SunRunAway SunRunAway Sep 2, 2019

Choose a reason for hiding this comment

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

It means no inner rows matched by the outer row in hashRowContainer so we need call onMissMatch.
The same logic in old code is at https://github.com/pingcap/tidb/blob/304619a/executor/join.go#L417 and https://github.com/pingcap/tidb/blob/304619a/executor/join.go#L434

Copy link
Contributor Author

@SunRunAway SunRunAway Sep 2, 2019

Choose a reason for hiding this comment

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

I've addressed this by removing the ambiguous isNull return value, and use the size of matched rows to determine whether to call onMissMatch.

matched = append(matched, matchedRow)
}
if len(matched) == 0 { // TODO(fengliyuan): add test case
hasNull = true
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@SunRunAway
Copy link
Contributor Author

/run-all-tests

@SunRunAway
Copy link
Contributor Author

All comments are addressed.
PTAL @zz-jason, @qw4990, @XuHuaiyu

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 2, 2019

// matchJoinKey checks if join keys of buildRow and probeRow are logically equal.
func (c *hashRowContainer) matchJoinKey(buildRow, probeRow chunk.Row, probeHCtx *hashContext) (ok bool, err error) {

Copy link
Contributor

Choose a reason for hiding this comment

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

This line is useless.

)

const (
// estCountMaxFactor defines the factor of maxStatCount with maxChunkSize.
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. What does maxStatCount mean?
  2. What does estCountMax mean?

// Set this threshold to prevent innerEstCount being too large and causing a performance regression.
estCountMaxFactor = 10 * 1024

// estCountMinFactor defines the factor of statCountMin with maxChunkSize.
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

// Set this threshold to prevent innerEstCount being too large and causing a performance and memory regression.
estCountMaxFactor = 10 * 1024

// estCountMinFactor defines the factor of statCountMin with maxChunkSize.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ statCountMin/ estCountMin ?

Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@XuHuaiyu XuHuaiyu added the status/can-merge Indicates a PR has been approved by a committer. label Sep 3, 2019
@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. status/PTAL labels Sep 3, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 3, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Sep 3, 2019

/run-all-tests

@XuHuaiyu XuHuaiyu merged commit 1ff620d into pingcap:master Sep 3, 2019
@SunRunAway SunRunAway deleted the disk-join-issue11607-reorg branch September 3, 2019 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants