forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Optimize Entities::entity_does_not_exist_error_details_message, remove UnsafeWorldCell from error (#17115)

Browse Source 
## 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 <benjamin.brienen@outlook.com>
This commit is contained in:
JaySpruce 2025-01-04 20:01:01 -06:00 committed by GitHub
parent 859c2d77f9
commit 4fde223831
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 97 additions and 93 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

43
crates/bevy_ecs/src/entity/mod.rs
View File

@ -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)"
)
}
}

32
crates/bevy_ecs/src/query/error.rs
View File

@ -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,17 +33,14 @@ impl<'w> core::fmt::Display for QueryEntityError<'w> {
)?;
format_archetype(f, world, entity)
}
Self::NoSuchEntity(entity, world) => {
write!(
f,
"Entity {entity} {}",
world
.entities()
.entity_does_not_exist_error_details_message(entity)
)
Self::NoSuchEntity(entity, details) => {
write!(f, "The entity with ID {entity} {details}")
}
Self::AliasedMutability(entity) => {
write!(f, "Entity {entity} was requested mutably more than once")
write!(
f,
"The entity with ID {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})"),
}

7
crates/bevy_ecs/src/query/state.rs
View File

@ -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())

6
crates/bevy_ecs/src/world/deferred_world.rs
View File

@ -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),
))
}
};

38
crates/bevy_ecs/src/world/entity_fetch.rs
View File

@ -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) });
}

58
crates/bevy_ecs/src/world/error.rs
View File

@ -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 {}

6
crates/bevy_ecs/src/world/mod.rs
View File

@ -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),
))
}
};