Skip to content

Migrate to encoding/json/v2 #292

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Migrate to encoding/json/v2 #292

wants to merge 4 commits into from
+253 −294

Conversation

inteon
Copy link
Member

@inteon inteon commented Jun 16, 2025 •
edited
Loading

Replaces the github.com/json-iterator/go dependency with encoding/json/v2.
Performance is not yet great (feel free to push improvements/ create new PRs based on this PR):

# based on 5b8c5551cd83d63c3dad23770785367bf805d91f
$ go test -benchmem -run=^$ -bench ^BenchmarkFieldSet/serialize.*$ sigs.k8s.io/structured-merge-diff/v6/fieldpath -count=6 > pr.txt
$ go test -benchmem -run=^$ -bench ^BenchmarkFieldSet/serialize.*$ sigs.k8s.io/structured-merge-diff/v6/fieldpath -count=6 > master.txt
$ benchstat master.txt pr.txt

goos: linux
goarch: amd64
pkg: sigs.k8s.io/structured-merge-diff/v6/fieldpath
cpu: Intel(R) Core(TM) Ultra 7 165H
                            │  master.txt  │                pr.txt                 │
                            │    sec/op    │    sec/op      vs base                │
FieldSet/serialize-20-8       7.487µ ±  3%   16.349µ ± 14%  +118.37% (p=0.002 n=6)
FieldSet/deserialize-20-8     27.96µ ±  9%    32.00µ ±  3%   +14.43% (p=0.002 n=6)
FieldSet/serialize-50-8       20.73µ ±  8%    34.93µ ± 26%   +68.55% (p=0.002 n=6)
FieldSet/deserialize-50-8     52.77µ ±  6%    83.94µ ±  3%   +59.08% (p=0.002 n=6)
FieldSet/serialize-100-8      56.34µ ± 25%   100.86µ ± 13%   +79.02% (p=0.002 n=6)
FieldSet/deserialize-100-8    110.6µ ± 34%    184.8µ ± 17%   +67.05% (p=0.002 n=6)
FieldSet/serialize-500-8      303.2µ ± 18%    565.6µ ± 16%   +86.55% (p=0.002 n=6)
FieldSet/deserialize-500-8    533.5µ ±  5%    900.9µ ±  4%   +68.85% (p=0.002 n=6)
FieldSet/serialize-1000-8     724.3µ ±  6%   1324.3µ ± 17%   +82.83% (p=0.002 n=6)
FieldSet/deserialize-1000-8   1.243m ± 11%    2.057m ±  7%   +65.51% (p=0.002 n=6)
geomean                       107.6µ          181.9µ         +69.08%

                            │  master.txt  │                pr.txt                │
                            │     B/op     │     B/op       vs base               │
FieldSet/serialize-20-8       1.584Ki ± 0%    2.885Ki ± 0%  +82.15% (p=0.002 n=6)
FieldSet/deserialize-20-8     9.788Ki ± 0%    8.244Ki ± 0%  -15.77% (p=0.002 n=6)
FieldSet/serialize-50-8       4.488Ki ± 0%    7.879Ki ± 0%  +75.56% (p=0.002 n=6)
FieldSet/deserialize-50-8     19.86Ki ± 0%    23.30Ki ± 0%  +17.33% (p=0.002 n=6)
FieldSet/serialize-100-8      15.60Ki ± 0%    26.23Ki ± 0%  +68.09% (p=0.002 n=6)
FieldSet/deserialize-100-8    59.01Ki ± 0%    80.03Ki ± 0%  +35.62% (p=0.002 n=6)
FieldSet/serialize-500-8      73.05Ki ± 0%   131.06Ki ± 0%  +79.41% (p=0.002 n=6)
FieldSet/deserialize-500-8    270.2Ki ± 0%    394.5Ki ± 0%  +46.03% (p=0.002 n=6)
FieldSet/serialize-1000-8     154.5Ki ± 1%    284.4Ki ± 0%  +84.15% (p=0.002 n=6)
FieldSet/deserialize-1000-8   592.9Ki ± 0%    869.4Ki ± 0%  +46.64% (p=0.002 n=6)
geomean                       34.37Ki         50.92Ki       +48.16%

                            │ master.txt  │               pr.txt                │
                            │  allocs/op  │  allocs/op   vs base                │
FieldSet/serialize-20-8        8.000 ± 0%   37.000 ± 0%  +362.50% (p=0.002 n=6)
FieldSet/deserialize-20-8      233.0 ± 0%    254.0 ± 0%    +9.01% (p=0.002 n=6)
FieldSet/serialize-50-8        14.00 ± 0%   102.00 ± 0%  +628.57% (p=0.002 n=6)
FieldSet/deserialize-50-8      656.0 ± 0%    739.0 ± 0%   +12.65% (p=0.002 n=6)
FieldSet/serialize-100-8       39.00 ± 0%   339.00 ± 0%  +769.23% (p=0.002 n=6)
FieldSet/deserialize-100-8    2.299k ± 0%   2.583k ± 0%   +12.35% (p=0.002 n=6)
FieldSet/serialize-500-8       185.0 ± 0%   1695.0 ± 0%  +816.22% (p=0.002 n=6)
FieldSet/deserialize-500-8    11.47k ± 0%   13.00k ± 0%   +13.34% (p=0.002 n=6)
FieldSet/serialize-1000-8      394.0 ± 0%   3651.5 ± 0%  +826.78% (p=0.002 n=6)
FieldSet/deserialize-1000-8   24.98k ± 0%   28.36k ± 0%   +13.52% (p=0.002 n=6)
geomean                        355.3        1.035k       +191.40%

closes #202

inteon added 2 commits June 14, 2025 09:17
@inteon
use UnmarshalJSONFrom function
ac6cc73 
Signed-off-by: Tim Ramlot <[email protected]>
@inteon
peformance tuning
5b8c555 
Signed-off-by: Tim Ramlot <[email protected]>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 16, 2025
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 16, 2025
@dims
Copy link
Member

dims commented Jun 17, 2025

xref: kubernetes/kubernetes#132312

@dims
Copy link
Member

dims commented Jun 18, 2025

For the pull-structured-merge-diff-test failure, please add this to fix the error @inteon

diff --git a/internal/cli/main_test.go b/internal/cli/main_test.go
index 3e409ede0673..8beed5c6cf55 100644
--- a/internal/cli/main_test.go
+++ b/internal/cli/main_test.go
@@ -21,6 +21,7 @@ import (
        "encoding/json"
        "io/ioutil"
        "path/filepath"
+       "strings"
        "testing"
 )

@@ -135,7 +136,7 @@ func (tt *testCase) checkOutput(t *testing.T, got []byte) {
                t.Fatalf("couldn't read expected output %q: %v", tt.expectedOutputPath, err)
        }

-       if a, e := string(got), string(want); a != e {
+       if a, e := strings.TrimSpace(string(got)), strings.TrimSpace(string(want)); a != e {
                t.Errorf("output didn't match expected output: got:\n%v\nwanted:\n%v\n", a, e)
        }
 }

@inteon inteon force-pushed the use_json_v2 branch 2 times, most recently from a4b6871 to bdce391 Compare June 18, 2025 10:14
@inteon
Copy link
Member Author

inteon commented Jun 18, 2025 •
edited
Loading

For the pull-structured-merge-diff-test failure, please add this to fix the error @inteon
...

I fixed the test failure.

@dims
Copy link
Member

dims commented Jun 18, 2025

/assign @BenTheElder @liggitt

@liggitt
Copy link
Contributor

liggitt commented Jun 19, 2025

/assign @jpbetz
who is the primary apimachinery approver on this bit and was deeply involved in the initial performance-driven use of json-iterator in these bits

@liggitt
Copy link
Contributor

liggitt commented Jun 19, 2025

For the pull-structured-merge-diff-test failure, please add this to fix the error @inteon

I suspect using a json marshal function (like MarshalWrite) that doesn't append a newline would be a more efficient way to accomplish that

liggitt
liggitt reviewed Jun 19, 2025
View reviewed changes
fieldpath/serialize-pe.go Outdated
return nil, fmt.Errorf("parsing JSON: %v", err)
}

k := rawKey.String()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is rawKey.String() the same as decoding to a string, in terms of interpreting escape sequences, etc?

value/reflectcache_test.go
Comment on lines -330 to -339
{
JSON: `1.0`,
IntoType: reflect.TypeOf(json.Number("")),
Want: json.Number("1.0"),
},
{
JSON: `1`,
IntoType: reflect.TypeOf(json.Number("")),
Want: json.Number("1"),
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious if it's ok to drop these... were they added to try to catch a specific issue?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: inteon
Once this PR has been reviewed and has the lgtm label, please ask for approval from jpbetz. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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

@liggitt
Copy link
Contributor

liggitt commented Jun 23, 2025

The .../deserialize... benchmarks actually don't look terrible now... I'd be willing to accept that performance drop in pursuit of correctness / safety.

The serialize benchmarks still look pretty rough. Need to see what we can improve there.

@liggitt
Copy link
Contributor

liggitt commented Jun 24, 2025

did you run the full set of benchmarks to see how we looked across all of them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@apelisse apelisse Awaiting requested review from apelisse

@Jefftree Jefftree Awaiting requested review from Jefftree

Assignees

@jpbetz jpbetz

@BenTheElder BenTheElder

@liggitt liggitt

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

remove use of json-iterator / reflect2
6 participants
@inteon @dims @liggitt @k8s-ci-robot @jpbetz @BenTheElder