Skip to content

Commit

Permalink
Remove duplicate lookups from Resource initialization (#7174)
Browse files Browse the repository at this point in the history
# 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.
  • Loading branch information
JoJoJet committed Jan 12, 2023
1 parent b47c466 commit feac2c2
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 13 deletions.
15 changes: 15 additions & 0 deletions crates/bevy_ecs/src/storage/resource.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::archetype::ArchetypeComponentId;
use crate::change_detection::{MutUntyped, TicksMut};
use crate::component::{ComponentId, ComponentTicks, Components, TickCells};
use crate::storage::{Column, SparseSet, TableRow};
use bevy_ptr::{OwningPtr, Ptr, UnsafeCellDeref};
Expand Down Expand Up @@ -99,6 +100,20 @@ impl<const SEND: bool> ResourceData<SEND> {
})
}

pub(crate) fn get_mut(
&mut self,
last_change_tick: u32,
change_tick: u32,
) -> Option<MutUntyped<'_>> {
let (ptr, ticks) = self.get_with_ticks()?;
Some(MutUntyped {
// SAFETY: We have exclusive access to the underlying storage.
value: unsafe { ptr.assert_unique() },
// SAFETY: We have exclusive access to the underlying storage.
ticks: unsafe { TicksMut::from_tick_cells(ticks, last_change_tick, change_tick) },
})
}

/// Inserts a value into the resource. If a value is already present
/// it will be replaced.
///
Expand Down
64 changes: 51 additions & 13 deletions crates/bevy_ecs/src/world/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use crate::{
entity::{AllocAtWithoutReplacement, Entities, Entity, EntityLocation},
event::{Event, Events},
ptr::UnsafeCellDeref,
query::{QueryState, ReadOnlyWorldQuery, WorldQuery},
query::{DebugCheckedUnwrap, QueryState, ReadOnlyWorldQuery, WorldQuery},
storage::{ResourceData, SparseSet, Storages},
system::Resource,
};
Expand Down Expand Up @@ -767,9 +767,20 @@ impl World {
/// and those default values will be here instead.
#[inline]
pub fn init_resource<R: Resource + FromWorld>(&mut self) {
if !self.contains_resource::<R>() {
let resource = R::from_world(self);
self.insert_resource(resource);
let component_id = self.components.init_resource::<R>();
if self
.storages
.resources
.get(component_id)
.map_or(true, |data| !data.is_present())
{
let value = R::from_world(self);
OwningPtr::make(value, |ptr| {
// SAFETY: component_id was just initialized and corresponds to resource of type R.
unsafe {
self.insert_resource_by_id(component_id, ptr);
}
});
}
}

Expand All @@ -782,7 +793,7 @@ impl World {
pub fn insert_resource<R: Resource>(&mut self, value: R) {
let component_id = self.components.init_resource::<R>();
OwningPtr::make(value, |ptr| {
// SAFETY: component_id was just initialized and corresponds to resource of type R
// SAFETY: component_id was just initialized and corresponds to resource of type R.
unsafe {
self.insert_resource_by_id(component_id, ptr);
}
Expand All @@ -802,9 +813,20 @@ impl World {
/// Panics if called from a thread other than the main thread.
#[inline]
pub fn init_non_send_resource<R: 'static + FromWorld>(&mut self) {
if !self.contains_non_send::<R>() {
let resource = R::from_world(self);
self.insert_non_send_resource(resource);
let component_id = self.components.init_non_send::<R>();
if self
.storages
.resources
.get(component_id)
.map_or(true, |data| !data.is_present())
{
let value = R::from_world(self);
OwningPtr::make(value, |ptr| {
// SAFETY: component_id was just initialized and corresponds to resource of type R.
unsafe {
self.insert_non_send_by_id(component_id, ptr);
}
});
}
}

Expand All @@ -821,7 +843,7 @@ impl World {
pub fn insert_non_send_resource<R: 'static>(&mut self, value: R) {
let component_id = self.components.init_non_send::<R>();
OwningPtr::make(value, |ptr| {
// SAFETY: component_id was just initialized and corresponds to resource of type R
// SAFETY: component_id was just initialized and corresponds to resource of type R.
unsafe {
self.insert_non_send_by_id(component_id, ptr);
}
Expand Down Expand Up @@ -959,18 +981,34 @@ impl World {
unsafe { self.get_resource_unchecked_mut() }
}

// PERF: optimize this to avoid redundant lookups
/// Gets a mutable reference to the resource of type `T` if it exists,
/// otherwise inserts the resource using the result of calling `func`.
#[inline]
pub fn get_resource_or_insert_with<R: Resource>(
&mut self,
func: impl FnOnce() -> R,
) -> Mut<'_, R> {
if !self.contains_resource::<R>() {
self.insert_resource(func());
let change_tick = self.change_tick();
let last_change_tick = self.last_change_tick();

let component_id = self.components.init_resource::<R>();
let data = self.initialize_resource_internal(component_id);
if !data.is_present() {
OwningPtr::make(func(), |ptr| {
// SAFETY: component_id was just initialized and corresponds to resource of type R.
unsafe {
data.insert(ptr, change_tick);
}
});
}
self.resource_mut()

// SAFETY: The resource must be present, as we would have inserted it if it was empty.
let data = unsafe {
data.get_mut(last_change_tick, change_tick)
.debug_checked_unwrap()
};
// SAFETY: The underlying type of the resource is `R`.
unsafe { data.with_type::<R>() }
}

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

0 comments on commit feac2c2

Please sign in to comment.