Lux-AI-Challenge / Lux-Design-S1

Home to the design and engine of the @Lux-AI-Challenge Season 1, hosted on @kaggle
https://lux-ai.org/
Apache License 2.0
897 stars 152 forks source link

Fix memory leaks #113

Closed djkeyes closed 2 years ago

djkeyes commented 2 years ago

Hey, I noticed that there were still some unresolved memory issues after this thread: #89

In particular:

  1. GameMaps dynamically allocate a large number of Cells in every constructor call, but never free the Cells, resulting in a memory leak.
  2. Agents allocate Cities, and Cities allocate CityTiles, but Agents are the ones who clean up everything, which makes the ownership a little confusing. This might also cause a memory leak if you re-use the Agent class multiple times (e.g. in a custom benchmarker tool) and forget to call resetPlayerStates at the end.

I verified 1. in valgrind. A simple solution to the problem would just be to store objects by value on the stack, rather an dynamically allocating them. Now if for some reason you really want dynamic allocation, then we could instead use the RAII pattern, e.g. with a unique_ptr, but that doesn't sound like it would be an improvement.

I just eyeballed problem 2., but I think this refactor makes sense. A simple solution is to clearly define who owns which blocks of memory, and denote ownership by using value member variables. In this PR, I've arranged it so that Cities own CityTiles (and are responsible for their cleanup), and Players own Cities (and are responsible for their cleanup). That way, when you call players[team].cities.clear(); you guarantee you have cleaned up all the Citytiles as well.

Let me know if you have any feedback. If you're trying to reduce code churn in main.cpp, I can refactor City::citytiles a little bit so that it will still store pointers and require no code changes to main.cpp.

StoneT2000 commented 2 years ago

This looks great! Thanks for the fixes, our c++ kit was always poorly written / not really checked much.

Also seems like API hasn't changed either so that's good!

djkeyes commented 2 years ago

Glad to help!