Optimize Animation::_track_update_hash#110400
Conversation
TokageItLab
left a comment
TokageItLab
left a comment
There was a problem hiding this comment.
Considering that hashes are fine as long as they don't conflict, and given that existing code is working with this, I assume it should be fine.
However, since I'm not an expert of hash algorithm, it might be better to have someone with more detailed knowledge review it.
The literature I found on the internet was as follows:
https://stackoverflow.com/questions/20511347/a-good-hash-function-for-a-vector
TokageItLab
moved this to Approved, Waiting for Production
in Animation Team Issue Triage
Ryan-000
force-pushed
the
Optimize-Animation_track_update_hash
branch
from
a7d43b7 to
0a50c8e
Compare
Ryan-000
force-pushed
the
Optimize-Animation_track_update_hash
branch
from
0a50c8e to
8c1ef2e
Compare
Ryan-000
force-pushed
the
Optimize-Animation_track_update_hash
branch
from
06d1b4d to
b9e9dd6
Compare
Any hash algorithm may have collisions. If a value with the same hash already exists in the container, we should handle hash collisions (comparing if the values are actually equal always, placing elements with the same hash into a bucket when inserting). It seems we are not doing so, using hash directly as a key seems problematic, which may be a separate issue. godot/scene/animation/animation_mixer.h Line 307 in 2dd26a0 |
|
It's true that it occurs with astronomical probability. If we want to add avoidance, I think adding it is nice. The track hash was originally implemented in #86687 to identify blendable track counts, so as long as it can identify them, it's fine. |
|
I think we just need to replace |
|
Benchmarking would be nice, you can use the MRP of #101494 if you don't have an MRP of your own to share |
This purpose of this PR is to make the current hashing implementation more efficient, not to rewrite the hashing backend. If you’d like to take it on, feel free to open a separate PR. |
I reviewed the MRP, but it is not suitable to benchmark this PR, as there are not enough animations or tracks to reveal the impact during the initial load (only 3 animations, with 4 tracks). If you would like to benchmark this PR, you would need a project with a much more animations/track counts (for reference, my game has 27,723 tracks across 375 skeletal animations). |
|
Yea wasn't sure if it'd be suitable, but I'm not aware of any other more appropriate benchmarks (hence the if you don't have your own MRP remark), and wanted to say that some Benchmarking would be nice. It's fine though if you're unable to share it, but results would still be welcome. |
Ivorforce
left a comment
Ivorforce
left a comment
There was a problem hiding this comment.
Looks good to me. Using the dedicated hash functions instead of hashing the string should work and will solve the performance issue.
I agree using the hash as keys should be fixed, but I also agree it's out of scope for this PR.
Co-Authored-By: Silc Lizard (Tokage) Renew <tokage.it.lab@gmail.com> Co-Authored-By: Luo Zhihao <luo_zhihao@outlook.com> Co-Authored-By: Lukas Tenbrink <lukas.tenbrink@gmail.com>
Ryan-000
force-pushed
the
Optimize-Animation_track_update_hash
branch
from
44d0609 to
44856c8
Compare
github-project-automation
bot
moved this from Approved, Waiting for Production
to Done
in Animation Team Issue Triage
|
Thanks! |
akien-mga
changed the title
Animation::_track_update_hash
6 participants
While I was profiling, I found that
Animation::_track_update_hashspent significant time allocating due to string concatenation,NodePath::get_concatenated_names(),NodePath::get_concatenated_subnames()internals and constructing aStringNamehad some overhead.I rewrote the method to resolve these issues.