Fix trajectory info memory leak

[JShep1]

Simple fix but took awhile to find and I think deserves some explanation as well as some tools I used that can be used in the future for memory profiling (possibly this part deserves its own tutorial or guide).

It was discovered that there was a significant memory leak in any ign-gazebo examples involving actors. I used a project called mem-usage-ui which was very simple yet effective in being able to visually confirm the existence of memory leaks. The output from the actor example that was experiencing serious leakage is as follows:

It can be seen that the memory leakage grew by about 100Mb in just one minute, if left for 15-20 minutes, this could crash the computer. Using heaptrack or valgrind didn't quite prove effective given it monitors the ruby executable, outputting little to no helpful information. For this reason, I'd recommend we get a standalone custom gui example out, similar to custom_server as this would allow valgrind to do its job. I did, however, find that memleax was quite useful in pinpointing memory leaks, the output is a bit complicated to discern and there are false positives, but it proved very helpful in pointing me right to the culprit function that was leaking (in this case, ActorMeshAnimationAt(...)).

After further investigation, I found that line 894 of SceneManager.cc auto trajs = this->dataPtr->actorTrajectories[_id]; was the problem line that was spawning new TrajectoryInfo's, invoking this constructor for every currently existing TrajectoryInfo in the this->dataPtr->actorTrajectories[_id] vector, I suppose when assigning that vector to a new variable, the (deep) copy operation is made and that vector is duplicated, resulting in the original vector remaining as the value of that this->dataPtr->actorTrajectories map and a new copy vector set to trajs.

So adding a simple deletion of this->dataPtr to the destructor of TrajectoryInfo fixed up the memory leak, which can be visually confirmed in the following graph

So, not perfect, there appears to be another smaller leak somewhere, but ~10Mb / minute is much better than the original. I'll continue to look for the remaining leaks.

Signed-off-by: John Shepherd john@openrobotics.org

[codecov]

Codecov Report

Merging #93 into ign-common3 will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##           ign-common3      #93   +/-   ##
============================================
  Coverage        73.94%   73.94%           
============================================
  Files               69       69           
  Lines             9390     9391    +1     
============================================
+ Hits              6943     6944    +1     
  Misses            2447     2447           

Impacted Files	Coverage Δ
graphics/src/Animation.cc	92.18% <100.00%> (+0.03%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcf1d2c...29f2371. Read the comment docs.

[chapulina]

Marking this as beta for Dome since it's a bug fix.

[JShep1]

Marking this as beta for Dome since it's a bug fix.

Sounds good, I'm going to test Dome in a bit to see if the memory leak occurs there too, I noticed there was some significant changes of the Actor code so I wanted to confirm that the leak still occurs for Dome.

[JShep1]

Looks like Dome has this memory leak as well and this fix will fix it. It also appears Dome doesn't have the extra memory leak that Citadel has 🤔

[mjcarroll]

Could we make this a smart pointer in dome, then you could probably get rid of all of the constructors and destructor here.

This also currently violates rule-of-five principle.

[chapulina]

Could we make this a smart pointer in dome

Dome will use ign-common3, so I don't think it can be changed there. But we could change it from ign-common4.

[iche033]

Thanks for the fix!

It also appears Dome doesn't have the extra memory leak that Citadel has

Dome switched from doing manual interpolation of skeletal bone poses (by ign-common) to interpolation done by the rendering engine. So one place I see that may leak is here:

https://github.com/ignitionrobotics/ign-common/blob/master/graphics/src/SkeletonAnimation.cc#L82

There are also a few other possible places for leaks which I don't a matching delete:
https://github.com/ignitionrobotics/ign-common/blob/master/graphics/src/Animation.cc#L505
https://github.com/ignitionrobotics/ign-common/blob/master/graphics/src/Animation.cc#L220

yikes, quite a few leaks in our animation code