code review 111230043: encoding/gob: speedup encoding

encoding/gob: speedup encoding

Replace typeLock with copy-on-write map.
Use double-checked locking when creating encoder program.

benchmark                               old ns/op     new ns/op     delta
BenchmarkEndToEndPipe                   7722          7709          -0.17%
BenchmarkEndToEndPipe-2                 5114          4344          -15.06%
BenchmarkEndToEndPipe-4                 3192          2429          -23.90%
BenchmarkEndToEndPipe-8                 1833          1438          -21.55%
BenchmarkEndToEndPipe-16                1332          983           -26.20%
BenchmarkEndToEndPipe-32                1444          675           -53.25%
BenchmarkEndToEndByteBuffer             6474          6019          -7.03%
BenchmarkEndToEndByteBuffer-2           4280          2810          -34.35%
BenchmarkEndToEndByteBuffer-4           2264          1774          -21.64%
BenchmarkEndToEndByteBuffer-8           1275          979           -23.22%
BenchmarkEndToEndByteBuffer-16          1257          753           -40.10%
BenchmarkEndToEndByteBuffer-32          1342          644           -52.01%
BenchmarkEndToEndArrayByteBuffer        727725        671349        -7.75%
BenchmarkEndToEndArrayByteBuffer-2      394079        320473        -18.68%
BenchmarkEndToEndArrayByteBuffer-4      211785        178175        -15.87%
BenchmarkEndToEndArrayByteBuffer-8      141003        118857        -15.71%
BenchmarkEndToEndArrayByteBuffer-16     139249        86367         -37.98%
BenchmarkEndToEndArrayByteBuffer-32     144128        73454         -49.04%

Benchmarks are from:
https://codereview.appspot.com/115960043/

[dvyukov, 2014-07-18 10:39:57]

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

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

[r, 2014-07-18 18:06:12]

NOT LGTM

I just spent a week getting unsafe out of this package.

[rsc, 2014-08-05 14:18:24]

This certainly looks better than using unsafe. However, I am not sure why it is
correct to return nil in the recursive types case. Shouldn't we return a correct
encoder engine to complete the recursion? Perhaps building needs to be a
map[*typeInfo]*encEngine?

[dvyukov, 2014-08-05 14:47:11]

On 2014/08/05 14:18:24, rsc wrote:
> This certainly looks better than using unsafe. However, I am not sure why it
is
> correct to return nil in the recursive types case. Shouldn't we return a
correct
> encoder engine to complete the recursion? Perhaps building needs to be a

> map[*typeInfo]*encEngine?

The only user of recursive types is encOpFor(reflect.Struct), and it ignores the
return value:
getEncEngine(userType(typ), building)

instead it remembers the *location* where the enc will reside and dereference it
later:
info := mustGetTypeInfo(typ)
enc := info.encoder.Load().(*encEngine)

You can see that currently we do return non-nil value in this case, but the
returned value is bad and must not be used anyway, so it's safer to just return
nil:

// Assign the encEngine now, so recursive types work correctly. But...
info.encoder = new(encEngine)
...
info.encoder = compileEnc(ut)

[r, 2014-08-05 21:06:13]

https://codereview.appspot.com/111230043/diff/100001/src/pkg/encoding/gob/enc...
File src/pkg/encoding/gob/encode.go (right):

https://codereview.appspot.com/111230043/diff/100001/src/pkg/encoding/gob/enc...
src/pkg/encoding/gob/encode.go:615: info, err1 := getTypeInfo(ut)
s/err1/err/
some dreg not doubt.

https://codereview.appspot.com/111230043/diff/100001/src/pkg/encoding/gob/typ...
File src/pkg/encoding/gob/type.go (right):

https://codereview.appspot.com/111230043/diff/100001/src/pkg/encoding/gob/typ...
src/pkg/encoding/gob/type.go:692: // to get the current version of the map.
While writers make a full copy of
s/While w/W/

https://codereview.appspot.com/111230043/diff/100001/src/pkg/encoding/gob/typ...
src/pkg/encoding/gob/type.go:693: // the map and atomically update the pointer
to point to the new map.
// Under heavy read contention, this is significantly faster than a map
protected by a mutex.

https://codereview.appspot.com/111230043/diff/100001/src/pkg/encoding/gob/typ...
src/pkg/encoding/gob/type.go:702: m, ok :=
typeInfoMap.Load().(map[reflect.Type]*typeInfo)
this code appears twice. refactor into a function lookupTypeInfo(ut
*userTypeInfo) *typeInfo. then

info := lookupTypeInfo(ut)
if info != nil {
 return info
}

no need to have second function; just flow  into code below.

https://codereview.appspot.com/111230043/diff/100001/src/pkg/encoding/gob/typ...
src/pkg/encoding/gob/type.go:720: }
info := lookupTypeInfo(ut)
if info != nil {
 return info
}

https://codereview.appspot.com/111230043/diff/100001/src/pkg/encoding/gob/typ...
src/pkg/encoding/gob/type.go:760: newm := make(map[reflect.Type]*typeInfo)
prefix with
// Create new map with old contents plus new entry.

[dvyukov, 2014-08-05 21:34:09]

https://codereview.appspot.com/111230043/diff/100001/src/pkg/encoding/gob/enc...
File src/pkg/encoding/gob/encode.go (right):

https://codereview.appspot.com/111230043/diff/100001/src/pkg/encoding/gob/enc...
src/pkg/encoding/gob/encode.go:615: info, err1 := getTypeInfo(ut)
On 2014/08/05 21:06:13, r wrote:
> s/err1/err/
> some dreg not doubt.

Done.

https://codereview.appspot.com/111230043/diff/100001/src/pkg/encoding/gob/typ...
File src/pkg/encoding/gob/type.go (right):

https://codereview.appspot.com/111230043/diff/100001/src/pkg/encoding/gob/typ...
src/pkg/encoding/gob/type.go:692: // to get the current version of the map.
While writers make a full copy of
On 2014/08/05 21:06:13, r wrote:
> s/While w/W/

Done.

https://codereview.appspot.com/111230043/diff/100001/src/pkg/encoding/gob/typ...
src/pkg/encoding/gob/type.go:693: // the map and atomically update the pointer
to point to the new map.
On 2014/08/05 21:06:13, r wrote:
> // Under heavy read contention, this is significantly faster than a map
> protected by a mutex.

Done.

https://codereview.appspot.com/111230043/diff/100001/src/pkg/encoding/gob/typ...
src/pkg/encoding/gob/type.go:702: m, ok :=
typeInfoMap.Load().(map[reflect.Type]*typeInfo)
On 2014/08/05 21:06:13, r wrote:
> this code appears twice. refactor into a function lookupTypeInfo(ut
> *userTypeInfo) *typeInfo. then
> 
> info := lookupTypeInfo(ut)
> if info != nil {
>  return info
> }

Done

> no need to have second function; just flow  into code below.

There is that https://code.google.com/p/go/issues/detail?id=5712 that results in
deferreturn call even if there are no uses of defer yet. I am not sure how much
it is an excuse for splitting functions, but it makes me make such weird
things...

https://codereview.appspot.com/111230043/diff/100001/src/pkg/encoding/gob/typ...
src/pkg/encoding/gob/type.go:720: }
On 2014/08/05 21:06:13, r wrote:
> info := lookupTypeInfo(ut)
> if info != nil {
>  return info
> }

Done.

https://codereview.appspot.com/111230043/diff/100001/src/pkg/encoding/gob/typ...
src/pkg/encoding/gob/type.go:760: newm := make(map[reflect.Type]*typeInfo)
On 2014/08/05 21:06:13, r wrote:
> prefix with
> // Create new map with old contents plus new entry.

Done.

[dvyukov, 2014-09-17 03:01:23]

recreated as cl/147720043
this is broken after pkg move