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

Fix file read memory leak #750

Merged
merged 4 commits into from
Mar 23, 2025
Merged

Fix file read memory leak #750

merged 4 commits into from
Mar 23, 2025

Conversation

SeppNel
Copy link
Contributor

@SeppNel SeppNel commented Mar 20, 2025

Fixed a memory leak in file.cpp that happened because we were storing a raw pointer in a unique pointer and never freeing it, making the unique pointer useless. This makes it so that the memory is actually handled by the unique pointer.

Before:
https://github.com/user-attachments/assets/b9bea646-71a5-4958-84c9-d287009bd0c5

You can see it increase around 3MB each transition

After:
https://github.com/user-attachments/assets/699f5b9f-84d1-443b-8c7b-e59cb36286c7

Memory still increases because of this issue but it is greatly reduced.

If we also workaround that issue:
https://github.com/user-attachments/assets/d6c009c0-ba21-4a3f-8776-db54ea10ef42

But I didn't submit that workaround since it may break games.

@OpenSauce04 OpenSauce04 added this to the 2121 milestone Mar 20, 2025
@OpenSauce04 OpenSauce04 requested a review from PabloMK7 March 20, 2025 16:52
@OpenSauce04 OpenSauce04 added the bug Something isn't working label Mar 20, 2025
@PabloMK7
Copy link
Member

Ah! I remember this. I actually did this change.
The reason I changed the u8[] is because that initializes the read buffer before reading to it. That is, it first gets memset to 0, then the data is read into it, which wastes an unneccessary initialization. I guess that making a a unique pointer of a raw pointer does not own ownership? Maybe there is a better way to prevent the initialization and not cause the memory leak.

@PabloMK7
Copy link
Member

What about

std::unique_ptr<u8, decltype(&std::free)> buffer(
        reinterpret_cast<u8*>(std::malloc(size)), 
        &std::free
    );

@SeppNel
Copy link
Contributor Author

SeppNel commented Mar 20, 2025

I think the best choice then is make_unique_for_overwrite like so: std::unique_ptr<u8[]> data = std::make_unique_for_overwrite<u8[]>(length);
That way memory doesn't get initialized and to me it looks cleaner

@PabloMK7
Copy link
Member

I also remember trying to use make_unique_for_overwrite, but I don't remember why I didn't use it in the end. I think it wasn't available on some platforms, let's see

@SeppNel
Copy link
Contributor Author

SeppNel commented Mar 21, 2025

All the workflows succeeded on my fork and according to this it should be well supported

@PabloMK7 PabloMK7 added the squash This pull request should be squashed if approved label Mar 21, 2025
@PabloMK7
Copy link
Member

Please fix the license

@OpenSauce04 OpenSauce04 merged commit 8cdafaa into azahar-emu:master Mar 23, 2025
12 checks passed
@SeppNel SeppNel deleted the file_leak branch March 23, 2025 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size/S squash This pull request should be squashed if approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants