Skip to content

Commit f4bd56d

Browse files
SarthakSingh31mockersf
authored andcommitted
Fixed memory leak in bindless material (#19041)
# Objective Fixed #19035. Fixed #18882. It consisted of two different bugs: - The allocations where being incremented even when a Data binding was created. - The ref counting on the binding was broken. ## Solution - Stopped incrementing the allocations when a data binding was created. - Rewrote the ref counting code to more reliably track the ref count. ## Testing Tested my fix for 10 minutes with the `examples/3d/animated_material.rs` example. I changed the example to spawn 51x51 meshes instead of 3x3 meshes to heighten the effects of the bug. My branch: (After 10 minutes of running the modified example) GPU: 172 MB CPU: ~700 MB Main branch: (After 2 minutes of running the modified example, my computer started to stutter so I had to end it early) GPU: 376 MB CPU: ~1300 MB
1 parent 75d92f4 commit f4bd56d

File tree

1 file changed

+59
-63
lines changed

1 file changed

+59
-63
lines changed

crates/bevy_pbr/src/material_bind_groups.rs

Lines changed: 59 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -1048,78 +1048,42 @@ where
10481048

10491049
for (bindless_index, owned_binding_resource) in binding_resources.drain(..) {
10501050
let bindless_index = BindlessIndex(bindless_index);
1051-
// If this is an other reference to an object we've already
1052-
// allocated, just bump its reference count.
1053-
if let Some(pre_existing_resource_slot) = allocation_candidate
1054-
.pre_existing_resources
1055-
.get(&bindless_index)
1056-
{
1057-
allocated_resource_slots.insert(bindless_index, *pre_existing_resource_slot);
1058-
1059-
match owned_binding_resource {
1060-
OwnedBindingResource::Buffer(_) => {
1061-
self.buffers
1062-
.get_mut(&bindless_index)
1063-
.expect("Buffer binding array should exist")
1064-
.bindings
1065-
.get_mut(*pre_existing_resource_slot as usize)
1066-
.and_then(|binding| binding.as_mut())
1067-
.expect("Slot should exist")
1068-
.ref_count += 1;
1069-
}
1070-
1071-
OwnedBindingResource::Data(_) => {
1072-
panic!("Data buffers can't be deduplicated")
1073-
}
1074-
1075-
OwnedBindingResource::TextureView(texture_view_dimension, _) => {
1076-
let bindless_resource_type =
1077-
BindlessResourceType::from(texture_view_dimension);
1078-
self.textures
1079-
.get_mut(&bindless_resource_type)
1080-
.expect("Texture binding array should exist")
1081-
.bindings
1082-
.get_mut(*pre_existing_resource_slot as usize)
1083-
.and_then(|binding| binding.as_mut())
1084-
.expect("Slot should exist")
1085-
.ref_count += 1;
1086-
}
1087-
1088-
OwnedBindingResource::Sampler(sampler_binding_type, _) => {
1089-
let bindless_resource_type =
1090-
BindlessResourceType::from(sampler_binding_type);
1091-
self.samplers
1092-
.get_mut(&bindless_resource_type)
1093-
.expect("Sampler binding array should exist")
1094-
.bindings
1095-
.get_mut(*pre_existing_resource_slot as usize)
1096-
.and_then(|binding| binding.as_mut())
1097-
.expect("Slot should exist")
1098-
.ref_count += 1;
1099-
}
1100-
}
11011051

1102-
continue;
1103-
}
1052+
let pre_existing_slot = allocation_candidate
1053+
.pre_existing_resources
1054+
.get(&bindless_index);
11041055

11051056
// Otherwise, we need to insert it anew.
11061057
let binding_resource_id = BindingResourceId::from(&owned_binding_resource);
1107-
match owned_binding_resource {
1058+
let increment_allocated_resource_count = match owned_binding_resource {
11081059
OwnedBindingResource::Buffer(buffer) => {
11091060
let slot = self
11101061
.buffers
11111062
.get_mut(&bindless_index)
11121063
.expect("Buffer binding array should exist")
11131064
.insert(binding_resource_id, buffer);
11141065
allocated_resource_slots.insert(bindless_index, slot);
1066+
1067+
if let Some(pre_existing_slot) = pre_existing_slot {
1068+
assert_eq!(*pre_existing_slot, slot);
1069+
1070+
false
1071+
} else {
1072+
true
1073+
}
11151074
}
11161075
OwnedBindingResource::Data(data) => {
1076+
if pre_existing_slot.is_some() {
1077+
panic!("Data buffers can't be deduplicated")
1078+
}
1079+
11171080
let slot = self
11181081
.data_buffers
11191082
.get_mut(&bindless_index)
11201083
.expect("Data buffer binding array should exist")
11211084
.insert(&data);
11221085
allocated_resource_slots.insert(bindless_index, slot);
1086+
false
11231087
}
11241088
OwnedBindingResource::TextureView(texture_view_dimension, texture_view) => {
11251089
let bindless_resource_type = BindlessResourceType::from(texture_view_dimension);
@@ -1129,6 +1093,14 @@ where
11291093
.expect("Texture array should exist")
11301094
.insert(binding_resource_id, texture_view);
11311095
allocated_resource_slots.insert(bindless_index, slot);
1096+
1097+
if let Some(pre_existing_slot) = pre_existing_slot {
1098+
assert_eq!(*pre_existing_slot, slot);
1099+
1100+
false
1101+
} else {
1102+
true
1103+
}
11321104
}
11331105
OwnedBindingResource::Sampler(sampler_binding_type, sampler) => {
11341106
let bindless_resource_type = BindlessResourceType::from(sampler_binding_type);
@@ -1138,11 +1110,21 @@ where
11381110
.expect("Sampler should exist")
11391111
.insert(binding_resource_id, sampler);
11401112
allocated_resource_slots.insert(bindless_index, slot);
1113+
1114+
if let Some(pre_existing_slot) = pre_existing_slot {
1115+
assert_eq!(*pre_existing_slot, slot);
1116+
1117+
false
1118+
} else {
1119+
true
1120+
}
11411121
}
1142-
}
1122+
};
11431123

11441124
// Bump the allocated resource count.
1145-
self.allocated_resource_count += 1;
1125+
if increment_allocated_resource_count {
1126+
self.allocated_resource_count += 1;
1127+
}
11461128
}
11471129

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

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

1637-
self.len += 1;
1638-
slot
1620+
slot
1621+
}
1622+
bevy_platform::collections::hash_map::Entry::Vacant(v) => {
1623+
let slot = self.free_slots.pop().unwrap_or(self.len);
1624+
v.insert(slot);
1625+
1626+
if self.bindings.len() < slot as usize + 1 {
1627+
self.bindings.resize_with(slot as usize + 1, || None);
1628+
}
1629+
self.bindings[slot as usize] = Some(MaterialBindlessBinding::new(resource));
1630+
1631+
self.len += 1;
1632+
slot
1633+
}
1634+
}
16391635
}
16401636

16411637
/// Removes a reference to an object from the slot.

0 commit comments

Comments
 (0)