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 size tracking to CowData, make it never reallocate when shrinking. #31694

Closed
wants to merge 3 commits into from

Conversation

profan
Copy link
Contributor

@profan profan commented Aug 27, 2019

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
Copy link
Member

YeldhamDev commented Aug 27, 2019

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;
      |      ^~~

@akien-mga akien-mga added this to the 3.2 milestone Aug 27, 2019
@akien-mga akien-mga requested a review from reduz August 27, 2019 06:19
Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

MunWolf commented Aug 27, 2019

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
Copy link
Contributor Author

profan commented Aug 27, 2019

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
Copy link
Contributor Author

profan commented Aug 27, 2019

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
Copy link
Contributor Author

profan commented Aug 27, 2019

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)

akien-mga added a commit that referenced this pull request Aug 27, 2019
Fix otherwise unitialized variables, found in #31694
* adds a capacity method to the public interface of vector and cowdata.
* adds tracking of the size to the cowdata structure itself.
* fixes inserting elements from within a cowdata's own data to itself.
* clear now only resets size, does not deallocate memory to accomodate reuse.
* changes growth rate to a factor of p_size * 1.618 (also makes growth rate actually work)
@lawnjelly
Copy link
Member

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
Copy link
Contributor Author

profan commented Aug 28, 2019

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
Copy link
Contributor Author

profan commented Sep 21, 2019

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

pchasco pushed a commit to pchasco/godot that referenced this pull request Oct 23, 2019
@akien-mga akien-mga removed this from the 3.2 milestone Nov 7, 2019
@akien-mga akien-mga added this to the 4.0 milestone Nov 7, 2019
akien-mga pushed a commit to akien-mga/godot that referenced this pull request Nov 8, 2019
@aaronfranke
Copy link
Member

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

@Zireael07
Copy link
Contributor

Still desired, considering it's a perf improvement.

@profan
Copy link
Contributor Author

profan commented Jul 10, 2020

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)

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.

7 participants