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

Sync with go1.23 #19

Merged
merged 4 commits into from
Oct 8, 2024
Merged

Sync with go1.23 #19

merged 4 commits into from
Oct 8, 2024

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Oct 8, 2024

First commit is a straight copy of the json/encoding files from stdlib on the go 1.23 release branch

Second commit is a reapply of carry patches to those files

Third commit is a tweak to make the external dependency check work with go 1.23 go mod graph behavior

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 8, 2024
@liggitt liggitt requested review from jpbetz and removed request for lavalamp October 8, 2024 20:04
@jpbetz
Copy link

jpbetz commented Oct 8, 2024

I compared the carry patch commit against the "patch:..." commits in the repo history and it all looks right to me. The other changes look good too.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit c46165d into kubernetes-sigs:main Oct 8, 2024
4 checks passed
@liggitt
Copy link
Contributor Author

liggitt commented Oct 8, 2024

oof... sorry... this was still WIP ... it appears the stdlib json byte use may have regressed between 1.19 and 1.23 and I was chasing that down... will follow that up and resolve or revert before bumping in k/k

@liggitt
Copy link
Contributor Author

liggitt commented Oct 8, 2024

benchcmp 1.20.14.txt 1.21.13.txt 
benchmark                                old ns/op     new ns/op     delta
BenchmarkUnmarshal/typed_stdlib-10       33821         32411         -4.17%
BenchmarkUnmarshal/typed_stdlib-10       33654         32313         -3.98%
BenchmarkUnmarshal/typed_stdlib-10       33417         32258         -3.47%
BenchmarkUnmarshal/typed_stdlib-10       33361         32598         -2.29%
BenchmarkUnmarshal/typed_stdlib-10       33344         32292         -3.15%
BenchmarkUnmarshal/typed_stdlib-10       33385         32259         -3.37%
BenchmarkUnmarshal/typed_stdlib-10       33198         32223         -2.94%
BenchmarkUnmarshal/typed_stdlib-10       33390         32296         -3.28%
BenchmarkUnmarshal/typed_stdlib-10       34015         32165         -5.44%
BenchmarkUnmarshal/typed_stdlib-10       34059         33392         -1.96%
BenchmarkUnmarshal/untyped_stdlib-10     39088         36741         -6.00%
BenchmarkUnmarshal/untyped_stdlib-10     38622         35609         -7.80%
BenchmarkUnmarshal/untyped_stdlib-10     38615         37185         -3.70%
BenchmarkUnmarshal/untyped_stdlib-10     41221         35654         -13.51%
BenchmarkUnmarshal/untyped_stdlib-10     38330         35452         -7.51%
BenchmarkUnmarshal/untyped_stdlib-10     38662         35238         -8.86%
BenchmarkUnmarshal/untyped_stdlib-10     38069         35582         -6.53%
BenchmarkUnmarshal/untyped_stdlib-10     38133         36464         -4.38%
BenchmarkUnmarshal/untyped_stdlib-10     37744         35498         -5.95%
BenchmarkUnmarshal/untyped_stdlib-10     38408         35433         -7.75%

benchmark                                old allocs     new allocs     delta
BenchmarkUnmarshal/typed_stdlib-10       161            162            +0.62%
BenchmarkUnmarshal/typed_stdlib-10       161            162            +0.62%
BenchmarkUnmarshal/typed_stdlib-10       161            162            +0.62%
BenchmarkUnmarshal/typed_stdlib-10       161            162            +0.62%
BenchmarkUnmarshal/typed_stdlib-10       161            162            +0.62%
BenchmarkUnmarshal/typed_stdlib-10       161            162            +0.62%
BenchmarkUnmarshal/typed_stdlib-10       161            162            +0.62%
BenchmarkUnmarshal/typed_stdlib-10       161            162            +0.62%
BenchmarkUnmarshal/typed_stdlib-10       161            162            +0.62%
BenchmarkUnmarshal/typed_stdlib-10       161            162            +0.62%
BenchmarkUnmarshal/untyped_stdlib-10     517            515            -0.39%
BenchmarkUnmarshal/untyped_stdlib-10     517            515            -0.39%
BenchmarkUnmarshal/untyped_stdlib-10     517            515            -0.39%
BenchmarkUnmarshal/untyped_stdlib-10     516            515            -0.19%
BenchmarkUnmarshal/untyped_stdlib-10     517            515            -0.39%
BenchmarkUnmarshal/untyped_stdlib-10     517            515            -0.39%
BenchmarkUnmarshal/untyped_stdlib-10     517            515            -0.39%
BenchmarkUnmarshal/untyped_stdlib-10     517            515            -0.39%
BenchmarkUnmarshal/untyped_stdlib-10     517            515            -0.39%
BenchmarkUnmarshal/untyped_stdlib-10     517            515            -0.39%

benchmark                                old bytes     new bytes     delta
BenchmarkUnmarshal/typed_stdlib-10       13240         20384         +53.96%
BenchmarkUnmarshal/typed_stdlib-10       13240         20384         +53.96%
BenchmarkUnmarshal/typed_stdlib-10       13240         20384         +53.96%
BenchmarkUnmarshal/typed_stdlib-10       13240         20384         +53.96%
BenchmarkUnmarshal/typed_stdlib-10       13240         20384         +53.96%
BenchmarkUnmarshal/typed_stdlib-10       13240         20384         +53.96%
BenchmarkUnmarshal/typed_stdlib-10       13240         20384         +53.96%
BenchmarkUnmarshal/typed_stdlib-10       13240         20384         +53.96%
BenchmarkUnmarshal/typed_stdlib-10       13240         20384         +53.96%
BenchmarkUnmarshal/typed_stdlib-10       13240         20384         +53.96%
BenchmarkUnmarshal/untyped_stdlib-10     28163         27733         -1.53%
BenchmarkUnmarshal/untyped_stdlib-10     28165         27737         -1.52%
BenchmarkUnmarshal/untyped_stdlib-10     28167         27733         -1.54%
BenchmarkUnmarshal/untyped_stdlib-10     28159         27734         -1.51%
BenchmarkUnmarshal/untyped_stdlib-10     28165         27734         -1.53%
BenchmarkUnmarshal/untyped_stdlib-10     28165         27736         -1.52%
BenchmarkUnmarshal/untyped_stdlib-10     28161         27735         -1.51%
BenchmarkUnmarshal/untyped_stdlib-10     28167         27734         -1.54%
BenchmarkUnmarshal/untyped_stdlib-10     28160         27739         -1.50%
BenchmarkUnmarshal/untyped_stdlib-10     28161         27736         -1.51%

@liggitt
Copy link
Contributor Author

liggitt commented Oct 8, 2024

yeah, 50% regression in byte use in typed unmarshals from stdlib 1.20 → 1.21

@jpbetz
Copy link

jpbetz commented Oct 8, 2024

oof... sorry... this was still WIP

My bad. Happy to review follow up when it is ready.

@liggitt
Copy link
Contributor Author

liggitt commented Oct 8, 2024

full change from 1.20 to 1.21:

git log --format=oneline upstream/release-branch.go1.20..upstream/release-branch.go1.21 -- src/encoding/json/

liggitt added a commit to liggitt/json that referenced this pull request Oct 9, 2024
This reverts commit c46165d, reversing
changes made to bc3834c.
@liggitt
Copy link
Contributor Author

liggitt commented Oct 9, 2024

opened a revert of this at #20 while I dig into the regression

k8s-ci-robot added a commit that referenced this pull request Oct 9, 2024
Revert "Merge pull request #19 from liggitt/go123"
@liggitt
Copy link
Contributor Author

liggitt commented Oct 9, 2024

bisected to golang/go@ac27b4d encoding/json: rely on reflect.Value.Grow

@liggitt
Copy link
Contributor Author

liggitt commented Oct 9, 2024

captured test results at https://gist.github.com/liggitt/bf28159afdaabf52b78ac43d86bc6362, will open upstream go issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants