code review 6494085: proto: make safe for App Engine

proto: make safe for App Engine

Factor unsafe code into its own file that can be replaced with
reflect code when building for App Engine.

benchmark                                old MB/s     new MB/s  speedup
BenchmarkMarshal                           126.34       120.38    0.95x
BenchmarkUnmarshal                         111.84       110.78    0.99x
BenchmarkMarshalBytes                      893.70       853.55    0.96x
BenchmarkUnmarshalBytes                    615.23       619.73    1.01x

Comparing the two new variants, old = unsafe, new = reflect:

benchmark                                old MB/s     new MB/s  speedup
BenchmarkMarshal                           120.38        50.52    0.42x
BenchmarkUnmarshal                         110.78        20.12    0.18x
BenchmarkMarshalBytes                      853.55       182.21    0.21x
BenchmarkUnmarshalBytes                    619.73       227.66    0.37x

But really these numbers are unfair since they ignore the
qualitative difference: proto was completely unavailable
on App Engine, and now it can be run there.

[rsc, 2012-09-06 00:42:53]

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

I'd like you to review this change to
https://code.google.com/p/goprotobuf

[dsymonds, 2012-09-06 01:05:20]

I haven't digested all of this, but it looks like a good cleanup independent of
the safety. Thanks!

Any idea why Unmarshal takes a bigger performance hit than Marshal?

https://codereview.appspot.com/6494085/diff/2003/proto/lib.go
File proto/lib.go (right):

https://codereview.appspot.com/6494085/diff/2003/proto/lib.go#newcode250
proto/lib.go:250: 
why the blank here? Are the next slices different?

https://codereview.appspot.com/6494085/diff/2003/proto/pointer_reflect.go
File proto/pointer_reflect.go (right):

https://codereview.appspot.com/6494085/diff/2003/proto/pointer_reflect.go#new...
proto/pointer_reflect.go:57: func (p pointer) rfield(f field) reflect.Value {
comment please

https://codereview.appspot.com/6494085/diff/2003/proto/pointer_reflect.go#new...
proto/pointer_reflect.go:68: func (p pointer) Byteses(f field) *[][]byte        
  { return p.field(f).(*[][]byte) }
I have a mild preference for the slice versions to have more than just an "s" or
"es" added. BytesSlice, BoolSlice, StringSlice, etc.

https://codereview.appspot.com/6494085/diff/2003/proto/pointer_reflect.go#new...
proto/pointer_reflect.go:109: type word32 struct {
comment on this please, as well as on the other word* types.

https://codereview.appspot.com/6494085/diff/2003/proto/properties.go
File proto/properties.go (right):

https://codereview.appspot.com/6494085/diff/2003/proto/properties.go#newcode120
proto/properties.go:120: unrecField field          // offset of the
XXX_unrecognized []byte field
s/offset/field/?

[rsc, 2012-09-06 02:19:39]

Sorry, I'll write detailed comments in pointer_*.go before you need to
read this carefully. I was so happy I got it working (and that my
power had come back on) that I forgot to clean it up.

[rsc, 2012-09-06 02:22:24]

> Any idea why Unmarshal takes a bigger performance hit than Marshal?

This particular Unmarshal benchmark spends nearly all its time
unmarshaling into an int32 slice. That's particularly expensive in the
safe version because 'int32' really means int32, uint32, float32, or
enum, so there's a lot of extra fiddling. Most protos spend their
bytes in []byte or string fields, not []int32 fields, and I wouldn't
expect those to see such a slowdown.

[rsc, 2012-09-06 03:10:37]

PTAL

[dsymonds, 2012-09-06 04:14:42]

LGTM

Thanks, this is a lot more digestible. I still haven't absorbed it all, but
there's no need to hold it up for that.

Over to r.

http://codereview.appspot.com/6494085/diff/6019/proto/pointer_reflect.go
File proto/pointer_reflect.go (right):

http://codereview.appspot.com/6494085/diff/6019/proto/pointer_reflect.go#newc...
proto/pointer_reflect.go:51: // The reflect value must itself be a pointer to a
struct.
worth checking and panicking if this isn't the case? I could imagine this would
be very annoying to debug if this invariant were to be violated.

http://codereview.appspot.com/6494085/diff/6019/proto/pointer_reflect.go#newc...
proto/pointer_reflect.go:77: var invalidField = field(nil)
This is just the zero type, right? So in properties.go you don't need to set
unrecField to this.

http://codereview.appspot.com/6494085/diff/6019/proto/pointer_reflect.go#newc...
proto/pointer_reflect.go:80: func (f field) IsValid() bool { return f != nil }
return len(f) > 0?

http://codereview.appspot.com/6494085/diff/6019/proto/pointer_reflect.go#newc...
proto/pointer_reflect.go:154: type structPointerSlice struct {
It's a bit awful, but the consistent name for this would be
structPointerSlicePointer. That's probably not worth it.

http://codereview.appspot.com/6494085/diff/6019/proto/pointer_unsafe.go
File proto/pointer_unsafe.go (right):

http://codereview.appspot.com/6494085/diff/6019/proto/pointer_unsafe.go#newco...
proto/pointer_unsafe.go:72: const invalidField = ^field(0)
ah. my suggestion in pointer_reflect.go isn't going to work with this. oh, well.

[rsc, 2012-09-06 17:29:05]

>
http://codereview.appspot.com/6494085/diff/6019/proto/pointer_reflect.go#newc...
> proto/pointer_reflect.go:51: // The reflect value must itself be a
> pointer to a struct.
> worth checking and panicking if this isn't the case? I could imagine
> this would be very annoying to debug if this invariant were to be
> violated.

Actually it is violated by extensions. There are comments about that
too, in the field method. :-(

But the very nice thing about having pointer_reflect.go at all is that
it does serve to double-check at run-time the encode/decode code when
testing in that mode. There's no guarantee that pointer_unsafe.go
matches, of course, but at least we have some validation that
encode/decode are type-safe.

>
http://codereview.appspot.com/6494085/diff/6019/proto/pointer_reflect.go#newc...
> proto/pointer_reflect.go:80: func (f field) IsValid() bool { return f !=
> nil }
> return len(f) > 0?

Nil is fine.

>
http://codereview.appspot.com/6494085/diff/6019/proto/pointer_reflect.go#newc...
> proto/pointer_reflect.go:154: type structPointerSlice struct {
> It's a bit awful, but the consistent name for this would be
> structPointerSlicePointer. That's probably not worth it.

Nope.

Russ

[r, 2012-09-06 18:03:12]

very cleanly done, and with the performance improvements to reflect it's now
practical.
nice.

try the type definition suggested

http://codereview.appspot.com/6494085/diff/6019/proto/pointer_unsafe.go
File proto/pointer_unsafe.go (right):

http://codereview.appspot.com/6494085/diff/6019/proto/pointer_unsafe.go#newco...
proto/pointer_unsafe.go:76: return f != ^field(0)
return f != invalidField

http://codereview.appspot.com/6494085/diff/6019/proto/pointer_unsafe.go#newco...
proto/pointer_unsafe.go:144: type word32 uintptr
how about
type word32 struct {
  **uint32
}
or
type word32 struct {
  ptr **uint32
}

[rsc, 2012-09-08 13:20:54]

Done and uploaded. You or David will have to clpatch + submit.

[dsymonds, 2012-09-09 01:32:31]

I was going to submit this, but running "make nuke install test" yielded:

  ...
  make -B my_test/test.pb.go
  protoc --go_out=. my_test/test.proto
  2012/09/09 11:30:56 protoc-gen-go: error:unknown type for ?
  --go_out: protoc-gen-go: Plugin failed with status code 1.
  ...

It works fine when reverting the clpatch.

[rsc, 2012-09-10 20:39:54]

On Sat, Sep 8, 2012 at 9:32 PM,  <dsymonds@golang.org> wrote:
> I was going to submit this, but running "make nuke install test"
> yielded:
>
>   ...
>   make -B my_test/test.pb.go
>   protoc --go_out=. my_test/test.proto
>   2012/09/09 11:30:56 protoc-gen-go: error:unknown type for ?
>   --go_out: protoc-gen-go: Plugin failed with status code 1.
>   ...
>
> It works fine when reverting the clpatch.

Sadness. I get 'protoc: command not found' or something like that. I
guess I finally have to install the protobuf code on my machine. It
was nice while it lasted.

Russ

[r, 2012-09-10 20:46:59]

> Sadness. I get 'protoc: command not found' or something like that. I
> guess I finally have to install the protobuf code on my machine. It
> was nice while it lasted.

Gack. I have no idea how you could make confident progress without
doing that first.

-rob

[rsc, 2012-09-10 21:04:16]

On Mon, Sep 10, 2012 at 4:46 PM, Rob Pike <r@golang.org> wrote:
>> Sadness. I get 'protoc: command not found' or something like that. I
>> guess I finally have to install the protobuf code on my machine. It
>> was nice while it lasted.
>
> Gack. I have no idea how you could make confident progress without
> doing that first.

You don't need protoc for 'go test'. You only need it if you change
the .proto files.

Anyway, it was a garbage collection bug, because of

type structPointer uintptr

hiding the pointers from the garbage collector. I fixed it (type
structPointer struct { p unsafe.Pointer }) and uploaded but I'd like
to rerun the performance comparison. On my Mac workstation I am
getting:

BenchmarkMarshal                           168.20       160.56    0.95x
BenchmarkUnmarshal                          90.53        79.08    0.87x

which is too slow, but I don't trust Mac profiling. The numbers in the
CL description are from my Linux laptop. I will try the code there
again tonight.

Russ

[rsc, 2012-09-11 23:05:26]

PTAL

I had to replace the methods with clunkily named functions in order to
get the pointer type.
(You can't have methods on named pointer types.)

It's the same structure, a little more long-winded in the code, and faster.

[r, 2012-09-11 23:48:19]

LGTM
leaving for dsymonds

[dsymonds, 2012-09-12 00:29:28]

LGTM

[dsymonds, 2012-09-12 00:36:31]

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

proto: make safe for App Engine

Factor unsafe code into its own file that can be replaced with
reflect code when building for App Engine.

benchmark                                old MB/s     new MB/s  speedup
BenchmarkMarshal                           126.34       120.38    0.95x
BenchmarkUnmarshal                         111.84       110.78    0.99x
BenchmarkMarshalBytes                      893.70       853.55    0.96x
BenchmarkUnmarshalBytes                    615.23       619.73    1.01x

Comparing the two new variants, old = unsafe, new = reflect:

benchmark                                old MB/s     new MB/s  speedup
BenchmarkMarshal                           120.38        50.52    0.42x
BenchmarkUnmarshal                         110.78        20.12    0.18x
BenchmarkMarshalBytes                      853.55       182.21    0.21x
BenchmarkUnmarshalBytes                    619.73       227.66    0.37x

But really these numbers are unfair since they ignore the
qualitative difference: proto was completely unavailable
on App Engine, and now it can be run there.

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

Committer: David Symonds <dsymonds@golang.org>

[rsc, 2012-09-12 00:41:41]

Sure, I'll give it a shot tomorrow.

[rsc, 2012-09-12 00:42:07]

On Tue, Sep 11, 2012 at 8:41 PM, Russ Cox <rsc@golang.org> wrote:
> Sure, I'll give it a shot tomorrow.

Well that went to the wrong place. Ignore.

[rog, 2012-09-12 10:57:24]

On 12 September 2012 00:05, Russ Cox <rsc@golang.org> wrote:
> PTAL
>
> I had to replace the methods with clunkily named functions in order to
> get the pointer type.
> (You can't have methods on named pointer types.)
>
> It's the same structure, a little more long-winded in the code, and faster.

I didn't realise that struct { *something } doesn't registerise as
well as *something.
It's a technique I use often. Did you find out how much impact using
that representation had on the performance of this package?

It seems to me that it would be better to use the natural representation
and fix the compiler, but perhaps that's hard to do.

[rsc, 2012-09-17 21:13:59]

> I didn't realise that struct { *something } doesn't registerise as
> well as *something.
> It's a technique I use often. Did you find out how much impact using
> that representation had on the performance of this package?
>
> It seems to me that it would be better to use the natural representation
> and fix the compiler, but perhaps that's hard to do.

Depends. It was 5-10% in this case but appeared in some tight loops.
I certainly agree that it shouldn't matter but I didn't want to hold
this CL up for it.

Russ

[rsc, 2012-09-17 21:14:40]

PS issue 4092