Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(17)

Issue 6127064: code review 6127064: compress/flate: optimize history-copy decoding. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 3 months ago by nigeltao
Modified:
10 years, 3 months ago
Reviewers:
CC:
rsc, rog, dfc, golang-dev, Ivan Krasin
Visibility:
Public.

Description

compress/flate: optimize history-copy decoding. The forwardCopy function could be re-written in asm, and the copyHuff method could probably be rolled into huffmanBlock and copyHist, but I'm leaving those changes for future CLs. compress/flate benchmarks: benchmark old ns/op new ns/op delta BenchmarkDecoderBestSpeed1K 385327 435140 +12.93% BenchmarkDecoderBestSpeed10K 1245190 1062112 -14.70% BenchmarkDecoderBestSpeed100K 8512365 5833680 -31.47% BenchmarkDecoderDefaultCompression1K 382225 421301 +10.22% BenchmarkDecoderDefaultCompression10K 867950 613890 -29.27% BenchmarkDecoderDefaultCompression100K 5658240 2466726 -56.40% BenchmarkDecoderBestCompression1K 383760 421634 +9.87% BenchmarkDecoderBestCompression10K 867743 614671 -29.16% BenchmarkDecoderBestCompression100K 5660160 2464996 -56.45% image/png benchmarks: benchmark old ns/op new ns/op delta BenchmarkDecodeGray 2540834 2389624 -5.95% BenchmarkDecodeNRGBAGradient 10052700 9534565 -5.15% BenchmarkDecodeNRGBAOpaque 8704710 8163430 -6.22% BenchmarkDecodePaletted 1458779 1325017 -9.17% BenchmarkDecodeRGB 7183606 6794668 -5.41% Wall time for Denis Cheremisov's PNG-decoding program given in https://groups.google.com/group/golang-nuts/browse_thread/thread/22aa8a05040fdd49 Before: 3.07s After: 2.32s Delta: -24% Before profile: Total: 304 samples 159 52.3% 52.3% 251 82.6% compress/flate.(*decompressor).huffmanBlock 58 19.1% 71.4% 76 25.0% compress/flate.(*decompressor).huffSym 32 10.5% 81.9% 32 10.5% hash/adler32.update 16 5.3% 87.2% 22 7.2% bufio.(*Reader).ReadByte 16 5.3% 92.4% 37 12.2% compress/flate.(*decompressor).moreBits 7 2.3% 94.7% 7 2.3% hash/crc32.update 7 2.3% 97.0% 7 2.3% runtime.memmove 5 1.6% 98.7% 5 1.6% scanblock 2 0.7% 99.3% 9 3.0% runtime.copy 1 0.3% 99.7% 1 0.3% compress/flate.(*huffmanDecoder).init After profile: Total: 230 samples 59 25.7% 25.7% 70 30.4% compress/flate.(*decompressor).huffSym 45 19.6% 45.2% 45 19.6% hash/adler32.update 35 15.2% 60.4% 35 15.2% compress/flate.forwardCopy 20 8.7% 69.1% 151 65.7% compress/flate.(*decompressor).huffmanBlock 16 7.0% 76.1% 24 10.4% compress/flate.(*decompressor).moreBits 15 6.5% 82.6% 15 6.5% runtime.memmove 11 4.8% 87.4% 50 21.7% compress/flate.(*decompressor).copyHist 7 3.0% 90.4% 7 3.0% hash/crc32.update 6 2.6% 93.0% 9 3.9% bufio.(*Reader).ReadByte 4 1.7% 94.8% 4 1.7% runtime.slicearray

Patch Set 1 #

Patch Set 2 : diff -r d115f111a4ba https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r d115f111a4ba https://go.googlecode.com/hg/ #

Total comments: 3

Patch Set 4 : diff -r 1d4d8324085b https://go.googlecode.com/hg/ #

Patch Set 5 : diff -r 1d4d8324085b https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+88 lines, -31 lines) Patch
A src/pkg/compress/flate/copy.go View 1 2 3 1 chunk +17 lines, -0 lines 0 comments Download
A src/pkg/compress/flate/copy_test.go View 1 2 3 4 1 chunk +42 lines, -0 lines 0 comments Download
M src/pkg/compress/flate/inflate.go View 1 2 3 2 chunks +29 lines, -31 lines 0 comments Download

Messages

Total messages: 7
nigeltao
Hello rsc@golang.org (cc: dave@cheney.net, golang-dev@googlegroups.com, krasin@golang.org), I'd like you to review this change to https://go.googlecode.com/hg/
10 years, 3 months ago (2012-04-30 05:11:42 UTC) #1
rog
http://codereview.appspot.com/6127064/diff/4001/src/pkg/compress/flate/copy.go File src/pkg/compress/flate/copy.go (right): http://codereview.appspot.com/6127064/diff/4001/src/pkg/compress/flate/copy.go#newcode13 src/pkg/compress/flate/copy.go:13: for i := range dst { it seems to ...
10 years, 3 months ago (2012-04-30 12:04:31 UTC) #2
dfc
> for i, b := range src[:len(dst)] { > dst[i] = b > } nice ...
10 years, 3 months ago (2012-04-30 12:20:36 UTC) #3
rsc
LGTM http://codereview.appspot.com/6127064/diff/4001/src/pkg/compress/flate/copy.go File src/pkg/compress/flate/copy.go (right): http://codereview.appspot.com/6127064/diff/4001/src/pkg/compress/flate/copy.go#newcode13 src/pkg/compress/flate/copy.go:13: for i := range dst { On 2012/04/30 ...
10 years, 3 months ago (2012-04-30 20:38:02 UTC) #4
nigeltao
On second thoughts, I've renamed naiveCopy to forwardCopy. Submitting... On 30 April 2012 22:04, <rogpeppe@gmail.com> ...
10 years, 3 months ago (2012-05-01 00:17:01 UTC) #5
nigeltao
*** Submitted as http://code.google.com/p/go/source/detail?r=3a8932ef3669 *** compress/flate: optimize history-copy decoding. The forwardCopy function could be re-written ...
10 years, 3 months ago (2012-05-01 00:51:44 UTC) #6
nigeltao
10 years, 3 months ago (2012-05-01 04:33:51 UTC) #7
On 1 May 2012 10:51,  <nigeltao@golang.org> wrote:
> The forwardCopy function could be re-written in asm,

For the curious, I tried re-writing compress/flate's forwardCopy in
asm. It looks like a win for very long, highly flate-compressed
inputs, but it's not as clear otherwise (such as decoding PNG files).
I don't know if it's worth submitting.

Code and benchmarks are at
http://codereview.appspot.com/6137062

Also, I'm not really fluent in amd64 asm, so I might have done something dumb.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b