code review 5310048: goprotobuf: remove unsafe buffer scratch allocator

goprotobuf: remove unsafe buffer scratch allocator
This is part of the work towards making protobufs run on App Engine.
By the way, it's faster now.

Before:
proto_test.BenchmarkUnmarshal	   20000	     87053 ns/op
After:
proto_test.BenchmarkUnmarshal	   20000	     84825 ns/op

[r, 2011-10-19 23:41:11]

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

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

[dsymonds, 2011-10-20 02:31:20]

LGTM

I love how much this cleans up.

http://codereview.appspot.com/5310048/diff/3001/proto/decode.go
File proto/decode.go (right):

http://codereview.appspot.com/5310048/diff/3001/proto/decode.go#newcode357
proto/decode.go:357: iv := unsafe.Unreflect(t, unsafe.Pointer(&ptr))
is the extra variable still needed? Can &base just be used here?

http://codereview.appspot.com/5310048/diff/3001/proto/decode.go#newcode434
proto/decode.go:434: o.int8s = (*new([16]int8))[:]
pull the 16 out as a constant, here and in the other functions.

http://codereview.appspot.com/5310048/diff/3001/proto/decode.go#newcode499
proto/decode.go:499: *v = append([]byte{}, b...)
This just copies the bytes, so pass true to o.DecodeRawBytes on line 493, and
change this line to
  *v = b

[rsc, 2011-10-20 15:11:41]

LGTM except the []string

If we drop the []string then there is no unbounded false sharing
anymore, which would be a great property to have.

http://codereview.appspot.com/5310048/diff/3001/proto/decode.go
File proto/decode.go (right):

http://codereview.appspot.com/5310048/diff/3001/proto/decode.go#newcode357
proto/decode.go:357: iv := unsafe.Unreflect(t, unsafe.Pointer(&ptr))
The o.ptr was to avoid the & causing a heap allocation.
That still seems worth doing.

http://codereview.appspot.com/5310048/diff/3001/proto/decode.go#newcode434
proto/decode.go:434: o.int8s = (*new([16]int8))[:]
o.int8s = make([]int8, 16).
Same each time below.

Should be identical in effect.
Also new([16]int8)[:] should have worked without the indirect.

http://codereview.appspot.com/5310048/diff/3001/proto/decode.go#newcode450
proto/decode.go:450: o.int32s = (*new([16]int32))[:]
Have you tried varying this?

I think this could be a smaller number than 16 as the base type gets larger.
4 seems like it would be fine here: I think the most important part is that
the total allocation be at least 16 bytes, to avoid wasting memory, and the
smaller the number the less false sharing there is.

http://codereview.appspot.com/5310048/diff/3001/proto/decode.go#newcode482
proto/decode.go:482: o.strings = (*new([16]string))[:]
The false sharing is especially important here.
I would try not having any cache here at all.

sp := new(string)
*sp = s
*(**string)(unsafe.Pointer(base + p.offset)) = sp
return nil

[r2, 2011-10-20 15:24:29]

On Oct 20, 2011, at 8:11 AM, rsc@golang.org wrote:

> LGTM except the []string
> 
> If we drop the []string then there is no unbounded false sharing
> anymore, which would be a great property to have.
> 
> 
> 
> http://codereview.appspot.com/5310048/diff/3001/proto/decode.go
> File proto/decode.go (right):
> 
> http://codereview.appspot.com/5310048/diff/3001/proto/decode.go#newcode357
> proto/decode.go:357: iv := unsafe.Unreflect(t, unsafe.Pointer(&ptr))
> The o.ptr was to avoid the & causing a heap allocation.
> That still seems worth doing.
> 
> http://codereview.appspot.com/5310048/diff/3001/proto/decode.go#newcode434
> proto/decode.go:434: o.int8s = (*new([16]int8))[:]
> o.int8s = make([]int8, 16).
> Same each time below.
> 
> Should be identical in effect.
> Also new([16]int8)[:] should have worked without the indirect.

i was just outthinking myself.

> http://codereview.appspot.com/5310048/diff/3001/proto/decode.go#newcode450
> proto/decode.go:450: o.int32s = (*new([16]int32))[:]
> Have you tried varying this?
> 
> I think this could be a smaller number than 16 as the base type gets
> larger.
> 4 seems like it would be fine here: I think the most important part is
> that
> the total allocation be at least 16 bytes, to avoid wasting memory, and
> the
> smaller the number the less false sharing there is.

i'll try different sizes and see how performance is affected. it's not just
about cache lines.

> http://codereview.appspot.com/5310048/diff/3001/proto/decode.go#newcode482
> proto/decode.go:482: o.strings = (*new([16]string))[:]
> The false sharing is especially important here.
> I would try not having any cache here at all.
> 
> sp := new(string)
> *sp = s
> *(**string)(unsafe.Pointer(base + p.offset)) = sp
> return nil

this one i don't like. the point is to get rid of unsafe.

-rob

[rsc, 2011-10-20 18:06:49]

> > I think this could be a smaller number than 16 as the base type gets
> > larger.
> > 4 seems like it would be fine here: I think the most important part is
> > that
> > the total allocation be at least 16 bytes, to avoid wasting memory, and
> > the
> > smaller the number the less false sharing there is.
> 
> i'll try different sizes and see how performance is affected. it's not just
about cache lines.

Sorry, I didn't mean that kind of sharing.  I meant that
putting multiple unrelated things in one allocation means
that the garbage collector can't free one until all are
unreferenced.

> > http://codereview.appspot.com/5310048/diff/3001/proto/decode.go#newcode482
> > proto/decode.go:482: o.strings = (*new([16]string))[:]
> > The false sharing is especially important here.
> > I would try not having any cache here at all.
> > 
> > sp := new(string)
> > *sp = s
> > *(**string)(unsafe.Pointer(base + p.offset)) = sp
> > return nil
> 
> this one i don't like. the point is to get rid of unsafe.

Of the 4 code lines above, only the first 1 is different.
The other 3 are already present in your rewrite.
(You're not getting rid of unsafe.)

If you allocate [16]string, any one of the entries might
be pointing at a large string, and having them all in one
block means that none of the pointed-at strings can be freed
until all 16 of the strings are unreferenced.  This is a
bigger deal than the [16]int because if you are pinning
15 other ints in memory just to keep 1, that's worst case 16x.
There is no upper bound on the worst case for pinning 15 
other strings.

Russ

[r, 2011-10-20 19:58:30]

*** Submitted as
http://code.google.com/p/goprotobuf/source/detail?r=9fe71978189f ***

goprotobuf: remove unsafe buffer scratch allocator
This is part of the work towards making protobufs run on App Engine.
By the way, it's faster now.

Before:
proto_test.BenchmarkUnmarshal	   20000	     87053 ns/op
After:
proto_test.BenchmarkUnmarshal	   20000	     84825 ns/op

R=dsymonds, rsc, ken, r
CC=golang-dev
http://codereview.appspot.com/5310048