switch from json-iterator to forked stdlib json decoder #105030
Conversation
k8s-ci-robot
added
do-not-merge/hold
k8s-ci-robot
added
approved
|
@liggitt golang/go#46293 may give us better results post 1.18, but meanwhile I'm wondering if we can merge #104949 to catch any other issue we may hit with the tip version. |
|
Beyond the potential improvements that would let reflect2 drop some unsafe aspects, there have been quite a few correctness issues with json-iterator in the past, and the current allocations benchmarks make me question whether we want to continue to use it long term |
|
I left some comments on #104949, I find it hard to reason about the correctness of the reflect2 changes |
liggitt
force-pushed
the
json-stdlib
branch
from
b4c635e
to
d60b1e7
Compare
k8s-ci-robot
added
sig/auth
Given that most of the load that we have on kube-apiserver is actually protobuf-based (all components talk via protobuf) and the only json-originating load are test-originating requests, this won't be significantly visible there. |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/retest |
k8s-ci-robot
removed
the
do-not-merge/hold
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
|
Correctness is worth it. /lgtm |
k8s-ci-robot
added
lgtm
|
Thanks! We're still using extensively jsoniter in structured-merge-diff, mostly because we don't deserialize into a struct, but into our own datastructure. We should probably fix that but we also need to be super careful about performance. Let's try to talk about that. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: apelisse, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
|
fun... node bootstrap failure on package sha mismatch: /retest |
|
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
That module uses github.com/modern-go/reflect2 which is broken. First, it is brittle because it relies on golang's implementation details that are not stable across golang releases. For example, reflect2 was broken when go 1.18 came out. Second, reflect2 is effectively unmaintained: modern-go/reflect2#24 (comment) Json-iterator itself has had correctness issues like json-iterator/go#413. The Kubernetes project has mostly removed the dependency on json-iterator in patches like kubernetes/kubernetes#105030. Moreover, the Kubernetes authors found it out that json-iterator puts a lot of load on the allocator, so even the performance gains are questionable. Let us remove dependencies on json-iterator and reflect2.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Explores dropping json-iterator in favor of a fork of the stdlib json decoder with case-sensitivity, preservation of int/float distinctions, and strict decoding added
initial benchmarks on small-to-normal-sized inputs show a significant improvement in allocations over json-iterator (~30-45%), slightly improved memory use (~0-10%), and worse CPU time (~100% worse on typed decoding, 15% worse on untyped decoding):
At least some of the decreased CPU cost of json-iterator comes at the expense of correctness (e.g. json-iterator/go#413). Combined with the memory-safe implementation, I would argue that the switch is worth it, but I'd like to see what the scale tests here show.
As a side benefit, performance on pathological json inputs is significantly better as well for existing users of our utiljson package, and for users of jsoniter switching to the new unmarshaler:
cc @lavalamp @deads2k @smarterclayton @wojtek-t
Builds/depends on these PRs/issues: