NonStaticGH / CPathHostProject

This is a space where I develop Customizable Pathfinding plugin for UE5. Later on, I will also include my Engineering Thesis and documentation.
54 stars 12 forks source link

Memory leaks #5

Open lihomanovdv opened 1 year ago

lihomanovdv commented 1 year ago

I found huge memory leaks, caused by plugin.

To reproduce:

1) Add single CPathVolume 2) Add Timer in any blueprint to fire event connected to Find Async Path with Volume ref connected. To get leaks faster - set small timer Time (0.05 for example) 3) Run Unreal Insights 4) Run in Standalone Game with params (Editor Preferences -> Additional Launch Parameters -> -trace=cpu,gpu,loadtime,counters,log,frame,memory -llm -tracehost=127.0.0.1 -counterstrace) 5) See how Untracked memory grows

NonStaticGH commented 1 year ago

Thank you, I will look into this

On Thu, Feb 9, 2023, 16:17 Lihomanov Daniil @.***> wrote:

I found huge memory leaks, caused by plugin.

To reproduce:

  1. Add single CPathVolume
  2. Add Timer in any blueprint to fire event connected to Find Async Path with Volume ref connected. To get leaks faster - set small timer Time (0.05 for example)
  3. Run Unreal Insights
  4. Run in Standalone Game with params (Editor Preferences -> Additional Launch Parameters -> -trace=cpu,gpu,loadtime,counters,log,frame,memory -llm -tracehost=127.0.0.1 -counterstrace)
  5. See how Untracked memory grows

— Reply to this email directly, view it on GitHub https://github.com/NonStaticGH/CPathHostProject/issues/5, or unsubscribe https://github.com/notifications/unsubscribe-auth/A3I6RJPJKGJKY3P5U2Q6ZELWWUDAHANCNFSM6AAAAAAUWVXZUY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

lihomanovdv commented 1 year ago

I found, that it happens only in PIE / Standalone Game. When launched in a packaged build - its all OK, no leaks

NonStaticGH commented 1 year ago

Then it is most likely related to epic's Async node, not to my code directly, but as I said I will check it next week :)

On Fri, Feb 10, 2023, 14:42 Lihomanov Daniil @.***> wrote:

I found, that it happens only in PIE / Standalone Game. When launched in a packaged build - its all OK, no leaks

— Reply to this email directly, view it on GitHub https://github.com/NonStaticGH/CPathHostProject/issues/5#issuecomment-1425829536, or unsubscribe https://github.com/notifications/unsubscribe-auth/A3I6RJJAGJJBXFASD2ESBJLWWZAV7ANCNFSM6AAAAAAUWVXZUY . You are receiving this because you commented.Message ID: @.***>

lihomanovdv commented 1 year ago

Thanks :) I'll wait for reply :)

lihomanovdv commented 1 year ago

Useful link: https://forums.unrealengine.com/t/memory-not-freed-using-frunnable-and-tarray/151731/13

NonStaticGH commented 1 year ago

Hello, I've been trying to reproduce your error with no success. I have performed 4401 FindPath calls over 25 seconds (which is completely unrealistic in an actual scenario). Since as of now every FindPath call creates it's own FRunnable thread (this will be changed for a proper task queue in the future), it allocates around 1MB of memory. I am attaching a screenshot of my Memory Insights graph: [image: image.png]

We can see that in seconds 26 to 51 tracked LLM steadily rises to nearly 5GB. After that, calls to FindPathAsync are no longer performed so it stops growing, and during the next garbage collection pass, everything gets properly collected. The untracked memory is most likely memory reserved by the standalone process. It is 2GB, and as tracked memory gets allocated, the untracked memory accordingly decreases - until it's completely depleted. After memory from FindPath calls is collected, the Untracked memory goes back to it's initial value as expected.

I could not find any actual memory leaks caused by my plugin, I have also checked if all the FRunnable objects are cleared after GC pass using the Investigation Queries in Unreal Insights. Could you please edit or remove the question on my marketplace page? I'm afraid that "Huge" memory leaks might scare away some people reading through the questions...

On Tue, Feb 14, 2023 at 10:37 AM Lihomanov Daniil @.***> wrote:

Useful link: https://forums.unrealengine.com/t/memory-not-freed-using-frunnable-and-tarray/151731/13

— Reply to this email directly, view it on GitHub https://github.com/NonStaticGH/CPathHostProject/issues/5#issuecomment-1429418134, or unsubscribe https://github.com/notifications/unsubscribe-auth/A3I6RJKVRAHKQ2ERYA2ZD7LWXNG6NANCNFSM6AAAAAAUWVXZUY . You are receiving this because you commented.Message ID: @.***>

lihomanovdv commented 1 year ago

What version did you use in tests? 5.1 or 4.27? Check this video, please. I recorded how I test this and what I get. I use 4.27.2 Maybe I do smth wrong?) Or if you tested with 5.1 version, so maybe Epics fixed something in 5.1, but it happens in 4.27

https://drive.google.com/file/d/1x7Yl5fYyXTZPsF1lFNvV9AxGssl3z4Qc/view?usp=share_link

lihomanovdv commented 1 year ago

[image: image.png]

By the way no image attached

NonStaticGH commented 1 year ago

Yes I have only tested on 5.1... It does seem to be an actual issue on 4.27 then.

I will see if spawning only few threads and reusing them will fix it, as I don't have any other ideas as of now. I'll let you know as soon as I push the change to github, if everything goes smoothly it should be ready tomorrow

On Tue, Feb 21, 2023, 15:11 Lihomanov Daniil @.***> wrote:

[image: image.png]

By the way no image attached

— Reply to this email directly, view it on GitHub https://github.com/NonStaticGH/CPathHostProject/issues/5#issuecomment-1438562357, or unsubscribe https://github.com/notifications/unsubscribe-auth/A3I6RJLFBJNGOSDSM7B3MITWYTEKRANCNFSM6AAAAAAUWVXZUY . You are receiving this because you commented.Message ID: @.***>

lihomanovdv commented 1 year ago

Thank you for your quick reaction! ❤️

Louspirit commented 1 year ago

To prevent memory leaks and increase this plugin's liability, there are some simple actions that can be done.

Replace raw pointers with smart pointers such as TUniquePtr or TSharedPtr provided by UE.

It seems that the inline keyword is also not used correctly. It breaks the API: I tried to inherit from one of the classes and calling the said methods led to an LNK2019 error (I had to duplicate).

In the same way, replace the std::vector, std::atomic, and std::set containers with the TArray, TAtomic, and TSet containers provided by Unreal Engine. These containers are optimized for game development and can provide better performance.

Use const references instead of passing arguments by value to avoid unnecessary copies. Use const and noexcept whenever appropriate to enable the compiler to optimize the code.

Remove unused headers includes.

Use UE_LOG instead of std::cout to log messages, as it is more efficient and compatible with the engine's logging system.

I wanted to start a pull request but haven't successfully rebuilt the plugin...

NonStaticGH commented 1 year ago

Unreal engine smart pointers and array implementations are not meant to be used everywhere. They are way too slow for low level usage. A lot of my code is written in raw c++ for performance reasons. I appreciate the suggestions, although I have studied low level c and assembly programming and I'm pretty sure I know what I'm doing in that regard. And as I mentioned before, there are no memory leaks caused by my code, at least from what we've found so far. Ue 4.27 has trouble garbage collecting FRunnable objects in standalone mode.

Inline methods are not supposed to be overridden.

On Wed, Feb 22, 2023, 17:31 Guillaume Escarieux @.***> wrote:

To prevent memory leaks, maybe replace raw pointers with smart pointers such as TUniquePtr or TSharedPtr provided by UE.

It seems that the inline keyword is also not used correctly. It breaks the API: I tried to inherit from one of the classes and using the said methods led to an LNK2019 error.

In the same way, replace the std::vector, std::atomic, and std::set containers with the TArray, TAtomic, and TSet containers provided by Unreal Engine. These containers are optimized for game development and can provide better performance.

Use const references instead of passing arguments by value to avoid unnecessary copies. Use const and noexcept whenever appropriate to enable the compiler to optimize the code.

Remove unused headers includes.

Use UE_LOG instead of std::cout to log messages, as it is more efficient and compatible with the engine's logging system.

I wanted to start a pull request but haven't successfully rebuilt the plugin...

— Reply to this email directly, view it on GitHub https://github.com/NonStaticGH/CPathHostProject/issues/5#issuecomment-1440369675, or unsubscribe https://github.com/notifications/unsubscribe-auth/A3I6RJL4IVEA7JWWBZXPCDDWYY5NPANCNFSM6AAAAAAUWVXZUY . You are receiving this because you commented.Message ID: @.***>

Louspirit commented 1 year ago

I am sorry I am not an expert in cpp so take my words with caution. I love this plugin and want to contribute. I only reported what experts told me or pieces of advice I could find online. But again, I'm not qualified to argue. Indeed, it was more regarding the performance aspect.

NonStaticGH commented 1 year ago

No problem, I understand, thanks for the engagement :) There will be an update soon with better thread management and a new feature

On Thu, Feb 23, 2023, 10:59 Guillaume Escarieux @.***> wrote:

I am sorry I am not an expert in cpp so take my words with caution. I love this plugin and want to contribute. I only reported what experts told me or pieces of advice I could find online.

— Reply to this email directly, view it on GitHub https://github.com/NonStaticGH/CPathHostProject/issues/5#issuecomment-1441478666, or unsubscribe https://github.com/notifications/unsubscribe-auth/A3I6RJOVZELBWVU6AEKGFR3WY4YHHANCNFSM6AAAAAAUWVXZUY . You are receiving this because you commented.Message ID: @.***>

NonStaticGH commented 1 year ago

I'm still working on a complete rework on C++ FindPath interface and threading, it turned out to be a much bigger task than I anticipated. In the meantime, thank you for changing the comment :)

On Thu, Feb 23, 2023 at 1:52 PM Dominik Trautman @.***> wrote:

No problem, I understand, thanks for the engagement :) There will be an update soon with better thread management and a new feature

On Thu, Feb 23, 2023, 10:59 Guillaume Escarieux @.***> wrote:

I am sorry I am not an expert in cpp so take my words with caution. I love this plugin and want to contribute. I only reported what experts told me or pieces of advice I could find online.

— Reply to this email directly, view it on GitHub https://github.com/NonStaticGH/CPathHostProject/issues/5#issuecomment-1441478666, or unsubscribe https://github.com/notifications/unsubscribe-auth/A3I6RJOVZELBWVU6AEKGFR3WY4YHHANCNFSM6AAAAAAUWVXZUY . You are receiving this because you commented.Message ID: @.***>

Louspirit commented 1 year ago

Can't wait for the new version🔥

RWOverdijk commented 11 months ago

Is this still a thing? It's been a couple of months since the last comment and I see there were commits in between so I wanted to check 😄