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 Memory Leak from Npc events #4483

Merged
merged 5 commits into from Jun 22, 2023
Merged

Conversation

MillhioreBT
Copy link
Contributor

@MillhioreBT MillhioreBT commented Jun 8, 2023

Pull Request Prelude

  • I have followed [proper The Forgotten Server code styling][code].
  • I have read and understood the [contribution guidelines][cont] before making this PR.
  • I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

These changes remove the global lua interface for all npc, and now each npc has its own lua interface, in turn the handler and interface become smart pointers so that their removal is handled automatically

asdasdsa
Here is a visual example of a graph showing how memory usage changes when creating and deleting 5000 Npcs, and then the garbage collector cleaning up everything.

Issues addressed: #4288

Co-Authored-By: yamaken93 <1008865+yamaken93@users.noreply.github.com>
@nekiro
Copy link
Member

nekiro commented Jun 8, 2023

I would advocate for making NpcEventsHandler per npc and not global for all npcs (also wrap it in unique_ptr to avoid destructor), for some odd reason its global for npcs and the memory usage changes are marginal

Co-Authored-By: yamaken93 <1008865+yamaken93@users.noreply.github.com>
nekiro
nekiro previously approved these changes Jun 8, 2023
src/npc.h Outdated Show resolved Hide resolved
src/npc.cpp Outdated Show resolved Hide resolved
src/npc.cpp Outdated Show resolved Hide resolved
src/npc.h Outdated Show resolved Hide resolved
@EPuncker EPuncker added enhancement Increase or improvement in quality, value, or extent bugfix labels Jun 11, 2023
ranisalt
ranisalt previously approved these changes Jun 22, 2023
Copy link
Member

@ranisalt ranisalt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM

@EPuncker EPuncker merged commit 2a1bad7 into otland:master Jun 22, 2023
20 checks passed
@MillhioreBT MillhioreBT deleted the memory_leak_npcs branch June 22, 2023 22:34
ralke23 added a commit to ralke23/Greed-TFS-1.5-Downgrades that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix enhancement Increase or improvement in quality, value, or extent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants