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.

Sign up for GitHub

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

Optimize AnimationMixer with new BitSet class. #92257

Closed
wants to merge 1 commit into from
Closed

Optimize AnimationMixer with new BitSet class. #92257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 97 additions & 0 deletions core/templates/bit_set.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/**************************************************************************/
/* bit_set.h */
/**************************************************************************/
/* This file is part of: */
/* GODOT ENGINE */
/* https://godotengine.org */
/**************************************************************************/
/* Copyright (c) 2014-present Godot Engine contributors (see AUTHORS.md). */
/* Copyright (c) 2007-2014 Juan Linietsky, Ariel Manzur. */
/* */
/* Permission is hereby granted, free of charge, to any person obtaining */
/* a copy of this software and associated documentation files (the */
/* "Software"), to deal in the Software without restriction, including */
/* without limitation the rights to use, copy, modify, merge, publish, */
/* distribute, sublicense, and/or sell copies of the Software, and to */
/* permit persons to whom the Software is furnished to do so, subject to */
/* the following conditions: */
/* */
/* The above copyright notice and this permission notice shall be */
/* included in all copies or substantial portions of the Software. */
/* */
/* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, */
/* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF */
/* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. */
/* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY */
/* CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, */
/* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE */
/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */
/**************************************************************************/

#ifndef BIT_SET_H
#define BIT_SET_H

#include "core/templates/local_vector.h"

template <typename U = uint64_t, bool tight = false>
class BitSet {
private:
LocalVector<U, uint32_t, false, tight> values;
uint32_t count = 0;

_FORCE_INLINE_ U _get_value_index(uint32_t p_index) const {
return p_index / (sizeof(U) * 8);
}

_FORCE_INLINE_ U _get_value_mask(uint32_t p_index) const {
return U(1) << (p_index % (sizeof(U) * 8));
}

public:
_FORCE_INLINE_ void clear() {
values.clear();
count = 0;
}

_FORCE_INLINE_ void resize(uint32_t p_size) {
uint32_t previous_size = values.size();
values.resize((p_size + (sizeof(U) * 8 - 1)) / (sizeof(U) * 8));

uint32_t count_remainder = count % (sizeof(U) * 8);
if (count < p_size && count_remainder != 0) {
// The remaining bits of the value should be cleared.
values[_get_value_index(count)] &= (U(1) << count_remainder) - U(1);
}

for (uint32_t i = previous_size; i < values.size(); i++) {
// All the remaining values should be initialized to zero.
values[i] = U(0);
}

count = p_size;
}

_FORCE_INLINE_ void set(uint32_t p_index, bool p_value) {
CRASH_BAD_UNSIGNED_INDEX(p_index, count);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a redundant check since LocalVector operator[] has the same one.

Copy link
Member

Choose a reason for hiding this comment

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

CC @DarioSamo Do you want to go ahead and remove the redundant check before we merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can no longer do this PR as @TokageItLab has changed the logic behind it to use hashes instead of indices. It needs to be re-evaluated completely from scratch. It's not possible to use a BitSet with hashes.


if (p_value) {
values[_get_value_index(p_index)] |= _get_value_mask(p_index);
} else {
values[_get_value_index(p_index)] &= ~_get_value_mask(p_index);
}
}

_FORCE_INLINE_ bool get(uint32_t p_index) const {
CRASH_BAD_UNSIGNED_INDEX(p_index, count);

return (values[_get_value_index(p_index)] & _get_value_mask(p_index)) != 0;
}

_FORCE_INLINE_ uint32_t size() const {
return count;
}

_FORCE_INLINE_ BitSet() {}
};

#endif // BIT_SET_H
13 changes: 8 additions & 5 deletions scene/animation/animation_mixer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

#include "core/config/engine.h"
#include "core/config/project_settings.h"
#include "core/templates/bit_set.h"
#include "scene/animation/animation_player.h"
#include "scene/resources/animation.h"
#include "servers/audio/audio_stream.h"
Expand Down Expand Up @@ -1068,11 +1069,13 @@ void AnimationMixer::blend_capture(double p_delta) {
}

void AnimationMixer::_blend_calc_total_weight() {
thread_local BitSet processed_indices;
for (const AnimationInstance &ai : animation_instances) {
Ref<Animation> a = ai.animation_data.animation;
real_t weight = ai.playback_info.weight;
Vector<real_t> track_weights = ai.playback_info.track_weights;
Vector<int> processed_indices;
processed_indices.clear();
processed_indices.resize(track_count);
Copy link
Member

@lawnjelly lawnjelly May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a maximum track count? If so and this is called often, you can also allocate on the stack with alloca. This is done in a lot of performance sensitive cases. (I haven't profiled this.)

(BTW not trying to suggest a hard requirement here, as I haven't examined, this is just for ideas.)

The reuse of processed_indices for all animation_instances should already reduce allocations significantly.

Copy link
Contributor Author

@DarioSamo DarioSamo May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

clear() and resize() won't actually affect the capacity of the vector in most cases. What it's simply doing here is just setting the size to 0, and then the resize sets the new size and clears all bits to its default (false), which is why these two steps are joined together. Clear and resize won't reallocate anything unless capacity needs to increase. There's no hard maximum as far as I can tell on track count.

The only allocations that happen are when across multiple calls (in the same thread), track_count increases enough to justify increasing the capacity. I believe it's either through power of twos or by 1.5 growth ratios. I've talked with @RandomShaper in the past about alloca, but it comes with the drawback of potentially introducing a scalability problem if the function ever needs a very large amount of data and stack memory is very limited.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, and anyway we can apply an enhancement later for all the thread_local vectors such that they are backed by memory from an arena allocator or something, getting the best of both worlds.

for (int i = 0; i < a->get_track_count(); i++) {
if (!a->track_is_enabled(i)) {
continue;
Expand All @@ -1083,13 +1086,13 @@ void AnimationMixer::_blend_calc_total_weight() {
}
TrackCache *track = track_cache[thash];
int blend_idx = track_map[track->path];
if (processed_indices.has(blend_idx)) {
continue; // There is the case different track type with same path... Is there more faster iterating way than has()?
}
ERR_CONTINUE(blend_idx < 0 || blend_idx >= track_count);
if (processed_indices.get(blend_idx)) {
continue; // Different track type with the same path.
}
real_t blend = blend_idx < track_weights.size() ? track_weights[blend_idx] * weight : weight;
track->total_weight += blend;
processed_indices.push_back(blend_idx);
processed_indices.set(blend_idx, true);
}
}
}
Expand Down
131 changes: 131 additions & 0 deletions tests/core/templates/test_bit_set.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
/**************************************************************************/
/* test_bit_set.h */
/**************************************************************************/
/* This file is part of: */
/* GODOT ENGINE */
/* https://godotengine.org */
/**************************************************************************/
/* Copyright (c) 2014-present Godot Engine contributors (see AUTHORS.md). */
/* Copyright (c) 2007-2014 Juan Linietsky, Ariel Manzur. */
/* */
/* Permission is hereby granted, free of charge, to any person obtaining */
/* a copy of this software and associated documentation files (the */
/* "Software"), to deal in the Software without restriction, including */
/* without limitation the rights to use, copy, modify, merge, publish, */
/* distribute, sublicense, and/or sell copies of the Software, and to */
/* permit persons to whom the Software is furnished to do so, subject to */
/* the following conditions: */
/* */
/* The above copyright notice and this permission notice shall be */
/* included in all copies or substantial portions of the Software. */
/* */
/* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, */
/* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF */
/* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. */
/* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY */
/* CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, */
/* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE */
/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */
/**************************************************************************/

#ifndef TEST_BIT_SET_H
#define TEST_BIT_SET_H

#include "core/templates/bit_set.h"

#include "tests/test_macros.h"

namespace TestBitSet {

TEST_CASE("[BitSet] Set elements") {
BitSet set;
set.resize(6);
set.set(0, true);
set.set(3, true);
set.set(5, true);

CHECK(set.size() == 6);
CHECK(set.get(0) == true);
CHECK(set.get(1) == false);
CHECK(set.get(2) == false);
CHECK(set.get(3) == true);
CHECK(set.get(4) == false);
CHECK(set.get(5) == true);

set.set(0, false);
set.set(1, true);
set.set(3, false);
set.set(4, true);

CHECK(set.get(0) == false);
CHECK(set.get(1) == true);
CHECK(set.get(2) == false);
CHECK(set.get(3) == false);
CHECK(set.get(4) == true);
CHECK(set.get(5) == true);
}

TEST_CASE("[BitSet] Clear and resize defaults") {
BitSet set;
set.resize(3);
set.set(0, true);
set.set(1, true);
set.set(2, true);
set.clear();

CHECK(set.size() == 0);

// After resizing, all values should be defaulted to zero.
set.resize(3);

CHECK(set.size() == 3);
CHECK(set.get(0) == false);
CHECK(set.get(1) == false);
CHECK(set.get(2) == false);

// Special case where the bit set should've cleared the individual bit to simulate
// the behavior a vector does when resizing to a bigger size and defaulting to zero,
// even if the contents might've been set before.
set.set(0, true);
set.set(1, true);
set.set(2, true);
set.resize(2);
set.resize(12);

CHECK(set.get(0) == true);
CHECK(set.get(1) == true);
CHECK(set.get(2) == false);
CHECK(set.get(11) == false);
}

TEST_CASE("[BitSet] Copy") {
BitSet set;
set.resize(3);
set.set(0, true);
set.set(1, false);
set.set(2, true);

BitSet set_copy = set;
CHECK(set_copy.size() == 3);
CHECK(set_copy.get(0) == true);
CHECK(set_copy.get(1) == false);
CHECK(set_copy.get(2) == true);
}

TEST_CASE("[BitSet] Large set") {
uint32_t large_count = 10000;
BitSet set;
set.resize(large_count);
set.set(100, true);
set.set(350, true);
set.set(5000, true);
set.set(1234, true);

CHECK(set.get(100) == true);
CHECK(set.get(350) == true);
CHECK(set.get(5000) == true);
CHECK(set.get(1234) == true);
}
} // namespace TestBitSet

#endif // TEST_BIT_SET_H
1 change: 1 addition & 0 deletions tests/test_main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@
#include "tests/core/string/test_string.h"
#include "tests/core/string/test_translation.h"
#include "tests/core/string/test_translation_server.h"
#include "tests/core/templates/test_bit_set.h"
#include "tests/core/templates/test_command_queue.h"
#include "tests/core/templates/test_hash_map.h"
#include "tests/core/templates/test_hash_set.h"
Expand Down
Loading