Intern entity paths for faster comparisons. #11711
Closed
Conversation
This file contains 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
This patch places all entity paths into a shared table so that comparing them is as cheap as a pointer comparison. We don't use the pre-existing `Interner` type because that leaks all strings added to it, and I was uncomfortable with leaking names, as Bevy apps might dynamically generate them. Instead, I reference count all the names and use a linked list to stitch together paths into a tree. The interner uses a weak hash set from the [`weak-table`] crate. This patch is especially helpful for the two-phase animation PR bevyengine#11707, because two-phase animation gets rid of the cache from name to animation-specific slot, thus increasing the load on the hash table that maps paths to bone indices. Note that the interned table is a global variable behind a `OnceLock` instead of a resource. This is because it must be accessed from the glTF `AssetLoader`, which unfortunately has no access to Bevy resources. [`weak-table`]: https://crates.io/crates/weak-table
SkiFire13
reviewed
Feb 5, 2024
There was a problem hiding this comment.
The interner uses a weak hash set from the weak-table crate.
FYI I can see two problems with that crate:
- it implements its own hashtable instead of using
hashbrown - it doesn't immediately remove
Weaks that can no longer be upgraded, instead it waits until some other value can overwrite it
| impl Hash for EntityPath { | ||
| fn hash<H: Hasher>(&self, state: &mut H) { | ||
| // Hash by address. This is safe because entity paths are unique. | ||
| (self.0.as_ref() as *const EntityPathNode).hash(state) |
There was a problem hiding this comment.
Nit: this could be self.0.as_ptr().hash(state)
Comment on lines
+89
to
+91
| pub fn iter(&self) -> EntityPathIter { | ||
| EntityPathIter(Some(self)) | ||
| } |
There was a problem hiding this comment.
This could make it clearer that the iterator is actually reversed.
Comment on lines
+93
to
+95
| pub fn root(&self) -> &Name { | ||
| &self.iter().last().unwrap().0.name | ||
| } |
There was a problem hiding this comment.
It's kind of unfortunate that common operations like root and (forward) iter have to be less efficient due to the way the linked list is constructed. Is there a reason it cannot be created reversed (or the whole path be interned together)?
alice-i-cecile
added
A-Animation
|
Closed as this is probably no longer relevant with #11707. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This patch places all entity paths into a shared table so that comparing them is as cheap as a pointer comparison. We don't use the pre-existing
Internertype because that leaks all strings added to it, and I was uncomfortable with leaking names, as Bevy apps might dynamically generate them. Instead, I reference count all the names and use a linked list to stitch together paths into a tree. The interner uses a weak hash set from theweak-tablecrate.This patch is especially helpful for the two-phase animation PR #11707, because two-phase animation gets rid of the cache from name to animation-specific slot, thus increasing the load on the hash table that maps paths to bone indices.
Note that the interned table is a global variable behind a
OnceLockinstead of a resource. This is because it must be accessed from the glTFAssetLoader, which unfortunately has no access to Bevy resources.Alternatives to this PR include:
Recreating the path cache in Rework animation to be done in two phases. #11707. This is technically doable, but has the strong downside that changing the currently-playing animation clip would require us to traverse the entire subtree rooted at that player and update the path cache for all nodes. The
AnimationGraphRFC would allow us to precalculate a path cache for all animations up front, since the graph is static. But that would be expensive because every single bone would have to cache an index for every single animation that the graph could possibly play, which would mean that updates to the bone structure would result in O(number of clips in the graph) operations.Using the
Internerand accepting Name leaks.Representing entity paths as SHA-256 or some other hash function that's guaranteed not to collide.
This PR is a draft to start the conversation.