Optimize Entities::entity_does_not_exist_error_details_message, remove UnsafeWorldCell from error#17115
Merged
alice-i-cecile merged 6 commits intobevyengine:mainfrom Jan 5, 2025
Conversation
JaySpruce
changed the title
UnsafeWorldCell from errorEntities::entity_does_not_exist_error_details_message, remove UnsafeWorldCell from error
alice-i-cecile
added
A-ECS
Contributor
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Could you add some context on what users should update when this change get released in a new version of Bevy? |
| #[derive(Clone, Copy)] | ||
| pub enum EntityFetchError<'w> { | ||
| #[derive(Error, Debug, Clone, Copy)] | ||
| pub enum EntityFetchError { |
alice-i-cecile
approved these changes
Jan 3, 2025
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
Much cleaner. I'd like to merge this first.
BenjaminBrienen
approved these changes
Jan 3, 2025
Contributor
BenjaminBrienen
left a comment
BenjaminBrienen
left a comment
There was a problem hiding this comment.
I suggested a small code style improvement. Otherwise, looks good!
Co-authored-by: Benjamin Brienen <benjamin.brienen@outlook.com>
BenjaminBrienen
approved these changes
Jan 3, 2025
JaySpruce
added a commit
to JaySpruce/bevy
that referenced
this pull request
Jan 4, 2025
also revert `EntityFetchError` changes for real this time (still could cause problems if left as-is, but bevyengine#17115 will fix it, and this will make the merge easier)
alice-i-cecile
added
S-Ready-For-Final-Review
mrchantey
pushed a commit
to mrchantey/bevy
that referenced
this pull request
Feb 4, 2025
…ove `UnsafeWorldCell` from error (bevyengine#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 bevyengine#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`  ### No `String`  </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>    </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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Objective
The error
EntityFetchError::NoSuchEntityhas anUnsafeWorldCellinside it, which it uses to callEntities::entity_does_not_exist_error_details_messagewhen being printed. That method returns aStringthat, if thetrack_locationfeature is enabled, contains the location of whoever despawned the relevant entity.I initially had to modify this error while working on #17043. The
UnsafeWorldCellwas causing borrow problems when being returned from a command, so I tried replacing it with theStringthat the method returns, since that was the world cell's only purpose.Unfortunately,
Strings are slow, and it significantly impacted performance (on top of that PR's performance hit):17043 benchmarks
With
StringNo
StringFor that PR, I just removed the error details entirely, but I figured I'd try to find a way to keep them around.
Solution
Stringwith a helper struct that holds the location, and only turn it into a string when someone actually wants to print it.UnsafeWorldCellwith the aforementioned struct.QueryEntityError::NoSuchEntity.Benchmarking
This had some interesting performance impact:
This PR vs main
Other work
QueryEntityError::QueryDoesNotMatchalso has anUnsafeWorldCellinside it. This one would be more complicated to rework while keeping the same functionality.Migration Guide
The errors
EntityFetchError::NoSuchEntityandQueryEntityError::NoSuchEntitynow contain anEntityDoesNotExistDetailsstruct instead of anUnsafeWorldCell. If you were just printing these, they should work identically.