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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 11 months ago by dfc
Modified:
2 years, 11 months ago
Reviewers:
CC:
rsc, 0xe2.0x9a.0x9b_gmail.com, aam, golang-dev, minux1
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 ...
2 years, 11 months ago (2012-06-06 14:55:35 UTC) #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: - ...
2 years, 11 months ago (2012-06-06 16:58:02 UTC) #2
rsc
I think you do need to check some of the others. You can have local_nfree ...
2 years, 11 months ago (2012-06-06 17:25:43 UTC) #3
0xe2.0x9a.0x9b_gmail.com
On Wed, Jun 6, 2012 at 7:25 PM, Russ Cox <rsc@golang.org> wrote: > I think ...
2 years, 11 months ago (2012-06-06 20:08:39 UTC) #4
rsc
Nice.
2 years, 11 months ago (2012-06-06 20:42:35 UTC) #5
dfc
Hello. Thank you for your comments. I've attempted to incorporate Atom's fast checking solution based ...
2 years, 11 months ago (2012-06-07 11:08:45 UTC) #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)) { ...
2 years, 11 months ago (2012-06-07 12:49:53 UTC) #7
rsc
On Thu, Jun 7, 2012 at 7:08 AM, <dave@cheney.net> wrote: > Hello. Thank you for ...
2 years, 11 months ago (2012-06-07 12:52:44 UTC) #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/
2 years, 11 months ago (2012-06-07 13:04:58 UTC) #9
rsc
>= in both tests please.
2 years, 11 months ago (2012-06-07 14:36:46 UTC) #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/
2 years, 11 months ago (2012-06-07 23:51:01 UTC) #11
dfc
linux/arm GOARM=5 benchmark old ns/op new ns/op delta BenchmarkBinaryTree17 115763682000 69593536000 -39.88% BenchmarkFannkuch11 64647655000 64645548000 ...
2 years, 11 months ago (2012-06-08 04:59:22 UTC) #12
aam
linux/386: benchmark old ns/op new ns/op delta BenchmarkBinaryTree17 9404107000 9602360000 +2.11% BenchmarkFannkuch11 10682824000 10813868000 +1.23% ...
2 years, 11 months ago (2012-06-08 20:03:05 UTC) #13
rsc
i'm very skeptical
2 years, 11 months ago (2012-06-08 20:04:48 UTC) #14
aam
> i'm very skeptical about the tests or about the patch?
2 years, 11 months ago (2012-06-08 20:11:18 UTC) #15
rsc
The patch seems fine. It's hard for me to believe that a new if statement ...
2 years, 11 months ago (2012-06-08 20:35:27 UTC) #16
aam
To make sure I wasn't getting wild swings in test results I reran the before ...
2 years, 11 months ago (2012-06-08 20:46:36 UTC) #17
rsc
On Fri, Jun 8, 2012 at 4:46 PM, andrey mirtchovski <mirtchovski@gmail.com> wrote: > BenchmarkParse 11959530 ...
2 years, 11 months ago (2012-06-08 21:19:58 UTC) #18
rsc
On an idle Core i7, benchmark old MB/s new MB/s speedup BenchmarkGobDecode 37.85 38.00 1.00x ...
2 years, 11 months ago (2012-06-08 21:35:00 UTC) #19
rsc
LGTM
2 years, 11 months ago (2012-06-08 21:35:02 UTC) #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 ...
2 years, 11 months ago (2012-06-08 21:35:20 UTC) #21
aam
just a bit more noise. I did full 25 permutations since i realized that 'old' ...
2 years, 11 months ago (2012-06-08 22:03:44 UTC) #22
dfc
Thanks for the thorough testing. Would you be able to try with the two overflow ...
2 years, 11 months ago (2012-06-08 22:12:29 UTC) #23
aam
linux/386, core2duo, pre-patch vs CL minus malloc.cgo overflow checks: benchmark old ns/op new ns/op deltaBenchmarkBinaryTree17 ...
2 years, 11 months ago (2012-06-08 22:34:07 UTC) #24
rsc
This smells like more memory layout nonsense. The tweaks are changing which lines of code ...
2 years, 11 months ago (2012-06-08 22:43:46 UTC) #25
dfc
2 years, 11 months ago (2012-06-08 23:32:27 UTC) #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 e6eadc5