Avoid creating most objects that Godot is going to use placement new to initialize (mark 2) #1379
Conversation
dsnopek
force-pushed
the
uninitialized-value-mk2
branch
from
6870ac2
to
f22dcc8
Compare
|
I think the array is being corrupted with the current state of this PR... Here's my test code: void GDExample::test_memory_behaviour() {
UtilityFunctions::print("GDExtension::test_memory_behaviour()...");
{
Array original_array;
original_array.push_back(123);
{
Variant my_variant = original_array;
{
Array my_array = my_variant;
int value = my_array[0]; // This line causes an exception.
UtilityFunctions::print("value: ", value);
}
}
}
UtilityFunctions::print("GDExtension::test_memory_behaviour() COMPLETE");
}
void GDExample::test_memory_behaviour_multiple() {
for (int i = 0; i < 1000; i++) {
test_memory_behaviour();
}
}With that, I get this exception when it hits the |
dsnopek
force-pushed
the
uninitialized-value-mk2
branch
from
f22dcc8
to
7d82c48
Compare
|
Ack, I made a really dumb mistake (fixed in my last push)! Sorry for wasting your effort on testing that. I'll do some testing of my own with your test code. |
|
@allenwp I just tested with the code you shared above, and in my latest version I'm not seeing the corruption or the memory leak! When I have a chance, I'm going to expand this PR to address all Variant types and take it out of draft. |
|
Oh, yeah, at first glance, this looks 1000x better than the other PR. Nicely done! Thanks! I've tried testing this with my boiled down minimum reproduction code as well as my personal project where I first noticed the leak: Both of these were fixed by this PR draft! |
dsnopek
changed the title
dsnopek
changed the title
|
I applied these changes to my project, and the memory leak disappeared. My benchmark, which usually leaks ~4 GB / min, shows a steady 183MiB static memory throughout. |
dsnopek
force-pushed
the
uninitialized-value-mk2
branch
2 times, most recently
from
4ff8785
to
50f7123
Compare
dsnopek
changed the title
|
@cyberpuffin-digital Thanks for the review and testing! I've updated this PR to cover most variant types: definitely all the ones that use reference counting and could leak, plus a couple others which were easy to do. So, I've taken this PR out of draft! There's a few types where we could avoid initializing them to their defaults, such as |
Maybe add a comment describing this? If any future issues come up, it would be good to easily see that there is no strong intention for the difference between types that use the new generated constructor vs. the "old" way of creating a temporary variable and calling |
dsnopek
force-pushed
the
uninitialized-value-mk2
branch
from
50f7123
to
c4fde85
Compare
Makes sense! I've added a |
dsnopek
changed the title
Thanks! That leaves only the following untouched, which all have slight differences compared to the other operators which have now been converted or had the bool
int64_t
double
Object *
ObjectID |
|
Yep, those ones should stay the way they are, because they are already not initializing their values. |
|
I've been using this PR in my fork of godot-cpp for a week or so and haven't discovered any instabilities with it. |
| @@ -145,6 +143,7 @@ class Variant { | |||
| static GDExtensionTypeFromVariantConstructorFunc to_type_constructor[VARIANT_MAX]; | |||
|
|
|||
| public: | |||
| _FORCE_INLINE_ GDExtensionVariantPtr _native_ptr() const { return const_cast<uint8_t(*)[GODOT_CPP_VARIANT_SIZE]>(&opaque); } | |||
There was a problem hiding this comment.
Should we keep the underscore here?
There was a problem hiding this comment.
All the other variant types have a public _native_ptr() method, so this would match them. For some reason this one was private instead of public. In any case, it's not really part of the public API (since this method isn't in Godot), but we need these to be public for technical reasons.
| } | ||
|
|
||
| Variant::operator Vector2() const { | ||
| // @todo Avoid initializing result before calling constructor (which will initialize it again) |
There was a problem hiding this comment.
It is possible, but we decided to postpone it to later.
This PR fixes the memory leak. Avoiding initializing those other results would be a slight performance improvement, but those aren't leaking memory, so fixing them isn't as urgent. The @todo's are there to remind us to fix it eventually, though :-)
|
Thanks! |
|
Is this somehow also fixed for other branches/tags, or is it just master? |
|
This is marked to be cherry-picked to 4.2 and 4.1, but it hasn't been done yet |
|
Cherry-picked for 4.1 in PR #1411 |
|
Cherry-picked for 4.2 in PR #1410 |
This a new idea for a way to fix issue #1240
It's a less hacky alternative to PR #1378
Rather than messing around with blobs of memory, and trying to cleverly avoid calling object constructors, this just adds a new private constructor that does what we want. :-)
The code is generating for all the variant types, but I've only updatedArrayandDictionaryto use the new constructors.