-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
[Core] Fix COW allocates too much memory #91286
base: master
Are you sure you want to change the base?
[Core] Fix COW allocates too much memory #91286
Conversation
Did you mean 128? I'm not sure about this, the power of two is expected to be on the allocated memory, otherwise it doesn't matter, it's relevant that the allocation is a power of two It doesn't make any sense to me to allocate for a power of two number of elements, so either it should be a power of two bytes, or nothing at all |
There is a lot of memory that is simply not used. Like, why allocate 32 bytes when you can allocate 20. For 2 elements, it will be 48, although there is 40 real data there. |
Well it depends on what we're trying to achieve here, often allocating a power of two memory is desirable to the OS, as far as I know, I don't think the use of this particular formula here was in any way an oversight by the implementer but desired |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above issues with _get_alloc_size_checked
need to be resolved, needs to either be reverted or confirmed that it does not reintroduce:
The other details are for discussion but this part is breaking and reverting important safety checks
I checked the project, from issue. Godot didn't crash, but my computer did. I don't know if this is the right behavior. |
Did you try with the check restored? |
I opened the file |
The issue isn't fully solved for that side, but this check should probably not be removed, it's not unnecessary, so unless you can confirm it's unnecessary it should be restored IMO, the zero check can be removed but not the |
If you change from |
And this zero check with I also took a closer look at your PR and noticed that before COW was 32-bit. This explains the overflow.
if (sizeof(USize) == 8) {
x |= x >> 32;
} This fixed the problem. |
See this explaination by Juan: https://youtu.be/BvpBEIe86CE?t=1630 |
I just realized today that we don't allocate
These are the expansion ranges before: 48 80 144 272 528 1040. This shows that the allocation is in no way similar to 2^n.
From the video, I understand the doubling of the size, not that it should be strictly 2^n. And also in other implementations of the vector, for example, |
What's the real memory usage difference, not the one shown by Godot? Underlying |
I've done my due diligence here, I'm not convinced this won't reintroduce the crash, but if you're convinced it'll be safe and other reviewers agree I'll wash my hands of it
My 173.3 MB It looks like a much smaller difference. I'm thinking about it now. For example, we have 1024 bytes of data and another 16 bytes of metadata(ref_couter + size). And as I understand it, |
This makes sense to take metadata into account. For small allocs it actually might have more options than powers of 2, like https://git.musl-libc.org/cgit/musl/tree/src/malloc/mallocng/malloc.c#n12 but it's platform/CRT dependent, so I would not relay on any specific values. |
Then do I need to do it like this so that both small and large data types take up less space? next_po2(p_elements) * sizeof(T) - DATA_OFFSET |
I found a place where COW allocates too much memory for types whose size is not a multiple of 2.
For example, we have a type size of 20 bytes and want to change the COW size to 4. To do this, call the
resize(4)
method.This method calls the
_get_alloc_size
method which allocates too much memory for us._next_po2(20 * 4) = 128
.But instead it should be
_next_po2(4) * 20 = 80
.I haven't done performance benchmarks yet, but only looked at the amount of memory used by the project from CI https://github.com/godotengine/regression-test-project/archive/4.0.zip.
Before:
After: