- 01 Apr, 2016 9 commits
-
-
David Chase authored
Added a debug flag "-d closure" to explain compilation of closures (should this be done some other way? Should we rewrite the "-m" flag to "-d escapes"?) Used this to discover that cause was an OXXX node in the captured vars list, and in turn noticed that OXXX nodes are explicitly ignored in all other processing of captured variables. Couldn't figure out a reproducer, did verify that this OXXX was not caused by an unnamed return value (which is one use of these). Verified lack of heap allocation by examining -S output. Assembly: (runtime/mgc.go:1371) PCDATA $0, $2 (runtime/mgc.go:1371) CALL "".notewakeup(SB) (runtime/mgc.go:1377) LEAQ "".gcBgMarkWorker.func1·f(SB), AX (runtime/mgc.go:1404) MOVQ AX, (SP) (runtime/mgc.go:1404) MOVQ "".autotmp_2242+88(SP), CX (runtime/mgc.go:1404) MOVQ CX, 8(SP) (runtime/mgc.go:1404) LEAQ go.string."GC worker (idle)"(SB), AX (runtime/mgc.go:1404) MOVQ AX, 16(SP) (runtime/mgc.go:1404) MOVQ $16, 24(SP) (runtime/mgc.go:1404) MOVB $20, 32(SP) (runtime/mgc.go:1404) MOVQ $0, 40(SP) (runtime/mgc.go:1404) PCDATA $0, $2 (runtime/mgc.go:1404) CALL "".gopark(SB) Added a check for compiling_runtime to ensure that this is caught in the future. Added a test to test the check. Verified that 1.5.3 did NOT reject the test case when compiled with -+ flag, so this is not a recently added bug. Cause of bug is two-part -- there was no leaking closure detection ever, and instead it relied on capture-of-variables to trigger compiling_runtime test, but closures improved in 1.5.3 so that mere capture of a value did not also capture the variable, which thus allowed closures to escape, as well as this case where the escape was spurious. In fixedbugs/issue14999.go, compare messages for f and g; 1.5.3 would reject g, but not f. 1.4 rejects both because 1.4 heap-allocates parameter x for both. Fixes #14999. Change-Id: I40bcdd27056810628e96763a44f2acddd503aee1 Reviewed-on: https://go-review.googlesource.com/21322 Run-TryBot: David Chase <drchase@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by:
Keith Randall <khr@golang.org>
-
Dominik Honnef authored
Change-Id: Iba82a5bd3846f7ab038cc10ec72ff6bcd2c0b484 Reviewed-on: https://go-review.googlesource.com/21377 Run-TryBot: Dave Cheney <dave@cheney.net> Reviewed-by:
Dave Cheney <dave@cheney.net>
-
Dave Cheney authored
Fix build error on darwin/{arm,arm64} caused by a62ae9f6. Change-Id: I7da4d147a529b11b3e71352a9276a57595ec4b05 Reviewed-on: https://go-review.googlesource.com/21356 Run-TryBot: Dave Cheney <dave@cheney.net> Reviewed-by:
Brad Fitzpatrick <bradfitz@golang.org>
-
Brad Fitzpatrick authored
Also cleans up return parameter stutter and missing periods. Change-Id: I47f5c230227ddfd1b105d5e06842f89ffea50760 Reviewed-on: https://go-review.googlesource.com/21362Reviewed-by:
Andrew Gerrand <adg@golang.org>
-
Brad Fitzpatrick authored
Issue #8633 (and #9134) noted that we didn't document the rules about closing the Response.Body when Client.Do returned both a non-nil *Response and a non-nil error (which can only happen when the user's CheckRedirect returns an error). In the process of investigating, I cleaned this code up a bunch, but no user-visible behavior should have changed, except perhaps some better error messages in some cases. It turns out it's always been the case that when a CheckRedirect error occurs, the Response.Body is already closed. Document that. And the new code makes that more obvious too. Fixes #8633 Change-Id: Ibc40cc786ad7fc4e0cf470d66bb559c3b931684d Reviewed-on: https://go-review.googlesource.com/21364 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by:
Andrew Gerrand <adg@golang.org>
-
Ian Lance Taylor authored
The change in 20907 fixed varexpr but broke aliased. After that change, a reference to a field in a struct would not be seen as aliasing itself. Before that change, it would, but only because all fields in a struct aliased everything. This CL changes the compiler to consider all references to a field as aliasing all other fields in that struct. This is imperfect--a reference to one field does not alias another field--but is a simple fix for the immediate problem. A better fix would require tracking the specific fields as well. Fixes #15042. Change-Id: I5c95c0dd7b0699e53022fce9bae2e8f50d6d1d04 Reviewed-on: https://go-review.googlesource.com/21390 Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by:
Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by:
Matthew Dempsky <mdempsky@google.com>
-
Brad Fitzpatrick authored
... when the backend can't be reached. Fixes #9864 Change-Id: I9e4814824943060e0101b6236d6dfd8d7822b4c7 Reviewed-on: https://go-review.googlesource.com/21363Reviewed-by:
Andrew Gerrand <adg@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
-
Brad Fitzpatrick authored
Updates x/net/http2 to git rev 31df19d6 for changes since Go 1.6. The main change was https://go-review.googlesource.com/19726 (move merging of HEADERS and CONTINUATION into Framer), but there were a few garbage reduction changes too. Change-Id: I882443d20749f8638f637a2835efe92538c95d31 Reviewed-on: https://go-review.googlesource.com/21365 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by:
Andrew Gerrand <adg@golang.org>
-
Brad Fitzpatrick authored
The default is 10MB, like http2, but can be configured with a new field http.Transport.MaxResponseHeaderBytes. Fixes #9115 Change-Id: I01808ac631ce4794ef2b0dfc391ed51cf951ceb1 Reviewed-on: https://go-review.googlesource.com/21329 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by:
Emmanuel Odeke <emm.odeke@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by:
Andrew Gerrand <adg@golang.org>
-
- 31 Mar, 2016 31 commits
-
-
Alexandru Moșoi authored
ANDQConst show up occassionally because of right shifting lowering. ORs and XORs are already folded properly during generic. Change-Id: I2f9134679555029c641264ce5333d70e167c65f7 Reviewed-on: https://go-review.googlesource.com/21375Reviewed-by:
Keith Randall <khr@golang.org> Run-TryBot: Alexandru Moșoi <alexandru@mosoi.ro> TryBot-Result: Gobot Gobot <gobot@golang.org>
-
Alexandru Moșoi authored
Very common, cuts about 70k from pkg/tools/linux_amd64/* binaries. Change-Id: Ied0c049e56e56a56810c781435d79027fbcaf274 Reviewed-on: https://go-review.googlesource.com/21374Reviewed-by:
Keith Randall <khr@golang.org> Run-TryBot: Alexandru Moșoi <alexandru@mosoi.ro>
-
Christopher Nelson authored
In a number of places the code was joining filepaths explicitly with "/", instead of using filepath.Join. This may cause problems on Windows (or other) platforms. This is in support of https://go-review.googlesource.com/#/c/18057 Change-Id: Ieb1334f35ddb2e125be690afcdadff8d7b0ace10 Reviewed-on: https://go-review.googlesource.com/21369Reviewed-by:
Brad Fitzpatrick <bradfitz@golang.org>
-
Joe Tsai authored
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>
-
Keith Randall authored
Compound AUTO types weren't named previously. That was because live variable analysis (plive.go) doesn't handle spilling to compound types. It can't handle them because there is no valid place to put VARDEFs when regalloc is spilling compound types. compound types = multiword builtin types: complex, string, slice, and interface. Instead, we split named AUTOs into individual one-word variables. For example, a string s gets split into a byte ptr s.ptr and an integer s.len. Those two variables can be spilled to / restored from independently. As a result, live variable analysis can handle them because they are one-word objects. This CL will change how AUTOs are described in DWARF information. Consider the code: func f(s string, i int) int { x := s[i:i+5] g() return lookup(x) } The old compiler would spill x to two consecutive slots on the stack, both named x (at offsets 0 and 8). The new compiler spills the pointer of x to a slot named x.ptr. It doesn't spill x.len at all, as it is a constant (5) and can be rematerialized for the call to lookup. So compound objects may not be spilled in their entirety, and even if they are they won't necessarily be contiguous. Such is the price of optimization. Re-enable live variable analysis tests. One test remains disabled, it fails because of #14904. Change-Id: I8ef2b5ab91e43a0d2136bfc231c05d100ec0b801 Reviewed-on: https://go-review.googlesource.com/21233 Run-TryBot: Keith Randall <khr@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by:
David Chase <drchase@google.com>
-
Brad Fitzpatrick authored
The conventional name for a sync.Mutex is "mu". These "lk" names date back to a time before conventions. Change-Id: Iee57f9f4423d04269e1125b5d82455c453aac26f Reviewed-on: https://go-review.googlesource.com/21361 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by:
Emmanuel Odeke <emm.odeke@gmail.com> Reviewed-by:
Brad Fitzpatrick <bradfitz@golang.org>
-
Keith Randall authored
For idx1 ops, SP can appear in the index slot. Swap SP into the base register slot so we can encode the instruction. Fixes #15053 Change-Id: I19000cc9d6c86c7611743481e6e2cb78b1ef04eb Reviewed-on: https://go-review.googlesource.com/21384 Run-TryBot: Keith Randall <khr@golang.org> Reviewed-by:
Alexandru Moșoi <alexandru@mosoi.ro> TryBot-Result: Gobot Gobot <gobot@golang.org>
-
Keith Randall authored
Find comparisons to constants and propagate that information down the dominator tree. Use it to resolve other constant comparisons on the same variable. So if we know x >= 7, then a x > 4 condition must return true. This change allows us to use "_ = b[7]" hints to eliminate bounds checks. Fixes #14900 Change-Id: Idbf230bd5b7da43de3ecb48706e21cf01bf812f7 Reviewed-on: https://go-review.googlesource.com/21008Reviewed-by:
Alexandru Moșoi <alexandru@mosoi.ro>
-
Ilya Tocar authored
Currently we test crc64 only with ISO polynomial. Change-Id: Ibc5e202db3b960369cbbb18e31eb0fea07b54dba Reviewed-on: https://go-review.googlesource.com/21309 Run-TryBot: Ilya Tocar <ilya.tocar@intel.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by:
Brad Fitzpatrick <bradfitz@golang.org>
-
Matthew Dempsky authored
We already keep the entire pragma bitset in n.Func.Pragma, so there's no need to track Nointerface separately. Passes toolstash -cmp. Change-Id: Ic027ece477fcf63b0c1df128a08b89ef0f34fd58 Reviewed-on: https://go-review.googlesource.com/21381 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by:
Brad Fitzpatrick <bradfitz@golang.org>
-
Matthew Dempsky authored
Change-Id: Ib500ee92ae1a3d15f7c9f3f46d238b75184b4304 Reviewed-on: https://go-review.googlesource.com/21382Reviewed-by:
Brad Fitzpatrick <bradfitz@golang.org>
-
Josh Bleecher Snyder authored
See discussion in #15029. Change-Id: I5cc8be5737ddb7c1f5e4a6cd92cf557af45e961d Reviewed-on: https://go-review.googlesource.com/21347Reviewed-by:
Brad Fitzpatrick <bradfitz@golang.org>
-
David Crawshaw authored
Fixes #15026. Change-Id: I61ed71152b99973270d79264d1e8f466f7343c02 Reviewed-on: https://go-review.googlesource.com/21286 Run-TryBot: David Crawshaw <crawshaw@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by:
Ian Lance Taylor <iant@golang.org>
-
Josh Bleecher Snyder authored
Add a constant for the magic -1 for slice bounds. Use it. Enforce more aggressively that bounds must be slice, ddd, or non-negative. Remove ad hoc check in plive.go. Check bounds before constructing an array type when typechecking. All changes are manual. Passes toolstash -cmp. Change-Id: I9fd9cc789d7d4b4eea3b30b24037a254d3788add Reviewed-on: https://go-review.googlesource.com/21348 Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com> Reviewed-by:
Matthew Dempsky <mdempsky@google.com>
-
Keith Randall authored
Pushed from an old client by mistake. These are the missing changes. Change-Id: Ia8d61c5c0bde907369366ea9ea98711823342803 Reviewed-on: https://go-review.googlesource.com/21349Reviewed-by:
Todd Neal <todd@tneal.org>
-
Keith Randall authored
Helpful for indexed loads and stores when the stride is not equal to the size being loaded/stored. Update #7927 Change-Id: I8714dd4c7b18a96a611bf5647ee21f753d723945 Reviewed-on: https://go-review.googlesource.com/21346 Run-TryBot: Todd Neal <todd@tneal.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by:
Todd Neal <todd@tneal.org>
-
Keith Randall authored
We need to make sure all the bounds checks pass before issuing a load which combines several others. We do this by issuing the combined load at the last load's block, where "last" = closest to the leaf of the dominator tree. Fixes #15002 Change-Id: I7358116db1e039a072c12c0a73d861f3815d72af Reviewed-on: https://go-review.googlesource.com/21246Reviewed-by:
Todd Neal <todd@tneal.org>
-
Josh Bleecher Snyder authored
Generated by eg, manually fixed up. I’m not thrilled about having a setter, but given the variety of contexts in which this gets fiddled with, it is the cleanest available alternative. Change-Id: Ibdf23e638fe0bdabded014c9e59d557fab8c955f Reviewed-on: https://go-review.googlesource.com/21341Reviewed-by:
Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by:
Matthew Dempsky <mdempsky@google.com>
-
Brad Fitzpatrick authored
Finishes cleanup which was too late to do when discovered during the Go 1.6 cycle. Fixes #14291 Change-Id: Idc69fadbba10baf246318a22b366709eff088a75 Reviewed-on: https://go-review.googlesource.com/21360 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by:
Andrew Gerrand <adg@golang.org>
-
Matthew Dempsky authored
Previously, cmd/compile rejected constant int->string conversions if the integer value did not fit into an "int" value. Also, runtime incorrectly truncated 64-bit values to 32-bit before checking if they're a valid Unicode code point. According to the Go spec, both of these cases should instead yield "\uFFFD". Fixes #15039. Change-Id: I3c8a3ad9a0780c0a8dc1911386a523800fec9764 Reviewed-on: https://go-review.googlesource.com/21344Reviewed-by:
Brad Fitzpatrick <bradfitz@golang.org>
-
Hiroshi Ioka authored
Fixes #14610 Change-Id: I3e57dd60b531c1495ea3bc37ef707a1e4e644baa Reviewed-on: https://go-review.googlesource.com/20180Reviewed-by:
Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
-
Brad Fitzpatrick authored
This exports the system cert pool. The system cert loading was refactored to let it be run multiple times (so callers get a copy, and can't mutate global state), and also to not discard errors. SystemCertPool returns an error on Windows. Maybe it's fixable later, but so far we haven't used it, since the system verifies TLS. Fixes #13335 Change-Id: I3dfb4656a373f241bae8529076d24c5f532f113c Reviewed-on: https://go-review.googlesource.com/21293 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by:
Andrew Gerrand <adg@golang.org>
-
Matthew Dempsky authored
We create appropriate ELF files automatically based on GOOS. There's no point in supporting -H elf flag, particularly since we need to emit different flavors of ELF depending on GOOS anyway. If that weren't reason enough, -H elf appears to be broken since at least Go 1.4. At least I wasn't able to find a way to make use of it. As best I can tell digging through commit history, -H elf is just an artifact leftover from Plan 9's 6l linker. Change-Id: I7393caaadbc60107bbd6bc99b976a4f4fe6b5451 Reviewed-on: https://go-review.googlesource.com/21343 Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by:
Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
-
Brad Fitzpatrick authored
This only tests amd64 because it's currently broken on non-SSA backends. Fixes #8613 Change-Id: I6bc501c81c395e533bb9c7335789750e0c6b7a8f Reviewed-on: https://go-review.googlesource.com/21325Reviewed-by:
Keith Randall <khr@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
-
Brad Fitzpatrick authored
The http2 spec defines a magic string which initates an http2 session: "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n" It was intentionally chosen to kinda look like an HTTP request, but just different enough to break things not ready for it. This change makes Go ready for it. Notably: Go now accepts the request header (the prefix "PRI * HTTP/2.0\r\n\r\n") as a valid request, even though it doesn't have a Host header. But we now mark it as "Connection: close" and teach the Server to never read a second request from the connection once that's seen. If the http.Handler wants to deal with the upgrade, it has to hijack the request, read out the "body", compare it against "SM\r\n\r\n", and then speak http2. One of the new tests demonstrates that hijacking. Fixes #14451 Updates #14141 (h2c) Change-Id: Ib46142f31c55be7d00c56fa2624ec8a232e00c43 Reviewed-on: https://go-review.googlesource.com/21327Reviewed-by:
Andrew Gerrand <adg@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
-
Brad Fitzpatrick authored
This makes sure the net/http package never attempts to transmit a bogus header field key or value and instead fails fast with an error to the user, rather than relying on the server to maybe return an error. It's still possible to use x/net/http2.Transport directly to send bogus stuff. This change only stops h1 & h2 usage via the net/http package. A future change will update x/net/http2. This change also moves some code from request.go to lex.go, which in a separate future change should be moved so it can be shared with http2 to reduce code bloat. Updates #14048 Change-Id: I0a44ae1ab357fbfcbe037aa4b5d50669a87f2856 Reviewed-on: https://go-review.googlesource.com/21326Reviewed-by:
Andrew Gerrand <adg@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
-
Ian Lance Taylor authored
Test to follow in a separate CL that arranges for the runtime package to store non-Go addresses in a CPU profile. Change-Id: I33ce1d66b77340b1e62b54505fc9b1abcec108a9 Reviewed-on: https://go-review.googlesource.com/21055Reviewed-by:
Austin Clements <austin@google.com> Run-TryBot: Ian Lance Taylor <iant@golang.org>
-
Keith Randall authored
Only use REP;MOVSB if: 1) The CPUID flag says it is fast, and 2) The pointers are unaligned Otherwise, use REP;MOVSQ. Update #14630 Change-Id: I946b28b87880c08e5eed1ce2945016466c89db66 Reviewed-on: https://go-review.googlesource.com/21300Reviewed-by:
Nigel Tao <nigeltao@golang.org>
-
Dave Cheney authored
As a followup to CL 21296, avoid append operations when constructing the fields of a Type if the length is known beforehand This also includes some small scoping driveby cleanups, and a change to tointerface0 to avoid iterating over the field list twice. compilebench shows a very small reduction in allocations. name old time/op new time/op delta Template 364ms ± 5% 363ms ± 4% ~ (p=0.945 n=20+19) Unicode 182ms ±11% 185ms ±12% ~ (p=0.445 n=20+20) GoTypes 1.14s ± 2% 1.14s ± 3% ~ (p=0.221 n=20+20) Compiler 5.85s ± 2% 5.84s ± 2% ~ (p=0.369 n=20+20) name old alloc/op new alloc/op delta Template 56.7MB ± 0% 56.7MB ± 0% -0.04% (p=0.000 n=20+20) Unicode 38.3MB ± 0% 38.3MB ± 0% ~ (p=0.728 n=20+19) GoTypes 180MB ± 0% 180MB ± 0% -0.02% (p=0.000 n=20+20) Compiler 812MB ± 0% 812MB ± 0% -0.02% (p=0.000 n=19+20) name old allocs/op new allocs/op delta Template 482k ± 0% 480k ± 0% -0.34% (p=0.000 n=20+20) Unicode 377k ± 0% 377k ± 0% -0.04% (p=0.010 n=20+20) GoTypes 1.36M ± 0% 1.35M ± 0% -0.24% (p=0.000 n=20+20) Compiler 5.47M ± 0% 5.46M ± 0% -0.11% (p=0.000 n=20+18) Change-Id: Ibb4c40229fa3816acd8de98ba41d1571a2aabacf Reviewed-on: https://go-review.googlesource.com/21352Reviewed-by:
Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Dave Cheney <dave@cheney.net>
-
Dave Cheney authored
Apply Robert's optimisation from CL 21241 to Type.Fields. The results are less impressive, possibly because of the makeup of the test data. name old time/op new time/op delta Template 365ms ± 5% 365ms ± 3% ~ (p=0.888 n=20+16) Unicode 182ms ±10% 180ms ± 9% ~ (p=0.883 n=20+20) GoTypes 1.14s ± 2% 1.13s ± 3% ~ (p=0.096 n=20+20) Compiler 5.74s ± 1% 5.76s ± 2% ~ (p=0.369 n=20+20) name old alloc/op new alloc/op delta Template 56.8MB ± 0% 56.7MB ± 0% -0.15% (p=0.000 n=19+20) Unicode 38.3MB ± 0% 38.3MB ± 0% -0.02% (p=0.006 n=20+19) GoTypes 180MB ± 0% 180MB ± 0% -0.13% (p=0.000 n=20+20) Compiler 805MB ± 0% 804MB ± 0% -0.05% (p=0.000 n=20+20) name old allocs/op new allocs/op delta Template 485k ± 0% 482k ± 0% -0.54% (p=0.000 n=19+20) Unicode 377k ± 0% 377k ± 0% -0.05% (p=0.005 n=20+20) GoTypes 1.37M ± 0% 1.36M ± 0% -0.53% (p=0.000 n=20+19) Compiler 5.42M ± 0% 5.41M ± 0% -0.21% (p=0.000 n=20+20) Change-Id: I6782659fadd605ce9931bf5c737c7058b96a29eb Reviewed-on: https://go-review.googlesource.com/21296Reviewed-by:
Brad Fitzpatrick <bradfitz@golang.org>
-
Nigel Tao authored
Fixes #14522. As I said on that issue: ---- This is a progressive JPEG image. There are two dimensions of progressivity: spectral selection (variables zs and ze in scan.go, ranging in [0, 63]) and successive approximation (variables ah and al in scan.go, ranging in [0, 8), from LSB to MSB, although ah=0 implicitly means ah=8). For this particular image, there are three components, and the SOS markers contain this progression: zs, ze, ah, al: 0 0 0 0 components: 0, 1, 2 zs, ze, ah, al: 1 63 0 0 components: 1 zs, ze, ah, al: 1 63 0 0 components: 2 zs, ze, ah, al: 1 63 0 2 components: 0 zs, ze, ah, al: 1 10 2 1 components: 0 zs, ze, ah, al: 11 63 2 1 components: 0 zs, ze, ah, al: 1 10 1 0 components: 0 The combination of all of these is complete (i.e. spectra 0 to 63 and bits 8 exclusive to 0) for components 1 and 2, but it is incomplete for component 0 (the luma component). In particular, there is no data for component 0, spectra 11 to 63 and bits 1 exclusive to 0. The image/jpeg code, as of Go 1.6, waits until both dimensions are complete before performing the de-quantization, IDCT and copy to an *image.YCbCr. This is the "if zigEnd != blockSize-1 || al != 0 { ... continue }" code and associated commentary in scan.go. Almost all progressive JPEG images end up complete in both dimensions for all components, but this particular image is incomplete for component 0, so the Go code never writes anything to the Y values of the resultant *image.YCbCr, which is why the broken output is so dark (but still looks recognizable in terms of red and blue hues). My reading of the ITU T.81 JPEG specification (Annex G) doesn't explicitly say that this is a valid image, but it also doesn't rule it out. In any case, the fix is, for progressive JPEG images, to always reconstruct the decoded blocks (by performing the de-quantization, IDCT and copy to an *image.YCbCr), regardless of whether or not they end up complete. Note that, in Go, the jpeg.Decode function does not return until the entire image is decoded, so we still only want to reconstruct each block once, not once per SOS (Start Of Scan) marker. ---- A test image was also added, based on video-001.progressive.jpeg. When decoding that image, inserting a println("nComp, zs, ze, ah, al:", nComp, zigStart, zigEnd, ah, al) into decoder.processSOS in scan.go prints: nComp, zs, ze, ah, al: 3 0 0 0 1 nComp, zs, ze, ah, al: 1 1 5 0 2 nComp, zs, ze, ah, al: 1 1 63 0 1 nComp, zs, ze, ah, al: 1 1 63 0 1 nComp, zs, ze, ah, al: 1 6 63 0 2 nComp, zs, ze, ah, al: 1 1 63 2 1 nComp, zs, ze, ah, al: 3 0 0 1 0 nComp, zs, ze, ah, al: 1 1 63 1 0 nComp, zs, ze, ah, al: 1 1 63 1 0 nComp, zs, ze, ah, al: 1 1 63 1 0 In other words, video-001.progressive.jpeg contains 10 different scans. This little program below drops half of them (remembering to keep the "\xff\xd9" End of Image marker): ---- package main import ( "bytes" "io/ioutil" "log" ) func main() { sos := []byte{0xff, 0xda} eoi := []byte{0xff, 0xd9} src, err := ioutil.ReadFile("video-001.progressive.jpeg") if err != nil { log.Fatal(err) } b := bytes.Split(src, sos) println(len(b)) // Prints 11. dst := bytes.Join(b[:5], sos) dst = append(dst, eoi...) if err := ioutil.WriteFile("video-001.progressive.truncated.jpeg", dst, 0666); err != nil { log.Fatal(err) } } ---- The video-001.progressive.truncated.jpeg was converted to png via libjpeg and ImageMagick: djpeg -nosmooth video-001.progressive.truncated.jpeg > tmp.tga convert tmp.tga video-001.progressive.truncated.png rm tmp.tga Change-Id: I72b20cd4fb6746d36d8d4d587f891fb3bc641f84 Reviewed-on: https://go-review.googlesource.com/21062Reviewed-by:
Rob Pike <r@golang.org>
-