code review 199400043: snappy: implement the framing format.

snappy: implement the framing format.

The format is described at
https://code.google.com/p/snappy/source/browse/trunk/framing_format.txt

[nigeltao, 2015-02-06 07:08:46]

Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/snappy-go

[gobot, 2015-02-06 07:11:04]

Replacing golang-dev with golang-codereviews.

To the author of this CL: 

If you are using 'hg mail -r golang-dev' to mail the CL, use simply 'hg mail'
instead.

If you did not name golang-dev explicitly and it was still added to the CL,
it means your working copy of the repo has a stale codereview.cfg
(or lib/codereview/codereview.cfg).
Please run 'hg sync' to update your client to the most recent codereview.cfg.
If the most recent codereview.cfg has defaultcc set to golang-dev instead of
golang-codereviews, please send a CL correcting it.

Thanks very much.

[nigeltao, 2015-02-09 22:48:00]

bradfitz: ping.

https://codereview.appspot.com/199400043/

[bradfitz, 2015-02-09 22:52:52]

Chrome or Rietveld is hanging trying to log in.  "Resolving host in proxy
script.... ", says Chrome.

What is this for?


On Mon, Feb 9, 2015 at 2:48 PM, Nigel Tao <nigeltao@golang.org> wrote:

> bradfitz: ping.
>
> https://codereview.appspot.com/199400043/
>

[bradfitz, 2015-02-09 22:57:33]

https://codereview.appspot.com/199400043/diff/40001/snappy/decode.go
File snappy/decode.go (right):

https://codereview.appspot.com/199400043/diff/40001/snappy/decode.go#newcode137
snappy/decode.go:137: decoded: make([]byte, maxUncompressedChunkLen),
do you have a plan for garbage?

https://codereview.appspot.com/199400043/diff/40001/snappy/decode.go#newcode138
snappy/decode.go:138: buf:     make([]byte,
MaxEncodedLen(maxUncompressedChunkLen)+checksumSize),
likewise

https://codereview.appspot.com/199400043/diff/40001/snappy/decode.go#newcode192
snappy/decode.go:192: case chunkType == 0x00:
constants for these magic bytes?

https://codereview.appspot.com/199400043/diff/40001/snappy/encode.go
File snappy/encode.go (right):

https://codereview.appspot.com/199400043/diff/40001/snappy/encode.go#newcode183
snappy/encode.go:183: enc: make([]byte, MaxEncodedLen(maxUncompressedChunkLen)),
garbage.

who's using this?

https://codereview.appspot.com/199400043/diff/40001/snappy/encode.go#newcode229
snappy/encode.go:229: w.buf[0] = chunkType
if you [ab]use append, you only get one bounds check I believe, if you care
about speed here.

https://codereview.appspot.com/199400043/diff/40001/snappy/encode.go#newcode237
snappy/encode.go:237: if _, err = w.w.Write(w.buf[:]); err != nil {
trusting io.Writer implementations to return the correct n when err == nil
always makes me nervous, but fine.

[nigeltao, 2015-02-09 23:16:18]

https://codereview.appspot.com/199400043/diff/40001/snappy/decode.go
File snappy/decode.go (right):

https://codereview.appspot.com/199400043/diff/40001/snappy/decode.go#newcode137
snappy/decode.go:137: decoded: make([]byte, maxUncompressedChunkLen),
On 2015/02/09 22:57:33, bradfitz wrote:
> do you have a plan for garbage?

I don't understand the question. A snappy reader has internal buffers, just like
a bufio reader or a gzip reader.

https://codereview.appspot.com/199400043/diff/40001/snappy/decode.go#newcode192
snappy/decode.go:192: case chunkType == 0x00:
On 2015/02/09 22:57:33, bradfitz wrote:
> constants for these magic bytes?

Done.

https://codereview.appspot.com/199400043/diff/40001/snappy/encode.go
File snappy/encode.go (right):

https://codereview.appspot.com/199400043/diff/40001/snappy/encode.go#newcode183
snappy/encode.go:183: enc: make([]byte, MaxEncodedLen(maxUncompressedChunkLen)),
On 2015/02/09 22:57:33, bradfitz wrote:
> who's using this?

I also don't understand this question. The w.enc field is used below.

https://codereview.appspot.com/199400043/diff/40001/snappy/encode.go#newcode229
snappy/encode.go:229: w.buf[0] = chunkType
On 2015/02/09 22:57:33, bradfitz wrote:
> if you [ab]use append, you only get one bounds check I believe, if you care
> about speed here.

I don't think there's a bounds check. w.buf is an array, not a slice.

https://codereview.appspot.com/199400043/diff/40001/snappy/encode.go#newcode237
snappy/encode.go:237: if _, err = w.w.Write(w.buf[:]); err != nil {
On 2015/02/09 22:57:33, bradfitz wrote:
> trusting io.Writer implementations to return the correct n when err == nil
> always makes me nervous, but fine.

I'm not overly fussed about the int returned by io.Writer implementations. In
practice, I think that the only code that cares are those that have to
themselves implement the io.Writer interface. Especially for compression (whose
atoms are bits, not bytes), counting bytes in case of partial success seems more
trouble than its worth.

[bradfitz, 2015-02-09 23:25:01]

LGTM

As for garbage: if you create a bunch of these readers/writers, each is
wasting 65k or more of garbage.  This is why we ended up adding Reset
methods to so many things (flate/gzip/bufio readers, etc)



On Mon, Feb 9, 2015 at 3:16 PM, <nigeltao@golang.org> wrote:

>
> https://codereview.appspot.com/199400043/diff/40001/snappy/decode.go
> File snappy/decode.go (right):
>
> https://codereview.appspot.com/199400043/diff/40001/
> snappy/decode.go#newcode137
> snappy/decode.go:137: decoded: make([]byte, maxUncompressedChunkLen),
> On 2015/02/09 22:57:33, bradfitz wrote:
>
>> do you have a plan for garbage?
>>
>
> I don't understand the question. A snappy reader has internal buffers,
> just like a bufio reader or a gzip reader.
>
> https://codereview.appspot.com/199400043/diff/40001/
> snappy/decode.go#newcode192
> snappy/decode.go:192: case chunkType == 0x00:
> On 2015/02/09 22:57:33, bradfitz wrote:
>
>> constants for these magic bytes?
>>
>
> Done.
>
> https://codereview.appspot.com/199400043/diff/40001/snappy/encode.go
> File snappy/encode.go (right):
>
> https://codereview.appspot.com/199400043/diff/40001/
> snappy/encode.go#newcode183
> snappy/encode.go:183: enc: make([]byte,
> MaxEncodedLen(maxUncompressedChunkLen)),
> On 2015/02/09 22:57:33, bradfitz wrote:
>
>> who's using this?
>>
>
> I also don't understand this question. The w.enc field is used below.
>
> https://codereview.appspot.com/199400043/diff/40001/
> snappy/encode.go#newcode229
> snappy/encode.go:229: w.buf[0] = chunkType
> On 2015/02/09 22:57:33, bradfitz wrote:
>
>> if you [ab]use append, you only get one bounds check I believe, if you
>>
> care
>
>> about speed here.
>>
>
> I don't think there's a bounds check. w.buf is an array, not a slice.
>
> https://codereview.appspot.com/199400043/diff/40001/
> snappy/encode.go#newcode237
> snappy/encode.go:237: if _, err = w.w.Write(w.buf[:]); err != nil {
> On 2015/02/09 22:57:33, bradfitz wrote:
>
>> trusting io.Writer implementations to return the correct n when err ==
>>
> nil
>
>> always makes me nervous, but fine.
>>
>
> I'm not overly fussed about the int returned by io.Writer
> implementations. In practice, I think that the only code that cares are
> those that have to themselves implement the io.Writer interface.
> Especially for compression (whose atoms are bits, not bytes), counting
> bytes in case of partial success seems more trouble than its worth.
>
> https://codereview.appspot.com/199400043/
>

[nigeltao, 2015-02-10 00:56:51]

*** Submitted as
https://code.google.com/p/snappy-go/source/detail?r=780a37fbd5b6 ***

snappy: implement the framing format.

The format is described at
https://code.google.com/p/snappy/source/browse/trunk/framing_format.txt

LGTM=bradfitz
R=bradfitz
CC=golang-codereviews
https://codereview.appspot.com/199400043