Optimize / refactor CowData, combining resize and fork to avoid unnecessary reallocations.

[Ivorforce]

This PR optimizes CowData resize operations. This affects all non-trivial resizes for String and Vector (e.g. Packed arrays), including concatenation.

This PR includes #100694, because without it, leaks are occurring due to cyclic references that the previous implementation luckily avoided through unnecessary forking.
This PR also supersedes #100672.

Reasoning

The current implementation of resize first forks if there are multiple owners, and then resizes the freshly forked array.
In the case that the needed array is larger than the previous array, time is wasted because a new location may have to be found for the larger buffer on the subsequent realloc, and the data has to be copied over again.
In the case that the needed array is smaller than the previous array, time is wasted looking for a larger buffer than necessary, and constructing (and subsequently destructing) more elements than necessary.

Benchmark

I've benchmarked plain String concatenation through +, which can be 1.6x as fast as before. The PR is likely to affect and optimize many more functions involving String or Vector.

	String s1 = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.";
	String s2 = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.";

	auto t0 = std::chrono::high_resolution_clock::now();
	for (int i = 0; i < 10000000; i ++) {
		// Test
		String s3 = s1 + s2;
	}
	auto t1 = std::chrono::high_resolution_clock::now();
	std::cout << std::chrono::duration_cast<std::chrono::milliseconds>(t1 - t0).count() << "ms\n";

This yields:

• 1349ms on master (8483d79 / ba2c5c1)
• 825ms on this PR (77d8900235a8b0fe7e3b597d4f664533f60ee82e)

[Ivorforce]

Hrm. I'm kinda stumped as to what's leaking. I think it's foolproof, in any case foolproof if the CowData itself is not accessed from other threads (though access of refcount should be safe cross-thread). And besides, the previous implementation was also not safe from forking a 1-ref'd CowData from another thread, I think (through realloc). Either I'm missing some odd edge case or race condition, or something fishy is going on.

[Ivorforce]

Oh my god... This leak is costing me and arm and a leg to debug. Here is a failing log for reference: https://github.com/godotengine/godot/actions/runs/12435300159/job/34720869143

I have finally tracked it down to the GDScript tests only. Normal operation does not appear to cause leaks. At this point, I am reasonably sure it's not my implementation that's causing it, but some kind of circular reference. Though it's hard to be completely certain.
If it is a circular reference, the current implementation of CowData appears to 'avoid' it somehow, but it's really hard to track down and I have, so far, not been able to re-create the bug / fix in my refactor by attempting to recreate CowData behavior in that class.

Running the unit tests locally without out_of_order_external.gd avoids the leak. Online, I had to disable all GDScript unit tests, because more than just one were causing the leak. But disabling them does avoid any leaks from appearing: https://github.com/Ivorforce/godot/actions/runs/12438440843

[hpvb]

I'll look at this PR over the weekend. I have a suspicion but it'll take some single stepping, and Friday night is not the time for single stepping through CowData.

[Ivorforce]

I have an update. Turns out one of the 'preconditions' I was imagining to be true was false all along - refcount can be 0. Who knew.

This is true on master as well, as this fails unit tests: master...Ivorforce:godot:cowdata-refcount-0

After meticulously re-creating my PR step by step, I'm pretty sure that's what's causing the issues. I don't know how it's possible that refcount is 0 at any time. At least in this PR, it's initialized with 1 and decremented and incremented in very controlled ways, and I recently checked nobody else is interfering. This is a really strange one...

[Ivorforce]

I think I finally fixed it.

So, GDScript does have cyclic references, and it calls a Vector destructor, and then halfway through the destruction, the array is referenced again through some pointer. We should probably figure out that cyclic reference, because it really smells. But it's out of scope for me to address it (for posterity, involved functions are logged here - one is constants in GDScriptFunction).

Instead, I managed to handle it more graciously through #100694. With the change, CowData invalidates itself before destructing the array, and graciously handles being referenced, reallocated, or re-deallocated halfway through its own destruction. We'll see if the remote leak sanitizers agree, but this fixed the leak on my end.

[hpvb]

@Ivorforce after the fix in cowdata, has this been updated?

[Ivorforce]

@Ivorforce after the fix in cowdata, has this been updated?

You mean #100694? Yep, I've integrated the changes and tested it's still valid!

[kiroxas]

So if we cannot alloc, this instance becomes null, and the old data gets destructed. Was this the old behaviour too ? (just making sure we do not have a subtle behaviour difference, because this branch should not get called much)

[Ivorforce]

Good observation! I remember thinking a lot about this problem too.

The old behavior was quite curious:
It would first try to allocate an array with the same size as the old one through _copy_on_write.
If that failed, it would ignore the error and continue to reallocate the old array if needed, potentially invalidating the old pointer (which would likely lead to a crash later).
If it succeeded, it would try to reallocate the fresh pointer to a new size. If that failed, it would keep the old freshly created backing array of the wrong size, and return to the caller (who would most likely ignore the failed resize and subsequently SEGFAULT).

What I'm doing instead is to let the old data go and resize to size 0. What follows is one of two things:
Either the caller realizes the resize failed, and will either crash or report back to the user (who will likely crash soon too, because who checks the output of resize?). I think it's unlikely that a resize rather than a new allocation will be out of memory, but still handleable on failure.
Or the caller doesn't realize the resize failed, and will immediately SEGFAULT because of out of bounds access.

It's pretty impossible to restore the old behavior, but I don't think we want to.
There is definitely an argument to be made though to try to recover / keep the old data though. I thought it might be better to encourage early failure by letting the old data go and not attempting to recover it. But I'm not married to the idea!

[kiroxas]

We probably don't want the old behaviour, no. To be honest I like how you handle it right now, it make it clear that there was an error. It would maybe be better to add a comment to make clear it's on purpose and not some oversight.

[kiroxas]

would have stick with (void *)(_ptr + prev_size) but that's style, so both will do

[Ivorforce]

I don't feel strongly about this, IIRC i adjusted this to be on-par with other lines using the [] syntax.

[kiroxas]

could we add an assert for that for debug builds ?

[Ivorforce]

Good idea!

[Ivorforce]

Seems like there's nothing like DEBUG_ASSERT, so I think DEV_ASSERT will be good enough?

[kiroxas]

probably yes