chunk: support capacity grow #7473

lysu · 2018-08-23T14:32:07Z

What problem does this PR solve?

This is first reviewable part of #7141, in first step, we make chunk be growable, then we can review for each operator's chunk modifition carefully.

What is changed and how it works?

add New() Renew() and GrowReset() method to chunk
add eraseGrow() in column
adaptor NewChunkWithCapacity() and Reset() works in chunk.

Check List

Tests

Benchmark Test

goos: linux
goarch: amd64
pkg: github.com/pingcap/tidb/util/chunk
BenchmarkChunkRenewConsumeExec10000000In1024Slice-8   	      10	 136971920 ns/op	83128428 B/op	   58596 allocs/op
BenchmarkChunkRenewConsumeExec10000000In32Slice-8     	       5	 206753238 ns/op	142500092 B/op	 1875000 allocs/op
BenchmarkChunkRenewConsumeExec10000000InGrow-8        	      10	 136599018 ns/op	83129195 B/op	   58598 allocs/op
BenchmarkChunkRestConsumeExec10000000In1024Slice-8    	      10	 113129478 ns/op	       0 B/op	       0 allocs/op
BenchmarkChunkResetConsumeExec10000000In32Slice-8     	      10	 118094010 ns/op	       0 B/op	       0 allocs/op
BenchmarkChunkResetConsumeExec10000000InGrow-8        	      10	 105698574 ns/op	     820 B/op	       1 allocs/op
BenchmarkChunkRenewConsumeExec10In1024Slice-8         	 1000000	      1939 ns/op	    8512 B/op	       6 allocs/op
BenchmarkChunkRenewConsumeExec10In32Slice-8           	 3000000	       422 ns/op	     456 B/op	       6 allocs/op
BenchmarkChunkRenewConsumeExec10InGrow-8              	 5000000	       382 ns/op	     328 B/op	       6 allocs/op
BenchmarkChunkRestConsumeExec10In1024Slice-8          	10000000	       137 ns/op	       0 B/op	       0 allocs/op
BenchmarkChunkResetConsumeExec10In32Slice-8           	10000000	       138 ns/op	       0 B/op	       0 allocs/op
BenchmarkChunkResetConsumeExec10InGrow-8              	10000000	       133 ns/op	       0 B/op	       0 allocs/op
PASS

PR's InGrow solution work "well" in both small result(10 per exec.Next) and big result(10000000) situtation(Renew&Reset).

Code changes

Has exported function/method change

Side effects

No effect for master configuration
But later PR will base on this.

Related changes

Need to be included in the release note

This change is

lysu · 2018-08-23T14:32:39Z

/run-all-tests

XuHuaiyu · 2018-08-24T02:49:13Z

util/chunk/chunk.go

 	})
 }

 // addVarLenColumn adds a variable length column with initial data capacity.
-func (c *Chunk) addVarLenColumn(initCap int) {
+func (c *Chunk) addVarLenColumn(elemLen int8, initCap int, old *column) {
+	guessElemLen := 4


s/ guessElemeLen/ fakeElemLen ?

no it's not fake, it is guess

XuHuaiyu · 2018-08-24T02:49:35Z

util/chunk/chunk.go

 		nullBitmap: make([]byte, 0, initCap>>3),
+		elemLen:    elemLen,


will this be better?

const VarElemLen = -1 elemLen: VarElemLen

XuHuaiyu · 2018-08-24T02:52:26Z

util/chunk/chunk.go

-func (c *Chunk) addVarLenColumn(initCap int) {
+func (c *Chunk) addVarLenColumn(elemLen int8, initCap int, old *column) {
+	guessElemLen := 4
+	if old != nil && old.length != 0 {


Add a comment for this check.

XuHuaiyu · 2018-08-24T02:53:28Z

util/chunk/chunk.go

 	})
 }

 // addColumnByFieldType adds a column by field type.
-func (c *Chunk) addColumnByFieldType(fieldTp *types.FieldType, initCap int) {
-	numFixedBytes := getFixedLen(fieldTp)
+func (c *Chunk) addColumnByFieldType(numFixedBytes int8, initCap int, old *column) {


s/ addColumnByFieldType/ addColumnByElemLen ?

XuHuaiyu · 2018-08-24T02:55:48Z

util/chunk/chunk.go

+	return newChunkByChunk(chk, nextCapacity)
+}
+
+func newChunkByChunk(old *Chunk, cap int) *Chunk {


If this function is only used in Renew,
I think we can put the implementation in Renew.

XuHuaiyu · 2018-08-24T03:34:22Z

util/chunk/chunk.go

+
+// Returns a power of two size for the given target size.
+func powerOf2Capacity(size, capLimit int) int {
+	n := size - 1


I think we'd better extract
line 201~206 as to a util function like NextPowerOfTwo

XuHuaiyu · 2018-08-24T03:35:37Z

util/chunk/chunk.go

+	n |= n >> 2
+	n |= n >> 4
+	n |= n >> 8
+	n |= n >> 16


we should n++ after this?

XuHuaiyu · 2018-08-24T03:36:26Z

util/chunk/chunk.go

+	n |= n >> 8
+	n |= n >> 16
+	if n < 0 {
+		return 1


Should we return capLimit if n <= 0?

XuHuaiyu · 2018-08-24T03:37:25Z

util/chunk/chunk.go

+	if n < 0 {
+		return 1
+	}
+	if n >= capLimit {


It seems this would not happen since currSize < c.capacity. capLimit is always the power of 2?

XuHuaiyu · 2018-08-24T03:38:07Z

util/chunk/chunk.go

+
+// GrowReset resets the chunk and grow capacity if need.
+// TODO: this method will be used in continue PR.
+func (c *Chunk) GrowReset(capLimit int) {


s/ GrowReset/ GrowAndReset?

XuHuaiyu · 2018-08-24T03:49:07Z

util/chunk/chunk.go

+	}
+	for _, col := range c.columns {
+		if needResize {
+			col.eraseGrow(nextCapacity)


Can we use newChunkByChunk here instead of col.eraseGrow?

winoros · 2018-08-24T05:22:09Z

util/chunk/chunk.go

@@ -30,6 +30,10 @@ type Chunk struct {
 	// numVirtualRows indicates the number of virtual rows, which have zero column.
 	// It is used only when this Chunk doesn't hold any data, i.e. "len(columns)==0".
 	numVirtualRows int
+	// capacity indicate the max number of rows that the chunk want to hold.


want -> wants, indicate -> indicates

winoros · 2018-08-24T05:23:31Z

util/chunk/chunk.go

@@ -30,6 +30,10 @@ type Chunk struct {
 	// numVirtualRows indicates the number of virtual rows, which have zero column.
 	// It is used only when this Chunk doesn't hold any data, i.e. "len(columns)==0".
 	numVirtualRows int
+	// capacity indicate the max number of rows that the chunk want to hold.
+	// The accurate row count after executor.Next is guaranteed by executor implement.
+	// It is used only during executor's execution, and be grow by `Renew` or `Reset`.


be grow -> is grown or just grown.

winoros · 2018-08-24T05:24:35Z

util/chunk/chunk.go

+}
+
+// New creates a new chunk.
+//  cap: the max number of rows that the chunk want to hold.


want -> wants

winoros · 2018-08-24T05:26:08Z

util/chunk/chunk.go

@@ -60,31 +94,36 @@ func (c *Chunk) MemoryUsage() (sum int64) {
 }

 // addFixedLenColumn adds a fixed length column with elemLen and initial data capacity.
-func (c *Chunk) addFixedLenColumn(elemLen, initCap int) {
+func (c *Chunk) addFixedLenColumn(elemLen int8, initCap int) {


Do we need to specify it's int8 not int?

coocood · 2018-08-24T07:23:13Z

util/chunk/chunk.go

+	return newChk
+}
+
+func resizeColumns(oldCol []*column, cap int) []*column {


Why not name it renewColumns?
resize implies modification on the old columns.

coocood · 2018-08-24T07:41:22Z

util/misc.go

+const maxCapacity int32 = 1 << 30
+
+// NextPowerOfTwo returns next power of two size for the given target cap.
+func NextPowerOfTwo(cap int32) int32 {


1 << uint(bits.Len64(x)) will do

it seems wrong when x = 2, 4, 8.... o.o

Then what about 1 << uint(bits.Len32(x-1)) ?

coocood · 2018-08-24T07:43:48Z

util/chunk/column.go

@@ -50,6 +50,7 @@ type column struct {
 	offsets    []int32
 	data       []byte
 	elemBuf    []byte
+	elemLen    int8


Why do we need this?
Isn't it can be inferred by elemBuf?

coocood · 2018-08-24T07:57:05Z

util/chunk/chunk.go

+//    and check `chk.NumRows == 0` to stop loop, so for last loop we can use a small chunk to check.
+//    but for Reset() situation in which the chunk keep reusing reallocate a smaller chunk is expensive in most situation.
+//
+func checkCapacity(c *Chunk, capLimit int, tryScaleDown bool) (needScaleUp bool, newCap int) {


Can we define different calculateCapacity functions for Renew and Reset?
I think that would be easier to read.

zz-jason · 2018-08-24T08:51:55Z

util/chunk/chunk.go

 )

 // NewChunkWithCapacity creates a new chunk with field types and capacity.
 func NewChunkWithCapacity(fields []*types.FieldType, cap int) *Chunk {
+	return New(fields, cap, cap) //FIX ME in continue PR, this keep every thing same.


In the next PR, we remove this function and use New instead?

zz-jason · 2018-08-24T08:52:26Z

util/chunk/chunk.go

+// New creates a new chunk.
+//  cap: the max number of rows that the chunk wants to hold.
+//  capLimit: the max limit for max number of rows.
+func New(fields []*types.FieldType, cap, capLimit int) *Chunk {


the meaning of cap and capLimit makes people confused...`

seems capLimit is not used in this function

having any good advise for this two variable~~~?

I make a bug in previous, address comment orz..., this funcation will check cap not over capLimit..fixed

Is capLimit equal to tidb_max_chunk_size in the production use cases?

yes, for current plan, it will be equal to tidb_max_chunk_size just like previous does.

lysu · 2018-08-24T11:11:48Z

@zz-jason @coocood @XuHuaiyu PTAL thx~

coocood · 2018-08-24T12:47:04Z

util/chunk/chunk.go

+		if col.isFixed() {
+			elemLen = len(col.elemBuf)
+		}
+		columns = append(columns, newColumn(elemLen, cap, col))


I think this way is more straightforward:

if col.isFixed() { columns = append(columns, newFixedLenColumn(len(col.elemBuf), initCap)) } else { columns = append(columns, newVarLenColumn(initCap, col)) }

coocood · 2018-08-24T12:49:15Z

util/chunk/chunk.go

+	// so, in first executor.Next we using a experience value --- 4(so it maybe make `runtime.growslice`)
+	// but in continue Next call we estimated length as AVG elemLen of previous call.
+	if old != nil && old.length != 0 {
+		estimatedElemLen = len(old.data) / old.length


Can we set it larger, make it more room, so it's more unlikely to grow slice.

zz-jason · 2018-08-24T13:00:57Z

util/chunk/chunk.go

+//
+//  this method will be used in call Executor#Next many times with a new chunk situation.
+//  so it will `calculateResetCap` to get a new cap then create a new chunk, whatever happens.
+func Renew(chk *Chunk, capLimit int) *Chunk {


capLimit can be obtained by chk.capacity, should we remove this argument?

wuu..it's not easy..capacity will be capcity in current round, capLimit will always equal to sessionVars.max_chunk_size, and as review meeting result we should not store capLimit in chunk and got it from sessionVars...so we need pass this argument into New/Renew/GrowAndReset call- -

lysu · 2018-08-24T13:15:54Z

util/chunk/chunk.go

@@ -112,12 +159,68 @@ func (c *Chunk) SetNumVirtualRows(numVirtualRows int) {
 // Reset resets the chunk, so the memory it allocated can be reused.
 // Make sure all the data in the chunk is not used anymore before you reuse this chunk.
 func (c *Chunk) Reset() {


zz-jason · 2018-08-24T13:04:55Z

util/chunk/chunk.go

+	return true, mathutil.Min(c.capacity*2, capLimit)
+}
+
+// calculateResetCap checks and return suitable capacity in next execution.


s/calculateResetCap/calculateRenewCap/

zz-jason · 2018-08-24T13:08:53Z

util/chunk/chunk.go

+//      using next power of 2 for renew if currSize < cap
+//      using capacity limit if full but capacity already meet capLimit
+//      using oldcap x 2 if chk is full but less than capLimit
+func calculateRenewCap(c *Chunk, capLimit int) int {


Maybe we only need calculateRenewCap, and calculateResetCap can be removed?

this two method has some different

calculateResetCap: if previous return chk not full, will keep not grow.
calculateRenewCap: if previous returned chk is not full, will have chance give a smaller capacity then previous.

as #7473 (comment) advise I divide them into two.

XuHuaiyu · 2018-08-27T02:52:18Z

util/chunk/chunk_test.go

@@ -642,3 +658,209 @@ func BenchmarkChunkMemoryUsage(b *testing.B) {
 		chk.MemoryUsage()
 	}
 }
+
+type mockExec struct {


Add a comment for the usage of thsi, if it's hard to reuse it in other tests
in this file, it'll be better to make the name more clearly.

XuHuaiyu · 2018-08-27T02:59:41Z

util/chunk/chunk.go

@@ -30,24 +32,69 @@ type Chunk struct {
 	// numVirtualRows indicates the number of virtual rows, which have zero column.
 	// It is used only when this Chunk doesn't hold any data, i.e. "len(columns)==0".
 	numVirtualRows int
+	// capacity indicates the max number of rows that the chunk wants to hold.


I think wants to hold may make the reader somewhat confused.
Can we change it to can hold here?

XuHuaiyu · 2018-08-27T03:00:41Z

util/chunk/chunk.go

@@ -30,24 +32,69 @@ type Chunk struct {
 	// numVirtualRows indicates the number of virtual rows, which have zero column.
 	// It is used only when this Chunk doesn't hold any data, i.e. "len(columns)==0".
 	numVirtualRows int
+	// capacity indicates the max number of rows that the chunk wants to hold.
+	// The accurate row count after executor.Next is guaranteed by executor implement.


s/ implement/ implementation

XuHuaiyu · 2018-08-27T03:09:05Z

util/chunk/chunk.go

+//
+//  this method will be used in call Executor#Next many times with a new chunk situation.
+//  so it will `calculateResetCap` to get a new cap then create a new chunk, whatever happens.
+func Renew(chk *Chunk, capLimit int) *Chunk {


Can we rename capLimit to maxChunkSize, which seems to be more readable.

XuHuaiyu · 2018-08-27T03:09:46Z

util/chunk/chunk.go

+	return newChk
+}
+
+func renewColumns(oldCol []*column, cap int) []*column {


add a comment for this func.

2333...IMHO, this's method is too simple, code is comment...ok..I will try to add comment

XuHuaiyu · 2018-08-27T03:12:41Z

util/chunk/chunk.go

 	}
 	c.numVirtualRows = 0
 }

+// GrowAndReset resets the chunk and grow capacity if need.


s/ if need/ if needed

XuHuaiyu · 2018-08-27T03:16:20Z

util/chunk/chunk.go

+//  It works like:
+//      keep old value if capacity == capLimit or chk isn't full, and no need renew columns.
+//      old value x 2  if chk is full, and need renew columns.
+func calculateResetCap(c *Chunk, capLimit int) (requireRenewCol bool, newCap int) {


s/calculateResetCap/getCapForReset
s/ calculateRenewCap/ getCapForRenew ?

XuHuaiyu · 2018-08-27T03:28:53Z

util/chunk/chunk.go

+//   capLimit: the max limit for max number of rows.
+//
+//  this method will be used in call Executor#Next many times with a new chunk situation.
+//  so it will `calculateResetCap` to get a new cap then create a new chunk, whatever happens.


calculateRenewCap?

XuHuaiyu · 2018-08-27T03:29:52Z

util/chunk/chunk.go

+//   chk: old chunk(often used in previous call).
+//   capLimit: the max limit for max number of rows.
+//
+//  this method will be used in call Executor#Next many times with a new chunk situation.


s/in/ in the situation when calling ....

XuHuaiyu · 2018-08-27T03:30:18Z

util/chunk/chunk.go

+//   capLimit: the max limit for max number of rows.
+//
+//  this method will be used in call Executor#Next many times with a new chunk situation.
+//  so it will `calculateResetCap` to get a new cap then create a new chunk, whatever happens.


what does whatever happens mean?

XuHuaiyu

rest LGTM

XuHuaiyu · 2018-08-27T06:38:47Z

util/chunk/chunk.go

+//   chk: old chunk(often used in previous call).
+//   maxChunkSize: the max limit for max number of rows.
+//
+//  this method will be used in the situation when calling call Executor#Next many times with a new chunk situation.


remove call and the situation at the end.

XuHuaiyu · 2018-08-27T06:39:49Z

util/chunk/chunk.go

+// newVarLenColumn creates a variable length column with initial data capacity.
+func newVarLenColumn(initCap int, old *column) *column {
+	estimatedElemLen := 4
+	// for varLenColumn(e.g. varchar) we could not take the accuracy length of element,


XuHuaiyu

LGTM

zz-jason · 2018-08-27T07:21:33Z

util/chunk/chunk.go

 	for _, f := range fields {
-		chk.addColumnByFieldType(f, cap)
+		elemLen := getFixedLen(f)
+		if elemLen != varElemLen {


how about:

if elemLen == varElemLen { chk.columns = append(chk.columns, newVarLenColumn(cap, nil)) } else ...

zz-jason · 2018-08-27T07:33:54Z

util/chunk/chunk.go

+	// so, in first executor.Next we using a experience value --- 4(so it maybe make `runtime.growslice`)
+	// but in continue Next call we estimated length as AVG x 1.5 elemLen of previous call.
+	if old != nil && old.length != 0 {
+		estimatedElemLen = (len(old.data) + len(old.data)/2) / old.length


why the estimated length for the var-length element is changed?

To reduce the probability of growing slice when appending string.

len(old.data)/old.length is enough to estimated the average length of the strings to be appended in the new column, no need to moltiply 1.5.

@coocood @zz-jason PTAL... I have conitune PR wait this...

len(old.data) + len(old.data)/8 may be large enough

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

util/chunk/chunk.go

+func calculateCapForRenew(c *Chunk, maxChunkSize int) int {
+	currSize := c.NumRows()
+	if currSize < c.capacity {
+		return NextPowerOfTwo(currSize)


why do we use NextPowerOfTwo(currSize) rather than c.capacity?

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

util/chunk/chunk.go

+	if c.capacity == maxChunkSize {
+		return maxChunkSize
+	}
+	return mathutil.Min(c.capacity*2, maxChunkSize)


Can we remove line 203~205? IMO, mathutil.Min(c.capacity*2, maxChunkSize) can ccover the case of c.capacity == maxChunkSize.

coocood · 2018-08-27T11:18:32Z

util/chunk/chunk_test.go

+
+func BenchmarkChunkRenewConsumeExec10000000In1024Slice(b *testing.B) {
+	b.ReportAllocs()
+	chk := New([]*types.FieldType{{Tp: mysql.TypeLong}}, 1024, 1024)


We can extract a common function for benchmarks, avoid duplicated code.

I found testing.B.Run is powerful

- add New() Renew() and GrowReset() method to chunk - add eraseGrow() in column - adaptor NewChunkWithCapacity() and Reset() works in chunk

coocood · 2018-08-28T04:31:26Z

LGTM

zz-jason · 2018-08-28T05:06:17Z

util/chunk/chunk.go

+	// so, in first executor.Next we using a experience value --- 8(so it maybe make `runtime.growslice`)
+	// but in continue Next call we estimated length as AVG x 1.125 elemLen of previous call.
+	if old != nil && old.length != 0 {
+		estimatedElemLen = (len(old.data) + len(old.data)/8) / old.length


it's "magic const" by @coocood 🤦‍♂️ , the idea is avg maybe not very suitable for next call

zz-jason · 2018-08-28T05:12:29Z

util/chunk/chunk.go

+	// capacity indicates the max number of rows that the chunk can hold.
+	// The accurate row count after executor.Next is guaranteed by executor implementation.
+	// It is used only during executor's execution, and grow by `Renew` or `GrowAndReset`.
+	capacity int


how about simplifying the comment to:

// capacity indicates the max number of rows this chunk can hold.

@zz-jason I prefer "this" in your sentence and it is better than "the".

It is better to use "this" instead of "the" to refer to the current thing.

zz-jason · 2018-08-28T05:14:50Z

util/chunk/chunk.go

-		chk.addColumnByFieldType(f, cap)
+		elemLen := getFixedLen(f)
+		if elemLen == varElemLen {
+			chk.columns = append(chk.columns, newVarLenColumn(cap, nil))


we should firstly calculate the capacity according to the input parameters:

chk.capacity = mathutil.Min(cap, maxChunkSize)

then use chk.capacity to create columns.

zz-jason · 2018-08-28T05:20:38Z

util/chunk/chunk.go

+//
+//  this method will be used in the situation when calling call Executor#Next many times with a new chunk.
+//  so it will `calculateCapacity` to get a new cap then create a new chunk.
+func Renew(chk *Chunk, maxChunkSize int) *Chunk {


how about change the comment to:

// Renew creates a new Chunk based on an existing Chunk. The newly created Chunk // has the same data schema with the old Chunk, the capacity of the new Chunk // might be doubled based on the capacity of the old Chunk and the maxChunkSize.

Jason's suggestion is good but a comma should be changed to a period:
// Renew creates a new Chunk based on an existing Chunk. The newly created Chunk
// has the same data schema with the old Chunk**. The** capacity of the new Chunk
// might be doubled based on the capacity of the old Chunk and the maxChunkSize.

zz-jason · 2018-08-28T05:24:52Z

util/chunk/chunk.go

+
+// renewColumns recreates columns in chunk.
+// create FixedLenColumn or VarLenColumn based on whether column is fixed.
+func renewColumns(oldCol []*column, cap int) []*column {


how about change the comment to:

// renewColumns creates the columns of a Chunk, the capacity of the newly // created columns is equal to cap.

Jason's suggestion is good but a comma should be changed to a period:
// renewColumns creates the columns of a Chunk**. The** capacity of the newly
// created columns is equal to cap.

zz-jason · 2018-08-28T05:25:36Z

util/chunk/chunk.go

-func (c *Chunk) addVarLenColumn(initCap int) {
-	c.columns = append(c.columns, &column{
+// newVarLenColumn creates a variable length column with initial data capacity.
+func newVarLenColumn(initCap int, old *column) *column {


how about s/initCap/cap/?

zz-jason · 2018-08-28T05:28:47Z

util/chunk/chunk.go

 	}
 	c.numVirtualRows = 0
 }

+// GrowAndReset resets the chunk and grow capacity if needed.
+// TODO: this method will be used in continue PR.
+func (c *Chunk) GrowAndReset(maxChunkSize int) {


how about changing the comment to:

// GrowAndReset doubles the capacity of the Chunk, the doubled capacity should // not be larger than maxChunkSize, resets the content of the Chunk.

zz-jason · 2018-08-28T05:32:56Z

util/chunk/chunk.go

+//      using capacity limit if full but capacity already meet maxChunkSize
+//      using oldcap x 2 if chk is full but less than maxChunkSize
+//      keep oldcap if chk isn't full
+func calculateCapacity(c *Chunk, maxChunkSize int) int {


how about s/calculateCapacity/reCalcCapacity/ and changing the comment to:

// reCalcCapacity calculates the capacity for another Chunk based the current // Chunk. The new capacity is doubled only when the current Chunk is full.

Based on Jason's suggestion:

// reCalcCapacity calculates the capacity for another Chunk based on the current
// Chunk. The new capacity is doubled only when the current Chunk is full.

zz-jason · 2018-08-28T05:33:48Z

util/chunk/chunk.go

+	return mathutil.Min(c.capacity*2, maxChunkSize)
+}
+
+// Capacity return the max number of rows that chunk want to hold.


// Capacity returns the capacity of the Chunk.

zz-jason · 2018-08-28T05:35:15Z

@lilin90 Could you help us to refine some code comments in this PR? Thanks!

lilin90 · 2018-08-28T07:15:21Z

@CaitinChen Pls help review the code comments. Thanks!

CaitinChen · 2018-08-28T07:26:39Z

util/chunk/chunk.go

@@ -30,6 +31,10 @@ type Chunk struct {
 	// numVirtualRows indicates the number of virtual rows, which have zero column.
 	// It is used only when this Chunk doesn't hold any data, i.e. "len(columns)==0".
 	numVirtualRows int
+	// capacity indicates the max number of rows that the chunk can hold.
+	// The accurate row count after executor.Next is guaranteed by executor implementation.
+	// It is used only during executor's execution, and grow by `Renew` or `GrowAndReset`.


grow -> grows

CaitinChen · 2018-08-28T07:29:26Z

util/chunk/chunk.go

+	// capacity indicates the max number of rows that the chunk can hold.
+	// The accurate row count after executor.Next is guaranteed by executor implementation.
+	// It is used only during executor's execution, and grow by `Renew` or `GrowAndReset`.
+	capacity int


@zz-jason I prefer "this" in your sentence and it is better than "the".

CaitinChen · 2018-08-28T07:31:33Z

util/chunk/chunk.go

+	// capacity indicates the max number of rows that the chunk can hold.
+	// The accurate row count after executor.Next is guaranteed by executor implementation.
+	// It is used only during executor's execution, and grow by `Renew` or `GrowAndReset`.
+	capacity int


It is better to use "this" instead of "the" to refer to the current thing.

CaitinChen · 2018-08-28T07:38:17Z

util/chunk/chunk.go

+
+// New creates a new chunk.
+//  cap: the max number of rows that the chunk can hold.
+//  maxChunkSize: the max limit for max number of rows.


-> maxChunkSize: the limit for the max number of rows.
or
-> maxChunkSize: the max limit for the number of rows.

CaitinChen · 2018-08-28T07:40:44Z

util/chunk/chunk.go

 	return chk
 }

+// Renew recreates a new chunk base on old chunk.
+//   chk: old chunk(often used in previous call).
+//   maxChunkSize: the max limit for max number of rows.


-> maxChunkSize: the limit for the max number of rows.
or
-> maxChunkSize: the max limit for the number of rows.

CaitinChen · 2018-08-28T08:13:27Z

util/chunk/chunk.go

@@ -39,15 +44,57 @@ const (

 // NewChunkWithCapacity creates a new chunk with field types and capacity.
 func NewChunkWithCapacity(fields []*types.FieldType, cap int) *Chunk {
+	return New(fields, cap, cap) //FIX ME in continue PR, this keep every thing same.


// FIX ME in the following PR. Keep everything consistent?

CaitinChen · 2018-08-28T08:28:51Z

util/chunk/chunk.go

+// newVarLenColumn creates a variable length column with initial data capacity.
+func newVarLenColumn(initCap int, old *column) *column {
+	estimatedElemLen := 8
+	// For varLenColumn(e.g. varchar) we could not take the accuracy length of element,


For varLenColumn (e.g. varchar), the accurate length of an element is unknown.

CaitinChen · 2018-08-28T08:30:15Z

util/chunk/chunk.go

+func newVarLenColumn(initCap int, old *column) *column {
+	estimatedElemLen := 8
+	// For varLenColumn(e.g. varchar) we could not take the accuracy length of element,
+	// so, in first executor.Next we using a experience value --- 8(so it maybe make `runtime.growslice`)


Therefore, in the first executor.Next we use an experience value -- 8 (so it may make runtime.growslice)

CaitinChen · 2018-08-28T08:31:22Z

util/chunk/chunk.go

+	estimatedElemLen := 8
+	// For varLenColumn(e.g. varchar) we could not take the accuracy length of element,
+	// so, in first executor.Next we using a experience value --- 8(so it maybe make `runtime.growslice`)
+	// but in continue Next call we estimated length as AVG x 1.125 elemLen of previous call.


but in the following Next call we estimate the length as AVG x 1.125 elemLen of the previous call.

CaitinChen · 2018-08-28T08:38:29Z

util/chunk/codec.go

@@ -163,6 +163,9 @@ func (c *Codec) bytesToI32Slice(b []byte) (i32s []int32) {
 	return i32s
 }

+// varElemLn indicates column is variable length column.


如果：varElemLn 取值有几种不同的情况，有的情况下，该 column 是 variable length column；有的情况下，该 column 不是 variable length column，那么：
varElemLn indicates whether this column is a variable length column.

如果：varElemLn 取值只能等于 -1 （我看代码中显示是常量，应该是固定值吧），column 一直是 variable length column，那么：
varElemLn indicates this column is a variable length column.

我倾向于后者

Why not change the name?

lysu · 2018-08-28T10:34:57Z

PTAL @zz-jason 🍂

zz-jason · 2018-08-28T10:38:17Z

util/chunk/chunk.go

+	// Therefore, in the first executor.Next we use an experience value -- 8 (so it may make runtime.growslice)
+	// but in the following Next call we estimate the length as AVG x 1.125 elemLen of the previous call.
+	if old != nil && old.length != 0 {
+		estimatedElemLen = (len(old.data) + len(old.data)/8) / old.length


why the estimatedElemLen is len(old.data)/old.length * 1.125?

zz-jason

LGTM

lysu added the sig/execution SIG execution label Aug 23, 2018

jackysp added the status/all tests passed label Aug 24, 2018

lysu requested a review from coocood August 24, 2018 03:41

XuHuaiyu reviewed Aug 24, 2018

View reviewed changes

winoros reviewed Aug 24, 2018

View reviewed changes

coocood reviewed Aug 24, 2018

View reviewed changes

zz-jason reviewed Aug 24, 2018

View reviewed changes

coocood reviewed Aug 24, 2018

View reviewed changes

zz-jason reviewed Aug 24, 2018

View reviewed changes

XuHuaiyu reviewed Aug 27, 2018

View reviewed changes

zz-jason reviewed Aug 27, 2018

View reviewed changes

coocood reviewed Aug 27, 2018

View reviewed changes

lysu added 9 commits August 28, 2018 12:24

chunk: support capacity grow.

6a95b8b

- add New() Renew() and GrowReset() method to chunk - add eraseGrow() in column - adaptor NewChunkWithCapacity() and Reset() works in chunk

address comments

3fce5b3

address comment

efab909

address comment

f54e3d3

fix ci.

34cf6fc

address comment

4be9e43

address comment

6119f3c

address comment

acc548a

fix compile

9fb2382

lysu added 2 commits August 28, 2018 12:24

address comment

4ab3471

address comment

d6d8664

zz-jason reviewed Aug 28, 2018

View reviewed changes

address comment

52dbfa0

CaitinChen reviewed Aug 28, 2018

View reviewed changes

lysu added 2 commits August 28, 2018 17:00

address comment

138bb5e

Merge branch 'master' into dev-chunk-add-grow-mechanism

2deb09e

zz-jason reviewed Aug 28, 2018

View reviewed changes

zz-jason approved these changes Aug 28, 2018

View reviewed changes

lysu merged commit 2fb3145 into pingcap:master Aug 28, 2018

lysu deleted the dev-chunk-add-grow-mechanism branch August 28, 2018 12:08

lysu mentioned this pull request Aug 29, 2018

*: use chunk grow for simple executor #7540

Merged

chunk: support capacity grow #7473

chunk: support capacity grow #7473

Conversation

lysu commented Aug 23, 2018 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

lysu commented Aug 23, 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

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

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

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

lysu Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu commented Aug 24, 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

This comment was marked as resolved.

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coocood commented Aug 28, 2018

Choose a reason for hiding this comment

lysu Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu commented Aug 23, 2018 •

edited

Loading

lysu Aug 24, 2018 •

edited

Loading

lysu Aug 28, 2018 •

edited

Loading