Skip to content

Conversation

@cwfitzgerald
Copy link
Member

@cwfitzgerald cwfitzgerald commented Jan 21, 2025

Related #7856

This removes unnecessary clones of resources which are already owned by the tracker.

Squash.

Edit: ayy funny number

@cwfitzgerald cwfitzgerald force-pushed the cw/clone-face branch 3 times, most recently from d1fff08 to 6e0ed0a Compare March 17, 2025 02:10
@cwfitzgerald cwfitzgerald changed the title Remove Clone Staring Me In the Face Remove unnecessary clone when redundantly adding resource to tracker Aug 23, 2025
@cwfitzgerald cwfitzgerald marked this pull request as ready for review August 23, 2025 07:14
@cwfitzgerald cwfitzgerald requested a review from a team as a code owner August 23, 2025 07:14
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

nice! Would be great though to see benchmarks justifying this. Yes, this looks faster and should help addressing one of our key hotspots, but I think we should hold ourselves to a high standard and justify this kind of added complexity/indirection with data.
("Request changes" bc of that.)

///
/// Because [`ResourceMetadata`] is generic over [`Arc`] and [`Weak`],
/// we need a trait to give us the implementations for each.
pub trait Insertable: Sized {
Copy link
Member

Choose a reason for hiding this comment

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

name isn't great, suggests a generality that isn't intended here. Maybe InsertableResource would be better already?

@cwfitzgerald
Copy link
Member Author

I had some profiles a while ago that pointed directly there, but they were in rend3 and vtune on an old machine so don't have them anymore. I'm not entirely sure what whole-stack tests are out there, but I'll poke around.

@Wumpf
Copy link
Member

Wumpf commented Aug 24, 2025

I'd think some of our existing benchmarks in the repo should hit this quite a bit as well

@cwfitzgerald
Copy link
Member Author

cwfitzgerald commented Aug 25, 2025

@Wumpf I have bad news.... The thing this PR is solving for doesn't happen.... At all. Yes there is a "redundant" clone when inserting a new resource, but that only happens (in hot code) inside a

let currently_owned = unsafe { self.metadata.contains_unchecked(index) };

if !currently_owned {
    clone happens in here
}

so...

uhh

Closing this...

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants