Migrate to encoding/json/v2#292
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XL
|
For the |
a4b6871 to
bdce391
Compare
I fixed the test failure. |
|
/assign @BenTheElder @liggitt |
|
/assign @jpbetz |
I suspect using a json marshal function (like MarshalWrite) that doesn't append a newline would be a more efficient way to accomplish that |
| return nil, fmt.Errorf("parsing JSON: %v", err) | ||
| } | ||
|
|
||
| k := rawKey.String() |
There was a problem hiding this comment.
is rawKey.String() the same as decoding to a string, in terms of interpreting escape sequences, etc?
| { | ||
| JSON: `1.0`, | ||
| IntoType: reflect.TypeOf(json.Number("")), | ||
| Want: json.Number("1.0"), | ||
| }, | ||
| { | ||
| JSON: `1`, | ||
| IntoType: reflect.TypeOf(json.Number("")), | ||
| Want: json.Number("1"), | ||
| }, |
There was a problem hiding this comment.
curious if it's ok to drop these... were they added to try to catch a specific issue?
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: inteon The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
The The serialize benchmarks still look pretty rough. Need to see what we can improve there. |
|
did you run the full set of benchmarks to see how we looked across all of them? |
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
|
Thanks for the updates, how are the overall benchmarks looking (not just the subset in the description)? As you were adjusting the implementation, were there any unit tests it would make sense to add to catch edges the previous implementations handled we want to ensure the new one does as well? I'm thinking specifically of things like:
|
|
First off- it's amazing to see this happening and the benchmarks are VERY promising. Thanks @inteon! To get this to the finish line, and merge, what should our criteria be? I chatted offline with @liggitt briefly and some of the criteria we discussed was:
Intuitively, it seems like the deserialization is already sufficiently fast. I suspect we need to optimize serialization a bit further since we serialize managed fields on all updates (not just patches). That said, I'm willing to be data driven here. If we can show downstream scale and performance is acceptable, I'm willing to accept a higher serialization perf regression in order to migrate to json/v2. Thoughts, concerns? |
liggitt
moved this to Dependencies to replace/remove
in [sig-architecture] Dependency management
|
Update check the new numbers in my PR description, upgrading encoding/json/v2 did improve performance! |
|
Did some further tuning and got the # allocations lower than on master. |
liggitt
moved this from Dependencies to replace/remove
to Dependency review complete
in [sig-architecture] Dependency management
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
|
@inteon do we want to pick this up when k8s 1.36 cycle reopens |
Yes. I updated the code, fixed some bugs and updated the benchmark results. |
|
Looks like we can drop the "Performance is not yet great" comment from the description now... this looks ~neutral or an improvement for memory use at this point 🎉 |
We want to pick it up as soon as possible, but we need to wait until json/v2 lands in stdlib in a non-experimental way. I'm not sure if that's happening in Go 1.26 or not (the status of golang/go#76406 and json/v2 working group (view) makes me think not). api-machinery folks have a detailed code review of this PR scheduled for later this week. Once we're happy with the shape of it, and do a final round of benchmarks, we'll submit this as positive community feedback on the current shape of json/v2, and then wait patiently for json/v2 to land in stdlib 🤞 |
| } | ||
|
|
||
| // DeserializePathElement parses a serialized path element | ||
| func DeserializePathElement(s string) (PathElement, error) { |
There was a problem hiding this comment.
@jpbetz @lalitc375: let's augment the unit tests that exercise successful DeserializePathElement calls (TestPathElementRoundTrip?) to also assert the in-memory state returned, not just the round-trip
| fields.Sort() | ||
| return PathElement{Key: &fields}, iter.Error | ||
| var fields value.FieldList | ||
| if err := json.UnmarshalRead(strings.NewReader(s[2:]), &fields); err != nil { |
There was a problem hiding this comment.
@jpbetz @lalitc375: can we add test-cases to exercise
- zero-length
- EOF
- leading / trailing whitespace in value
- multi-token should fail identically
| v, err := readJSONIter(iter) | ||
| if err != nil { | ||
| var v any | ||
| if err := json.UnmarshalRead(strings.NewReader(s[2:]), &v); err != nil { |
There was a problem hiding this comment.
@jpbetz @lalitc375: can we add test-cases to ensure behave identically between readJSONIter / json.UnmarshalRead
- zero-length - json-iter masks, json/v2 returns EOF error? https://go.dev/play/p/D80jALZ7ZRq
- non-zero length with EOF (not sure this is possible)
- leading / trailing whitespace in value
- multi-token should fail identically
| return fmt.Errorf("parsing JSON: %v", err) | ||
| } | ||
|
|
||
| fields = append(fields, Field{Name: k, Value: NewValueInterface(v)}) |
There was a problem hiding this comment.
This looks it would accept duplicate key entries. Did the original ReadObjectCB prevent duplicates or accept them? Is there a test case to make sure these behave identically to json-iterator?
| } else if err != nil { | ||
| return fmt.Errorf("parsing JSON: %v", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
is it possible to encounter a token other than EndObject or String here? will ReadToken() error in that case?
@jpbetz @lalitc375: can we make sure there's a test that some other token type instead of a string key errors correctly here?
There was a problem hiding this comment.
I would expect ReadToken to error in this case but I want us to verify we can rely on that
We can try adding a test that exercises this in master and see if it exercises this code path in this PR. If it doesn't, we may need to also add a specific test in this PR.
| break | ||
| } | ||
|
|
||
| k := rawKey.String() |
There was a problem hiding this comment.
@jpbetz @lalitc375: can we ensure we have test cases that hit this point to compare behavior between json-iterator and json/v2 and we end up with the same in-memory values:
- escaped/encoded keys ("\u... \n ...")
- multi-byte keys ("Iñtërnâtiônàlizætiøn,💝🐹🌇⛔")
- keys containing bytes that are invalid unicode characters
| return children, isMember, nil | ||
| } | ||
|
|
||
| // FromJSON clears s and reads a JSON formatted set structure. |
There was a problem hiding this comment.
nit: move this back up to where FromJSON was previously to minimize the diff
| func readIterV1(iter *jsoniter.Iterator) (children *Set, isMember bool) { | ||
| iter.ReadMapCB(func(iter *jsoniter.Iterator, key string) bool { | ||
| if key == "." { | ||
| func (s *setContentsV1) readIterV1(parser *jsontext.Decoder) (children *Set, isMember bool, err error) { |
There was a problem hiding this comment.
the receiver isn't used, change this back to a function:
| func (s *setContentsV1) readIterV1(parser *jsontext.Decoder) (children *Set, isMember bool, err error) { | |
| func readIterV1(parser *jsontext.Decoder) (children *Set, isMember bool, err error) { |
|
|
||
| // Append the member to the members list, we will sort it later | ||
| m := &children.Members.members | ||
| // Since we expect that most of the time these will have been |
There was a problem hiding this comment.
let's leave the insert-in-sorted-order code as it was here and below if there's not a reason it has to be connected to this PR
@jpbetz, let's separately consider whether to switch this to a post-accumulation sort
| if children == nil { | ||
| children = &Set{} | ||
| } | ||
| // Since we expect that most of the time these will have been |
| }) | ||
| } | ||
|
|
||
| // Sort the members and children |
There was a problem hiding this comment.
revert the post-accumulation sort
|
@inteon, thanks so much for this PR, we spent several hours today reviewing the details deeply now that the performance has converged to something we could actually merge. There were four main categories of feedback:
Don't feel like you have to be the one to resolve all the comments, @jpbetz and @lalitc375 will be jumping in to help as well. We can gradually work on this and continue getting this ready for when json/v2 lands in stdlib (which looks like it will now be Go 1.27 at the earliest) |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
lifecycle/stale
|
PR needs rebase. DetailsInstructions 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-sigs/prow repository. |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
/remove-lifecycle rotten |
k8s-ci-robot
removed
the
lifecycle/rotten
9 participants
Replaces the
github.com/json-iterator/godependency withencoding/json/v2.closes #202