Skip to content

Reduce allocations in ProtoClient codec #3139

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

Merged

Conversation

codefromthecrypt
Copy link
Contributor

I was trying to understand the proto client and noticed some things that could be improved, if you are pretty familiar with how okio works. I ran the ProtoExample afterwards and it has the same behaviors.

Note: The upstream ApiClient isn't managing resources very well, particularly ensuring responses close. So, even though I adjusted in proto, there are other paths which can leak responses, especially on exception.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 1, 2024
Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Contributor Author

@brendanburns I don't want to swamp you with review work, but if you are keen to have a "benchmarks" module which uses JMH, one day I'll add it. That way, you can see before/after and we don't need to guess if allocations are better or worse than before. It works similar (enough) to go's bench, even if not as convenient as isn't built-in.

@brendandburns
Copy link
Contributor

/lgtm
/approve

The proto client hasn't been worked on in a long time and the protocol buffer objects haven't been regenerated for an even longer time. So if you want to take on improvements in this area, contributions are very welcome, but there's definitely going to be rough edges in the current experience.

Here's where the protos are generated:
https://github.com/kubernetes-client/gen/tree/master/proto

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, codefromthecrypt

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 1, 2024
@k8s-ci-robot k8s-ci-robot merged commit 59889c2 into kubernetes-client:master Mar 1, 2024
@codefromthecrypt codefromthecrypt deleted the proto-allocations branch March 1, 2024 23:01
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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants