Skip to content

Fixed memory leak in bindless material #19041

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

Merged
merged 1 commit into from
May 30, 2025
Merged

Fixed memory leak in bindless material #19041

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 59 additions & 63 deletions crates/bevy_pbr/src/material_bind_groups.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1048,78 +1048,42 @@ where

for (bindless_index, owned_binding_resource) in binding_resources.drain(..) {
let bindless_index = BindlessIndex(bindless_index);
// If this is an other reference to an object we've already
// allocated, just bump its reference count.
if let Some(pre_existing_resource_slot) = allocation_candidate
.pre_existing_resources
.get(&bindless_index)
{
allocated_resource_slots.insert(bindless_index, *pre_existing_resource_slot);

match owned_binding_resource {
OwnedBindingResource::Buffer(_) => {
self.buffers
.get_mut(&bindless_index)
.expect("Buffer binding array should exist")
.bindings
.get_mut(*pre_existing_resource_slot as usize)
.and_then(|binding| binding.as_mut())
.expect("Slot should exist")
.ref_count += 1;
}

OwnedBindingResource::Data(_) => {
panic!("Data buffers can't be deduplicated")
}

OwnedBindingResource::TextureView(texture_view_dimension, _) => {
let bindless_resource_type =
BindlessResourceType::from(texture_view_dimension);
self.textures
.get_mut(&bindless_resource_type)
.expect("Texture binding array should exist")
.bindings
.get_mut(*pre_existing_resource_slot as usize)
.and_then(|binding| binding.as_mut())
.expect("Slot should exist")
.ref_count += 1;
}

OwnedBindingResource::Sampler(sampler_binding_type, _) => {
let bindless_resource_type =
BindlessResourceType::from(sampler_binding_type);
self.samplers
.get_mut(&bindless_resource_type)
.expect("Sampler binding array should exist")
.bindings
.get_mut(*pre_existing_resource_slot as usize)
.and_then(|binding| binding.as_mut())
.expect("Slot should exist")
.ref_count += 1;
}
}

continue;
}
let pre_existing_slot = allocation_candidate
.pre_existing_resources
.get(&bindless_index);

// Otherwise, we need to insert it anew.
let binding_resource_id = BindingResourceId::from(&owned_binding_resource);
match owned_binding_resource {
let increment_allocated_resource_count = match owned_binding_resource {
OwnedBindingResource::Buffer(buffer) => {
let slot = self
.buffers
.get_mut(&bindless_index)
.expect("Buffer binding array should exist")
.insert(binding_resource_id, buffer);
allocated_resource_slots.insert(bindless_index, slot);

if let Some(pre_existing_slot) = pre_existing_slot {
assert_eq!(*pre_existing_slot, slot);

false
} else {
true
}
}
OwnedBindingResource::Data(data) => {
if pre_existing_slot.is_some() {
panic!("Data buffers can't be deduplicated")
}

let slot = self
.data_buffers
.get_mut(&bindless_index)
.expect("Data buffer binding array should exist")
.insert(&data);
allocated_resource_slots.insert(bindless_index, slot);
false
}
OwnedBindingResource::TextureView(texture_view_dimension, texture_view) => {
let bindless_resource_type = BindlessResourceType::from(texture_view_dimension);
Expand All @@ -1129,6 +1093,14 @@ where
.expect("Texture array should exist")
.insert(binding_resource_id, texture_view);
allocated_resource_slots.insert(bindless_index, slot);

if let Some(pre_existing_slot) = pre_existing_slot {
assert_eq!(*pre_existing_slot, slot);
Copy link
Member

Choose a reason for hiding this comment

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

Should these be debug_assert_eq! ?

Copy link
Contributor Author

@SarthakSingh31 SarthakSingh31 May 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think this PR makes pre_existing_slot here kinda redundant. Its just a sanity check to make sure everything is working correctly.

I can make it a debug assertion if you think that would be better.

Instead of using the pre_existing_slot ideally we want to know if a allocation has actually been made (we should get this from the insert API). The pre_existing_slot is mostly a easy way to check that.


false
} else {
true
}
}
OwnedBindingResource::Sampler(sampler_binding_type, sampler) => {
let bindless_resource_type = BindlessResourceType::from(sampler_binding_type);
Expand All @@ -1138,11 +1110,21 @@ where
.expect("Sampler should exist")
.insert(binding_resource_id, sampler);
allocated_resource_slots.insert(bindless_index, slot);

if let Some(pre_existing_slot) = pre_existing_slot {
assert_eq!(*pre_existing_slot, slot);

false
} else {
true
}
}
}
};

// Bump the allocated resource count.
self.allocated_resource_count += 1;
if increment_allocated_resource_count {
self.allocated_resource_count += 1;
}
}

allocated_resource_slots
Expand Down Expand Up @@ -1626,16 +1608,30 @@ where
/// Inserts a bindless resource into a binding array and returns the index
/// of the slot it was inserted into.
fn insert(&mut self, binding_resource_id: BindingResourceId, resource: R) -> u32 {
let slot = self.free_slots.pop().unwrap_or(self.len);
self.resource_to_slot.insert(binding_resource_id, slot);
match self.resource_to_slot.entry(binding_resource_id) {
bevy_platform::collections::hash_map::Entry::Occupied(o) => {
let slot = *o.get();

if self.bindings.len() < slot as usize + 1 {
self.bindings.resize_with(slot as usize + 1, || None);
}
self.bindings[slot as usize] = Some(MaterialBindlessBinding::new(resource));
self.bindings[slot as usize]
.as_mut()
.expect("A slot in the resource_to_slot map should have a value")
.ref_count += 1;

self.len += 1;
slot
slot
}
bevy_platform::collections::hash_map::Entry::Vacant(v) => {
let slot = self.free_slots.pop().unwrap_or(self.len);
v.insert(slot);

if self.bindings.len() < slot as usize + 1 {
self.bindings.resize_with(slot as usize + 1, || None);
}
self.bindings[slot as usize] = Some(MaterialBindlessBinding::new(resource));

self.len += 1;
slot
}
}
}

/// Removes a reference to an object from the slot.
Expand Down