Implement preallocated buffer #996

ashish-goswami · 2019-08-20T15:49:05Z

Ran this on both branch

go test -bench BenchmarkBuilder$ -run ^$ --count=5 -benchmem

Master

goos: linux
goarch: amd64
pkg: github.com/dgraph-io/badger/table
BenchmarkBuilder-16    	       3	 478921486 ns/op	264389517 B/op	 2673998 allocs/op
BenchmarkBuilder-16    	       3	 472378778 ns/op	264389565 B/op	 2673999 allocs/op
BenchmarkBuilder-16    	       3	 476744803 ns/op	264390378 B/op	 2674000 allocs/op
BenchmarkBuilder-16    	       3	 473738428 ns/op	264388901 B/op	 2673994 allocs/op
BenchmarkBuilder-16    	       3	 472036524 ns/op	264389282 B/op	 2673996 allocs/op
PASS
ok  	github.com/dgraph-io/badger/table	14.416s

This PR

goos: linux
goarch: amd64
pkg: github.com/dgraph-io/badger/table
BenchmarkBuilder-16    	       2	 527921308 ns/op	259690376 B/op	 2659254 allocs/op
BenchmarkBuilder-16    	       2	 522870453 ns/op	259689704 B/op	 2659255 allocs/op
BenchmarkBuilder-16    	       2	 523489554 ns/op	259689704 B/op	 2659254 allocs/op
BenchmarkBuilder-16    	       2	 526129561 ns/op	259689296 B/op	 2659250 allocs/op
BenchmarkBuilder-16    	       2	 520691506 ns/op	259689976 B/op	 2659259 allocs/op
PASS
ok  	github.com/dgraph-io/badger/table	7.914s

Benchstats

name        old time/op    new time/op    delta
Builder-16     475ms ± 1%     517ms ± 2%  +8.91%  (p=0.000 n=5+20)

name        old alloc/op   new alloc/op   delta
Builder-16     264MB ± 0%     260MB ± 0%  -1.78%  (p=0.000 n=5+18)

name        old allocs/op  new allocs/op  delta
Builder-16     2.67M ± 0%     2.66M ± 0%  -0.55%  (p=0.000 n=5+18)

This change is

golangcibot · 2019-08-20T15:50:19Z

y/iterator.go

+// this function exists is to avoid creating byte arrays per key-value pair in
+// table/builder.go.
+func (v *ValueStruct) EncodeToYBuf(buf *Buffer) {
+	buf.WriteByte(v.Meta)


Error return value of buf.WriteByte is not checked (from errcheck)

golangcibot · 2019-08-20T15:50:20Z

y/iterator.go

+// table/builder.go.
+func (v *ValueStruct) EncodeToYBuf(buf *Buffer) {
+	buf.WriteByte(v.Meta)
+	buf.WriteByte(v.UserMeta)


Error return value of buf.WriteByte is not checked (from errcheck)

golangcibot · 2019-08-20T15:50:20Z

y/iterator.go

+	buf.WriteByte(v.UserMeta)
+	var enc [binary.MaxVarintLen64]byte
+	sz := binary.PutUvarint(enc[:], v.ExpiresAt)
+	buf.Write(enc[:sz])


Error return value of buf.Write is not checked (from errcheck)

golangcibot · 2019-08-20T15:50:20Z

y/iterator.go

+	var enc [binary.MaxVarintLen64]byte
+	sz := binary.PutUvarint(enc[:], v.ExpiresAt)
+	buf.Write(enc[:sz])
+	buf.Write(v.Value)


Error return value of buf.Write is not checked (from errcheck)

manishrjain

Reviewed 1 of 5 files at r2, 9 of 9 files at r4.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ashish-goswami and @jarifibrahim)

table/builder.go, line 225 at r4 (raw file):

+---------+------------+-----------+---------------+
*/
func (b *Builder) Finish() *y.Buffer {

Don't change the output from this. Keep it to []byte.

y/y.go, line 355 at r4 (raw file):

// in chunks(pages). Once a chunk(page) is full, it will allocate double the size of previous
// chunk. Its function are not thread safe.
type Buffer struct {

In the first PR, can you just introduce the Buffer: Call it PageBuffer and tests for it.

y/y.go, line 472 at r4 (raw file):

// TODO: reader can be removed.

func (b *Buffer) NewReader() io.Reader {

You can remove this, if not needed.

ashish-goswami

Reviewable status: 0 of 7 files reviewed, 7 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)

table/builder.go, line 225 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Don't change the output from this. Keep it to []byte.

Done.

y/y.go, line 355 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

In the first PR, can you just introduce the Buffer: Call it PageBuffer and tests for it.

Done.

y/y.go, line 472 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

You can remove this, if not needed.

Done.

pullrequest

✅ A review job has been created and sent to the PullRequest network.

@ashish-goswami you can click here to see the review status or cancel the code review job.

jarifibrahim

Reviewed 3 of 10 files at r5.
Reviewable status: 3 of 7 files reviewed, 10 unresolved discussions (waiting on @ashish-goswami and @manishrjain)

y/y.go, line 352 at r5 (raw file):

// PageBuffer consists of many pages. A page is a wrapper over []byte. PageBuffer can act as a
// replacement of bytes.Buffer. Instead of having single underlying buffer, it has multiple
// underlying buffers. Hence it avoids any copy during relocation(as happenes in bytes.Buffer).

Typo: happenes => happens

y/y.go, line 353 at r5 (raw file):

// replacement of bytes.Buffer. Instead of having single underlying buffer, it has multiple
// underlying buffers. Hence it avoids any copy during relocation(as happenes in bytes.Buffer).
// Buffer allocates memory in pages. Once a page is full, it will allocate page with double the size

PageBuffer allocates memory in pages...

y/y.go, line 456 at r5 (raw file):

}

// WriteTo writes whole buffer to w. It returns number of bytes returned and any error encountered.

typo: returned => written

ashish-goswami

Reviewable status: 2 of 7 files reviewed, 10 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)

y/y.go, line 352 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Typo: happenes => happens

Done.

y/y.go, line 353 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

PageBuffer allocates memory in pages...

Done.

y/y.go, line 456 at r5 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

typo: returned => written

Done.

pullrequest

Overall the changes here look like a good optimization. Although left one comment that you may want to double check usage or add more documentation around the shared buffer used for ReadAt to avoid any issues in the future.

Reviewed with ❤️ by PullRequest

pullrequest · 2019-08-22T17:15:52Z

y/y.go

+	if length > cap(b.readBuf) {
+		b.readBuf = make([]byte, length) // Allocate whole buffer at start.
+	} else {
+		b.readBuf = b.readBuf[:length]


I know you mentioned on the PageBuffer that its functions aren't thread safe. Although it also seems like multiple calls to ReadAt will potentially mutate the data returned by previous calls since this b.readBuf's underlying data is shared between calls. This might be worth adding to the docstring for this method and PageBuffer because I could see this being pretty surprising and have data leakage/security issues if a buffer was ever shared across accounts or something.

Valid point. Thanks for pointing it out. Will add to comments.

pullrequest · 2019-08-22T17:15:52Z

y/y.go

+// NewPageBuffer returns a new PageBuffer with first page having size pageSize.
+func NewPageBuffer(pageSize int) *PageBuffer {
+	b := &PageBuffer{curPageSize: pageSize}
+	b.pages = make([]*page, 0)


I believe that this line is redundant currently since the append on the line below will do this even if b.pages is nil, although it is fine to keep it if you prefer.

Fixed it. Thanks.

pullrequest · 2019-08-22T17:15:53Z

y/y.go

@@ -340,3 +341,128 @@ func BytesToU32Slice(b []byte) []uint32 {
 	hdr.Data = uintptr(unsafe.Pointer(&b[0]))
 	return u32s
 }
+
+// page struct contains one underlying buffer.
+type page struct {


Do you plan to add more items to this struct or would it make sense to just do something like this instead:

type page []byte

We might add some functions to page.

pullrequest · 2019-08-22T17:15:53Z

y/iterator.go

+// EncodeToPageBuf should be kept in sync with the Encode function above. The reason
+// this function exists is to avoid creating byte arrays per key-value pair in
+// table/builder.go.
+func (v *ValueStruct) EncodeToPageBuf(buf *PageBuffer) {


I wish there was a bytes.Buffer interface that could be used instead of having to duplicate the code and keep these in sync but otherwise. But makes sense and thanks for adding the comment to this method.

Agreed. PageBuffer is experimental as of now. Once we decide to use it in Builder, we can do that.

manishrjain

Can you remove the usage in table builder for now, as we had discussed. Let's get the library in. Then we can figure out if it is worth using it in various places.

Reviewable status: 2 of 8 files reviewed, 13 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, @manishrjain, and @pullrequest[bot])

jarifibrahim

Reviewable status: 2 of 8 files reviewed, 10 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, @manishrjain, and @pullrequest[bot])

manishrjain

Reviewed 3 of 10 files at r5, 1 of 3 files at r7, 1 of 1 files at r8, 2 of 3 files at r9.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @ashish-goswami, @manishrjain, and @pullrequest[bot])

y/y.go, line 361 at r9 (raw file):

	length      int // Length of PageBuffer.
	curPageSize int // Size of last page allocated.

nextPageSize

pullrequest

Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard

manishrjain

Just merge the y.PageBuffer one.

Reviewable status: 4 of 6 files reviewed, 10 unresolved discussions (waiting on @ashish-goswami, @manishrjain, and @pullrequest[bot])

jarifibrahim

Thank you for adding so many tests. 👍

Reviewed 3 of 10 files at r5, 2 of 3 files at r7, 1 of 1 files at r8, 2 of 3 files at r9, 2 of 2 files at r10.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @ashish-goswami, @manishrjain, and @pullrequest[bot])

y/y.go, line 381 at r10 (raw file):

			break
		}
		data = data[n:]

I don't think we should do this. The caller of write would never expect the passed sliced to be modified in any way. The documentation for io.Writer interface clearly states

 Write must not modify the slice data, even temporarily.

https://golang.org/pkg/io/#Writer

y/y.go, line 392 at r10 (raw file):

// WriteByte writes data byte to PageBuffer and returns any encountered error.
func (b *PageBuffer) WriteByte(data byte) error {
	_, err := b.Write([]byte{data}) // Can be changed later.

I don't understand the comment. What can be changed later?

y/y_test.go, line 42 at r10 (raw file):

	rand.Seed(time.Now().Unix())

	var bytesBuffer bytes.Buffer // This is just of verifying result.

just of for verifying the result.

y/y_test.go, line 144 at r10 (raw file):

	require.NoError(t, err, "unable to read error")
	require.True(t, n == len(rb), "length read should be equal")
	// Read same number of bytes using ReaderAt.

This comment looks outdated.

y/y_test.go, line 152 at r10 (raw file):

	require.NoError(t, err, "unable to read error")
	require.True(t, n == 10, "length read should be equal")
	// Read same number of bytes using ReaderAt.

This comment looks outdated.

y/y_test.go, line 158 at r10 (raw file):

	// Check if EOF is returned at end or not.
	n, err = reader.Read(rb[:10])
	require.Equal(t, err, io.EOF, "EOF should be returned at end")

Can you add another check

require.Zero(t, n, "read should return zero bytes")

jarifibrahim

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @ashish-goswami, @manishrjain, and @pullrequest[bot])

y/y.go, line 381 at r10 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

I don't think we should do this. The caller of write would never expect the passed sliced to be modified in any way. The documentation for io.Writer interface clearly states
 Write must not modify the slice data, even temporarily.
https://golang.org/pkg/io/#Writer

My bad. The underlying data slice is not being modified.

ashish-goswami

Reviewable status: 4 of 6 files reviewed, 15 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, @manishrjain, and @pullrequest[bot])

y/y.go, line 361 at r9 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

nextPageSize

Done.

y/y.go, line 392 at r10 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

I don't understand the comment. What can be changed later?

Removed it.

y/y_test.go, line 42 at r10 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

just of for verifying the result.

Done.

y/y_test.go, line 144 at r10 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

This comment looks outdated.

Done.

y/y_test.go, line 152 at r10 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

This comment looks outdated.

Done.

y/y_test.go, line 158 at r10 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Can you add another check
require.Zero(t, n, "read should return zero bytes")

Done.

manishrjain and others added 20 commits July 29, 2019 12:40

A draft of the new buffer class which avoids reallocations.

fe89341

Replace bytes.buffer with custom buffer

f9f508c

Implement reader

6f3144d

Implement WriteTo Interface for reader

a04f3bd

Fix bugs in Read and WriteTo method of Reader

96e6ac2

Add builder benchmark

d4aa567

Merge remote-tracking branch 'origin/master' into mrjn/zbuffer

2f56ace

Fix compilation failures

b2aed3d

Remove benchmark builder temp

5285c7e

temp

96528de

Merge remote-tracking branch 'origin/master' into mrjn/zbuffer

0b17def

temp

ca10f22

Minor changes

9d07d54

Minor changes

4cf3040

Remove the func within the benchmark

7fa634f

Increase the size of page by 2 for every new page.

ab2a884

Merge branch 'master' into mrjn/zbuffer

cb7f6e5

Fix ReadAt method

250e136

Modify ReadAt function and add comments to all functons

9e149d3

Fix compilation errors

02b25c7

ashish-goswami requested review from jarifibrahim and manishrjain August 20, 2019 15:49

golangcibot reviewed Aug 20, 2019

View reviewed changes

manishrjain reviewed Aug 21, 2019

View reviewed changes

Address review comments

574bb0a

ashish-goswami commented Aug 21, 2019

View reviewed changes

pullrequest bot reviewed Aug 22, 2019

View reviewed changes

jarifibrahim approved these changes Aug 22, 2019

View reviewed changes

Address review comments

37159fc

ashish-goswami commented Aug 22, 2019

View reviewed changes

pullrequest bot reviewed Aug 22, 2019

View reviewed changes

ashish-goswami added 5 commits August 23, 2019 14:35

Add tests for PageBuffer

2e75e11

Add Reader and Truncate to PageBuffer

42817f8

Merge branch 'master' into mrjn/zbuffer

8d996ba

Fix comments

333e8c5

Fix spellings y_test.go

589e03d

manishrjain reviewed Aug 27, 2019

View reviewed changes

jarifibrahim approved these changes Aug 27, 2019

View reviewed changes

Remove PageBuffer from TableBuilder

7055c56

manishrjain reviewed Sep 2, 2019

View reviewed changes

ashish-goswami added 2 commits September 5, 2019 22:01

Refactor PageBuffer

ccc2b55

Minor Fixes

9760ab2

pullrequest bot reviewed Sep 6, 2019

View reviewed changes

ashish-goswami added 2 commits September 9, 2019 14:08

Merge branch 'master' into mrjn/zbuffer

44e998b

Refactor PageBufferReader Read()

b76f897

manishrjain reviewed Sep 9, 2019

View reviewed changes

jarifibrahim approved these changes Sep 10, 2019

View reviewed changes

ashish-goswami marked this pull request as ready for review September 10, 2019 14:08

ashish-goswami requested a review from a team September 10, 2019 14:08

jarifibrahim reviewed Sep 10, 2019

View reviewed changes

Address review comments

5a7d172

ashish-goswami commented Sep 11, 2019

View reviewed changes

ashish-goswami merged commit 86a77bb into master Sep 11, 2019

ashish-goswami deleted the mrjn/zbuffer branch September 11, 2019 11:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement preallocated buffer #996

Implement preallocated buffer #996

ashish-goswami commented Aug 20, 2019 •

edited

Loading

golangcibot Aug 20, 2019

golangcibot Aug 20, 2019

golangcibot Aug 20, 2019

golangcibot Aug 20, 2019

manishrjain left a comment

ashish-goswami left a comment

pullrequest bot left a comment

jarifibrahim left a comment

ashish-goswami left a comment

pullrequest bot left a comment

pullrequest bot Aug 22, 2019

ashish-goswami Aug 26, 2019

pullrequest bot Aug 22, 2019

ashish-goswami Aug 26, 2019

pullrequest bot Aug 22, 2019

ashish-goswami Aug 26, 2019

pullrequest bot Aug 22, 2019

ashish-goswami Aug 26, 2019

manishrjain left a comment

jarifibrahim left a comment

manishrjain left a comment

pullrequest bot left a comment

manishrjain left a comment

jarifibrahim left a comment

jarifibrahim left a comment

ashish-goswami left a comment

Implement preallocated buffer #996

Implement preallocated buffer #996

Conversation

ashish-goswami commented Aug 20, 2019 • 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

manishrjain left a comment

Choose a reason for hiding this comment

ashish-goswami left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

jarifibrahim left a comment

Choose a reason for hiding this comment

ashish-goswami left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Aug 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pullrequest bot Aug 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pullrequest bot Aug 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pullrequest bot Aug 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

jarifibrahim left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

jarifibrahim left a comment

Choose a reason for hiding this comment

jarifibrahim left a comment

Choose a reason for hiding this comment

ashish-goswami left a comment

Choose a reason for hiding this comment

ashish-goswami commented Aug 20, 2019 •

edited

Loading