Before, if an underlying writer errored within 10 bytes (plus any gzip
header metadata), a gzip.Write would erroneously report up to 10 bytes
written that were not actually written of the input slice. This is
especially problematic when the input slice is less than 10 bytes.
The error came from counting the 10 header byte write. If writing the
header is completely successful, the 10 bytes written is overridden by
the flate write with the input slice.
This removes counting the 10 required header bytes, and also changes the
return to use zero until the slice is used.
The old Write could return one byte written when it actually was not.
This is difficult to verify because the smallest input slice is one
byte; a test checking that the input slice was the byte written would be
quite involved. Thankfully, gzip's minimum header write is 10 bytes. If
we test that two bytes are not falsely written, we indirectly cover the
one byte case.
Fixes#24625
Change-Id: I1c1f8cd791e0c4cffc22aa8acd95186582c832ba
Reviewed-on: https://go-review.googlesource.com/103861
Reviewed-by: Joe Tsai <joetsai@google.com>
Run-TryBot: Joe Tsai <joetsai@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Previously, the read method checked whether the current block
was fully consumed or not based on whether the buffer could be filled
with a non-zero number of bytes. This check is problematic because
zero bytes could be read if the provided buffer is empty.
We fix this case by simply checking for whether the input buffer
provided by the user was empty or not. If empty, we assume that
we could not read any bytes because the buffer was too small,
rather than indicating that the current block was fully exhausted.
This check causes bzip2.Reader to be unable to make progress
on the next block unless a non-empty buffer is provided.
However, that is an entirely reasonable expectation since a
non-empty buffer needs to be provided eventually anyways to
read the actual contents of subsequent blocks.
Fixes#22028
Change-Id: I2bb1b2d54e78567baf2bf7b490a272c0853d7bfe
Reviewed-on: https://go-review.googlesource.com/66110
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Some constants were added to flate that seem to be an experimental
attempt at increasing the window size. However, according to RFC1951,
the largest window size is 32KiB, so these constants are non-standard.
Delete them.
Fixes#18458
Change-Id: Ia94989637ca031a56bce2548624fa48044caa7b9
Reviewed-on: https://go-review.googlesource.com/60490
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
The logic performs a series of shifts, which are useless given
that they are followed by an assignment that overrides the
value of the previous computation.
I suspect (but cannot prove) that this is leftover logic from an
original approach that attempted to store both the Huffman code
and the length within the same variable instead of using two
different variables as it currently does now.
Fixes#17949
Change-Id: Ibf6c807c6cef3b28bfdaf2b68d9bc13503ac21b2
Reviewed-on: https://go-review.googlesource.com/44091
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This reverts commit 467109bf56.
Replaced by a improved strategy later in the CL relation chain.
Change-Id: Ib90813b5a6c4716b563c8496013d2d57f9c022b8
Reviewed-on: https://go-review.googlesource.com/36066
Reviewed-by: David Crawshaw <crawshaw@golang.org>
Run-TryBot: Elias Naur <elias.naur@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The working directory is now adjusted to match the typical Go test
working directory in main, as the old trick for adjusting earlier
stopped working with the latest version of LLDB bugs.
That means the small number of places where testdata files are
read before main is called no longer work. This CL adjusts those
reads to happen after main is called. (This has the bonus effect of
not reading some benchmark testdata files in all.bash.)
Fixes compress/bzip2, go/doc, go/parser, os, and time package
tests on the iOS builder.
Change-Id: If60f026aa7848b37511c36ac5e3985469ec25209
Reviewed-on: https://go-review.googlesource.com/35255
Run-TryBot: David Crawshaw <crawshaw@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Ranging over an array causes the array to be copied over to the
stack, which cause large re-growths. Instead, we should iterate
over slices of the array.
Also, assigning a large struct literal uses the stack even
though the actual fields being populated are small in comparison
to the entirety of the struct (see #18636).
Fixing the stack growth does not alter CPU-time performance much
since the stack-growth and copying was such a tiny portion of the
compression work:
name old time/op new time/op delta
Encode/Digits/Default/1e4-8 332µs ± 1% 332µs ± 1% ~ (p=0.796 n=10+10)
Encode/Digits/Default/1e5-8 5.07ms ± 2% 5.05ms ± 1% ~ (p=0.815 n=9+8)
Encode/Digits/Default/1e6-8 53.7ms ± 1% 53.9ms ± 1% ~ (p=0.075 n=10+10)
Encode/Twain/Default/1e4-8 380µs ± 1% 380µs ± 1% ~ (p=0.684 n=10+10)
Encode/Twain/Default/1e5-8 5.79ms ± 2% 5.79ms ± 1% ~ (p=0.497 n=9+10)
Encode/Twain/Default/1e6-8 61.5ms ± 1% 61.8ms ± 1% ~ (p=0.247 n=10+10)
name old speed new speed delta
Encode/Digits/Default/1e4-8 30.1MB/s ± 1% 30.1MB/s ± 1% ~ (p=0.753 n=10+10)
Encode/Digits/Default/1e5-8 19.7MB/s ± 2% 19.8MB/s ± 1% ~ (p=0.795 n=9+8)
Encode/Digits/Default/1e6-8 18.6MB/s ± 1% 18.5MB/s ± 1% ~ (p=0.072 n=10+10)
Encode/Twain/Default/1e4-8 26.3MB/s ± 1% 26.3MB/s ± 1% ~ (p=0.616 n=10+10)
Encode/Twain/Default/1e5-8 17.3MB/s ± 2% 17.3MB/s ± 1% ~ (p=0.484 n=9+10)
Encode/Twain/Default/1e6-8 16.3MB/s ± 1% 16.2MB/s ± 1% ~ (p=0.238 n=10+10)
Updates #18636Fixes#18625
Change-Id: I471b20339bf675f63dc56d38b3acdd824fe23328
Reviewed-on: https://go-review.googlesource.com/35122
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
I used the slowtests.go tool as described in
https://golang.org/cl/32684 on packages that stood out.
go test -short std drops from ~56 to ~52 seconds.
This isn't a huge win, but it was mostly an exercise.
Updates #17751
Change-Id: I9f3402e36a038d71e662d06ce2c1d52f6c4b674d
Reviewed-on: https://go-review.googlesource.com/32751
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Previously, we were off by one.
Also fix a comment typo.
Change-Id: Ib94d23acc56d5fccd44144f71655481f98803ac8
Reviewed-on: https://go-review.googlesource.com/32149
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The GZIP format records the ModTime as an uint32 counting seconds since
the Unix epoch. The zero value is explicitly defined in section 2.3.1
as meaning no timestamp is available.
Currently, the Writer always encodes the ModTime even if it is the zero
time.Time value, which causes the Writer to try and encode the value
-62135596800 into the uint32 MTIME field. This causes an overflow and
results in our GZIP files having MTIME fields indicating a date in 2042-07-13.
We alter the Writer to only encode ModTime if the value does not underflow
the MTIME field (i.e., it is newer than the Unix epoch). We do not attempt
to fix what happens when the timestamp overflows in the year 2106.
We alter the Reader to only decode ModTime if the value is non-zero.
There is no risk of overflowing time.Time when decoding.
Fixes#17663
Change-Id: Ie1b65770c6342cd7b14aeebe10e5a49e6c9eb730
Reviewed-on: https://go-review.googlesource.com/32325
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Tests for determinism was not working as intended since io.Copybuffer
uses the io.WriterTo if available.
This exposed that level 0 (no compression) changed output
based on the number of writes and buffers given to the
writer.
Previously, Write would emit a new raw block (BTYPE=00) for
every non-empty call to Write.
This CL fixes it such that a raw block is only emitted upon
the following conditions:
* A full window is obtained (every 65535 bytes)
* Flush is called
* Close is called
Change-Id: I807f866d97e2db7820f11febab30a96266a6cbf1
Reviewed-on: https://go-review.googlesource.com/31174
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
This exposes HuffmanOnly in zlib and gzip packages, which is currently
unavailable.
Change-Id: If5d103bbc8b5fce2f5d740fd103a235c5d1ed7cd
Reviewed-on: https://go-review.googlesource.com/31186
Reviewed-by: Nigel Tao <nigeltao@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The incorrect table was used for estimating output size.
This can give suboptimal selection of entropy encoder in rare cases.
Change-Id: I8b358200f2d1f9a3f9b79a44269d7be704e1d2d9
Reviewed-on: https://go-review.googlesource.com/31172
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
In the event of an unexpected error, we should always flush available
decompressed data to the user.
Fixes#16924
Change-Id: I0bc0824c3201f3149e84e6a26e3dbcba72a1aae5
Reviewed-on: https://go-review.googlesource.com/28216
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
For persistent error handling, the methods of huffmanBitWriter have to be
consistent about how they check errors. It must either consistently
check error *before* every operation OR immediately *after* every
operation. Since most of the current logic uses the previous approach,
we apply the same style of error checking to writeBits and all calls
to Write such that they only operate if w.err is already nil going
into them.
The error handling approach is brittle and easily broken by future commits to
the code. In the near future, we should switch the logic to use panic at the
lowest levels and a recover at the edge of the public API to ensure
that errors are always persistent.
Fixes#16749
Change-Id: Ie1d83e4ed8842f6911a31e23311cd3cbf38abe8c
Reviewed-on: https://go-review.googlesource.com/27200
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
As rendered on https://tip.golang.org/pkg/compress/flate/, there is an
extra new-line because of the unexported constants in the same block.
<<<
const (
NoCompression = 0
BestSpeed = 1
BestCompression = 9
DefaultCompression = -1
HuffmanOnly = -2 // Disables match search and only does Huffman entropy reduction.
)
>>>
Instead, seperate the exported compression level constants into its own
const block. This is both more readable and also fixes the issue.
Change-Id: I60b7966c83fb53356c02e4640d05f55a3bee35b7
Reviewed-on: https://go-review.googlesource.com/23557
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This causes the large files to be loaded only once per benchmark.
This CL also serves as an example use case of sub(tests|-benchmarks).
This CL ensures that names are identical to the original
except for an added slashes. Things could be
simplified further if this restriction were dropped.
Change-Id: I45e303e158e3152e33d0d751adfef784713bf997
Reviewed-on: https://go-review.googlesource.com/23420
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Marcel van Lohuizen <mpvl@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Address two documentation issues:
1) Document that the GZIP and ZLIB footer is only verified when the
reader has been fully consumed.
2) The zlib reader is guaranteed to not read past the EOF if the
input io.Reader is also a io.ByteReader. This functionality was
documented in the flate and gzip packages but not on zlib.
Fixes#14867
Change-Id: I43d46b93e38f98a04901dc7d4f18ed2f9e09f6fb
Reviewed-on: https://go-review.googlesource.com/21218
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This makes compress/flate's version of Snappy diverge from the upstream
golang/snappy version, but the latter has a goal of matching C++ snappy
output byte-for-byte. Both C++ and the asm version of golang/snappy can
use a smaller N for the O(N) zero-initialization of the hash table when
the input is small, even if the pure Go golang/snappy algorithm cannot:
"var table [tableSize]uint16" zeroes all tableSize elements.
For this package, we don't have the match-C++-snappy goal, so we can use
a different (constant) hash table size.
This is a small win, in terms of throughput and output size, but it also
enables us to re-use the (constant size) hash table between
encodeBestSpeed calls, avoiding the cost of zero-initializing the hash
table altogether. This will be implemented in follow-up commits.
This package's benchmarks:
name old speed new speed delta
EncodeDigitsSpeed1e4-8 72.8MB/s ± 1% 73.5MB/s ± 1% +0.86% (p=0.000 n=10+10)
EncodeDigitsSpeed1e5-8 77.5MB/s ± 1% 78.0MB/s ± 0% +0.69% (p=0.000 n=10+10)
EncodeDigitsSpeed1e6-8 82.0MB/s ± 1% 82.7MB/s ± 1% +0.85% (p=0.000 n=10+9)
EncodeTwainSpeed1e4-8 65.1MB/s ± 1% 65.6MB/s ± 0% +0.78% (p=0.000 n=10+9)
EncodeTwainSpeed1e5-8 80.0MB/s ± 0% 80.6MB/s ± 1% +0.66% (p=0.000 n=9+10)
EncodeTwainSpeed1e6-8 81.6MB/s ± 1% 82.1MB/s ± 1% +0.55% (p=0.017 n=10+10)
Input size in bytes, output size (and time taken) before and after on
some larger files:
1073741824 57269781 ( 3183ms) 57269781 ( 3177ms) adresser.001
1000000000 391052000 ( 11071ms) 391051996 ( 11067ms) enwik9
1911399616 378679516 ( 13450ms) 378679514 ( 13079ms) gob-stream
8558382592 3972329193 ( 99962ms) 3972329193 ( 91290ms) rawstudio-mint14.tar
200000000 200015265 ( 776ms) 200015265 ( 774ms) sharnd.out
Thanks to Klaus Post for the original suggestion on cl/21021.
Change-Id: Ia4c63a8d1b92c67e1765ec5c3c8c69d289d9a6ce
Reviewed-on: https://go-review.googlesource.com/22604
Reviewed-by: Russ Cox <rsc@golang.org>
This encoding algorithm, which prioritizes speed over output size, is
based on Snappy's LZ77-style encoder: github.com/golang/snappy
This commit keeps the diff between this package's encodeBestSpeed
function and and Snappy's encodeBlock function as small as possible (see
the diff below). Follow-up commits will improve this package's
performance and output size.
This package's speed benchmarks:
name old speed new speed delta
EncodeDigitsSpeed1e4-8 40.7MB/s ± 0% 73.0MB/s ± 0% +79.18% (p=0.008 n=5+5)
EncodeDigitsSpeed1e5-8 33.0MB/s ± 0% 77.3MB/s ± 1% +134.04% (p=0.008 n=5+5)
EncodeDigitsSpeed1e6-8 32.1MB/s ± 0% 82.1MB/s ± 0% +156.18% (p=0.008 n=5+5)
EncodeTwainSpeed1e4-8 42.1MB/s ± 0% 65.0MB/s ± 0% +54.61% (p=0.008 n=5+5)
EncodeTwainSpeed1e5-8 46.3MB/s ± 0% 80.0MB/s ± 0% +72.81% (p=0.008 n=5+5)
EncodeTwainSpeed1e6-8 47.3MB/s ± 0% 81.7MB/s ± 0% +72.86% (p=0.008 n=5+5)
Here's the milliseconds taken, before and after this commit, to compress
a number of test files:
Go's src/compress/testdata files:
4 1 e.txt
8 4 Mark.Twain-Tom.Sawyer.txt
github.com/golang/snappy's benchmark files:
3 1 alice29.txt
12 3 asyoulik.txt
6 1 fireworks.jpeg
1 1 geo.protodata
1 0 html
2 2 html_x_4
6 3 kppkn.gtb
11 4 lcet10.txt
5 1 paper-100k.pdf
14 6 plrabn12.txt
17 6 urls.10K
Larger files linked to from
https://docs.google.com/spreadsheets/d/1VLxi-ac0BAtf735HyH3c1xRulbkYYUkFecKdLPH7NIQ/edit#gid=166102500
2409 3182 adresser.001
16757 11027 enwik9
13764 12946 gob-stream
153978 74317 rawstudio-mint14.tar
4371 770 sharnd.out
Output size is larger. In the table below, the first column is the input
size, the second column is the output size prior to this commit, the
third column is the output size after this commit.
100003 47707 50006 e.txt
387851 172707 182930 Mark.Twain-Tom.Sawyer.txt
152089 62457 66705 alice29.txt
125179 54503 57274 asyoulik.txt
123093 122827 123108 fireworks.jpeg
118588 18574 20558 geo.protodata
102400 16601 17305 html
409600 65506 70313 html_x_4
184320 49007 50944 kppkn.gtb
426754 166957 179355 lcet10.txt
102400 82126 84937 paper-100k.pdf
481861 218617 231988 plrabn12.txt
702087 241774 258020 urls.10K
1073741824 43074110 57269781 adresser.001
1000000000 365772256 391052000 enwik9
1911399616 340364558 378679516 gob-stream
8558382592 3807229562 3972329193 rawstudio-mint14.tar
200000000 200061040 200015265 sharnd.out
The diff between github.com/golang/snappy's encodeBlock function and
this commit's encodeBestSpeed function:
1c1,7
< func encodeBlock(dst, src []byte) (d int) {
---
> func encodeBestSpeed(dst []token, src []byte) []token {
> // This check isn't in the Snappy implementation, but there, the caller
> // instead of the callee handles this case.
> if len(src) < minNonLiteralBlockSize {
> return emitLiteral(dst, src)
> }
>
4c10
< // and len(src) <= maxBlockSize and maxBlockSize == 65536.
---
> // and len(src) <= maxStoreBlockSize and maxStoreBlockSize == 65535.
65c71
< if load32(src, s) == load32(src, candidate) {
---
> if s-candidate < maxOffset && load32(src, s) == load32(src, candidate) {
73c79
< d += emitLiteral(dst[d:], src[nextEmit:s])
---
> dst = emitLiteral(dst, src[nextEmit:s])
90c96
< // This is an inlined version of:
---
> // This is an inlined version of Snappy's:
93c99,103
< for i := candidate + 4; s < len(src) && src[i] == src[s]; i, s = i+1, s+1 {
---
> s1 := base + maxMatchLength
> if s1 > len(src) {
> s1 = len(src)
> }
> for i := candidate + 4; s < s1 && src[i] == src[s]; i, s = i+1, s+1 {
96c106,107
< d += emitCopy(dst[d:], base-candidate, s-base)
---
> // matchToken is flate's equivalent of Snappy's emitCopy.
> dst = append(dst, matchToken(uint32(s-base-3), uint32(base-candidate-minOffsetSize)))
114c125
< if uint32(x>>8) != load32(src, candidate) {
---
> if s-candidate >= maxOffset || uint32(x>>8) != load32(src, candidate) {
124c135
< d += emitLiteral(dst[d:], src[nextEmit:])
---
> dst = emitLiteral(dst, src[nextEmit:])
126c137
< return d
---
> return dst
This change is based on https://go-review.googlesource.com/#/c/21021/ by
Klaus Post, but it is a separate changelist as cl/21021 seems to have
stalled in code review, and the Go 1.7 feature freeze approaches.
Golang-dev discussion:
https://groups.google.com/d/topic/golang-dev/XYgHX9p8IOk/discussion and
of course cl/21021.
Change-Id: Ib662439417b3bd0b61c2977c12c658db3e44d164
Reviewed-on: https://go-review.googlesource.com/22370
Reviewed-by: Russ Cox <rsc@golang.org>
cmd and runtime were handled separately, and I'm intentionally skipped
syscall. This is the rest of the standard library.
CL generated mechanically with github.com/mdempsky/unconvert.
Change-Id: I9e0eff886974dedc37adb93f602064b83e469122
Reviewed-on: https://go-review.googlesource.com/22104
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
It's not a big deal (the for loop drops from 130-ish to 120-ish
milliseconds for me) but it's not a big change either.
Change-Id: I161a49caab5cae5a2b87866ed1dfb93627be8013
Reviewed-on: https://go-review.googlesource.com/22110
Reviewed-by: Klaus Post <klauspost@gmail.com>
Reviewed-by: Nigel Tao <nigeltao@golang.org>
RFC 1952, section 3.2.3 says:
>>>
If FHCRC is set, a CRC16 for the gzip header is present,
immediately before the compressed data. The CRC16 consists of the two
least significant bytes of the CRC32 for all bytes of the
gzip header up to and not including the CRC16.
<<<
Thus, instead of computing the CRC only over the first 10 bytes
of the header, we compute it over the whole header (minus CRC16).
Fixes#15070
Change-Id: I55703fd30b535b12abeb5e3962d4da0a86ed615a
Reviewed-on: https://go-review.googlesource.com/21466
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This improves the short version of the writer test.
First of all, it has a much quicker setup. Previously that
could take up towards 0.5 second.
Secondly, it will test all compression levels in short mode as well.
Execution time is 1.7s/0.03s for normal/short mode.
Change-Id: I275a21f712daff6f7125cc6a493415e86439cb19
Reviewed-on: https://go-review.googlesource.com/21800
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Rather than specifying every field that should be cleared in Reset,
it is better to just zero the entire struct and only preserve or set the
fields that we actually care about. This ensures that the Header field
is reset for the next use.
Fixes#15077
Change-Id: I41832e506d2d64c62b700aa1986e7de24a577511
Reviewed-on: https://go-review.googlesource.com/21465
Run-TryBot: Joe Tsai <joetsai@digital-static.net>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Changes made:
* Reader.flg is not used anywhere else other than readHeader and
does not need to be stored.
* Store Reader.digest and Writer.digest as uint32s rather than as
a hash.Hash32 and use the crc32.Update function instead. This simplifies
initialization logic since the zero value of uint32 is the initial
CRC-32 value. There are no performance detriments to doing this since
the hash.Hash32 returned by crc32 simply calls crc32.Update as well.
* s/[0:/[:/ Consistently use shorter notation for slicing.
* s/RFC1952/RFC 1952/ Consistently use RFC notation.
Change-Id: I55416a19f4836cbed943adaa3f672538ea5d166d
Reviewed-on: https://go-review.googlesource.com/21429
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Joe Tsai <joetsai@digital-static.net>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Rather than checking the block final bit on the next invocation
of nextBlock, we check it at the termination of the current block.
This ensures that we return (n, io.EOF) instead of (0, io.EOF)
more frequently for most streams.
However, there are certain situations where an eager io.EOF is not done:
1) We previously returned from Read because the write buffer of the internal
dictionary was full, and it just so happens that there is no more data
remaining in the stream.
2) There exists a [non-final, empty, raw block] after all blocks that
actually contain uncompressed data. We cannot return io.EOF eagerly here
since it would break flushing semantics.
Both situations happen infrequently, but it is still important to note that
this change does *not* guarantee that flate will *always* return (n, io.EOF).
Furthermore, this CL makes no changes to the pattern of ReadByte calls
to the underlying io.ByteReader.
Below is the motivation for this change, pulling the text from
@bradfitz's CL/21290:
net/http and other things work better when io.Reader implementations
return (n, io.EOF) at the end, instead of (n, nil) followed by (0,
io.EOF). Both are legal, but the standard library has been moving
towards n+io.EOF.
An investigation of net/http connection re-use in
https://github.com/google/go-github/pull/317 revealed that with gzip
compression + http/1.1 chunking, the net/http package was not
automatically reusing the underlying TCP connections when the final
EOF bytes were already read off the wire. The net/http package only
reuses the connection if the underlying Readers (many of them nested
in this case) all eagerly return io.EOF.
Previous related CLs:
https://golang.org/cl/76400046 - tls.Reader
https://golang.org/cl/58240043 - http chunked reader
In addition to net/http, this behavior also helps things like
ioutil.ReadAll (see comments about performance improvements in
https://codereview.appspot.com/49570044)
Updates #14867
Updates google/go-github#317
Change-Id: I637c45552efb561d34b13ed918b73c660f668378
Reviewed-on: https://go-review.googlesource.com/21302
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
The Read logic should not assume that only (0, io.EOF) is returned
instead of (n, io.EOF) where n is positive.
The fix done here is very similar to the fix to compress/zlib
in CL/20292.
Change-Id: Icb76258cdcf8cfa386a60bab330fefde46fc071d
Reviewed-on: https://go-review.googlesource.com/21308
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Travis Bischel
Ilya Tocar
Diogo Pinela
Joe Kyo
Joe Tsai
Kunpei Sakai
Nigel Tao
Elias Naur
Robert Griesemer
David Crawshaw
Brad Fitzpatrick
Klaus Post
Josh Bleecher Snyder
Emmanuel Odeke
Vladimir Mihailenco
Marcel van Lohuizen
Matthew Dempsky
Dominik Honnef