code review 5416046: json: speed up encoding, caching reflect calls

json: speed up encoding, caching reflect calls

Before
json.BenchmarkCodeEncoder  10  181232100 ns/op  10.71 MB/s
json.BenchmarkCodeMarshal  10  184578000 ns/op  10.51 MB/s

After:
json.BenchmarkCodeEncoder  10  146444000 ns/op  13.25 MB/s
json.BenchmarkCodeMarshal  10  151428500 ns/op  12.81 MB/s

[bradfitz, 2011-11-18 14:40:55]

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

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

[rsc, 2011-11-18 15:48:23]

This seems reasonable but maybe is not enough?
I was wondering if there should just be a global
per-type struct with useful information.

type structInfo struct {
    field []reflect.StructField  // for use by Marshal
    byName map[string]reflect.StructField  // for use by Unmarshal
}

var typeCache struct {
    sync.RWMutex
    m map[reflect.Type]*structInfo
}

Russ

[bradfitz, 2011-11-20 16:49:29]

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

Please take another look.

[bradfitz, 2011-11-20 16:58:11]

PTAL. I took it further, but just for encode so far. I initially tried like
you suggested (an outer structInfo that could potentially hold stuff for
either encode or decode), but it resulted in lots of nil checks, since
different parts of structInfo could be populated based on whether encode or
decode had been already called on that type.  (or the map entry not present
at all.)

I think it's simpler as I have it now, caching just what's needed for
encode.  Then we can do similarly for decode.

On Fri, Nov 18, 2011 at 7:48 AM, Russ Cox <rsc@golang.org> wrote:

> This seems reasonable but maybe is not enough?
> I was wondering if there should just be a global
> per-type struct with useful information.
>
> type structInfo struct {
>    field []reflect.StructField  // for use by Marshal
>    byName map[string]reflect.StructField  // for use by Unmarshal
> }
>
> var typeCache struct {
>    sync.RWMutex
>    m map[reflect.Type]*structInfo
> }
>
> Russ
>

[r, 2011-11-20 17:47:41]

This is just a cache. The package needs a rewrite.

This change is OK, but only as a temporary measure.

[bradfitz, 2011-11-20 17:59:17]

On Sun, Nov 20, 2011 at 12:47 PM, <r@golang.org> wrote:

> This is just a cache. The package needs a rewrite.
>

A Theseusian rewrite worked for the http package. Might work here too.

[rsc, 2011-11-21 15:06:50]

http://codereview.appspot.com/5416046/diff/4001/src/pkg/encoding/json/encode.go
File src/pkg/encoding/json/encode.go (right):

http://codereview.appspot.com/5416046/diff/4001/src/pkg/encoding/json/encode....
src/pkg/encoding/json/encode.go:201: // tagCache is a cache of seen StructTag's
"json" fields.
Unused, I believe.

http://codereview.appspot.com/5416046/diff/4001/src/pkg/encoding/json/encode....
src/pkg/encoding/json/encode.go:479: defer typeCacheLock.RUnlock()
The locking here is suspect.  I think you need:

typeCacheLock.RLock()
fs, ok := encodeFieldsCache[t]
typeCacheLock.RUnlock()
if ok {
    return fs
}

typeCacheLock.Lock()
defer typeCacheLock.Unlock()
fs, ok := encodeFieldsCache[t]
if ok {
    return fs
}

[bradfitz, 2011-11-21 15:25:18]

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

Please take another look.

[bradfitz, 2011-11-21 15:25:54]

PTAL

http://codereview.appspot.com/5416046/diff/4001/src/pkg/encoding/json/encode.go
File src/pkg/encoding/json/encode.go (right):

http://codereview.appspot.com/5416046/diff/4001/src/pkg/encoding/json/encode....
src/pkg/encoding/json/encode.go:201: // tagCache is a cache of seen StructTag's
"json" fields.
On 2011/11/21 15:06:51, rsc wrote:
> Unused, I believe.

Indeed.

http://codereview.appspot.com/5416046/diff/4001/src/pkg/encoding/json/encode....
src/pkg/encoding/json/encode.go:479: defer typeCacheLock.RUnlock()
On 2011/11/21 15:06:51, rsc wrote:
> The locking here is suspect.  

uh, whoops.

[rsc, 2011-11-21 15:43:38]

LGTM

[bradfitz, 2011-11-21 15:49:18]

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

json: speed up encoding, caching reflect calls

Before
json.BenchmarkCodeEncoder  10  181232100 ns/op  10.71 MB/s
json.BenchmarkCodeMarshal  10  184578000 ns/op  10.51 MB/s

After:
json.BenchmarkCodeEncoder  10  146444000 ns/op  13.25 MB/s
json.BenchmarkCodeMarshal  10  151428500 ns/op  12.81 MB/s

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