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

replace encoding/json with goccy/go-json (#2421 expanded) #3161

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

roeest
Copy link
Contributor

@roeest roeest commented Jun 25, 2024

This is a revival of pr #2421.

I switched over to https://github.com/goccy/go-json instead of encoding/json. Previously it wasn't accepted because it only showed a ~5% improvement in performance in the starwars test suite. I added a test that handles larger json requests and responses (_examples/benchmarking) and in those cases the improvement was better than 5%.

goarch: arm64
pkg: github.com/99designs/gqlgen/_examples/benchmarking
                                                                      │ libjson.txt  │            goccyjson.txt            │
                                                                      │    sec/op    │   sec/op     vs base                │
QueriesOfVariableSizes/input_size:_100000_output_size_100000-10          1.367m ± 0%   1.004m ± 1%  -26.60% (p=0.000 n=10)
QueriesOfVariableSizes/input_size:_1000000_output_size_1000000-10       13.176m ± 1%   8.746m ± 1%  -33.62% (p=0.000 n=10)
QueriesOfVariableSizes/input_size:_10000000_output_size_1000000-10       61.62m ± 1%   28.30m ± 1%  -54.07% (p=0.000 n=10)
QueriesOfVariableSizes/input_size:_10000000_output_size_10000000-10     127.81m ± 0%   83.67m ± 0%  -34.54% (p=0.000 n=10)
QueriesOfVariableSizes/input_size:_100000000_output_size_100000000-10   1293.4m ± 3%   822.3m ± 1%  -36.42% (p=0.000 n=10)
geomean                                                                  44.95m        27.96m       -37.79%

                                                                      │ libjson.txt  │            goccyjson.txt             │
                                                                      │     B/op     │     B/op      vs base                │
QueriesOfVariableSizes/input_size:_100000_output_size_100000-10         1.491Mi ± 0%   1.648Mi ± 1%  +10.50% (p=0.000 n=10)
QueriesOfVariableSizes/input_size:_1000000_output_size_1000000-10       14.94Mi ± 1%   17.06Mi ± 1%  +14.19% (p=0.000 n=10)
QueriesOfVariableSizes/input_size:_10000000_output_size_1000000-10      134.4Mi ± 0%   135.3Mi ± 0%   +0.72% (p=0.000 n=10)
QueriesOfVariableSizes/input_size:_10000000_output_size_10000000-10     160.6Mi ± 2%   186.8Mi ± 0%  +16.34% (p=0.000 n=10)
QueriesOfVariableSizes/input_size:_100000000_output_size_100000000-10   1.754Gi ± 0%   1.848Gi ± 0%   +5.31% (p=0.000 n=10)
geomean                                                                 61.27Mi        66.94Mi        +9.26%

                                                                      │ libjson.txt │           goccyjson.txt            │
                                                                      │  allocs/op  │ allocs/op   vs base                │
QueriesOfVariableSizes/input_size:_100000_output_size_100000-10          214.0 ± 0%   213.0 ± 0%   -0.47% (p=0.000 n=10)
QueriesOfVariableSizes/input_size:_1000000_output_size_1000000-10        239.5 ± 0%   235.0 ± 0%   -1.88% (p=0.000 n=10)
QueriesOfVariableSizes/input_size:_10000000_output_size_1000000-10       259.0 ± 0%   253.0 ± 0%   -2.32% (p=0.000 n=10)
QueriesOfVariableSizes/input_size:_10000000_output_size_10000000-10      269.0 ± 1%   262.0 ± 0%   -2.60% (p=0.000 n=10)
QueriesOfVariableSizes/input_size:_100000000_output_size_100000000-10    419.0 ± 0%   336.5 ± 0%  -19.69% (p=0.000 n=10)
geomean                                                                  272.3        256.8        -5.69%

As you can see here the runtime improvement is 37% on average and the memory allocations are less frequent averaging 5% less allocations and the memory usage increased by less than 10% on average.

If you prefer i can make this a configurable value, it'll be a bit tricky and i'll probably have to use some sort of global variable.

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@coveralls
Copy link

Coverage Status

coverage: 74.663% (-0.04%) from 74.705%
when pulling 77212b2 on roeest:fast-json
into dd37ea0 on 99designs:master.

@coveralls
Copy link

Coverage Status

coverage: 74.673% (-0.03%) from 74.705%
when pulling 93f2dfd on roeest:fast-json
into dd37ea0 on 99designs:master.

@coveralls
Copy link

Coverage Status

coverage: 74.705%. remained the same
when pulling 47121ce on roeest:fast-json
into dd37ea0 on 99designs:master.

@coveralls
Copy link

Coverage Status

coverage: 74.673% (-0.03%) from 74.705%
when pulling 92628d2 on roeest:fast-json
into dd37ea0 on 99designs:master.

@coveralls
Copy link

Coverage Status

coverage: 74.673% (-0.03%) from 74.705%
when pulling ab2980d on roeest:fast-json
into dd37ea0 on 99designs:master.

@roeest roeest changed the title replace encoding/json with goccy/go-json (#2421 expanded)Fast json replace encoding/json with goccy/go-json (#2421 expanded) Jun 26, 2024
@adomaskizogian
Copy link

adomaskizogian commented Sep 13, 2024

DO NOT MERGE THIS
I REPEAT
DO NOT EVER MERGE THIS

goccy/go-json IS NOT A DROP IN REPLACEMENT FOR STDLIB encoding/json.
I went the same route before and tried replacing the encoder in one of my codebases. Luckily test suite was wide enough to detect that marshaled responses are different and are missing fields. It would fail to produce an equivalent json string if the struct had more than 16 fields. Some random field would be missing.
There are many other bugs besides the one I encountered. Look at the goccy/go-json issues list. All of them a very serious and there are plenty bugs that cause panics.
It is extremely broken implementation and the bugs are not some edge cases. These all are very common cases and easy to reproduce.

I would personally never use it on production.

goccy/go-json#510
goccy/go-json#512
goccy/go-json#519

@StevenACoffman
Copy link
Collaborator

@adomaskizogian Yeah, I actually tried merging this a couple of times and ran into problems like what you described. I'm not categorically opposed to using goccy/go-json but there would need to be some better proof that the library is more of a drop in replacement for the standards lib.

For instance, https://github.com/nst/JSONTestSuite is a comprehensive test suite for RFC 8259 compliant JSON parsers, and it would require goccy/go-json to pass that.

@giautm
Copy link
Contributor

giautm commented Sep 16, 2024

It should be configurable, to generate code depends on the end-user choose, but I prefer standard library for all of cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants