Add size tracking to CowData, make it never reallocate when shrinking.

[profan]

Motivation

Strings/Vectors/etc in GDScript and the core engine alike use CowData as the backing structure, today this structure reallocates its memory on nearly every operation.
(push_back, remove, etc, see: #22561, #24731).

Luckily this is possible to improve without changing any existing Godot code, while making it easier for people to reuse memory in vectors and reduce the amount of necessary overall allocations.

Usecases in Godot

Currently structures like Godot's A* use Vector for the open list (where a significant improvement can be seen with this change), even the scene tree itself uses vector as the datastructure in nodes to hold children and today incurs a reallocation every time an element is added or removed, so some improvement should be seen in these case (likely not as significant as for something like a stack/queue type usecase, but nonetheless).

Changes

• Adds tracking of the size to the CowData structure itself.
• Adds a capacity method to the public interface of Vector and CowData.
• Fixes inserting elements from within a CowData's own data to itself (per issue surfaced in Line2D UV texture glitch #16961).
• Changes growth rate to a factor of p_size * 1.618 (also makes growth rate work, per Arrays and vectors always resize after a push or a remove #22561).
• clear now only resets size, does not deallocate memory to accomodate reuse.
• push_back/remove etc do not resize unless capacity is exceeded, shrinking never allocates.

Discussion

So this change might be slightly controversial but I'd been sitting on this for a while so I figured I'd float this because it doesn't break any existing code and from my own testing does not increase memory usage in any significant way.

(there's probably a bunch in this code that needs looking at by contributors familiar with core)

Testing

I've tested this on Godot proper as well as a bunch of my own projects and it seems to be operating just fine, given that deploying it in all of Godot is a pretty good benchmark for if it's sane or not, but having more experienced contributors take a look would be much appreciated.

Future Additions

Most vector implementations also have a way to reserve memory ahead of time without also resizing to that size, so adding a reserve method to CowData and Vector is a logical next step if this change is accepted.

Also adding a shrink_to_fit method to shrink to the current logical size would also make sense for cases where you do want it to shrink, even if I think this should not be the default as it is currently.

Probably exposing these in GDScript/GDNative would also be of interest, capacity, reserve, etc.

[YeldhamDev]

Travis is complaining about an uninitialized variable:

./core/cowdata.h: In member function 'void AnimationTree::_update_properties_for_node(const String&, Ref<AnimationNode>)':
./core/cowdata.h:208:6: error: '*((void*)& a +8)' may be used uninitialized in this function [-Werror=maybe-uninitialized]
  208 |    T val = p_val;
      |      ^~~

[lawnjelly]

Am I right in saying the old push_back was more efficient? You changed it to use insert, which mostly does the same but it includes the for loop which is unnecessary if you know it will always be adding to the end.

Also _get_size is doing:

if (!_ptr)

Is this necessary? Would it be possible just to ensure _size is set to zero, thus removing a redundant if check?

In _get_data() you have:

	_FORCE_INLINE_ T *_get_data() const {

		if (!_ptr)
			return NULL;
		return reinterpret_cast<T *>(_ptr);
	}

If _ptr is NULL surely you can just return it, is there a need for an if check?

[MunWolf]

Am I right in saying the old push_back was more efficient? You changed it to use insert, which mostly does the same but it includes the for loop which is unnecessary if you know it will always be adding to the end.

Also _get_size is doing:

if (!_ptr)

Is this necessary? Would it be possible just to ensure _size is set to zero, thus removing a redundant if check?

In _get_data() you have:

	_FORCE_INLINE_ T *_get_data() const {

		if (!_ptr)
			return NULL;
		return reinterpret_cast<T *>(_ptr);
	}

If _ptr is NULL surely you can just return it, is there a need for an if check?

The only extra performance hit that will happen here is an extra check for if we are adding something that is already in the vector, which fixes some issues and one i > p_pos check. So the only unneeded extra would be the latter check which doesn't really add a noticeable hit to the performance.

[profan]

Am I right in saying the old push_back was more efficient? You changed it to use insert, which mostly does the same but it includes the for loop which is unnecessary if you know it will always be adding to the end.

Also _get_size is doing:

if (!_ptr)

Is this necessary? Would it be possible just to ensure _size is set to zero, thus removing a redundant if check?

In _get_data() you have:

	_FORCE_INLINE_ T *_get_data() const {

		if (!_ptr)
			return NULL;
		return reinterpret_cast<T *>(_ptr);
	}

If _ptr is NULL surely you can just return it, is there a need for an if check?

The push_back change was mostly a simplification but I could certainly include a specific one inside CowData, just duplicating the piece that checks for adding from within itself (as insert does now).

The null check is probably superstitious yes, but that's old code either way.

[profan]

Travis is complaining about an uninitialized variable:

./core/cowdata.h: In member function 'void AnimationTree::_update_properties_for_node(const String&, Ref<AnimationNode>)':
./core/cowdata.h:208:6: error: '*((void*)& a +8)' may be used uninitialized in this function [-Werror=maybe-uninitialized]
  208 |    T val = p_val;
      |      ^~~

Indeed I noticed that one too.. I think this may be something that was uninitialized before (two places I could see in CI?) but the compiler didn't manage to catch it until now, so I'm taking a look at that. (curious that it's showing up in the headless build but not the normal editor build though? .. seems to be showing up locally though so digging some more)

[profan]

So a little digging later, two places came up, I made a separate PR to deal with them: #31716
(rebased back onto master, so CI has run with this PR + the changes that fixed the two places of unitialized variables)

[lawnjelly]

Is it possible we can fix this bug #31736 at the same time with this PR?

Also I think you should definitely put in shrink_to_fit or something similar as part of the PR, as otherwise users will have no control to cleanup their memory after use (aside from deleting the vector etc, which may not be desired).

[profan]

Is it possible we can fix this bug #31736 at the same time with this PR?

Also I think you should definitely put in shrink_to_fit or something similar as part of the PR, as otherwise users will have no control to cleanup their memory after use (aside from deleting the vector etc, which may not be desired).

Yes, rather this PR already solves the issue (if you read above, and the changes) by taking approach Nr. 3 outlined in the issue you made, adding Nr. 4 (copy only when a resize might actually occur) is also possible.

I thought about doing Nr. 1 but the CoW machinery makes this a little more messy.

As for shrink_to_fit or similar, I'm waiting for word on if this whole PR will actually be accepted or not, I created this PR knowing reduz might very well just straight up reject it, so I don't want to put more work in before I know if it is in the clear or not :)

[profan]

Will need to be rebased in the face of #32181 which should definitely be merged first..

[aaronfranke]

@profan Is this still desired? If so, it needs to be rebased on the latest master branch.

[Zireael07]

Still desired, considering it's a perf improvement.

[profan]

Fairly sure this is safe to abandon as reduz implemented a limited version of this that still reallocates every time the cowdata shrinks and without capacity but at least only reallocates on every power of two increase in size afaik (and I think the reassignment to self issue is also solved?) so I will close this as I think it's also unlikely to ever get accepted even if it was amended 👀

(though I think clear still reallocates also.. which is still daft I think, but alas)