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 tree felling helper memory leak #2677

Merged

Conversation

omergunr100
Copy link
Contributor

What

Previously we would allocate helpers but never release them, moreover we would iterate over all finished ones every level tick.

Implementation Details

Just added a set to collect the inactive helpers each iteration to remove them at the end.

Outcome

Players cannot bring down servers by chopping too many trees.

@omergunr100 omergunr100 requested a review from a team as a code owner December 30, 2024 22:46
@MatthiasMann
Copy link

wouldn't using an iterator explicitly and using it's remove method be better then making a 2nd set?
and if not the finished could be a simple ArrayList which is a lot faster to add and iterator.

@Spicierspace153 Spicierspace153 added the type: refactor Suggestion to refactor a section of code label Dec 30, 2024
@omergunr100
Copy link
Contributor Author

It does make more sense to use the iterator in this context, I keep forgetting that they let you remove an element while iterating

@MatthiasMann
Copy link

Also it makes no sense that helpers is a set if they are added in the constructor - this class has no hashCode() or equals() method - so each object is unique - so the set will never fail to insert - but perform a lot of useless operations. using an ArrayList would be better.

@krossgg
Copy link
Contributor

krossgg commented Dec 30, 2024

Also it makes no sense that helpers is a set if they are added in the constructor - this class has no hashCode() or equals() method - so each object is unique - so the set will never fail to insert - but perform a lot of useless operations. using an ArrayList would be better.

Onion moment

@omergunr100
Copy link
Contributor Author

Also it makes no sense that helpers is a set if they are added in the constructor - this class has no hashCode() or equals() method - so each object is unique - so the set will never fail to insert - but perform a lot of useless operations. using an ArrayList would be better.

imo now that the memory leak is gone the performance impact of the collection shouldn't matter unless you have 1000's of players chopping trees permanently with the fastest chainsaws they can find.
that being said now that I've seen this I can't unsee it, I'm going to replace it with a linked list.

@MatthiasMann
Copy link

linked list is possible the worst possible list to use in any case - it has the highest memory allocation overhead - and the slowest iteration.

@omergunr100
Copy link
Contributor Author

yeah, I was mostly messing with you, using a linkedlist is triggering people

@screret screret merged commit f5bdec0 into GregTechCEu:1.20.1 Jan 4, 2025
2 checks passed
@omergunr100 omergunr100 deleted the 1.20.1-tree-felling-memory-leak branch January 4, 2025 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: refactor Suggestion to refactor a section of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants