code review 6554060: runtime: add types to MSpan

runtime: add types to MSpan

[atom, 2012-09-21 15:13:03]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[atom, 2012-09-21 15:28:06]

https://codereview.appspot.com/6554060/diff/1/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/6554060/diff/1/src/pkg/runtime/runtime.h#newco...
src/pkg/runtime/runtime.h:842: UseSpanType = 1,
Maybe this flag should be called MSpanType. I am not sure about the name.

[rsc, 2012-09-21 16:33:46]

Do you have a sense of the memory overhead this incurs? I wonder if
for the smaller sizes we will want to have separate spans for
pointer-free data.

Russ

[atom, 2012-09-21 17:28:30]

On 2012/09/21 16:33:46, rsc wrote:
> Do you have a sense of the memory overhead this incurs?

It varies with application. Maximum increase in memory consumption I have seen
was a few percent. There is no noticeable memory overhead when running "godoc
-http=:6060".

> I wonder if
> for the smaller sizes we will want to have separate spans for
> pointer-free data.

2 spans for objects of a particular size (instead of 1 span) may in some cases
increase memory consumption because free items in the additional span are
consuming extra memory.

[rsc, 2012-09-24 17:21:50]

The code looks good. Please write the comment I asked for in malloc.h describing
the layout of 'data' for each case and then I will do a second pass.

Thanks.

https://codereview.appspot.com/6554060/diff/1/src/pkg/runtime/malloc.goc
File src/pkg/runtime/malloc.goc (right):

https://codereview.appspot.com/6554060/diff/1/src/pkg/runtime/malloc.goc#newc...
src/pkg/runtime/malloc.goc:490: if(s->sizeclass != 0) {
Please invert this so we can avoid some indenting and mental context:

if(s->sizeclass == 0) {
    ... code from bottom ...
    continue;
}

...

https://codereview.appspot.com/6554060/diff/1/src/pkg/runtime/malloc.goc#newc...
src/pkg/runtime/malloc.goc:521: if( ((uintptr*)data3)[j] == typ ) {
throughout, s/( /(/g s/ )/)/g

https://codereview.appspot.com/6554060/diff/1/src/pkg/runtime/malloc.goc#newc...
src/pkg/runtime/malloc.goc:542: bool sysalloc3 = s->types.sysalloc;
Please move the bool declaration to the top.

https://codereview.appspot.com/6554060/diff/1/src/pkg/runtime/malloc.goc#newc...
src/pkg/runtime/malloc.goc:632: runtime·gettype(void *v) {
{ on next line

https://codereview.appspot.com/6554060/diff/1/src/pkg/runtime/malloc.h
File src/pkg/runtime/malloc.h (right):

https://codereview.appspot.com/6554060/diff/1/src/pkg/runtime/malloc.h#newcod...
src/pkg/runtime/malloc.h:306: enum
Please move this enum and the struct MTypes definition above the MSpan enum, so
that you can put it in a separate section and write a comment for it.

// MTypes describes the types of blocks allocated within a span.
// The compression field describes the layout of the data.
//
// MTypes_Empty:
//     XXX description here
// MTypes_Single:
//     XXX description here
// ...

https://codereview.appspot.com/6554060/diff/1/src/pkg/runtime/malloc.h#newcod...
src/pkg/runtime/malloc.h:317: uintptr	data;
Why not void*?

https://codereview.appspot.com/6554060/diff/1/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/6554060/diff/1/src/pkg/runtime/runtime.h#newco...
src/pkg/runtime/runtime.h:842: UseSpanType = 1,
On 2012/09/21 15:28:06, atom wrote:
> Maybe this flag should be called MSpanType. I am not sure about the name.

UseSpanType is fine.

[atom, 2012-09-24 19:06:50]

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

Please take another look.

[atom, 2012-09-24 19:07:07]

https://codereview.appspot.com/6554060/diff/1/src/pkg/runtime/malloc.h
File src/pkg/runtime/malloc.h (right):

https://codereview.appspot.com/6554060/diff/1/src/pkg/runtime/malloc.h#newcod...
src/pkg/runtime/malloc.h:306: enum
On 2012/09/24 17:21:50, rsc wrote:
> Please move this enum and the struct MTypes definition above the MSpan enum,
so
> that you can put it in a separate section and write a comment for it.
> 
> // MTypes describes the types of blocks allocated within a span.
> // The compression field describes the layout of the data.
> //
> // MTypes_Empty:
> //     XXX description here
> // MTypes_Single:
> //     XXX description here
> // ...

Done.

https://codereview.appspot.com/6554060/diff/1/src/pkg/runtime/malloc.h#newcod...
src/pkg/runtime/malloc.h:317: uintptr	data;
On 2012/09/24 17:21:50, rsc wrote:
> Why not void*?

In case of MTypes_Single, the field 'data' directly holds the type information.

The function malloc.goc:new (and other functions in forthcoming patches) uses
the low bits of 'typ'. The mask used in malloc.goc:new is TypeInfo_SingleObject.
The resulting value is therefore of type uintptr.

Struct MTypes completely abstracts away from the meaning of uintptr values
stored within the struct. From the viewpoint of MTypes as such, the values have
type uintptr.

[rsc, 2012-09-24 19:16:47]

Looks good. Thanks for the comment: it was a big help understanding the details
of the Bytes compression. Please tweak the below and sync. I just submitted some
changes to make the runtime 64-bit int safe, so you might have a minor merge
conflict or two. Apologies if so.

https://codereview.appspot.com/6554060/diff/6001/src/pkg/runtime/malloc.goc
File src/pkg/runtime/malloc.goc (right):

https://codereview.appspot.com/6554060/diff/6001/src/pkg/runtime/malloc.goc#n...
src/pkg/runtime/malloc.goc:601: MSpan *s = runtime·MHeap_Lookup(&runtime·mheap,
v);
I know I'm annoying but please put the MSpan *s at the top.

https://codereview.appspot.com/6554060/diff/6001/src/pkg/runtime/malloc.h
File src/pkg/runtime/malloc.h (right):

https://codereview.appspot.com/6554060/diff/6001/src/pkg/runtime/malloc.h#new...
src/pkg/runtime/malloc.h:315: //     The span contains multiple blocks.
The span contains at most seven different types of blocks.
?

[atom, 2012-09-24 19:51:54]

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

Please take another look.

[rsc, 2012-09-24 20:00:57]

LGTM

[rsc, 2012-09-25 00:08:09]

*** Submitted as http://code.google.com/p/go/source/detail?r=0a3866d6cc6b ***

runtime: add types to MSpan

R=rsc
CC=golang-dev
http://codereview.appspot.com/6554060

Committer: Russ Cox <rsc@golang.org>