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

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

Conversation

Ivorforce
Copy link
Contributor

@Ivorforce Ivorforce commented Mar 12, 2025

This is a high performance Vector-like object. In practice, we can use it to reduce the number of per-frame allocations throughout Godot.

In particular, FixedVector is useful if the maximum number of objects is small and known, and the objects are needed only temporarily. It could also be used to return a small, uncertain number of objects from a function in a performant fashion if the maximum is known.

The class can be expanded with more useful functions in the future as needed.

Comparisons

FixedVector is designed to be as easily exchangeable for Vector and LocalVector as possible. This can be most easily achieved through Span interfaces.

FixedVector is similar to 3.x' FixedArray, but some decisions about its design are different.
FixedVector is similar to std::array, but it supports adding and removing objects, and a number of other goodies (like conversion to Span).
FixedVector is somewhat similar to StringBuffer, but focuses on being a general purpose collection and does not support arbitrary expansion into the heap.

@Ivorforce Ivorforce requested review from a team as code owners March 12, 2025 22:05
@Ivorforce Ivorforce changed the title Add FixedVector template. Add FixedVector template to core - a collection that can be used completely on the stack. Mar 12, 2025
@Repiteo Repiteo added this to the 4.x milestone Mar 12, 2025
@ze2j
Copy link
Contributor

ze2j commented Mar 12, 2025

Love it! Very useful in many cases (like to store the neighbors of a cell for example).

StaticVector is usualy the name given to this kind of fixed capacity container. Why not using that name ?

@Ivorforce Ivorforce force-pushed the fixed-vector branch 2 times, most recently from fc10d68 to 1df6129 Compare March 12, 2025 23:18
@Ivorforce
Copy link
Contributor Author

Ivorforce commented Mar 12, 2025

Love it! Very useful in many cases (like to store the neighbors of a cell for example).

StaticVector is usualy the name given to this kind of fixed capacity container. Why not using that name ?

I've seen Fixed- used as a prefix for collections where the size is chosen at compile time — hence I chose the name FixedVector.
I've not seen Static used for this concept. Can you quote some examples?

@ze2j
Copy link
Contributor

ze2j commented Mar 12, 2025

Boost static_vector for example

@ze2j
Copy link
Contributor

ze2j commented Mar 12, 2025

Could you explain what's the rationale for using placement new instead of a relying on a std::array<T, CAPACITY> ? I don't get it. Is to pack the data more efficiently ?

@Ivorforce
Copy link
Contributor Author

Ivorforce commented Mar 12, 2025

Boost static_vector for example

That's a good example. I'd like to hear what others think of this alternate name; I don't have a strong preference.

Could you explain what's the rationale for using placement new instead of a relying on a std::array<T, CAPACITY> ? I don't get it. Is to pack the data more efficiently ?

Of course! The reason is performance.

Basically, using std::array<T, CAPACITY> will automatically populate the entire array with elements upon construction. For trivial types, like int, this is unimportant, and makes no difference.

For non-trivial types though, like Variant, this would necessitate the whole array to initialize with an initial value (NULL). In addition, pushing new objects requires assigning over the old one, which involves branches that account for possible old data. Finally, destruction requires running over all data, even that which wasn't actually used. All waste time.
By using a raw data array, we can skip these computations for improved efficiency.

@Repiteo
Copy link
Contributor

Repiteo commented Mar 13, 2025

No preference on the name—both convey the same general idea

@lawnjelly
Copy link
Member

lawnjelly commented Mar 13, 2025

imo something other than static ('Fixed' or any better word we can come up with) is probably better in the name (as I did in 3.x for FixedArray) than static as the lifetime usually isn't static.

It can either be global/static, or more commonly on the stack. One alternative I suggested in RC was StackVector, but for the same reason I wasn't super keen on this, as the lifetime can also be static (although I doubt we'll use this much in Godot). Come to think of it, a FixedVector could even be allocated on the heap, so there's that. 😁

This disambiguation is an issue because although we have the basic classification of static (unchanging) & dynamic (changing), in c++ the word (and keyword) static is used to refer to objects that have a lifecycle for the entire app.

@lawnjelly
Copy link
Member

lawnjelly commented Mar 13, 2025

Just to throw an idea, I know people love STL, but I go by the philosophy of if a routine in a template (or a header in general) can fail, and it returns void, return a bool instead for success or failure. This will be compiled out if you don't use it, and provides extra info if you want it.

e.g. bool push_back(..):

if (vec.push_back(1))
{
...
}
else
{
// full
}

versus

if (!vec.is_full())
{
vec.push_back(1);
}
else
{
// full
}

I guess the second branch in the push_back will be compiled out, but I like the emphasis that this can fail. Same with pop_back().

Incidentally, Grok tells me part of the reason STL pop_back() doesn't return success or fail is because it doesn't check for failure, it is UB if you call it on an empty vector. Presumably same for push_back().

This of course is an issue with all our containers as we tend to favour "safe" versions.

In C++, the pop_back() function for containers like std::vector, std::deque, or std::list doesn't return a success or failure indicator because its design assumes a specific contract: it only operates on a non-empty container, and if that contract is met, it will always succeed. This is a deliberate choice rooted in C++'s philosophy of performance, simplicity, and exception handling.

Here’s why:

Precondition-Based Design:

pop_back() is defined with the precondition that the container must not be empty. If you call it on an empty container, it results in undefined behavior (UB) rather than a guaranteed failure state. This avoids the overhead of runtime checks (like returning a success/failure flag) in the common case where the programmer ensures the container isn’t empty. C++ prioritizes "don’t pay for what you don’t use"—if you want safety, you’re expected to check empty() or size() beforehand.

Performance Optimization:

Adding a return value (e.g., a bool for success/failure) would introduce additional complexity and potentially cost CPU cycles for something that’s typically unnecessary in well-written code. Since pop_back() is a simple operation (removing the last element and adjusting the size), the standard library avoids burdening it with extra bookkeeping.

Error Handling via Exceptions or Assertions:

C++ prefers mechanisms like exceptions or debug-mode assertions over return codes for error handling. For example, some debug implementations of the standard library (e.g., libstdc++ or MSVC) will trigger an assertion failure if you call pop_back() on an empty container, helping catch bugs during development. In release mode, it’s up to the programmer to avoid UB, aligning with C++’s trust in the developer.

Historical Context and Consistency:

The STL (Standard Template Library), which influenced C++’s standard containers, was designed by Alexander Stepanov with a focus on generic programming and minimalism. Functions like pop_back() were kept lean and consistent with other operations (e.g., push_back() doesn’t return success either—it either works or throws an exception on allocation failure). Adding a success/failure return would deviate from this established pattern.

Alternative: You Can Check Yourself:

If you need to verify whether pop_back() is safe, you can explicitly check:
cpp

if (!myVector.empty()) {
    myVector.pop_back();
    // Success is guaranteed here
} else {
    // Handle empty case
}

This puts the responsibility on the programmer, which is typical in C++’s design.

Contrast this with languages like Java or Python, where methods often return status indicators or throw exceptions for invalid operations. In C++, the choice to omit a success/failure return from pop_back() reflects its low-level, performance-driven ethos. If you really want a "safe" version, you could wrap it in a custom function:
cpp

bool safe_pop_back(std::vector<int>& vec) {
    if (vec.empty()) return false;
    vec.pop_back();
    return true;
}

But the standard library doesn’t include this because it assumes you’ll manage the preconditions yourself. Thoughts on this design choice? Does it align with how you approach C++ coding?

@Ivorforce
Copy link
Contributor Author

Ivorforce commented Mar 13, 2025

Just to throw an idea, I know people love STL, but I go by the philosophy of if a routine in a template (or a header in general) can fail, and it returns void, return a bool instead for success or failure. This will be compiled out if you don't use it, and provides extra info if you want it.

This would definitely be a valid way to handle this!
My current thinking was that if you push to a FixedVector that's lacking space, you probably have a bug on your hand because you didn't make it large enough statically. So a failure would be most appropriate. But I could also see FixedVector being used in scenarios where a push against a full vector is reasonable behavior.

As for pop_back(), I also considered returning the popped T, as Rust likes to do. But we don't do this in the codebase elsewhere and it's admittedly less useful than in Rust, which likes to optimize object lifetimes as much as possible. I could also see popping from an empty FixedVector returning bool (I think I saw that in a library or two as well?), but I think it might more often be a bug we shouldn't ignore, especially in the stack case.

@lawnjelly
Copy link
Member

As for pop_back(), I also considered returning the popped T, as Rust likes to do.

That's also something I've considered in various forms in the past, although returning a T means you have no good options in the case of popping from an empty vector (except crashing or returning T()).

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

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.

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

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

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

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.

This is a high performance `Vector`-like object that can be used if the maximum number of objects is small and known, and the objects are needed only temporarily.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants