Skip to content

Commit 764a531

Browse files
joseph-gioalradish
authored andcommitted
Remove duplicate lookups from Resource initialization (bevyengine#7174)
# Objective * `World::init_resource` and `World::get_resource_or_insert_with` are implemented naively, and as such they perform duplicate `TypeId -> ComponentId` lookups. * `World::get_resource_or_insert_with` contains an additional duplicate `ComponentId -> ResourceData` lookup. * This function also contains an unnecessary panic branch, which we rely on the optimizer to be able to remove. ## Solution Implement the functions using engine-internal code, instead of combining high-level functions. This allows computed variables to persist across different branches, instead of being recomputed.
1 parent e540eff commit 764a531

File tree

2 files changed

+66
-13
lines changed

2 files changed

+66
-13
lines changed

crates/bevy_ecs/src/storage/resource.rs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
use crate::archetype::ArchetypeComponentId;
2+
use crate::change_detection::{MutUntyped, TicksMut};
23
use crate::component::{ComponentId, ComponentTicks, Components, TickCells};
34
use crate::storage::{Column, SparseSet, TableRow};
45
use bevy_ptr::{OwningPtr, Ptr, UnsafeCellDeref};
@@ -99,6 +100,20 @@ impl<const SEND: bool> ResourceData<SEND> {
99100
})
100101
}
101102

103+
pub(crate) fn get_mut(
104+
&mut self,
105+
last_change_tick: u32,
106+
change_tick: u32,
107+
) -> Option<MutUntyped<'_>> {
108+
let (ptr, ticks) = self.get_with_ticks()?;
109+
Some(MutUntyped {
110+
// SAFETY: We have exclusive access to the underlying storage.
111+
value: unsafe { ptr.assert_unique() },
112+
// SAFETY: We have exclusive access to the underlying storage.
113+
ticks: unsafe { TicksMut::from_tick_cells(ticks, last_change_tick, change_tick) },
114+
})
115+
}
116+
102117
/// Inserts a value into the resource. If a value is already present
103118
/// it will be replaced.
104119
///

crates/bevy_ecs/src/world/mod.rs

Lines changed: 51 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ use crate::{
1717
entity::{AllocAtWithoutReplacement, Entities, Entity, EntityLocation},
1818
event::{Event, Events},
1919
ptr::UnsafeCellDeref,
20-
query::{QueryState, ReadOnlyWorldQuery, WorldQuery},
20+
query::{DebugCheckedUnwrap, QueryState, ReadOnlyWorldQuery, WorldQuery},
2121
storage::{ResourceData, SparseSet, Storages},
2222
system::Resource,
2323
};
@@ -767,9 +767,20 @@ impl World {
767767
/// and those default values will be here instead.
768768
#[inline]
769769
pub fn init_resource<R: Resource + FromWorld>(&mut self) {
770-
if !self.contains_resource::<R>() {
771-
let resource = R::from_world(self);
772-
self.insert_resource(resource);
770+
let component_id = self.components.init_resource::<R>();
771+
if self
772+
.storages
773+
.resources
774+
.get(component_id)
775+
.map_or(true, |data| !data.is_present())
776+
{
777+
let value = R::from_world(self);
778+
OwningPtr::make(value, |ptr| {
779+
// SAFETY: component_id was just initialized and corresponds to resource of type R.
780+
unsafe {
781+
self.insert_resource_by_id(component_id, ptr);
782+
}
783+
});
773784
}
774785
}
775786

@@ -782,7 +793,7 @@ impl World {
782793
pub fn insert_resource<R: Resource>(&mut self, value: R) {
783794
let component_id = self.components.init_resource::<R>();
784795
OwningPtr::make(value, |ptr| {
785-
// SAFETY: component_id was just initialized and corresponds to resource of type R
796+
// SAFETY: component_id was just initialized and corresponds to resource of type R.
786797
unsafe {
787798
self.insert_resource_by_id(component_id, ptr);
788799
}
@@ -802,9 +813,20 @@ impl World {
802813
/// Panics if called from a thread other than the main thread.
803814
#[inline]
804815
pub fn init_non_send_resource<R: 'static + FromWorld>(&mut self) {
805-
if !self.contains_non_send::<R>() {
806-
let resource = R::from_world(self);
807-
self.insert_non_send_resource(resource);
816+
let component_id = self.components.init_non_send::<R>();
817+
if self
818+
.storages
819+
.resources
820+
.get(component_id)
821+
.map_or(true, |data| !data.is_present())
822+
{
823+
let value = R::from_world(self);
824+
OwningPtr::make(value, |ptr| {
825+
// SAFETY: component_id was just initialized and corresponds to resource of type R.
826+
unsafe {
827+
self.insert_non_send_by_id(component_id, ptr);
828+
}
829+
});
808830
}
809831
}
810832

@@ -821,7 +843,7 @@ impl World {
821843
pub fn insert_non_send_resource<R: 'static>(&mut self, value: R) {
822844
let component_id = self.components.init_non_send::<R>();
823845
OwningPtr::make(value, |ptr| {
824-
// SAFETY: component_id was just initialized and corresponds to resource of type R
846+
// SAFETY: component_id was just initialized and corresponds to resource of type R.
825847
unsafe {
826848
self.insert_non_send_by_id(component_id, ptr);
827849
}
@@ -959,18 +981,34 @@ impl World {
959981
unsafe { self.get_resource_unchecked_mut() }
960982
}
961983

962-
// PERF: optimize this to avoid redundant lookups
963984
/// Gets a mutable reference to the resource of type `T` if it exists,
964985
/// otherwise inserts the resource using the result of calling `func`.
965986
#[inline]
966987
pub fn get_resource_or_insert_with<R: Resource>(
967988
&mut self,
968989
func: impl FnOnce() -> R,
969990
) -> Mut<'_, R> {
970-
if !self.contains_resource::<R>() {
971-
self.insert_resource(func());
991+
let change_tick = self.change_tick();
992+
let last_change_tick = self.last_change_tick();
993+
994+
let component_id = self.components.init_resource::<R>();
995+
let data = self.initialize_resource_internal(component_id);
996+
if !data.is_present() {
997+
OwningPtr::make(func(), |ptr| {
998+
// SAFETY: component_id was just initialized and corresponds to resource of type R.
999+
unsafe {
1000+
data.insert(ptr, change_tick);
1001+
}
1002+
});
9721003
}
973-
self.resource_mut()
1004+
1005+
// SAFETY: The resource must be present, as we would have inserted it if it was empty.
1006+
let data = unsafe {
1007+
data.get_mut(last_change_tick, change_tick)
1008+
.debug_checked_unwrap()
1009+
};
1010+
// SAFETY: The underlying type of the resource is `R`.
1011+
unsafe { data.with_type::<R>() }
9741012
}
9751013

9761014
/// Gets a mutable reference to the resource of the given type, if it exists

0 commit comments

Comments
 (0)