Optimize Entities::entity_does_not_exist_error_details_message, rem…

…ove `UnsafeWorldCell` from error (#17115) ## Objective The error `EntityFetchError::NoSuchEntity` has an `UnsafeWorldCell` inside it, which it uses to call `Entities::entity_does_not_exist_error_details_message` when being printed. That method returns a `String` that, if the `track_location` feature is enabled, contains the location of whoever despawned the relevant entity. I initially had to modify this error while working on #17043. The `UnsafeWorldCell` was causing borrow problems when being returned from a command, so I tried replacing it with the `String` that the method returns, since that was the world cell's only purpose. Unfortunately, `String`s are slow, and it significantly impacted performance (on top of that PR's performance hit): <details> <summary>17043 benchmarks</summary> ### With `String` ![error_handling_insert_slow](https://github.com/user-attachments/assets/5629ba6d-69fc-4c16-84c9-8be7e449232d) ### No `String` ![error_handling_insert_fixed](https://github.com/user-attachments/assets/6393e2d6-e61a-4558-8ff1-471ff8356c1c) </details> For that PR, I just removed the error details entirely, but I figured I'd try to find a way to keep them around. ## Solution - Replace the `String` with a helper struct that holds the location, and only turn it into a string when someone actually wants to print it. - Replace the `UnsafeWorldCell` with the aforementioned struct. - Do the same for `QueryEntityError::NoSuchEntity`. ## Benchmarking This had some interesting performance impact: <details> <summary>This PR vs main</summary> ![dne_rework_1](https://github.com/user-attachments/assets/05bf91b4-dddc-4d76-b2c4-41c9d25c7a57) ![dne_rework_2](https://github.com/user-attachments/assets/34aa76b2-d8a7-41e0-9670-c213207e457d) ![dne_rework_3](https://github.com/user-attachments/assets/8b9bd4e4-77c8-45a7-b058-dc0dfd3dd323) </details> ## Other work `QueryEntityError::QueryDoesNotMatch` also has an `UnsafeWorldCell` inside it. This one would be more complicated to rework while keeping the same functionality. ## Migration Guide The errors `EntityFetchError::NoSuchEntity` and `QueryEntityError::NoSuchEntity` now contain an `EntityDoesNotExistDetails` struct instead of an `UnsafeWorldCell`. If you were just printing these, they should work identically. --------- Co-authored-by: Benjamin Brienen <[email protected]>
bevyengine · Jan 5, 2025 · 4fde223 · 4fde223
1 parent 859c2d7
commit 4fde223
Show file tree

Hide file tree

Showing 7 changed files with 96 additions and 92 deletions.
diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs
@@ -69,12 +69,12 @@ use crate::{
     },
     storage::{SparseSetIndex, TableId, TableRow},
 };
-use alloc::{borrow::ToOwned, string::String, vec::Vec};
+use alloc::vec::Vec;
 use core::{fmt, hash::Hash, mem, num::NonZero};
 use log::warn;
 
 #[cfg(feature = "track_location")]
-use {alloc::format, core::panic::Location};
+use core::panic::Location;
 
 #[cfg(feature = "serialize")]
 use serde::{Deserialize, Serialize};
@@ -991,19 +991,38 @@ impl Entities {
     }
 
     /// Constructs a message explaining why an entity does not exists, if known.
-    pub(crate) fn entity_does_not_exist_error_details_message(&self, _entity: Entity) -> String {
+    pub(crate) fn entity_does_not_exist_error_details_message(
+        &self,
+        _entity: Entity,
+    ) -> EntityDoesNotExistDetails {
+        EntityDoesNotExistDetails {
+            #[cfg(feature = "track_location")]
+            location: self.entity_get_spawned_or_despawned_by(_entity),
+        }
+    }
+}
+
+/// Helper struct that, when printed, will write the appropriate details
+/// regarding an entity that did not exist.
+#[derive(Copy, Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
+pub struct EntityDoesNotExistDetails {
+    #[cfg(feature = "track_location")]
+    location: Option<&'static Location<'static>>,
+}
+
+impl fmt::Display for EntityDoesNotExistDetails {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         #[cfg(feature = "track_location")]
-        {
-            if let Some(location) = self.entity_get_spawned_or_despawned_by(_entity) {
-                format!("was despawned by {location}",)
-            } else {
-                "was never spawned".to_owned()
-            }
+        if let Some(location) = self.location {
+            write!(f, "was despawned by {}", location)
+        } else {
+            write!(f, "was never spawned")
         }
         #[cfg(not(feature = "track_location"))]
-        {
-            "does not exist (enable `track_location` feature for more details)".to_owned()
-        }
+        write!(
+            f,
+            "does not exist (enable `track_location` feature for more details)"
+        )
     }
 }
 

diff --git a/crates/bevy_ecs/src/query/error.rs b/crates/bevy_ecs/src/query/error.rs
@@ -1,6 +1,9 @@
 use thiserror::Error;
 
-use crate::{entity::Entity, world::unsafe_world_cell::UnsafeWorldCell};
+use crate::{
+    entity::{Entity, EntityDoesNotExistDetails},
+    world::unsafe_world_cell::UnsafeWorldCell,
+};
 
 /// An error that occurs when retrieving a specific [`Entity`]'s query result from [`Query`](crate::system::Query) or [`QueryState`](crate::query::QueryState).
 // TODO: return the type_name as part of this error
@@ -11,7 +14,7 @@ pub enum QueryEntityError<'w> {
     /// Either it does not have a requested component, or it has a component which the query filters out.
     QueryDoesNotMatch(Entity, UnsafeWorldCell<'w>),
     /// The given [`Entity`] does not exist.
-    NoSuchEntity(Entity, UnsafeWorldCell<'w>),
+    NoSuchEntity(Entity, EntityDoesNotExistDetails),
     /// The [`Entity`] was requested mutably more than once.
     ///
     /// See [`QueryState::get_many_mut`](crate::query::QueryState::get_many_mut) for an example.
@@ -30,18 +33,15 @@ impl<'w> core::fmt::Display for QueryEntityError<'w> {
                 )?;
                 format_archetype(f, world, entity)
             }
-            Self::NoSuchEntity(entity, world) => {
+            Self::NoSuchEntity(entity, details) => {
+                write!(f, "The entity with ID {entity} {details}")
+            }
+            Self::AliasedMutability(entity) => {
                 write!(
                     f,
-                    "Entity {entity} {}",
-                    world
-                        .entities()
-                        .entity_does_not_exist_error_details_message(entity)
+                    "The entity with ID {entity} was requested mutably more than once"
                 )
             }
-            Self::AliasedMutability(entity) => {
-                write!(f, "Entity {entity} was requested mutably more than once")
-            }
         }
     }
 }
@@ -54,14 +54,8 @@ impl<'w> core::fmt::Debug for QueryEntityError<'w> {
                 format_archetype(f, world, entity)?;
                 write!(f, ")")
             }
-            Self::NoSuchEntity(entity, world) => {
-                write!(
-                    f,
-                    "NoSuchEntity({entity} {})",
-                    world
-                        .entities()
-                        .entity_does_not_exist_error_details_message(entity)
-                )
+            Self::NoSuchEntity(entity, details) => {
+                write!(f, "NoSuchEntity({entity} {details})")
             }
             Self::AliasedMutability(entity) => write!(f, "AliasedMutability({entity})"),
         }

diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs
@@ -1020,7 +1020,12 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
         let location = world
             .entities()
             .get(entity)
-            .ok_or(QueryEntityError::NoSuchEntity(entity, world))?;
+            .ok_or(QueryEntityError::NoSuchEntity(
+                entity,
+                world
+                    .entities()
+                    .entity_does_not_exist_error_details_message(entity),
+            ))?;
         if !self
             .matched_archetypes
             .contains(location.archetype_id.index())

diff --git a/crates/bevy_ecs/src/world/deferred_world.rs b/crates/bevy_ecs/src/world/deferred_world.rs
@@ -109,7 +109,11 @@ impl<'w> DeferredWorld<'w> {
                 return Err(EntityFetchError::AliasedMutability(entity))
             }
             Err(EntityFetchError::NoSuchEntity(..)) => {
-                return Err(EntityFetchError::NoSuchEntity(entity, self.world))
+                return Err(EntityFetchError::NoSuchEntity(
+                    entity,
+                    self.entities()
+                        .entity_does_not_exist_error_details_message(entity),
+                ))
             }
         };
 

diff --git a/crates/bevy_ecs/src/world/entity_fetch.rs b/crates/bevy_ecs/src/world/entity_fetch.rs
@@ -124,7 +124,11 @@ unsafe impl WorldEntityFetch for Entity {
         let location = cell
             .entities()
             .get(self)
-            .ok_or(EntityFetchError::NoSuchEntity(self, cell))?;
+            .ok_or(EntityFetchError::NoSuchEntity(
+                self,
+                cell.entities()
+                    .entity_does_not_exist_error_details_message(self),
+            ))?;
         // SAFETY: caller ensures that the world cell has mutable access to the entity.
         let world = unsafe { cell.world_mut() };
         // SAFETY: location was fetched from the same world's `Entities`.
@@ -135,9 +139,11 @@ unsafe impl WorldEntityFetch for Entity {
         self,
         cell: UnsafeWorldCell<'_>,
     ) -> Result<Self::DeferredMut<'_>, EntityFetchError> {
-        let ecell = cell
-            .get_entity(self)
-            .ok_or(EntityFetchError::NoSuchEntity(self, cell))?;
+        let ecell = cell.get_entity(self).ok_or(EntityFetchError::NoSuchEntity(
+            self,
+            cell.entities()
+                .entity_does_not_exist_error_details_message(self),
+        ))?;
         // SAFETY: caller ensures that the world cell has mutable access to the entity.
         Ok(unsafe { EntityMut::new(ecell) })
     }
@@ -209,9 +215,11 @@ unsafe impl<const N: usize> WorldEntityFetch for &'_ [Entity; N] {
 
         let mut refs = [const { MaybeUninit::uninit() }; N];
         for (r, &id) in core::iter::zip(&mut refs, self) {
-            let ecell = cell
-                .get_entity(id)
-                .ok_or(EntityFetchError::NoSuchEntity(id, cell))?;
+            let ecell = cell.get_entity(id).ok_or(EntityFetchError::NoSuchEntity(
+                id,
+                cell.entities()
+                    .entity_does_not_exist_error_details_message(id),
+            ))?;
             // SAFETY: caller ensures that the world cell has mutable access to the entity.
             *r = MaybeUninit::new(unsafe { EntityMut::new(ecell) });
         }
@@ -267,9 +275,11 @@ unsafe impl WorldEntityFetch for &'_ [Entity] {
 
         let mut refs = Vec::with_capacity(self.len());
         for &id in self {
-            let ecell = cell
-                .get_entity(id)
-                .ok_or(EntityFetchError::NoSuchEntity(id, cell))?;
+            let ecell = cell.get_entity(id).ok_or(EntityFetchError::NoSuchEntity(
+                id,
+                cell.entities()
+                    .entity_does_not_exist_error_details_message(id),
+            ))?;
             // SAFETY: caller ensures that the world cell has mutable access to the entity.
             refs.push(unsafe { EntityMut::new(ecell) });
         }
@@ -312,9 +322,11 @@ unsafe impl WorldEntityFetch for &'_ EntityHashSet {
     ) -> Result<Self::Mut<'_>, EntityFetchError> {
         let mut refs = EntityHashMap::with_capacity(self.len());
         for &id in self {
-            let ecell = cell
-                .get_entity(id)
-                .ok_or(EntityFetchError::NoSuchEntity(id, cell))?;
+            let ecell = cell.get_entity(id).ok_or(EntityFetchError::NoSuchEntity(
+                id,
+                cell.entities()
+                    .entity_does_not_exist_error_details_message(id),
+            ))?;
             // SAFETY: caller ensures that the world cell has mutable access to the entity.
             refs.insert(id, unsafe { EntityMut::new(ecell) });
         }

diff --git a/crates/bevy_ecs/src/world/error.rs b/crates/bevy_ecs/src/world/error.rs
@@ -2,9 +2,11 @@
 
 use thiserror::Error;
 
-use crate::{component::ComponentId, entity::Entity, schedule::InternedScheduleLabel};
-
-use super::unsafe_world_cell::UnsafeWorldCell;
+use crate::{
+    component::ComponentId,
+    entity::{Entity, EntityDoesNotExistDetails},
+    schedule::InternedScheduleLabel,
+};
 
 /// The error type returned by [`World::try_run_schedule`] if the provided schedule does not exist.
 ///
@@ -25,53 +27,17 @@ pub enum EntityComponentError {
 }
 
 /// An error that occurs when fetching entities mutably from a world.
-#[derive(Clone, Copy)]
-pub enum EntityFetchError<'w> {
+#[derive(Error, Debug, Clone, Copy)]
+pub enum EntityFetchError {
     /// The entity with the given ID does not exist.
-    NoSuchEntity(Entity, UnsafeWorldCell<'w>),
+    #[error("The entity with ID {0} {1}")]
+    NoSuchEntity(Entity, EntityDoesNotExistDetails),
     /// The entity with the given ID was requested mutably more than once.
+    #[error("The entity with ID {0} was requested mutably more than once")]
     AliasedMutability(Entity),
 }
 
-impl<'w> core::error::Error for EntityFetchError<'w> {}
-
-impl<'w> core::fmt::Display for EntityFetchError<'w> {
-    fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
-        match *self {
-            Self::NoSuchEntity(entity, world) => {
-                write!(
-                    f,
-                    "Entity {entity} {}",
-                    world
-                        .entities()
-                        .entity_does_not_exist_error_details_message(entity)
-                )
-            }
-            Self::AliasedMutability(entity) => {
-                write!(f, "Entity {entity} was requested mutably more than once")
-            }
-        }
-    }
-}
-
-impl<'w> core::fmt::Debug for EntityFetchError<'w> {
-    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
-        match *self {
-            Self::NoSuchEntity(entity, world) => {
-                write!(
-                    f,
-                    "NoSuchEntity({entity} {})",
-                    world
-                        .entities()
-                        .entity_does_not_exist_error_details_message(entity)
-                )
-            }
-            Self::AliasedMutability(entity) => write!(f, "AliasedMutability({entity})"),
-        }
-    }
-}
-
-impl<'w> PartialEq for EntityFetchError<'w> {
+impl PartialEq for EntityFetchError {
     fn eq(&self, other: &Self) -> bool {
         match (self, other) {
             (Self::NoSuchEntity(e1, _), Self::NoSuchEntity(e2, _)) if e1 == e2 => true,
@@ -81,4 +47,4 @@ impl<'w> PartialEq for EntityFetchError<'w> {
     }
 }
 
-impl<'w> Eq for EntityFetchError<'w> {}
+impl Eq for EntityFetchError {}
diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs
@@ -1327,7 +1327,11 @@ impl World {
                 return Err(EntityFetchError::AliasedMutability(entity))
             }
             Err(EntityFetchError::NoSuchEntity(..)) => {
-                return Err(EntityFetchError::NoSuchEntity(entity, self.into()))
+                return Err(EntityFetchError::NoSuchEntity(
+                    entity,
+                    self.entities()
+                        .entity_does_not_exist_error_details_message(entity),
+                ))
             }
         };