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

Issue 131840043: code review 131840043: bzip2: improve performance (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 1 month ago by jra
Modified:
6 years, 1 month ago
Reviewers:
agl1, adg, josharian
CC:
adg, agl, agl1, josharian, golang-codereviews
Visibility:
Public.

Description

bzip2: improve performance Improve performance of move-to-front by using cache-friendly copies instead of doubly-linked list. Simplify so that the underlying slice is the object. Remove the n=0 special case, which was actually slower with the copy approach. benchmark old ns/op new ns/op delta BenchmarkDecodeDigits 26429714 23859699 -9.72% BenchmarkDecodeTwain 76684510 67591946 -11.86% benchmark old MB/s new MB/s speedup BenchmarkDecodeDigits 1.63 1.81 1.11x BenchmarkDecodeTwain 1.63 1.85 1.13x Updates issue 6754.

Patch Set 1 #

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

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

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

Total comments: 6

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+55 lines, -62 lines) Patch
M src/pkg/compress/bzip2/bzip2_test.go View 1 1 chunk +38 lines, -0 lines 0 comments Download
M src/pkg/compress/bzip2/move_to_front.go View 1 2 3 4 1 chunk +17 lines, -62 lines 0 comments Download

Messages

Total messages: 8
jra
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
6 years, 1 month ago (2014-08-17 21:59:07 UTC) #1
adg
LGTM +agl, who wrote the package https://codereview.appspot.com/131840043/diff/60001/src/pkg/compress/bzip2/move_to_front.go File src/pkg/compress/bzip2/move_to_front.go (right): https://codereview.appspot.com/131840043/diff/60001/src/pkg/compress/bzip2/move_to_front.go#newcode57 src/pkg/compress/bzip2/move_to_front.go:57: copy(m.symbols[1:], m.symbols[0:n]) s/0//
6 years, 1 month ago (2014-08-18 02:16:30 UTC) #2
agl1
LGTM with adg's comments addressed.
6 years, 1 month ago (2014-08-18 17:31:28 UTC) #3
josharian
LGTM after adg and my comments https://codereview.appspot.com/131840043/diff/60001/src/pkg/compress/bzip2/move_to_front.go File src/pkg/compress/bzip2/move_to_front.go (right): https://codereview.appspot.com/131840043/diff/60001/src/pkg/compress/bzip2/move_to_front.go#newcode16 src/pkg/compress/bzip2/move_to_front.go:16: len int Looks ...
6 years, 1 month ago (2014-08-18 18:00:09 UTC) #4
jra
Hello adg@golang.org, agl@chromium.org, agl@golang.org, josharian@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
6 years, 1 month ago (2014-08-18 21:16:33 UTC) #5
jra
Updated the performace figures to show even better perf I got from your comments. Thanks! ...
6 years, 1 month ago (2014-08-18 21:17:02 UTC) #6
agl1
*** Submitted as https://code.google.com/p/go/source/detail?r=4fa514b5c635 *** bzip2: improve performance Improve performance of move-to-front by using cache-friendly ...
6 years, 1 month ago (2014-08-18 21:41:46 UTC) #7
adg
6 years, 1 month ago (2014-08-18 22:29:42 UTC) #8
Hooray for much less code! My favourite kind of CL.

Thanks.


On 19 August 2014 07:41, <agl@golang.org> wrote:

> *** Submitted as
> https://code.google.com/p/go/source/detail?r=4fa514b5c635 ***
>
>
> bzip2: improve performance
>
> Improve performance of move-to-front by using cache-friendly
> copies instead of doubly-linked list. Simplify so that the
> underlying slice is the object. Remove the n=0 special case,
>       which was actually slower with the copy approach.
>
>
> benchmark                 old ns/op     new ns/op     delta
> BenchmarkDecodeDigits     26429714      23859699      -9.72%
> BenchmarkDecodeTwain      76684510      67591946      -11.86%
>
>
> benchmark                 old MB/s     new MB/s     speedup
> BenchmarkDecodeDigits     1.63         1.81         1.11x
> BenchmarkDecodeTwain      1.63         1.85         1.13x
>
> Updates issue 6754.
>
> LGTM=adg, agl, josharian
> R=adg, agl, josharian
> CC=golang-codereviews
> https://codereview.appspot.com/131840043
>
> Committer: Adam Langley <agl@golang.org>
>
>
> https://codereview.appspot.com/131840043/
>
Sign in to reply to this message.

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