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

Add FixedVector template to core - a collection that can be used completely on the stack. #104055

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+241 −0
Open

Add FixedVector template to core - a collection that can be used completely on the stack. #104055

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
155 changes: 155 additions & 0 deletions core/templates/fixed_vector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
/**************************************************************************/
/* fixed_vector.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. */
/**************************************************************************/

#pragma once

/**
* A high performance Vector of fixed capacity.
* Especially useful if you need to create an array on the stack, to
* prevent dynamic allocations (especially in bottleneck code).
*
* Choose CAPACITY such that it is enough for all elements that could be added through all branches.
*
* Set force_trivial to true to gain a slight performance improvement on resize calls
* if the default value of the type can be ignored.
*/
template <class T, uint32_t CAPACITY, bool force_trivial = false>
class FixedVector {
// This declaration allows us to access other FixedVector's private members.
template <class T_, uint32_t CAPACITY_, bool force_trivial_>
friend class FixedVector;

static_assert(!force_trivial || std::is_trivially_destructible_v<T>, "force_trivial may only be true if the type is trivially destructible.");

uint32_t _size = 0;
alignas(T) uint8_t _data[CAPACITY * sizeof(T)];
Copy link
Contributor

Choose a reason for hiding this comment

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

if you wrap this inside a struct and declare one like :

struct FixedVectorInner
{
     uint32_t _size = 0;
     alignas(T) uint8_t _data[CAPACITY * sizeof(T)];
};
FixedVectorInner inner;

Then you don't need the DATA_PADDING information as you can just memcpy with adress of inner and sizeof(inner). Just a remark.

Copy link
Contributor Author

@Ivorforce Ivorforce Mar 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hrm, that's true. I do think the DATA_PADDING thing is a bit weird.
Actually, thinking about your suggestion, it should just be possible to use memcpy with sizeof(other)! That should do the exact same thing.
Edit: Oh, actually, the reason both aren't possible is because we only want to copy the elements that are actually constructed right now, not the whole buffer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well you still can with memcpySize = &(inner.data + size * sizeof(T)) - &inner but that's not really better than the current one, agreed.

Copy link
Member

@lawnjelly lawnjelly Mar 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I suspect another option might be to put the _size inside a union and calculate the size of the union from the T alignment. Something like that. Maybe there would be some spanner in the works which prevents it...

union
{
uint32_t size = 0;
uint8_t spacer[MIN_SIZE_SIZE];
};

Realistically in most cases it's more likely to just be have the size section use 8 bytes as I guess 8 byte aligned may be slightly faster on 64 bit CPU.

I could possibly do that on FixedArray but I can't remember what I was storing in it, and I wasn't going for that level of micro-optimization. (It may even be possible to backport this and replace my use there just so we don't have duplicate containers.)

I suppose you could want e.g. T 64 byte aligned to line up with cache lines, something like that, and SIMD sometimes has more alignment requirements. I suppose it depends how much more complex it makes the code to read versus something more basic at this stage.

Copy link
Contributor Author

@Ivorforce Ivorforce Mar 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That's also an interesting idea! C++ sure can do the same thing in so many different ways 😄
I think for now I still like the data padding constant best, because it's only awkward in its construction but looks pretty normal elsewhere.


constexpr static uint32_t DATA_PADDING = MAX(alignof(T), alignof(uint32_t)) - alignof(uint32_t);

public:
_FORCE_INLINE_ constexpr FixedVector() = default;
constexpr FixedVector(std::initializer_list<T> p_init) {
ERR_FAIL_COND(p_init.size() > CAPACITY);
for (const T &element : p_init) {
memnew_placement(ptr() + _size++, T(element));
}
}

template <uint32_t p_capacity, bool p_force_trivial = false>
constexpr FixedVector(const FixedVector<T, p_capacity, p_force_trivial> &p_from) {
ERR_FAIL_COND(p_from.size() > CAPACITY);
if constexpr (std::is_trivially_copyable_v<T>) {
// Copy size and all provided elements at once.
memcpy((void *)&_size, (void *)&p_from._size, sizeof(_size) + DATA_PADDING + p_from.size() * sizeof(T));
} else {
for (const T &element : p_from) {
memnew_placement(ptr() + _size++, T(element));
}
}
}

template <uint32_t p_capacity, bool p_force_trivial = false>
constexpr FixedVector(FixedVector<T, p_capacity, p_force_trivial> &&p_from) {
ERR_FAIL_COND(p_from.size() > CAPACITY);
// Copy size and all provided elements at once.
// Note: Assumes trivial relocatability.
memcpy((void *)&_size, (void *)&p_from._size, sizeof(_size) + DATA_PADDING + p_from.size() * sizeof(T));
p_from._size = 0;
}

~FixedVector() {
if constexpr (!std::is_trivially_destructible_v<T>) {
for (uint32_t i = 0; i < _size; i++) {
ptr()[i].~T();
}
}
}

_FORCE_INLINE_ constexpr T *ptr() { return (T *)(_data); }
_FORCE_INLINE_ constexpr const T *ptr() const { return (const T *)(_data); }

_FORCE_INLINE_ constexpr operator Span<T>() const { return Span<T>(ptr(), size()); }
_FORCE_INLINE_ constexpr Span<T> span() const { return operator Span<T>(); }

_FORCE_INLINE_ constexpr uint32_t size() const { return _size; }
_FORCE_INLINE_ constexpr bool is_empty() const { return !_size; }
_FORCE_INLINE_ constexpr bool is_full() const { return _size == CAPACITY; }
_FORCE_INLINE_ constexpr uint32_t capacity() const { return CAPACITY; }

_FORCE_INLINE_ constexpr void clear() { resize(0); }

template <bool p_ensure_zero = false>
constexpr Error resize(uint32_t p_size) {
if (!force_trivial && p_size > _size) {
ERR_FAIL_COND_V(p_size > CAPACITY, ERR_OUT_OF_MEMORY);
memnew_arr_placement<p_ensure_zero>(ptr() + _size, p_size - _size);
} else if (p_size < _size) {
if constexpr (!std::is_trivially_destructible_v<T>) {
for (uint32_t i = p_size; i < _size; i++) {
ptr()[i].~T();
}
}
}

_size = p_size;
return OK;
}

constexpr void push_back(const T &p_val) {
ERR_FAIL_COND(_size >= CAPACITY);
memnew_placement(ptr() + _size, T(p_val));
_size++;
}

constexpr void pop_back() {
ERR_FAIL_COND(_size == 0);
_size--;
ptr()[_size].~T();
Copy link
Contributor

Choose a reason for hiding this comment

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

no if constexpr (!std::is_trivially_destructible_v<T>) here ?

Copy link
Contributor Author

@Ivorforce Ivorforce Mar 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think it just gets compiled away for trivial types:

https://godbolt.org/z/71oWoY19P

I think technically we should expect the compiler to optimize away the loop cases as well, but the reason I'm checking there is to make it easier for the compiler / not to risk it leaving in the loop for whatever reason.

}

// NOTE: Subscripts sanity check the bounds to avoid undefined behavior.
// This is slower than direct buffer access and can prevent autovectorization.
// If the bounds are known, use ptr() subscript instead.
constexpr const T &operator[](uint32_t p_index) const {
CRASH_COND(p_index >= _size);
return ptr()[p_index];
}

constexpr T &operator[](uint32_t p_index) {
CRASH_COND(p_index >= _size);
return ptr()[p_index];
}

_FORCE_INLINE_ constexpr T *begin() { return ptr(); }
_FORCE_INLINE_ constexpr T *end() { return ptr() + _size; }

_FORCE_INLINE_ constexpr const T *begin() const { return ptr(); }
_FORCE_INLINE_ constexpr const T *end() const { return ptr() + _size; }
};
85 changes: 85 additions & 0 deletions tests/core/templates/test_fixed_vector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
/**************************************************************************/
/* test_fixed_vector.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. */
/**************************************************************************/

#pragma once

#include "core/templates/fixed_vector.h"

#include "tests/test_macros.h"

namespace TestFixedVector {

TEST_CASE("[FixedVector] Basic Checks") {
Copy link
Member

Choose a reason for hiding this comment

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

I'd also add a test for force_trivial being set to true. It can be a copy of an existing test that uses a trivially destructible type, just to make sure it works correctly.

Copy link
Contributor Author

@Ivorforce Ivorforce Mar 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You mean check that it works in general, or that it works as force_trivial? For the latter I'd have no idea how!
It's a good idea to check it's not broken though I agree, perhaps I'll just repurpose one of the test cases below. It's redundant enough as is.

Edit: Done, let me know if that works for you :)

FixedVector<uint16_t, 1> vector;
CHECK_EQ(vector.capacity(), 1);

CHECK_EQ(vector.size(), 0);
CHECK(vector.is_empty());
CHECK(!vector.is_full());

vector.push_back(5);
CHECK_EQ(vector.size(), 1);
CHECK_EQ(vector[0], 5);
CHECK_EQ(vector.ptr()[0], 5);
CHECK(!vector.is_empty());
CHECK(vector.is_full());

vector.pop_back();
CHECK_EQ(vector.size(), 0);
CHECK(vector.is_empty());
CHECK(!vector.is_full());

FixedVector<uint16_t, 2, true> vector1 = { 1, 2 };
CHECK_EQ(vector1.capacity(), 2);
CHECK_EQ(vector1.size(), 2);
CHECK_EQ(vector1[0], 1);
CHECK_EQ(vector1[1], 2);

FixedVector<uint16_t, 3> vector2(vector1);
CHECK_EQ(vector2.capacity(), 3);
CHECK_EQ(vector2.size(), 2);
CHECK_EQ(vector2[0], 1);
CHECK_EQ(vector2[1], 2);

FixedVector<Variant, 3> vector_variant;
CHECK_EQ(vector_variant[0], Variant());
CHECK_EQ(vector_variant[1], Variant());
CHECK_EQ(vector_variant[2], Variant());
vector_variant.resize(3);
vector_variant[0] = "Test";
vector_variant[1] = 1;
CHECK_EQ(vector_variant.capacity(), 3);
CHECK_EQ(vector_variant.size(), 3);
CHECK_EQ(vector_variant[0], "Test");
CHECK_EQ(vector_variant[1], Variant(1));
CHECK_EQ(vector_variant[2].get_type(), Variant::NIL);
}

} //namespace TestFixedVector
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 @@ -94,6 +94,7 @@
#include "tests/core/string/test_translation_server.h"
#include "tests/core/templates/test_a_hash_map.h"
#include "tests/core/templates/test_command_queue.h"
#include "tests/core/templates/test_fixed_vector.h"
#include "tests/core/templates/test_hash_map.h"
#include "tests/core/templates/test_hash_set.h"
#include "tests/core/templates/test_list.h"
Expand Down
Loading