code review 122090043: runtime: convert markallocated from C to Go

runtime: convert markallocated from C to Go

benchmark                      old ns/op     new ns/op     delta
BenchmarkMalloc8               28.7          22.4          -21.95%
BenchmarkMalloc16              44.8          33.8          -24.55%
BenchmarkMallocTypeInfo8       49.0          32.9          -32.86%
BenchmarkMallocTypeInfo16      46.7          35.8          -23.34%
BenchmarkMallocLargeStruct     907           901           -0.66%
BenchmarkGobDecode             13235542      12036851      -9.06%
BenchmarkGobEncode             10639699      9539155       -10.34%
BenchmarkJSONEncode            25193036      21898922      -13.08%
BenchmarkJSONDecode            96104044      89464904      -6.91%

Fixes issue 8452.

[dvyukov, 2014-08-05 18:30:57]

Hello golang-codereviews@googlegroups.com (cc: khr@golang.org, rsc@golang.org),

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[bradfitz, 2014-08-05 18:39:13]

It pays not to bound between two stacks, huh? Nice.



On Tue, Aug 5, 2014 at 11:30 AM, dvyukov via golang-codereviews <
golang-codereviews@googlegroups.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com (cc: khr@golang.org,
> rsc@golang.org),
>
> I'd like you to review this change to
> https://dvyukov%40google.com@code.google.com/p/go/
>
>
> Description:
> runtime: convert markallocated from C to Go
>
> benchmark                      old ns/op     new ns/op     delta
> BenchmarkMalloc8               28.7          22.4          -21.95%
> BenchmarkMalloc16              44.8          33.8          -24.55%
> BenchmarkMallocTypeInfo8       49.0          32.9          -32.86%
> BenchmarkMallocTypeInfo16      46.7          35.8          -23.34%
> BenchmarkMallocLargeStruct     907           901           -0.66%
> BenchmarkGobDecode             13235542      12036851      -9.06%
> BenchmarkGobEncode             10639699      9539155       -10.34%
> BenchmarkJSONEncode            25193036      21898922      -13.08%
> BenchmarkJSONDecode            96104044      89464904      -6.91%
>
> Fixes issue 8452.
>
> Please review this at https://codereview.appspot.com/122090043/
>
> Affected files (+141, -119 lines):
>   M src/pkg/runtime/malloc.h
>   M src/pkg/runtime/malloc.go
>   M src/pkg/runtime/mgc0.c
>   M src/pkg/runtime/stubs.go
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[dvyukov, 2014-08-05 18:42:44]

Yes, but it just recovers performance that we lost several CLs ago.
This is not a genuine gain.


On Tue, Aug 5, 2014 at 10:39 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> It pays not to bound between two stacks, huh? Nice.
>
> On Tue, Aug 5, 2014 at 11:30 AM, dvyukov via golang-codereviews
> <golang-codereviews@googlegroups.com> wrote:
>>
>> Reviewers: golang-codereviews,
>>
>> Message:
>> Hello golang-codereviews@googlegroups.com (cc: khr@golang.org,
>> rsc@golang.org),
>>
>> I'd like you to review this change to
>> https://dvyukov%40google.com@code.google.com/p/go/
>>
>>
>> Description:
>> runtime: convert markallocated from C to Go
>>
>> benchmark                      old ns/op     new ns/op     delta
>> BenchmarkMalloc8               28.7          22.4          -21.95%
>> BenchmarkMalloc16              44.8          33.8          -24.55%
>> BenchmarkMallocTypeInfo8       49.0          32.9          -32.86%
>> BenchmarkMallocTypeInfo16      46.7          35.8          -23.34%
>> BenchmarkMallocLargeStruct     907           901           -0.66%
>> BenchmarkGobDecode             13235542      12036851      -9.06%
>> BenchmarkGobEncode             10639699      9539155       -10.34%
>> BenchmarkJSONEncode            25193036      21898922      -13.08%
>> BenchmarkJSONDecode            96104044      89464904      -6.91%
>>
>> Fixes issue 8452.
>>
>> Please review this at https://codereview.appspot.com/122090043/
>>
>> Affected files (+141, -119 lines):
>>   M src/pkg/runtime/malloc.h
>>   M src/pkg/runtime/malloc.go
>>   M src/pkg/runtime/mgc0.c
>>   M src/pkg/runtime/stubs.go
>>
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "golang-codereviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-codereviews+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.
>
>

[rsc, 2014-08-05 18:49:41]

R=khr

[dave_cheney.net, 2014-08-06 01:38:17]

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/malloc.go
File src/pkg/runtime/malloc.go (right):

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/malloc.go...
src/pkg/runtime/malloc.go:240: //if (runtime·atomicload((uint32*)ptrmask)&0xff)
== 0 {
left over debugging ?

[dvyukov, 2014-08-06 14:57:46]

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/malloc.go
File src/pkg/runtime/malloc.go (right):

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/malloc.go...
src/pkg/runtime/malloc.go:240: //if (runtime·atomicload((uint32*)ptrmask)&0xff)
== 0 {
On 2014/08/06 01:38:17, dfc wrote:
> left over debugging ?

good catch
This is not leftover, this need to be converted to Go as well.
But we don't have atomic functions in Go yet. Keith has a pending CL that adds
goatomicload/store. I will either wait for Keith CL or roll out own atomics. But
the rest of the CL except this single line is ready for review.

[khr, 2014-08-06 22:29:16]

LGTM, pending the atomic stuff.

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/malloc.go
File src/pkg/runtime/malloc.go (right):

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/malloc.go...
src/pkg/runtime/malloc.go:294: *xbitsb = 0
= bitsDead << 2

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:1818: runtime·unrollgcproginplace(void)
name this with a _m suffix.

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:1828: size0 = g->m->scalararg[1];
Clear ptrarg fields after reading them.

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:1851: runtime·unrollgcprog(void)
Name with _m suffix also.

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:1859: typ = g->m->ptrarg[0];
Clear ptrarg[0] here also (athough it shouldn't matter in this case).

[dvyukov, 2014-08-07 09:32:48]

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/malloc.go
File src/pkg/runtime/malloc.go (right):

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/malloc.go...
src/pkg/runtime/malloc.go:240: //if (runtime·atomicload((uint32*)ptrmask)&0xff)
== 0 {
On 2014/08/06 14:57:45, dvyukov wrote:
> On 2014/08/06 01:38:17, dfc wrote:
> > left over debugging ?
> 
> good catch
> This is not leftover, this need to be converted to Go as well.
> But we don't have atomic functions in Go yet. Keith has a pending CL that adds
> goatomicload/store. I will either wait for Keith CL or roll out own atomics.
But
> the rest of the CL except this single line is ready for review.



I've took goatomicloadp/storep pieces from the Keith's change. And use
goatomicloadp here.

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/malloc.go...
src/pkg/runtime/malloc.go:294: *xbitsb = 0
On 2014/08/06 22:29:15, khr wrote:
> = bitsDead << 2

Done.

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:1818: runtime·unrollgcproginplace(void)
On 2014/08/06 22:29:15, khr wrote:
> name this with a _m suffix.

Done.

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:1828: size0 = g->m->scalararg[1];
On 2014/08/06 22:29:16, khr wrote:
> Clear ptrarg fields after reading them.

Done.

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:1851: runtime·unrollgcprog(void)
On 2014/08/06 22:29:15, khr wrote:
> Name with _m suffix also.

Done.

https://codereview.appspot.com/122090043/diff/60001/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:1859: typ = g->m->ptrarg[0];
On 2014/08/06 22:29:15, khr wrote:
> Clear ptrarg[0] here also (athough it shouldn't matter in this case).

Done.

[dvyukov, 2014-08-07 09:34:36]

*** Submitted as https://code.google.com/p/go/source/detail?r=d50ba39e60ab ***

runtime: convert markallocated from C to Go

benchmark                      old ns/op     new ns/op     delta
BenchmarkMalloc8               28.7          22.4          -21.95%
BenchmarkMalloc16              44.8          33.8          -24.55%
BenchmarkMallocTypeInfo8       49.0          32.9          -32.86%
BenchmarkMallocTypeInfo16      46.7          35.8          -23.34%
BenchmarkMallocLargeStruct     907           901           -0.66%
BenchmarkGobDecode             13235542      12036851      -9.06%
BenchmarkGobEncode             10639699      9539155       -10.34%
BenchmarkJSONEncode            25193036      21898922      -13.08%
BenchmarkJSONDecode            96104044      89464904      -6.91%

Fixes issue 8452.

LGTM=khr
R=golang-codereviews, bradfitz, rsc, dave, khr
CC=golang-codereviews
https://codereview.appspot.com/122090043