switch the json backend to json-iterator #19

neolit123 · 2019-04-26T17:02:32Z

add a couple of json interator configurations:

jsonIterator should be compatible with the old encoding/json
there is a breaking change here because the "JSONOpt" is now
a no-op, although the options in encoding/json are not very useful.
caseSensitiveStrictJSONIterator is compatible with
econding/json WRT to DisallowUnknownFields, but it also has
CaseSensitive. This is a breaking change for strict unmarshal users
that previously allowed case-insensitive fields.

Something else to note is that json-iter does not seem to tolerate
field keys called "true", which the one from stdlib does.
Unit tests had to be adapted because of that.

Other changes:

add a new public method UnmarshalWithConfig that allows passing
a json-iter configuration
update/add unit tests
update go.mod/sum
remove yaml_go110*.go
These files had the purpose to handle DisallowUnknownFields for
json.Decoder which is no longer needed.

/kind feature
/priority important-longterm

fixes #17
fixes #15

/assign @dims
/cc @sttts @errordeveloper @stealthybox
/hold

k8s-ci-robot · 2019-04-26T17:02:34Z

@neolit123: GitHub didn't allow me to request PR reviews from the following users: stealthybox, errordeveloper.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

add a couple of json interator configurations:

jsonIterator should be compatible with the old encoding/json
there is a breaking change here because the "JSONOpt" is now
a no-op, although the options in encoding/json are not very useful.

caseSensitiveStrictJSONIterator is compatible with
econding/json WRT to DisallowUnknownFields, but it also has
CaseSensitive. This is a breaking change for strict unmarshal users
that previously allowed case-insensitive fields.

Something else to note is that json-iter does not seem to tolerate
field keys called "true", which the one from stdlib does.
Unit tests had to be adapted because of that.

Other changes:

add a new public method UnmarshalWithConfig that allows passing
a json-iter configuration

update/add unit tests

update go.mod/sum

remove yaml_go110*.go
These files had the purpose to handle DisallowUnknownFields for
json.Decoder which is no longer needed.

/kind featute
/priority important-longterm

fixes #17
fixes #15

/assign @dims
/cc @sttts @errordeveloper @stealthybox
/hold

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

liggitt · 2019-04-26T17:08:30Z

yaml.go

 // Marshal marshals the object into JSON then converts JSON to YAML and returns the
 // YAML.
 func Marshal(o interface{}) ([]byte, error) {
-	j, err := json.Marshal(o)
+	j, err := caseSensitiveStrictJSONIterator.Marshal(o)


@smarterclayton, didn't you see performance regressions in json-iter marshaling?

Would they really be relevant for YAML encoding? It's not that we do that in critical paths, do we?

Also, this doesn't affect all of YAML code paths in the current form.

Btw json-iterator isn't fully compatible with the standard library right now, created json-iterator/go#371. PTAL.

json-iterator/go#371 has merged.

martina-if · 2019-05-21T10:50:31Z

Thanks for making these changes! LGTM Is there anything we can do to help get this merged? :)

errordeveloper · 2019-05-29T14:21:17Z

Hey @neolit123, as Martina said - we'd be happy to help :)

neolit123 · 2019-05-29T18:33:47Z

@errordeveloper up to the reviewers at this point. :)

nikhita · 2019-06-03T20:21:24Z

@smarterclayton, didn't you see performance regressions in json-iter marshaling?

Ref: kubernetes/kubernetes#70574 (comment), kubernetes/kubernetes#63242 (comment)

The second PR referenced above did have high allocations:

Running go test ./vendor/k8s.io/apiserver/pkg/endpoints/ -test.bench='Benchmark(Get|WatchHTTP)' -test.run=XXX -benchmem -test.memprofile=/tmp/mem2 -test.benchtime 5s

On k/k master right now:

goos: linux
goarch: amd64
pkg: k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/endpoints
BenchmarkGet-4              	   30000	    256834 ns/op	   15794 B/op	     127 allocs/op
BenchmarkWatchHTTP-4        	  100000	     92040 ns/op	    1523 B/op	      24 allocs/op
BenchmarkWatchHTTP_UTF8-4   	  100000	     96523 ns/op	    1829 B/op	      26 allocs/op
PASS
ok  	k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/endpoints	31.248s

k/k right now with changes in the second PR (kubernetes/kubernetes#63242):

goos: linux
goarch: amd64
pkg: k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/endpoints
BenchmarkGet-4                     10000	    691435 ns/op	   19122 B/op	     163 allocs/op
BenchmarkWatchHTTP-4        	  200000	     48193 ns/op	    5495 B/op	      64 allocs/op
BenchmarkWatchHTTP_UTF8-4   	  200000	     53738 ns/op	    5909 B/op	      69 allocs/op
PASS
ok  	k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/endpoints	28.604s

k/k with changes in this PR:

goos: linux
goarch: amd64
pkg: k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/endpoints
BenchmarkGet-4              	   50000	    139204 ns/op	   15793 B/op	     127 allocs/op
BenchmarkWatchHTTP-4        	  200000	     56627 ns/op	    1522 B/op	      24 allocs/op
BenchmarkWatchHTTP_UTF8-4   	  100000	     62922 ns/op	    1829 B/op	      26 allocs/op
PASS
ok  	k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/endpoints	27.378s

So it should probably be fine...

smarterclayton · 2019-06-04T03:51:33Z

You should also run the serialization benchmarks in pkg/api/testing/serialization_test.go to get a before and after round trip time (that tests several variants of objects, whereas BenchmarkGet tests a single known object).

nikhita · 2019-06-04T12:17:13Z

You should also run the serialization benchmarks in pkg/api/testing/serialization_test.go to get a before and after round trip time (that tests several variants of objects, whereas BenchmarkGet tests a single known object).

Running existing benchmarks (before and after) yielded similar results...but that isn't surprising because none of the benchmarks actually look for yaml marshaling and unmarshaling.

Created kubernetes/kubernetes#78688 for adding benchmarks for yaml marshaling and unmarshaling as well. Using these benchmarks, incorporating changes in this PR into k/k yield higher allocations... :(

Running go test k8s.io/kubernetes/pkg/api/testing/ -test.bench='BenchmarkEncodeYAMLMarshal|BenchmarkDecodeIntoYAML' -test.run=XXX -benchmem -test.memprofile=/tmp/mem2 -test.benchtime 5s

Before:

goos: linux
goarch: amd64
pkg: k8s.io/kubernetes/pkg/api/testing
BenchmarkEncodeYAMLMarshal-4   	   10000	    781873 ns/op	  256102 B/op	    1438 allocs/op
BenchmarkDecodeIntoYAML-4      	   10000	    627891 ns/op	  167788 B/op	    1485 allocs/op
PASS
ok  	k8s.io/kubernetes/pkg/api/testing	14.354s

After:

goos: linux
goarch: amd64
pkg: k8s.io/kubernetes/pkg/api/testing
BenchmarkEncodeYAMLMarshal-4   	   10000	    867119 ns/op	  260776 B/op	    1443 allocs/op
BenchmarkDecodeIntoYAML-4      	   10000	    692040 ns/op	  181813 B/op	    1609 allocs/op
PASS
ok  	k8s.io/kubernetes/pkg/api/testing	15.842s

smarterclayton · 2019-06-04T15:47:15Z

What is the underlying goal of this PR? Use json-iterator because ? Consistency with Kube is important, but it's not all. Was there a discussion about the why? I don't see many comments in #17

neolit123 · 2019-06-04T16:06:45Z

What is the underlying goal of this PR? Use json-iterator because ? Consistency with Kube is important, but it's not all. Was there a discussion about the why? I don't see many comments in #17

main driver was discussed in #15
the current library's strict unmarshal is not case-sensitive.

neolit123 · 2019-06-19T13:44:59Z

/assign @smarterclayton

dims · 2019-07-08T14:45:41Z

/unassign

neolit123 · 2019-07-26T12:54:55Z

here is an example of this problem in the wild:
kubernetes/kubernetes#80582

kubeadm has strict unmarshaling to show warnings and then perform regular unmarshall (two pass).
however the upper case ClusterConfiguration field was not caught; should have been clusterConfiguration.

neolit123 · 2019-08-04T00:51:18Z

so going back to the OP and the discussion there are at lease 3 concerns here:

benchmark results
the overhead after the change is not high and fixing the bug for StrictUnmarshal users can justify the small decrease in performance. on the other hand there aren't that many StrictUnmarshal users out there, so the decrease in performance is probably making a more apparent tilt of the scale.
breaking change 1:

This is a breaking change for strict unmarshal users
that previously allowed case-insensitive fields.

we enabled strict unmarshal in this library not so long ago, and given strict unmarshal is optional it seems that this might not affect that many users. but this is still probably going to break someone, somewhere once they move to the latest SHA of this repository.

breaking change 2:

there is a breaking change here because the "JSONOpt" is now
a no-op, although the options in encoding/json are not very useful.

not sure if this will break anyone, probably not.

i will update this PR and leave it for a couple of more weeks.

k8s-ci-robot · 2019-08-04T01:06:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: neolit123
To complete the pull request process, please assign smarterclayton
You can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

add a couple of json interator configurations: 1) jsonIterator should be compatible with the old encoding/json there is a breaking change here because the "JSONOpt" is now a no-op, although the options in encoding/json are not very useful. 2) caseSensitiveStrictJSONIterator is compatible with econding/json WRT to DisallowUnknownFields, but it also has CaseSensitive. This is a breaking change for strict unmarshal users that previously allowed case-insensitive fields. Something else to note is that json-iter does not seem to tolerate field keys called "true", which the one from stdlib does. Unit tests had to be adapted because of that. Other changes: - add a new public method UnmarshalWithConfig that allows passing a json-iter configuration - update/add unit tests - update go.mod/sum - remove yaml_go110*.go These files had the purpose to handle DisallowUnknownFields for json.Decoder which is no longer needed.

neolit123 · 2019-08-15T21:03:43Z

clearing some pending PRs from my backlog.
if someone feels like they want to take this over i will leave the branch.
thanks!

neolit123 · 2020-06-08T18:02:42Z

we are seeing more cases of this in the wild:
kubernetes/kubeadm#2167

k8s-ci-robot assigned dims Apr 26, 2019

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Apr 26, 2019

k8s-ci-robot requested a review from sttts April 26, 2019 17:02

liggitt reviewed Apr 26, 2019

View reviewed changes

This was referenced Apr 26, 2019

CI: only test using 1.12.x #20

Merged

Strict unmarshalling should be case-sensitive #15

Open

nikhita mentioned this pull request Jun 4, 2019

Add benchmarks for yaml marshaling and unmarshaling kubernetes/kubernetes#78688

Merged

k8s-ci-robot assigned smarterclayton Jun 19, 2019

errordeveloper mentioned this pull request Jun 21, 2019

strict parser is case-insensitive eksctl-io/eksctl#753

Closed

k8s-ci-robot unassigned dims Jul 8, 2019

neolit123 force-pushed the use-json-iter branch from 722a658 to 652a983 Compare August 4, 2019 01:06

neolit123 force-pushed the use-json-iter branch from 652a983 to d8dcdb8 Compare August 4, 2019 01:09

neolit123 closed this Aug 15, 2019

neolit123 mentioned this pull request Aug 25, 2019

Should this library use json-iterator? #17

Closed

neolit123 mentioned this pull request Sep 15, 2021

switch from json-iterator to forked stdlib json decoder kubernetes/kubernetes#105030

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

switch the json backend to json-iterator #19

switch the json backend to json-iterator #19

neolit123 commented Apr 26, 2019 •

edited

Loading

k8s-ci-robot commented Apr 26, 2019

liggitt Apr 26, 2019

sttts Apr 27, 2019

errordeveloper May 29, 2019

nikhita Jun 2, 2019

nikhita Jun 3, 2019

martina-if commented May 21, 2019

errordeveloper commented May 29, 2019

neolit123 commented May 29, 2019

nikhita commented Jun 3, 2019 •

edited

Loading

smarterclayton commented Jun 4, 2019

nikhita commented Jun 4, 2019

smarterclayton commented Jun 4, 2019

neolit123 commented Jun 4, 2019

neolit123 commented Jun 19, 2019

dims commented Jul 8, 2019

neolit123 commented Jul 26, 2019

neolit123 commented Aug 4, 2019

k8s-ci-robot commented Aug 4, 2019

neolit123 commented Aug 15, 2019

neolit123 commented Jun 8, 2020

switch the json backend to json-iterator #19

switch the json backend to json-iterator #19

Conversation

neolit123 commented Apr 26, 2019 • edited Loading

k8s-ci-robot commented Apr 26, 2019

liggitt Apr 26, 2019

Choose a reason for hiding this comment

sttts Apr 27, 2019

Choose a reason for hiding this comment

errordeveloper May 29, 2019

Choose a reason for hiding this comment

nikhita Jun 2, 2019

Choose a reason for hiding this comment

nikhita Jun 3, 2019

Choose a reason for hiding this comment

martina-if commented May 21, 2019

errordeveloper commented May 29, 2019

neolit123 commented May 29, 2019

nikhita commented Jun 3, 2019 • edited Loading

smarterclayton commented Jun 4, 2019

nikhita commented Jun 4, 2019

smarterclayton commented Jun 4, 2019

neolit123 commented Jun 4, 2019

neolit123 commented Jun 19, 2019

dims commented Jul 8, 2019

neolit123 commented Jul 26, 2019

neolit123 commented Aug 4, 2019

k8s-ci-robot commented Aug 4, 2019

neolit123 commented Aug 15, 2019

neolit123 commented Jun 8, 2020

neolit123 commented Apr 26, 2019 •

edited

Loading

nikhita commented Jun 3, 2019 •

edited

Loading