Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

encoding/json: marshaling RawMessage has poor performance #33422

Open
rittneje opened this issue Aug 1, 2019 · 9 comments
Open

encoding/json: marshaling RawMessage has poor performance #33422

rittneje opened this issue Aug 1, 2019 · 9 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Performance
Milestone

Comments

@rittneje
Copy link

rittneje commented Aug 1, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12.6 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jrittner/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/jrittner/go-workspace"
GOPROXY=""
GORACE=""
GOROOT="/home/jrittner/go"
GOTMPDIR=""
GOTOOLDIR="/home/jrittner/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build506812392=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Ran a benchmark to compare marshaling a json.RawMessage, a string and a []byte.

package jsontest

import (
	"encoding/json"
	"testing"
)

const msg = `{"a":"aaaaaaa","b":{"c":["d","e"]}}`

var benchmarkResult interface{}

func BenchmarkRawMessage(b *testing.B) {
	x := json.RawMessage(msg)
	for i := 0; i < b.N; i++ {
		j, err := json.Marshal(x)
		if err != nil {
			b.Fatal(err)
		}
		benchmarkResult = j
	}
}

func BenchmarkString(b *testing.B) {
	x := msg
	for i := 0; i < b.N; i++ {
		j, err := json.Marshal(x)
		if err != nil {
			b.Fatal(err)
		}
		benchmarkResult = j
	}
}

func BenchmarkBytes(b *testing.B) {
	x := []byte(msg)
	for i := 0; i < b.N; i++ {
		j, err := json.Marshal(x)
		if err != nil {
			b.Fatal(err)
		}
		benchmarkResult = j
	}
}

What did you expect to see?

I expected marshaling a json.RawMessage to have the best performance of the three, since it should be a no-op.

What did you see instead?

It is 2 times slower than marshaling a string, and 3 times slower than marshaling a []byte.

BenchmarkRawMessage-2 1000000 1513 ns/op 232 B/op 7 allocs/op
BenchmarkString-2 2000000 869 ns/op 112 B/op 3 allocs/op
BenchmarkBytes-2 3000000 561 ns/op 128 B/op 3 allocs/op

@rittneje
Copy link
Author

rittneje commented Aug 1, 2019

Investigating further, I believe the slowdown is caused by it trying to unnecessarily compact/validate the json.

err = compact(&e.Buffer, b, opts.escapeHTML)
Replacing this with e.Buffer.Write(b) yields much better performance - 477 ns/op.

@agnivade
Copy link
Contributor

agnivade commented Aug 2, 2019

@mvdan @dsnet

@dsnet
Copy link
Member

dsnet commented Aug 2, 2019

trying to unnecessarily compact/validate the json

In some applications, this may be considered true, but as a general principle, the encoding/json package strives for correctness first over performance. We can speed up the implementation of compact, but we can't just trivially avoid the call here.

@rittneje
Copy link
Author

rittneje commented Aug 2, 2019

As far as I can tell, compact would only accomplish anything if you had a buggy json.Marshaler implementation. In all the years I've used Go, that has never happened, so I honestly cannot see any justification for this performance hit. It would be nice if there was at least an option on json.Encoder to say "I trust the output of all json.Marshalers to be correct, do not attempt to compact/validate it." (On that note, it occurs to me that there is no way for the encoder options to actually get passed into a custom MarshalJSON method. https://play.golang.org/p/b_DZHIrABif)

@dsnet
Copy link
Member

dsnet commented Aug 2, 2019

In all the years I've used Go, that has never happened, so I honestly cannot see any justification for this performance hit.

Earlier I said: "In some applications, this may be considered true". I don't doubt that this is probably true of your use case. However, it is the current behavior and we can't just remove it as some are relying on this property. Keep in mind that compact does more than simply validate, but also enforces consistent whitespace (or rather lack of) in all outputs.

On that note, it occurs to me that there is no way for the encoder options to actually get passed into a custom MarshalJSON method

Yes. This is a problem that I've written about before regarding encoding/json. In my opinion, this is the primary reason that an option like what you're requesting is hard to fit into the existing API and its current behaviors.

There are many reasonable features to add to encoding/json in isolation, but the problem is that none (or very few) of them operate orthogonally with the existing features.

@katiehockman katiehockman added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 5, 2019
@mvdan mvdan added this to the Unplanned milestone Aug 15, 2019
@mvdan
Copy link
Member

mvdan commented Aug 15, 2019

I'd suggest investigating ways to optimize the current code without changing the API nor adding any options. If it's still too slow, perhaps file a proposal to change the API.

My thinking is similar to @dsnet's; encoding/json values correctness above performance, so any proposed changes to change the package's API should be well thought out.

@qingyu31
Copy link

I'd suggest investigating ways to optimize the current code without changing the API nor adding any options. If it's still too slow, perhaps file a proposal to change the API.

My thinking is similar to @dsnet's; encoding/json values correctness above performance, so any proposed changes to change the package's API should be well thought out.

I've got same problem when I analysis performance of my application. Is it not ok to add options to encOpts?

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/205018 mentions this issue: encoding/json: prevent compact twice to improve precomputed performance.

@dsnet
Copy link
Member

dsnet commented Oct 6, 2023

Hi all, we kicked off a discussion for a possible "encoding/json/v2" package that addresses the spirit of this proposal.

The prototype v2 implementation has a better parser, able to verify and reformat the result of a MarshalJSON method call much fater. See https://github.com/go-json-experiment/jsonbench#rawvalue-types, which shows that v2 is between 3.6x to 9.1x faster for this situation.

agnivade added a commit to mattermost/mattermost that referenced this issue Nov 3, 2023
Marshalling a json.RawMessage is not zero overhead. Instead,
it compacts the raw message which starts to have an overhead
at scale.

golang/go#33422

Since we have full control over the message constructed, we
can simply write the byte slice into the network stream.
This gives considerable performance boost.

```
goos: linux
goarch: amd64
pkg: github.com/mattermost/mattermost/server/public/model
cpu: Intel(R) Core(TM) i5-8265U CPU @ 1.60GHz
             │   old.txt    │              new_2.txt              │
             │    sec/op    │   sec/op     vs base                │
EncodeJSON-8   1640.5n ± 2%   289.6n ± 1%  -82.35% (p=0.000 n=10)

             │  old.txt   │             new_2.txt             │
             │    B/op    │    B/op     vs base               │
EncodeJSON-8   528.0 ± 0%   503.0 ± 0%  -4.73% (p=0.000 n=10)

             │  old.txt   │             new_2.txt              │
             │ allocs/op  │ allocs/op   vs base                │
EncodeJSON-8   5.000 ± 0%   4.000 ± 0%  -20.00% (p=0.000 n=10)
```

P.S. No concerns over changing the model API because we are
still using 0.x

https://mattermost.atlassian.net/browse/MM-54998

```release-note
Improve websocket event marshalling performance
```
agnivade added a commit to mattermost/mattermost that referenced this issue Nov 8, 2023
Marshalling a json.RawMessage is not zero overhead. Instead,
it compacts the raw message which starts to have an overhead
at scale.

golang/go#33422

Since we have full control over the message constructed, we
can simply write the byte slice into the network stream.
This gives considerable performance boost.

```
goos: linux
goarch: amd64
pkg: github.com/mattermost/mattermost/server/public/model
cpu: Intel(R) Core(TM) i5-8265U CPU @ 1.60GHz
             │   old.txt    │              new_2.txt              │
             │    sec/op    │   sec/op     vs base                │
EncodeJSON-8   1640.5n ± 2%   289.6n ± 1%  -82.35% (p=0.000 n=10)

             │  old.txt   │             new_2.txt             │
             │    B/op    │    B/op     vs base               │
EncodeJSON-8   528.0 ± 0%   503.0 ± 0%  -4.73% (p=0.000 n=10)

             │  old.txt   │             new_2.txt              │
             │ allocs/op  │ allocs/op   vs base                │
EncodeJSON-8   5.000 ± 0%   4.000 ± 0%  -20.00% (p=0.000 n=10)
```

P.S. No concerns over changing the model API because we are
still using 0.x

https://mattermost.atlassian.net/browse/MM-54998

```release-note
Improve websocket event marshalling performance
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Performance
Projects
None yet
Development

No branches or pull requests

7 participants