Optimized BindGroupLayoutCache to reduce unnecessary cloning#21757
Optimized BindGroupLayoutCache to reduce unnecessary cloning#21757brianreavis wants to merge 5 commits into
BindGroupLayoutCache to reduce unnecessary cloning#21757Conversation
brianreavis
added
D-Trivial
| bgls: HashMap<BindGroupLayoutDescriptor, Pin<Box<BindGroupLayout>>>, | ||
| } | ||
|
|
||
| impl BindGroupLayoutCache { |
There was a problem hiding this comment.
Note for reviewers, this file is the key part of the PR, the rest is mechanical
alice-i-cecile
added
D-Straightforward
tychedelia
left a comment
tychedelia
left a comment
There was a problem hiding this comment.
I'd kinda like to make sure the juice is worth the unsafe squeeze here. There are some situations where we do create bind groups eagerly every frame which I think would be the main source of performance slowdown. But the changes make mechanical sense on first pass. Will try to look at some benchmarks just to get a sense of where we were left post #21205
| let boxed_layout_ptr = Pin::as_ref(boxed_layout).get_ref() as *const BindGroupLayout; | ||
|
|
||
| // SAFETY: | ||
| // - Cached `BindGroupLayout` entries are immutable: they're never replaced, modified, or evicted from the hashmap. |
There was a problem hiding this comment.
People have complained to us about so-called "memory leaks" but I think it's pretty hard to generate unique permutations of bgls unless you're doing something truly really weird. In any case, this requirement seems quite conservative so we could cross the bridge if it mattered in practice, which it's unlikely to.
|
Just ran a quick profile on a local project that does more bind group creation per frame than it should (yellow = this pull request, red = main): 
|
|
Adding this to 0.18, given the |
|
I don't have the time to review this atm, but I'm fine with the general idea. Users can always clone the BGL from the reference if they want. |
brianreavis
force-pushed
the
pr-bindgrouplayoutcache-optimization
branch
from
b97070f to
c339c31
Compare
| .lock() | ||
| .unwrap() | ||
| .get(&self.device, bind_group_layout_descriptor.clone()) | ||
| ) -> &BindGroupLayout { |
There was a problem hiding this comment.
As the lifetime of the returned &BindGroupLayout is being created out of thin air here, it is important to be explicit with the lifetime relationships here.
There was a problem hiding this comment.
What do you have in mind? Something like this?
- pub fn get_bind_group_layout(
+ pub fn get_bind_group_layout<'a>(
- &self,
+ &'a self,
bind_group_layout_descriptor: &BindGroupLayoutDescriptor,
- ) -> &BindGroupLayout {
+ ) -> &'a BindGroupLayout {There was a problem hiding this comment.
That looks correct to me. The important thing is that the resulting BGL does not outlive the immutable borrow on self, or it risks keeping a borrow past the Drop of the owning cache.
…tion (bevyengine#24503) # Objective Reduce `LayoutCache::get` and `BindGroupLayoutCache::get` heap allocation Part of this adopts bevyengine#21757 ## Solution Reduce `LayoutCache::get` allaction by using `SmallVec` with 8 elements inlined on the stack. Avoid cloning `BindGroupLayoutDescriptor` in `BindGroupLayoutCache::get` (adopts bevyengine#21757). bevyengine#21757 also avoids cloning `BindGroupLayout`, but I don't think it's unnecessary because `BindGroupLayout` is cheap to clone (it just clones id + Arc). ## Testing CI
beicause
added
S-Nominated-To-Close
8 participants
Objective
Currently,
BindGroupLayoutCacheends up cloning theBindGroupLayoutDescriptorandBindGroupLayoutfor every get. This PR removes unnecessary cloning.Solution
BindGroupLayoutCachevalues to a stable memory location and havepipeline_cache.get_bind_group_layout(...)return a reference to theBindGroupLayoutinstead of a clone.BindGroupLayoutCache::get(), operate on the hashmap usingraw_entry_mutinstead ofentryso that we don't need to unnecessarily need to clone theBindGroupLayoutDescriptorwhen theBindGroupLayoutis already in the cache.Testing
unsafeis required in order to get a reference that outlives the mutex guard.