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

util/chunk: introduce Chunk to replace current datum slice Row. #4856

Merged
merged 9 commits into from Oct 25, 2017

Conversation

Projects
None yet
8 participants
@coocood
Member

coocood commented Oct 21, 2017

Chunk is an efficient data structure to store rows of data.
It will improve both performance and memory usage.

util/chunk: introduce Chunk to replace current datum slice Row.
Chunk is an efficient data structure to store rows of data.
It will improve both performance and memory usage.
@coocood

This comment has been minimized.

Show comment
Hide comment
Member

coocood commented Oct 21, 2017

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli

shenli Oct 21, 2017

Member

Any benchmark result?

Member

shenli commented Oct 21, 2017

Any benchmark result?

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Oct 21, 2017

Member

@shenli
The test contains benchmark code.

Write(Append) is about 7 ns/op, Read is about 2 ns/op.

Member

coocood commented Oct 21, 2017

@shenli
The test contains benchmark code.

Write(Append) is about 7 ns/op, Read is about 2 ns/op.

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Oct 21, 2017

Member

@coocood how about using a column store inside a chunk ?

Member

zz-jason commented Oct 21, 2017

@coocood how about using a column store inside a chunk ?

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Oct 21, 2017

Member

@zz-jason
I've considered column store, it's best for write once, read multiple times scenario.

But the write performance is worse than row store.

In our execution flow, Chunk should be used in each executor, doing reading and writing.

The executor reads a chunk from children, process it, then write data to a new chunk, return to its parent.

So I think row store is a better choice.

Member

coocood commented Oct 21, 2017

@zz-jason
I've considered column store, it's best for write once, read multiple times scenario.

But the write performance is worse than row store.

In our execution flow, Chunk should be used in each executor, doing reading and writing.

The executor reads a chunk from children, process it, then write data to a new chunk, return to its parent.

So I think row store is a better choice.

@ngaut

This comment has been minimized.

Show comment
Hide comment
@ngaut

ngaut Oct 21, 2017

Member

Well done. We can move step by step. For now, chunk is a better choice.

Member

ngaut commented Oct 21, 2017

Well done. We can move step by step. For now, chunk is a better choice.

@iamxy

This comment has been minimized.

Show comment
Hide comment
@iamxy

iamxy Oct 21, 2017

Member

/run-integration-compatibility-test tidb_test=connor/add-compatible-test

Member

iamxy commented Oct 21, 2017

/run-integration-compatibility-test tidb_test=connor/add-compatible-test

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood
Member

coocood commented Oct 22, 2017

@ngaut

This comment has been minimized.

Show comment
Hide comment
@ngaut

ngaut Oct 22, 2017

Member

Cool, Are we using standard apache arrow format? Could you add a reference link to the memory row format of apache arrow?

Member

ngaut commented Oct 22, 2017

Cool, Are we using standard apache arrow format? Could you add a reference link to the memory row format of apache arrow?

return nullByte&(1<<(uint(rowIdx)&7)) == 0
}
func (c *column) setNullBitmap(on bool) {

This comment has been minimized.

@zz-jason

zz-jason Oct 22, 2017

Member

if we let 1 represents NULL, we can:

  • change function signature to func (c *column) setNullBitmap(isNull int) and:
  • remove the branch predicate in line 205~210:
pos := uint(c.length) & 7
c.nullBitmap[idx] |= byte(isNull << pos)
c.nullCount += isNull
@zz-jason

zz-jason Oct 22, 2017

Member

if we let 1 represents NULL, we can:

  • change function signature to func (c *column) setNullBitmap(isNull int) and:
  • remove the branch predicate in line 205~210:
pos := uint(c.length) & 7
c.nullBitmap[idx] |= byte(isNull << pos)
c.nullCount += isNull
func (c *column) isNull(rowIdx int) bool {
nullByte := c.nullBitmap[rowIdx/8]
return nullByte&(1<<(uint(rowIdx)&7)) == 0

This comment has been minimized.

@zz-jason

zz-jason Oct 22, 2017

Member

0 in nullBitmap represents null is a little confusing, how about make 1 represent null ?

@zz-jason

zz-jason Oct 22, 2017

Member

0 in nullBitmap represents null is a little confusing, how about make 1 represent null ?

This comment has been minimized.

@coocood

coocood Oct 22, 2017

Member

I agree that use 1 to represent null is more efficient, but Apache Arrow uses 0 to represent null.

@coocood

coocood Oct 22, 2017

Member

I agree that use 1 to represent null is more efficient, but Apache Arrow uses 0 to represent null.

c.data = append(c.data, buf[:]...)
c.data = append(c.data, name...)
c.finishAppendVar()
}

This comment has been minimized.

@zz-jason

zz-jason Oct 22, 2017

Member

how about move appendDuration and appendMyDecimal together with the fixed length value append functions and move appendNameValue together with the variant length value append functions ?

@zz-jason

zz-jason Oct 22, 2017

Member

how about move appendDuration and appendMyDecimal together with the fixed length value append functions and move appendNameValue together with the variant length value append functions ?

}
func (c *column) isNull(rowIdx int) bool {
nullByte := c.nullBitmap[rowIdx/8]

This comment has been minimized.

@zz-jason

zz-jason Oct 22, 2017

Member

rowIdx >> 3

@zz-jason

zz-jason Oct 22, 2017

Member

rowIdx >> 3

This comment has been minimized.

@coocood

coocood Oct 22, 2017

Member

For efficiency, the compiler does it automatically.
For readability, I think '/ 8' is more readable.

@coocood

coocood Oct 22, 2017

Member

For efficiency, the compiler does it automatically.
For readability, I think '/ 8' is more readable.

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Oct 22, 2017

Member

@ngaut
For each component (bitmap, offsets, data) it's the same as Apache Arrow, but each component has its own buffer instead of sharing a single one, so we can build chunk before we know how many rows going to be.

Member

coocood commented Oct 22, 2017

@ngaut
For each component (bitmap, offsets, data) it's the same as Apache Arrow, but each component has its own buffer instead of sharing a single one, so we can build chunk before we know how many rows going to be.

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood
Member

coocood commented Oct 22, 2017

@zz-jason PTAL

}
sum += v
}
fmt.Println(sum)

This comment has been minimized.

@tiancaiamao

tiancaiamao Oct 24, 2017

Contributor

Debugging?

@tiancaiamao

tiancaiamao Oct 24, 2017

Contributor

Debugging?

This comment has been minimized.

@coocood

coocood Oct 24, 2017

Member

No, this is used to prevent compiler optimization.

@coocood

coocood Oct 24, 2017

Member

No, this is used to prevent compiler optimization.

@@ -159,6 +159,9 @@ func digitsToWords(digits int) int {
return (digits + digitsPerWord - 1) / digitsPerWord
}
// MyDecimalStructSize is the struct size of MyDecimal.
const MyDecimalStructSize = 40

This comment has been minimized.

@tiancaiamao

tiancaiamao Oct 24, 2017

Contributor

unsafe.Sizeof(MyDecimal{}) ?

@tiancaiamao

tiancaiamao Oct 24, 2017

Contributor

unsafe.Sizeof(MyDecimal{}) ?

This comment has been minimized.

@coocood

coocood Oct 24, 2017

Member

It will be more clear how large MyDecimal is, and we should not change the struct of the MyDecimal.

@coocood

coocood Oct 24, 2017

Member

It will be more clear how large MyDecimal is, and we should not change the struct of the MyDecimal.

@coocood coocood added the status/LGT1 label Oct 24, 2017

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Oct 25, 2017

Contributor

LGTM

Contributor

tiancaiamao commented Oct 25, 2017

LGTM

@tiancaiamao tiancaiamao added status/LGT2 and removed status/LGT1 labels Oct 25, 2017

@hanfei1991 hanfei1991 merged commit 5ada5ab into pingcap:master Oct 25, 2017

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@coocood coocood deleted the coocood:chunk branch Oct 25, 2017

dbjoa added a commit to cloud-pi/tidb that referenced this pull request Oct 25, 2017

@ddorian

This comment has been minimized.

Show comment
Hide comment
@ddorian

ddorian May 1, 2018

I'm trying to understand a little the chunk from a high level overview. Looks like each row is stored as 1 key-value in tikv, but when a rocksdb block is cached in memory it's converted to small column store (the chunk/block only) right ?
So on disk each row is a tuple while on ram each block is a small column store ?
Or chunk used only for memstore ?

ddorian commented May 1, 2018

I'm trying to understand a little the chunk from a high level overview. Looks like each row is stored as 1 key-value in tikv, but when a rocksdb block is cached in memory it's converted to small column store (the chunk/block only) right ?
So on disk each row is a tuple while on ram each block is a small column store ?
Or chunk used only for memstore ?

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao May 2, 2018

Contributor

So on disk each row is a tuple while on ram each block is a small column store ?

Exactly. @ddorian

Contributor

tiancaiamao commented May 2, 2018

So on disk each row is a tuple while on ram each block is a small column store ?

Exactly. @ddorian

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment