-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Remove estimateMinSizeJSON calls for CEL #111156
Remove estimateMinSizeJSON calls for CEL #111156
Conversation
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. |
Would you mind try with existing benchmark test(or write your own if needed) to show the performance different? |
/assign @jpbetz |
Per offline discussion, we should close this in favor of #110135 but still need a PR to optimize the minSize calculations performed by kubernetes/staging/src/k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model/schemas.go Line 246 in 5b92e46
|
/triage accepted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see the code for this change fell out roughly as I had hoped. It's a nice simplification and optimization combined into one change.
staging/src/k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model/types.go
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model/schemas.go
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model/schemas_test.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model/schemas.go
Show resolved
Hide resolved
/lgtm |
@jpbetz: GitHub didn't allow me to assign the following users: for, approval. Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this cleanup... putting the size computation in the recursive construction we were already doing makes a ton of sense.
staging/src/k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model/schemas.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model/types.go
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model/schemas.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model/schemas.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model/types.go
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model/types.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model/types.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model/types.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model/schemas.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model/schemas.go
Outdated
Show resolved
Hide resolved
c28c304
to
e39887f
Compare
/test pull-kubernetes-e2e-gce-ubuntu-containerd |
1 similar comment
/test pull-kubernetes-e2e-gce-ubuntu-containerd |
e39887f
to
6c06286
Compare
6c06286
to
ae2319b
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DangerOnTheRanger, 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 |
/retest |
What type of PR is this?
/kind feature
What this PR does / why we need it:
estimateMinSizeJSON
is a function that is used to estimate the minimum valid size for a given schema node when serialized into JSON (this is used in turn to calculate how many values things like maps and arrays can have). The function works by walking through everything below the schema node it was given, though sinceSchemaDeclType
(which is what callsestimateMinSizeJSON
) is already walking the entire schema and callsestimateMinSizeJSON
for all arrays/maps, this results in a lot of redundantestimateMinSizeJSON
calls. This PR removes the need entirely forestimateMinSizeJSON
by adding aMinSerializedSize
attribute toDeclTypes
instead.The below two tables show the performance gained from this change; the tables were generated with the benchmark included in this branch (the
estimateMinSizeJSON
table used the benchmark patched onto master), with the depth parameter passed togenNestedSchema
adjusted as per the values in then
column. All runs were performed on a 16-core Intel Xeon clocked at 2.3Ghz with 66GB of RAM.with estimateMinSizeJSON
with MinSerializedSize
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: