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

Issue 6297047: code review 6297047: runtime: use uintptr where possible in malloc stats (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 10 months ago by dfc
Modified:
1 year, 9 months ago
Reviewers:
CC:
rsc, 0xe2.0x9a.0x9b_gmail.com, aam, golang-dev, minux
Visibility:
Public.

Description

runtime: use uintptr where possible in malloc stats

linux/arm OMAP4 pandaboard

benchmark                 old ns/op    new ns/op    delta
BenchmarkBinaryTree17   68723297000  37026214000  -46.12%
BenchmarkFannkuch11     34962402000  35958435000   +2.85%
BenchmarkGobDecode        137298600    124182150   -9.55%
BenchmarkGobEncode         60717160     60006700   -1.17%
BenchmarkGzip            5647156000   5550873000   -1.70%
BenchmarkGunzip          1196350000   1198670000   +0.19%
BenchmarkJSONEncode       863012800    782898000   -9.28%
BenchmarkJSONDecode      3312989000   2781800000  -16.03%
BenchmarkMandelbrot200     45727540     45703120   -0.05%
BenchmarkParse             74781800     59990840  -19.78%
BenchmarkRevcomp          140043650    139462300   -0.42%
BenchmarkTemplate        6467682000   5832153000   -9.83%

benchmark                  old MB/s     new MB/s  speedup
BenchmarkGobDecode             5.59         6.18    1.11x
BenchmarkGobEncode            12.64        12.79    1.01x
BenchmarkGzip                  3.44         3.50    1.02x
BenchmarkGunzip               16.22        16.19    1.00x
BenchmarkJSONEncode            2.25         2.48    1.10x
BenchmarkJSONDecode            0.59         0.70    1.19x
BenchmarkParse                 0.77         0.97    1.26x
BenchmarkRevcomp              18.15        18.23    1.00x
BenchmarkTemplate              0.30         0.33    1.10x

darwin/386 core duo

benchmark                 old ns/op    new ns/op    delta
BenchmarkBinaryTree17   10591616577   9678245733   -8.62%
BenchmarkFannkuch11     10758473315  10749303846   -0.09%
BenchmarkGobDecode         34379785     34121250   -0.75%
BenchmarkGobEncode         23523721     23475750   -0.20%
BenchmarkGzip            2486191492   2446539568   -1.59%
BenchmarkGunzip           444179328    444250293   +0.02%
BenchmarkJSONEncode       221138507    219757826   -0.62%
BenchmarkJSONDecode      1056034428   1048975133   -0.67%
BenchmarkMandelbrot200     19862516     19868346   +0.03%
BenchmarkRevcomp         3742610872   3724821662   -0.48%
BenchmarkTemplate         960283112    944791517   -1.61%

benchmark                  old MB/s     new MB/s  speedup
BenchmarkGobDecode            22.33        22.49    1.01x
BenchmarkGobEncode            32.63        32.69    1.00x
BenchmarkGzip                  7.80         7.93    1.02x
BenchmarkGunzip               43.69        43.68    1.00x
BenchmarkJSONEncode            8.77         8.83    1.01x
BenchmarkJSONDecode            1.84         1.85    1.01x
BenchmarkRevcomp              67.91        68.24    1.00x
BenchmarkTemplate              2.02         2.05    1.01x

Patch Set 1 #

Patch Set 2 : diff -r 9b455eb64690 https://code.google.com/p/go #

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

Total comments: 12

Patch Set 4 : diff -r b720fc58b147 https://code.google.com/p/go #

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

Patch Set 6 : diff -r b720fc58b147 https://go.googlecode.com/hg/ #

Patch Set 7 : diff -r b720fc58b147 https://go.googlecode.com/hg/ #

Patch Set 8 : diff -r b720fc58b147 https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 9 : diff -r b720fc58b147 https://go.googlecode.com/hg/ #

Patch Set 10 : diff -r f33da81baac2 https://go.googlecode.com/hg/ #

Patch Set 11 : diff -r f33da81baac2 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -10 lines) Patch
M src/pkg/runtime/malloc.h View 1 2 4 7 1 chunk +10 lines, -10 lines 0 comments Download
M src/pkg/runtime/malloc.goc View 1 2 3 4 5 6 7 8 9 2 chunks +15 lines, -0 lines 0 comments Download

Messages

Total messages: 26
rsc
I think you can do this, but it will require a few more checks. They ...
1 year, 10 months ago #1
atom
http://codereview.appspot.com/6297047/diff/4001/src/pkg/runtime/malloc.h File src/pkg/runtime/malloc.h (right): http://codereview.appspot.com/6297047/diff/4001/src/pkg/runtime/malloc.h#newcode273 src/pkg/runtime/malloc.h:273: struct MCache You only need to check these: - ...
1 year, 10 months ago #2
rsc
I think you do need to check some of the others. You can have local_nfree ...
1 year, 10 months ago #3
0xe2.0x9a.0x9b_gmail.com
On Wed, Jun 6, 2012 at 7:25 PM, Russ Cox <rsc@golang.org> wrote: > I think ...
1 year, 10 months ago #4
rsc
Nice.
1 year, 10 months ago #5
dfc
Hello. Thank you for your comments. I've attempted to incorporate Atom's fast checking solution based ...
1 year, 10 months ago #6
rsc
http://codereview.appspot.com/6297047/diff/8003/src/pkg/runtime/malloc.goc File src/pkg/runtime/malloc.goc (right): http://codereview.appspot.com/6297047/diff/8003/src/pkg/runtime/malloc.goc#newcode76 src/pkg/runtime/malloc.goc:76: if (sizeof(void*) == 4 && c->local_total_alloc > (1<<30)) { ...
1 year, 10 months ago #7
rsc
On Thu, Jun 7, 2012 at 7:08 AM, <dave@cheney.net> wrote: > Hello. Thank you for ...
1 year, 10 months ago #8
dfc
Hello rsc@golang.org, 0xe2.0x9a.0x9b@gmail.com (cc: golang-dev@googlegroups.com, minux.ma@gmail.com), I'd like you to review this change to https://go.googlecode.com/hg/
1 year, 10 months ago #9
rsc
>= in both tests please.
1 year, 10 months ago #10
dfc
Hello rsc@golang.org, 0xe2.0x9a.0x9b@gmail.com (cc: golang-dev@googlegroups.com, minux.ma@gmail.com), I'd like you to review this change to https://go.googlecode.com/hg/
1 year, 10 months ago #11
dfc
linux/arm GOARM=5 benchmark old ns/op new ns/op delta BenchmarkBinaryTree17 115763682000 69593536000 -39.88% BenchmarkFannkuch11 64647655000 64645548000 ...
1 year, 10 months ago #12
aam
linux/386: benchmark old ns/op new ns/op delta BenchmarkBinaryTree17 9404107000 9602360000 +2.11% BenchmarkFannkuch11 10682824000 10813868000 +1.23% ...
1 year, 10 months ago #13
rsc
i'm very skeptical
1 year, 10 months ago #14
aam
> i'm very skeptical about the tests or about the patch?
1 year, 10 months ago #15
rsc
The patch seems fine. It's hard for me to believe that a new if statement ...
1 year, 10 months ago #16
aam
To make sure I wasn't getting wild swings in test results I reran the before ...
1 year, 10 months ago #17
rsc
On Fri, Jun 8, 2012 at 4:46 PM, andrey mirtchovski <mirtchovski@gmail.com> wrote: > BenchmarkParse 11959530 ...
1 year, 10 months ago #18
rsc
On an idle Core i7, benchmark old MB/s new MB/s speedup BenchmarkGobDecode 37.85 38.00 1.00x ...
1 year, 10 months ago #19
rsc
LGTM
1 year, 10 months ago #20
rsc
*** Submitted as http://code.google.com/p/go/source/detail?r=d2d54e5b3317 *** runtime: use uintptr where possible in malloc stats linux/arm OMAP4 ...
1 year, 10 months ago #21
aam
just a bit more noise. I did full 25 permutations since i realized that 'old' ...
1 year, 10 months ago #22
dfc
Thanks for the thorough testing. Would you be able to try with the two overflow ...
1 year, 10 months ago #23
aam
linux/386, core2duo, pre-patch vs CL minus malloc.cgo overflow checks: benchmark old ns/op new ns/op deltaBenchmarkBinaryTree17 ...
1 year, 10 months ago #24
rsc
This smells like more memory layout nonsense. The tweaks are changing which lines of code ...
1 year, 10 months ago #25
dfc
1 year, 10 months ago #26
Thanks Russ and Andrey.

On Sat, Jun 9, 2012 at 8:43 AM, Russ Cox <rsc@golang.org> wrote:
> This smells like more memory layout nonsense. The tweaks are changing
> which lines of code sit where in memory and causing strange changes in
> tight loops.
>
> Russ
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1278:e6ce13d99bf5