Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intern entity paths for faster comparisons. #11711

Closed
wants to merge 1 commit into from

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Feb 5, 2024

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 #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.

Alternatives to this PR include:

  1. 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 AnimationGraph RFC 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.

  2. Using the Interner and accepting Name leaks.

  3. 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.

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
Copy link
Contributor

@SkiFire13 SkiFire13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this could be self.0.as_ptr().hash(state)

Comment on lines +89 to +91
pub fn iter(&self) -> EntityPathIter {
EntityPathIter(Some(self))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 alice-i-cecile added A-Animation Make things move and change over time C-Performance A change motivated by improving speed, memory usage or compile times labels Feb 5, 2024
@james7132 james7132 self-requested a review February 5, 2024 20:10
@pcwalton pcwalton closed this Feb 6, 2024
@pcwalton
Copy link
Contributor Author

pcwalton commented Feb 6, 2024

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
Labels
A-Animation Make things move and change over time C-Performance A change motivated by improving speed, memory usage or compile times
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants