Memory leak when loading RideFiles

[Joachim Kohlhammer]

Dear all,

since starting to create route segments, I experienced Golden Cheetah being killed by the OS (Linux in my case) on a regular basis because of the huge amount of allocated memory. This usually happens after the creation of 3-4 new route segments.

I think I found the leaking code (see also the attached diff):
The method "void RideFile::appendOrUpdatePoint(double ..., bool forceAppend)" allocates a RideFilePoint twice and ignores the first instance if forceAppend is true.
Additionally replacing a RideFilePoint leaks memory in a similar way (creating a new instance of the point, forgetting the pointer to the original without deleting). According to my logs this didn't happen to me, nevertheless I included a fix in my patch.

This leak is relevant in every situation where activities are loaded, not only when creating route segments.

My measurements gave me the number of up to 4GB of leaked memory per created route segment. After applying my fix, the memory usage increased only slightly during this task, resulting in improved stability and user experience.

According to https://github.com/GoldenCheetah/GoldenCheetah/issues/2635 the activities should be kept in memory after being read once. Therefore I still have the open question why they are being re-read over and over again. Is this an issue you guys think I could and should investigate - is it worth it?

I am aware of the nearing 3.6 release and the necessary caution not to introduce regressions. Nevertheless I see this as a change that can increase stability of Golden Cheetah significantly. If you consider it to risky at the moment, please consider applying it after the release.

Regards
 Joachim

[Mark Liversedge]

On Saturday, 23 July 2022 at 12:28:29 UTC+1 tiefgara...@gmail.com wrote:

since starting to create route segments, I experienced Golden Cheetah being killed by the OS (Linux in my case) on a regular basis because of the huge amount of allocated memory. This usually happens after the creation of 3-4 new route segments.

I think I found the leaking code (see also the attached diff):
The method "void RideFile::appendOrUpdatePoint(double ..., bool forceAppend)" allocates a RideFilePoint twice and ignores the first instance if forceAppend is true.

This is definitely a memory leak.

 

This leak is relevant in every situation where activities are loaded, not only when creating route segments.

This applies for rides on the initial downloaded from Strava, or the initial imported of FIT or EPM files, since these are the only places that function is used.

  

My measurements gave me the number of up to 4GB of leaked memory per created route segment. After applying my fix, the memory usage increased only slightly during this task, resulting in improved stability and user experience.

How are you creating the route segment?

 

According to https://github.com/GoldenCheetah/GoldenCheetah/issues/2635 the activities should be kept in memory after being read once. Therefore I still have the open question why they are being re-read over and over again. Is this an issue you guys think I could and should investigate - is it worth it?

They are only retained in memory when they are selected by the user. Any process that runs across ridefiles opening them en-masse will free the memory they use once the process has completed (unless they were already opened by the user); e.g. importing lots of files, refreshing metrics, searching for segments.

Please also note that on some OSes freeing memory within an application will not necessarily mean the memory is immediately available for use by other applications, be careful about how you read performance statistics. 

Mark

[Mark Liversedge]

On Saturday, 23 July 2022 at 12:48:04 UTC+1 Mark Liversedge wrote:

This leak is relevant in every situation where activities are loaded, not only when creating route segments.

This applies for rides on the initial downloaded from Strava, or the initial imported of FIT or EPM files, since these are the only places that function is used.

My apologies. this is false- it looks like it is being used by appendPoint() too (so used absolutely everywhere), its pretty horrible. Will fix, this has been in the code for way too long.

Thank you for the update and doing the hard yards on this, will look to apply a fix.

Mark 

[Joachim Kohlhammer]

liver...@gmail.com schrieb am Samstag, 23. Juli 2022 um 13:48:04 UTC+2:

On Saturday, 23 July 2022 at 12:28:29 UTC+1 tiefgara...@gmail.com wrote:

This leak is relevant in every situation where activities are loaded, not only when creating route segments.

This applies for rides on the initial downloaded from Strava, or the initial imported of FIT or EPM files, since these are the only places that function is used.

  

My measurements gave me the number of up to 4GB of leaked memory per created route segment. After applying my fix, the memory usage increased only slightly during this task, resulting in improved stability and user experience.

How are you creating the route segment?

Marking the path on the map, renaming the usersegment and turning it into route segment.

 

 

According to https://github.com/GoldenCheetah/GoldenCheetah/issues/2635 the activities should be kept in memory after being read once. Therefore I still have the open question why they are being re-read over and over again. Is this an issue you guys think I could and should investigate - is it worth it?

They are only retained in memory when they are selected by the user. Any process that runs across ridefiles opening them en-masse will free the memory they use once the process has completed (unless they were already opened by the user); e.g. importing lots of files, refreshing metrics, searching for segments.

Please also note that on some OSes freeing memory within an application will not necessarily mean the memory is immediately available for use by other applications, be careful about how you read performance statistics. 

Thanks, so I wont jump into this.

BTW, the main task was done using heaptrack, so I could learn a new tool :)

Regards

 Joachim

[Ale Martinez]

The append case was the worst offender, but the update case also memory leeks since the old point is not freed, this may be tricky since we don’t want existing pointers to it become invalid, so the solution proposed by the OP can be dangerous.

I think the safe way would be to copy the new point over the old point once updated, and free the new point afterwards.

[Mark Liversedge]

On Saturday, 23 July 2022 at 13:52:54 UTC+1 Ale Martinez wrote:

The append case was the worst offender, but the update case also memory leeks since the old point is not freed, this may be tricky since we don’t want existing pointers to it become invalid, so the solution proposed by the OP can be dangerous.

I think the safe way would be to copy the new point over the old point once updated, and free the new point afterwards.

Am looking at it to decide what to do, I haven't applied that bit of the patch for the reasons you cite.

Mark

[Ale Martinez]

Mark's fix for this issue is included in latest snapshot builds, see https://groups.google.com/g/golden-cheetah-users/c/NuXT89nbwBk/m/_oH8PrEvAQAJ