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 depot memory leak #4879

Merged
merged 1 commit into from
Dec 22, 2024
Merged

Fix depot memory leak #4879

merged 1 commit into from
Dec 22, 2024

Conversation

nekiro
Copy link
Member

@nekiro nekiro commented Dec 21, 2024

Pull Request Prelude

Changes Proposed

Fix #3346

Issues addressed:
Removed memory leak

How to test:
Add item to first slot of your depot, preferably backpack (can be nested).
Spam the code below lots of times. I've tested with 360 pages of full backpacks, which adds like 50mb to memory, after few relogs memory was not increased.

local chest = player:getDepotChest(0)
local item = chest:getItem(0)

for i = 1, 500 do
	chest:addItemEx(item:clone(), INDEX_WHEREEVER, FLAG_NOLIMIT)
end

@nekiro nekiro added the bugfix label Dec 21, 2024
@ArturKnopik
Copy link
Contributor

image

Are depos cached somehow? I see memory allocation but not deallocation after logout, relogging does not re-allocate new memory.

@ramon-bernardo
Copy link
Contributor

@ArturKnopik this allocation, if I remember correctly, its related to IOLogin, where it adds the items to the vectors to save them

See,
image

image

I see a small memory spike whenever a player join/leave the game. It ranges from 3Mb at minimum to 15Mb at maximum, which is pretty strange

Copy link
Contributor

@ramon-bernardo ramon-bernardo left a comment

Choose a reason for hiding this comment

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

The memory leak in the depot has been fixed. Check below to see how it was before. (never deallocate)
image

Note that there’s still a small leak, around 3mb to 15mb, every time a player login into server. This can be investigated later, as the current fix a more significant issue

@nekiro
Copy link
Member Author

nekiro commented Dec 22, 2024

Noticed the small leak too, didn't really investigate what causes it, but I might at some point. It's not really related though.

@ArturKnopik
Copy link
Contributor

Valgrind report when login and logout
items in many dp bp of bps
leaks.log

@ArturKnopik ArturKnopik mentioned this pull request Dec 22, 2024
2 tasks
@ArturKnopik ArturKnopik merged commit 8d79ba4 into otland:master Dec 22, 2024
16 checks passed
@nekiro nekiro deleted the fix-depot-leak branch December 22, 2024 22:05
@nekiro nekiro mentioned this pull request Dec 22, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Possible memory leak - depots not getting deleted from memory at player deletion
3 participants