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.

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

Resource unnecessarily clones #233

Open
roy-work opened this issue Apr 23, 2020 · 2 comments · May be fixed by #311
Open

Resource unnecessarily clones #233

roy-work opened this issue Apr 23, 2020 · 2 comments · May be fixed by #311
Labels
api Api abstraction related

Comments

@roy-work
Copy link

roy-work commented Apr 23, 2020 •
edited

The Meta helper trait has the following signatures,

fn name(&self) -> String
fn namespace(&self) -> Option<String>
fn resource_ver(&self) -> Option<String>

All of these end up cloning the underlying thing they're returning; none of them need to; they could return &str references.

In my particular case, I'm iterating through every object of a particular resource type, this results in quite a few allocations that I end up never needing. (I'm just comparing the name, for example.)

This would be a breaking change, however, since kube-rs isn't at 1.0 yet, I thought I'd suggest it.
@clux
Copy link
Member

clux commented Apr 24, 2020

I did originally start out with references, but it made it hard to work with the big openapi struct because of borrows happening in many places, and a couple of extra copies didn't feel that worth optimizing for. Besides, you can always just reach into metadata directly if you are concerned about the cost of using Meta.

That said, you can try changing the trait and seeing how the examples expand. If it's not as bad as I originally felt it was, then we could possibly change it.

@clux clux added the api Api abstraction related label Apr 27, 2020
@clux clux mentioned this issue Jul 19, 2020
Closed
2 tasks
clux added a commit that referenced this issue Aug 18, 2020
@clux
Meta no longer clones - fixes #233
badea43
@clux clux linked a pull request Aug 18, 2020 that will close this issue
Meta no longer clones - fixes #233 #311
Draft
@clux clux linked a pull request Feb 8, 2021 that will close this issue
Decouple Meta from k8s-openapi #385
Merged
@clux clux removed a link to a pull request Feb 8, 2021
Decouple Meta from k8s-openapi #385
Merged
@clux clux changed the title Meta unnecessarily clones Resource unnecessarily clones Apr 2, 2021
@clux
Copy link
Member

clux commented Apr 2, 2021

The experiments for this in #311 did leave a lot to be desired in terms of usability (which is primarily why it still clones like it does today).

Resurrecting this discussion now as the trait is being proposed to be split into a more foundational one + a user-facing one in #486.

This type of split potentially opens up the door to having an alternative trait that does not clone as heavily (one that users can even define themselves, as it's not tied to how kube_runtime or kube uses it). Maybe if there's demand for such a trait, we could even have it in here as an alternate ResourceExt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
api Api abstraction related
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Meta no longer clones - fixes #233 kube-rs/kube
2 participants
@clux @roy-work