code review 4160058: json: use base64 to encode []byte

json: use base64 to encode []byte

[rog, 2011-02-17 17:35:45]

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

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

[rsc, 2011-02-17 17:58:08]

Looks pretty good.  If we're doing this to make []byte
handling more efficient, let's clean up a few other
inefficiencies here too.

In decode, please add an unquoteBytes that returns []byte and
make unquote a wrapper around unquoteBytes.
In unquoteBytes, it would be nice to keep track of
whether any changes need to be made and delay
the allocation of the new []byte b until that is a certainty.
In the common case unquoteBytes can return s itself.
Then pick off []byte earlier in literal so that a base64
decode has 0 incidental mallocs as compared to
the 3 that has now: []byte -unquote-> []byte -> string -> []byte.

In encode, please use a base64.Encoder to avoid the
copy into the temporary (and arbitrarily large) []byte buffer.

Russ

[rog, 2011-02-18 11:24:22]

On 2011/02/17 17:58:08, rsc wrote:
> Looks pretty good.  If we're doing this to make []byte
> handling more efficient, let's clean up a few other
> inefficiencies here too.
> 
> In decode, please add an unquoteBytes that returns []byte and
> make unquote a wrapper around unquoteBytes.
> In unquoteBytes, it would be nice to keep track of
> whether any changes need to be made and delay
> the allocation of the new []byte b until that is a certainty.
> In the common case unquoteBytes can return s itself.
> Then pick off []byte earlier in literal so that a base64
> decode has 0 incidental mallocs as compared to
> the 3 that has now: []byte -unquote-> []byte -> string -> []byte.

done

> 
> In encode, please use a base64.Encoder to avoid the
> copy into the temporary (and arbitrarily large) []byte buffer.

in my tests using an Encoder directly was about 80% faster for
small buffers, and about 5% slower for large (50K+) buffers,
so i changed the code to choose the method depending on
the size of the buffer, but YMMV.

[rsc, 2011-02-22 20:57:17]

Very nearly there.

http://codereview.appspot.com/4160058/diff/5005/src/pkg/json/decode.go
File src/pkg/json/decode.go (right):

http://codereview.appspot.com/4160058/diff/5005/src/pkg/json/decode.go#newcode11
src/pkg/json/decode.go:11: "encoding/base64"
sort

http://codereview.appspot.com/4160058/diff/5005/src/pkg/json/decode.go#newcod...
src/pkg/json/decode.go:805: for i, c := range s {
good but could be better.
"☺" is no harder than "smiley"

for r = 0; r < len(s); {
    c = s[r]
    if c == '\\' || c == '"' || c < ' ' {
        break
    }
    if c < utf8.RuneSelf {
        r++
        continue
    }
    rune, size := utf8.DecodeRune(s[r:])
    if rune == utf8.RuneError && size == 1 {
        break
    }
    r += size
}
if r == len(s) {
    return s, true
}

http://codereview.appspot.com/4160058/diff/5005/src/pkg/json/encode.go
File src/pkg/json/encode.go (right):

http://codereview.appspot.com/4160058/diff/5005/src/pkg/json/encode.go#newcod...
src/pkg/json/encode.go:286: e.WriteByte('"')
a
break
.

drop else

[rog, 2011-02-23 08:50:37]

PTAL

http://codereview.appspot.com/4160058/diff/5005/src/pkg/json/decode.go
File src/pkg/json/decode.go (right):

http://codereview.appspot.com/4160058/diff/5005/src/pkg/json/decode.go#newcode11
src/pkg/json/decode.go:11: "encoding/base64"
On 2011/02/22 20:57:17, rsc wrote:
> sort

Done.

http://codereview.appspot.com/4160058/diff/5005/src/pkg/json/decode.go#newcod...
src/pkg/json/decode.go:805: for i, c := range s {
On 2011/02/22 20:57:17, rsc wrote:
> good but could be better.
> "☺" is no harder than "smiley"

ok. i'd wanted to keep the loop as a range, which is quite a bit faster (20% on
my machine), but it's worth avoiding the copy penalty for well-formed non-ascii.

we could even use three loops (range, the above loop and the original loop) in
sequence to keep the common-case range loop, but that's probably overkill.

done.

http://codereview.appspot.com/4160058/diff/5005/src/pkg/json/encode.go
File src/pkg/json/encode.go (right):

http://codereview.appspot.com/4160058/diff/5005/src/pkg/json/encode.go#newcod...
src/pkg/json/encode.go:286: e.WriteByte('"')
On 2011/02/22 20:57:17, rsc wrote:
> a
> break
> .
> 
> drop else

i had tried this, and thought it looked better with the else, since the
two pieces of code are roughly equal weight.
but done anyway.

[rsc, 2011-02-23 15:51:38]

> ok. i'd wanted to keep the loop as a range, which is quite a bit faster
> (20% on my machine), but it's worth avoiding the copy penalty for
> well-formed non-ascii.

the loop will get faster.

> i had tried this, and thought it looked better with the else, since the
> two pieces of code are roughly equal weight.
> but done anyway.

thanks.  my rationale was to exit early on the
special case, leaving the general case as the
straight-line code.

[rsc, 2011-02-23 15:55:51]

LGTM

[rsc, 2011-02-23 16:32:36]

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

json: use base64 to encode []byte

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

Committer: Russ Cox <rsc@golang.org>