Use net.Buffers in Go 1.8 for encoding. #70

tmthrgd · 2017-03-17T01:58:47Z

This pull request covers issue #68.

On go1.7 and below it behaves as before. On go1.8 with the packed encoder it also behaves as before. On go1.8 with the regular encoder it uses net.Buffers which will use writev in a single system call where possible.

It adds go1.8 to travis to test the new code path.

zombiezen

Thanks for looking at this! Can you also add yourself to the AUTHORS+CONTRIBUTORS files?

zombiezen · 2017-03-17T20:39:00Z

mem.go

-	if err := e.write(e.hdrbuf); err != nil {
-		return err
-	}
+	bufs := make([][]byte, 1+nsegs)


Seems like this list of bufs could be reused between calls.

zombiezen · 2017-03-17T20:40:28Z

mem_18.go

+	"zombiezen.com/go/capnproto2/internal/packed"
+)
+
+func (e *Encoder) write(bufs net.Buffers) error {


Keep the function parameter type the same between 1.8 and non-1.8.

zombiezen · 2017-03-17T20:41:56Z

mem_other.go

+
+func (e *Encoder) write(bufs [][]byte) error {
+	for _, b := range bufs {
+		if e.packed {


Pull out the e.packed logic into a separate (non-tagged) function.

zombiezen · 2017-03-17T20:43:29Z

mem.go

-	if err := e.write(e.hdrbuf); err != nil {
-		return err
-	}
+	bufs := make([][]byte, 1+nsegs)


GH is not letting me add a comment farther up... while you're visiting this code, it seems like traversing the segments twice (using segmentSizes) isn't really necessary. Mind rolling that all into one?

zombiezen · 2017-03-20T14:38:00Z

mem.go

-	if uint64(cap(e.hdrbuf)) < hdrSize {
-		e.hdrbuf = make([]byte, hdrSize)
+	if int64(cap(e.bufs)) < 1+nsegs {
+		e.bufs = make([][]byte, 1+nsegs)


Let's let append deal with the resizing. Instead of these two ifs, let's have:

e.sizes = e.sizes[:0] e.bufs = append(e.bufs[:0], nil) // first element is placeholder for header

(although see below, because I don't think we need the sizes buffer.)

zombiezen · 2017-03-20T14:46:37Z

mem.go

@@ -571,40 +573,51 @@ func (e *Encoder) Encode(m *Message) error {
 	if nsegs == 0 {


You can remove the TODO above, I don't think it will ever happen.

zombiezen · 2017-03-20T14:48:15Z

mem.go

 	}
 	for i := int64(0); i < nsegs; i++ {
 		s, err := m.Segment(SegmentID(i))
 		if err != nil {
 			return err
 		}
-		if err := e.write(s.data); err != nil {
-			return err
+		n := len(s.data)


nit: make this n := Size(len(s.data))

That would risk an overflow that the int64(n) > int64(maxSize) is designed to avoid. Size is a uin32 but len(x) returns an int which can easily be larger than the maximum value of a uint32.

zombiezen · 2017-03-20T14:49:03Z

mem.go

 		}
+		e.sizes[i] = Size(n)
+		e.bufs[1+i] = s.data


As part of the above comment, e.bufs = append(e.bufs, s.data)

zombiezen · 2017-03-20T15:04:33Z

mem.go

+	} else {
+		e.hdrbuf = e.hdrbuf[:hdrSize]
+	}
+	marshalStreamHeader(e.hdrbuf, e.sizes)


TBH, I think I want to nuke marshalStreamHeader. It's adding an allocation for not much clarity. Let's inline it here (and define a new helper, appendUint32):

if uint64(cap(e.hdrbuf)) < hdrSize { e.hdrbuf = make([]byte, 0, hdrSize) } e.hdrbuf = appendUint32(e.hdrbuf[:0], maxSeg) for i, buf := range e.bufs { e.hdrbuf = appendUint32(e.hdrbuf, uint32(Size(len(buf))/wordSize)) } if len(e.hdrbuf) % int(wordSize) != 0 { e.hdrbuf = appendUint32(e.hdrbuf, 0) }

Interestingly, I think that the current Encoder has a padding bug in this regard. Can you add a test where a 3-segment message is encoded then a 2-segment message is encoded, and then assert that the 2-segment message's header properly contains zero-padding (instead of garbage)?

I've been able to reproduce that bug and have added a failing test case for it. I'll inline marshalStreamHeader and fix the bug now.

I've only touched the (*Encoder).Encode part so far and I've left the use of marshalStreamHeader in (*Message).Marshal. So the bug still exists in (*Message).Marshal.

I'm not sure how you want me to handle that, but I'm thinking it would be best if (*Message).Marshal and (*Message).MarshalPacked were just made wrappers around (*Encoder).Encode. That would mean the encoding logic only exists once and would reduce duplication. I think I'll go ahead and add a commit for that and you can decide whether you like it or not.

How does the bug exist in Marshal? The issue is that the buffer gets reused and there's no buffer to reuse for Marshal.

IIRC, having different codepaths does have a big performance impact (i.e. Marshal is faster when it doesn't go through a Writer). I would rather keep the separation unless you can prove that this doesn't significantly alter performance.

I realize this is a bit bigger of a change than you probably wanted to do. Let me know if you want me to pick up this part.

I backed that change out.

I think I misread you're desire to 'nuke' marshalStreamHeader as a request for me to eliminate it. I'll leave removing marshalStreamHeader up to you as a separate matter.

zombiezen · 2017-03-20T15:05:45Z

mem_18.go

+import "net"
+
+func (e *Encoder) write(bufs [][]byte) error {
+	_, err := (*net.Buffers)(&bufs).WriteTo(e.w)


No need for pointer indirection, just convert to net.Buffers.

I tried that before but (*net.Buffers).WriteTo is a pointer receiver.

# zombiezen.com/go/capnproto2 ./mem_18.go:8: cannot call pointer method on net.Buffers(bufs) ./mem_18.go:8: cannot take the address of net.Buffers(bufs)

How bizarre. Opened golang/go#19680

It's a result of how net.Buffers is implemented so it's very intentional. net.Buffers needs to work with multiple calls to Read and WriteTo (for things like io.Copy), to do this it needs to be able to modify the slice as it consumes more data - see src/net/net.go.

marshalStreamHeader fails to zero the padding bytes after the last segment size. This is a problem because e.hdrbuf is reused between calls to Encode. This leaves garbage in the header in certain circumstances. This will be fixed in a future commit. (See the discussion of #70 for more details of the bug).

This also fixes TestStreamHeaderPadding introduced in 3f1f84d. This still leaves marshalStreamHeader to be removed from (*Message).Marshal.

This removes all uses of marshalStreamHeader and (*Message).segmentSizes, both of which are removed.

zombiezen · 2017-03-23T21:09:54Z

mem_test.go

@@ -708,6 +708,47 @@ func TestDecoder_MaxMessageSize(t *testing.T) {
 	}
 }

+func TestStreamHeaderPadding(t *testing.T) {


Add a comment noting that this is a regression check for header buffer padding.

…ncode." This reverts commit 994ce2d.

Use net.Buffers in Go 1.8 for encoding.

d4ed121

zombiezen suggested changes Mar 17, 2017

View reviewed changes

tmthrgd added 6 commits March 18, 2017 08:47

Breakout packed logic from (*Encoder).write into seperate function

76a28bf

Use the same signature for (*Encoder).write

0cbfe5c

Add Tom Thorogood to AUTHORS+CONTRIBUTORS

540d327

Reuse bufs between calls to (*Encoder).Encode

1937cea

Inline (*Message).segmentSizes into (*Encoder).Encode

871f9cd

Reuse sizes between calls to (*Encoder).Encode

2b9fa68

zombiezen suggested changes Mar 20, 2017

View reviewed changes

tmthrgd added 5 commits March 21, 2017 09:08

Remove TODO from (*Encoder).Encode

d6f7290

Let append handle resizing of sizes and bufs in (*Encoder).Encode

bf80d62

Inline marshalStreamHeader logic into (*Encoder).Encode.

8c874eb

This also fixes TestStreamHeaderPadding introduced in 3f1f84d. This still leaves marshalStreamHeader to be removed from (*Message).Marshal.

Make (*Message).Marshal{,Packed} wrappers around (*Encoder).Encode.

994ce2d

This removes all uses of marshalStreamHeader and (*Message).segmentSizes, both of which are removed.

zombiezen reviewed Mar 23, 2017

View reviewed changes

tmthrgd added 2 commits March 24, 2017 09:09

Add explanation comment to TestStreamHeaderPadding

b9ed916

Revert "Make (*Message).Marshal{,Packed} wrappers around (*Encoder).E…

98a420c

…ncode." This reverts commit 994ce2d.

tmthrgd mentioned this pull request Mar 26, 2017

Add 'Code generated by capnp.' comment to capnpc-go #73

Merged

zombiezen approved these changes Mar 29, 2017

View reviewed changes

zombiezen merged commit 69ac692 into capnproto:master Mar 29, 2017

tmthrgd deleted the net.Buffers branch March 29, 2017 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use net.Buffers in Go 1.8 for encoding. #70

Use net.Buffers in Go 1.8 for encoding. #70

tmthrgd commented Mar 17, 2017

zombiezen left a comment

zombiezen Mar 17, 2017

tmthrgd Mar 17, 2017

zombiezen Mar 17, 2017

tmthrgd Mar 17, 2017

zombiezen Mar 17, 2017

tmthrgd Mar 17, 2017

zombiezen Mar 17, 2017

tmthrgd Mar 17, 2017

zombiezen Mar 20, 2017

tmthrgd Mar 20, 2017

zombiezen Mar 20, 2017

tmthrgd Mar 20, 2017

zombiezen Mar 20, 2017

tmthrgd Mar 20, 2017 •

edited

Loading

zombiezen Mar 20, 2017

tmthrgd Mar 20, 2017

zombiezen Mar 20, 2017

tmthrgd Mar 20, 2017 •

edited

Loading

tmthrgd Mar 20, 2017 •

edited

Loading

zombiezen Mar 23, 2017

tmthrgd Mar 23, 2017

zombiezen Mar 20, 2017

tmthrgd Mar 20, 2017 •

edited

Loading

zombiezen Mar 23, 2017

tmthrgd Mar 23, 2017

zombiezen Mar 23, 2017

tmthrgd Mar 23, 2017

		@@ -571,40 +573,51 @@ func (e Encoder) Encode(m Message) error {
		if nsegs == 0 {

Use net.Buffers in Go 1.8 for encoding. #70

Use net.Buffers in Go 1.8 for encoding. #70

Conversation

tmthrgd commented Mar 17, 2017

zombiezen 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmthrgd Mar 20, 2017 • 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

tmthrgd Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

tmthrgd Mar 20, 2017 • 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

tmthrgd Mar 20, 2017 • 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

Choose a reason for hiding this comment

tmthrgd Mar 20, 2017 •

edited

Loading

tmthrgd Mar 20, 2017 •

edited

Loading

tmthrgd Mar 20, 2017 •

edited

Loading

tmthrgd Mar 20, 2017 •

edited

Loading