logdog: Add frame read/write library.

logdog: Add frame read/write library.

BUG=

Committed: https://github.com/luci/luci-go/commit/ea9b62df552da49335766fcd66d852cb2e6e14e5

[dnj, 2015-07-31 23:47:30]

dnj@chromium.org changed reviewers:
+ iannucci@chromium.org

[dnj, 2015-07-31 23:47:31]

PTAL

[iannucci, 2015-08-03 22:21:24]

https://chromiumcodereview.appspot.com/1253353008/diff/1/common/logdog/frame/...
File common/logdog/frame/reader.go (right):

https://chromiumcodereview.appspot.com/1253353008/diff/1/common/logdog/frame/...
common/logdog/frame/reader.go:33: }
interface assertion?

https://chromiumcodereview.appspot.com/1253353008/diff/1/common/logdog/frame/...
common/logdog/frame/reader.go:43: br = &simpleByteReader{Reader: r}
not sure if this is needed? why not just have this take interface{Reader;
ByteReader}?

https://chromiumcodereview.appspot.com/1253353008/diff/1/common/logdog/frame/...
common/logdog/frame/reader.go:74: buf [1]byte
not threadsafe? why have it in the struct at all? You end up doing an allocation
for the slice anyway...

https://chromiumcodereview.appspot.com/1253353008/diff/1/common/logdog/frame/...
File common/logdog/frame/writer.go (right):

https://chromiumcodereview.appspot.com/1253353008/diff/1/common/logdog/frame/...
common/logdog/frame/writer.go:18: }
interface assertion

https://chromiumcodereview.appspot.com/1253353008/diff/1/common/logdog/frame/...
common/logdog/frame/writer.go:22: var sizeBuf [binary.MaxVarintLen64]byte
why not just `make([]byte, binary.MaxVarintLen64)`?

https://chromiumcodereview.appspot.com/1253353008/diff/1/common/logdog/frame/...
common/logdog/frame/writer.go:48: // (even for zero bytes) has been called for
that frame.
I don't think this is true anymore? Looks like this is just Writer+Flush, not
WriterCloser+Flush

[dnj, 2015-08-03 22:42:58]

https://chromiumcodereview.appspot.com/1253353008/diff/1/common/logdog/frame/...
File common/logdog/frame/reader.go (right):

https://chromiumcodereview.appspot.com/1253353008/diff/1/common/logdog/frame/...
common/logdog/frame/reader.go:33: }
On 2015/08/03 22:21:24, iannucci wrote:
> interface assertion?

Going with vadimsh@ style: since NewReader returns a Reader interface from
&reader, the assertion isn't necessary.

https://chromiumcodereview.appspot.com/1253353008/diff/1/common/logdog/frame/...
common/logdog/frame/reader.go:43: br = &simpleByteReader{Reader: r}
On 2015/08/03 22:21:24, iannucci wrote:
> not sure if this is needed? why not just have this take interface{Reader;
> ByteReader}?

It puts the burden on the caller to implement ByteReader for Reader streams.
This way, the public interface stays simple.

https://chromiumcodereview.appspot.com/1253353008/diff/1/common/logdog/frame/...
common/logdog/frame/reader.go:74: buf [1]byte
On 2015/08/03 22:21:24, iannucci wrote:
> not threadsafe? why have it in the struct at all? You end up doing an
allocation
> for the slice anyway...

No, struct is not threadsafe :) Most readers aren't!

Since the array is static, it should be allocated as part of the struct
allocation.

https://chromiumcodereview.appspot.com/1253353008/diff/1/common/logdog/frame/...
File common/logdog/frame/writer.go (right):

https://chromiumcodereview.appspot.com/1253353008/diff/1/common/logdog/frame/...
common/logdog/frame/writer.go:18: }
On 2015/08/03 22:21:24, iannucci wrote:
> interface assertion

(see above, NewWriter).

https://chromiumcodereview.appspot.com/1253353008/diff/1/common/logdog/frame/...
common/logdog/frame/writer.go:22: var sizeBuf [binary.MaxVarintLen64]byte
On 2015/08/03 22:21:24, iannucci wrote:
> why not just `make([]byte, binary.MaxVarintLen64)`?

I _think_ because this is a stack allocation, and I'd prefer that to a heap
allocation since the number is known/small.

https://chromiumcodereview.appspot.com/1253353008/diff/1/common/logdog/frame/...
common/logdog/frame/writer.go:48: // (even for zero bytes) has been called for
that frame.
On 2015/08/03 22:21:24, iannucci wrote:
> I don't think this is true anymore? Looks like this is just Writer+Flush, not
> WriterCloser+Flush

Oh sorry, updated.

[iannucci, 2015-08-04 17:13:51]

lgtm % C++ nonsense :)

https://codereview.chromium.org/1253353008/diff/1/common/logdog/frame/reader.go
File common/logdog/frame/reader.go (right):

https://codereview.chromium.org/1253353008/diff/1/common/logdog/frame/reader....
common/logdog/frame/reader.go:74: buf [1]byte
On 2015/08/03 22:42:58, dnj wrote:
> On 2015/08/03 22:21:24, iannucci wrote:
> > not threadsafe? why have it in the struct at all? You end up doing an
> allocation
> > for the slice anyway...
> 
> No, struct is not threadsafe :) Most readers aren't!
> 
> Since the array is static, it should be allocated as part of the struct
> allocation.

Right, but you have to slice it every time, which is an allocation. Might as
well just do a make in the function right?

https://codereview.chromium.org/1253353008/diff/1/common/logdog/frame/writer.go
File common/logdog/frame/writer.go (right):

https://codereview.chromium.org/1253353008/diff/1/common/logdog/frame/writer....
common/logdog/frame/writer.go:22: var sizeBuf [binary.MaxVarintLen64]byte
On 2015/08/03 22:42:58, dnj wrote:
> On 2015/08/03 22:21:24, iannucci wrote:
> > why not just `make([]byte, binary.MaxVarintLen64)`?
> 
> I _think_ because this is a stack allocation, and I'd prefer that to a heap
> allocation since the number is known/small.

No it's not... the compiler analyzes all pointers to determine if they escape in
order to put them on the stack or the heap. You can pass a pointer to sizeBuf
out of this function without issue. This isn't C++ :)

Also... you realize that stack frames in go are allocated on the heap, right?

https://golang.org/doc/faq#stack_or_heap

https://groups.google.com/forum/#!topic/golang-nuts/KdbtOqNK6JQ

https://codereview.chromium.org/1253353008/diff/40001/common/logdog/frame/rea...
File common/logdog/frame/reader.go (right):

https://codereview.chromium.org/1253353008/diff/40001/common/logdog/frame/rea...
common/logdog/frame/reader.go:91: return data, nil
I think ioutils.ReadAll is what you want

https://codereview.chromium.org/1253353008/diff/40001/common/logdog/frame/rea...
File common/logdog/frame/reader_test.go (right):

https://codereview.chromium.org/1253353008/diff/40001/common/logdog/frame/rea...
common/logdog/frame/reader_test.go:26: for _, d := range data {
s/d/chunk

maybe?

https://codereview.chromium.org/1253353008/diff/40001/common/logdog/frame/rea...
common/logdog/frame/reader_test.go:44: nativeReadByte bool
why not just have 2 struct types (e.g. testByteReaderNative)?

[dnj, 2015-08-04 17:51:42]

https://codereview.chromium.org/1253353008/diff/1/common/logdog/frame/writer.go
File common/logdog/frame/writer.go (right):

https://codereview.chromium.org/1253353008/diff/1/common/logdog/frame/writer....
common/logdog/frame/writer.go:22: var sizeBuf [binary.MaxVarintLen64]byte
On 2015/08/04 17:13:50, iannucci wrote:
> On 2015/08/03 22:42:58, dnj wrote:
> > On 2015/08/03 22:21:24, iannucci wrote:
> > > why not just `make([]byte, binary.MaxVarintLen64)`?
> > 
> > I _think_ because this is a stack allocation, and I'd prefer that to a heap
> > allocation since the number is known/small.
> 
> No it's not... the compiler analyzes all pointers to determine if they escape
in
> order to put them on the stack or the heap. You can pass a pointer to sizeBuf
> out of this function without issue. This isn't C++ :)

Oh, interesting. I didn't know that. This suggests that the slice itself can
also be allocated on the stack.

It looks like you aren't supposed to care. "make" probably collapses into the
same things (array allocation + several slice allocations). It _should_ be easy
enough for Go compiler to choose stack here in either case.

I'll go ahead and use "make" because it is more conventional, but I don't think
there's anything inherently wrong with this notation even if the attempt to
optimize is taken out of the picture.

> Also... you realize that stack frames in go are allocated on the heap, right?

Sure, but even so stack growth is more efficient than individual heap
allocations.

https://codereview.chromium.org/1253353008/diff/40001/common/logdog/frame/rea...
File common/logdog/frame/reader.go (right):

https://codereview.chromium.org/1253353008/diff/40001/common/logdog/frame/rea...
common/logdog/frame/reader.go:91: return data, nil
On 2015/08/04 17:13:50, iannucci wrote:
> I think ioutils.ReadAll is what you want

No wai! ioutils.ReadAll has no a priori knowledge of the size of the data that
is being read, so it will read into a buffer in chunks, then return that buffer.
Since I know the size, I have the advantage of allocating exactly as much as I
need here.

https://codereview.chromium.org/1253353008/diff/40001/common/logdog/frame/rea...
File common/logdog/frame/reader_test.go (right):

https://codereview.chromium.org/1253353008/diff/40001/common/logdog/frame/rea...
common/logdog/frame/reader_test.go:26: for _, d := range data {
On 2015/08/04 17:13:50, iannucci wrote:
> s/d/chunk
> 
> maybe?

Done.

https://codereview.chromium.org/1253353008/diff/40001/common/logdog/frame/rea...
common/logdog/frame/reader_test.go:44: nativeReadByte bool
On 2015/08/04 17:13:51, iannucci wrote:
> why not just have 2 struct types (e.g. testByteReaderNative)?

Opted to just have a separate error field for ReadByte. The goal was for one
specific test to succeed in reading the frame header but later fail reading the
data.

[iannucci, 2015-08-04 18:08:46]

lgtm

[dnj, 2015-11-04 18:22:29]

The CQ bit was checked by dnj@chromium.org

[dnj, 2015-11-04 18:22:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/1253353008/#ps100001
(title: "Refresh?")

[commit-bot: I haz the power, 2015-11-04 18:22:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1253353008/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1253353008/100001

[commit-bot: I haz the power, 2015-11-04 18:25:11]

Committed patchset #6 (id:100001) as
https://github.com/luci/luci-go/commit/ea9b62df552da49335766fcd66d852cb2e6e14e5