Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#8266 closed defect (fixed)

memory leaks in backend

Reported by: udovdh@… Owned by: Isaac Richards
Priority: major Milestone: unknown
Component: MythTV - General Version: 0.22-fixes
Severity: medium Keywords:
Cc: Ticket locked: yes

Description

Some valgrind logs which could point to memory leaks in backend

Attachments (2)

backend-valgrind.tar.bz2 (53.5 KB) - added by udovdh@… 10 years ago.
valgrind logs
backend-valgrind.log.new.bz2 (85.6 KB) - added by udovdh@… 10 years ago.
new log, with different valgrind options

Download all attachments as: .zip

Change History (27)

Changed 10 years ago by udovdh@…

Attachment: backend-valgrind.tar.bz2 added

valgrind logs

comment:1 Changed 10 years ago by Dibblah

Status: newinfoneeded_new

Nothing significant that I can see in this valgrind. Please try with --leak-check=full --show-reachable=yes

comment:2 Changed 10 years ago by anonymous

It says memory was _definitely_ lost. Those were the recommended settings from the mythtv-users list. After the weekend I will be running 0.23-fixes if all goes well. I can give it another try after that.

comment:3 Changed 10 years ago by anonymous

It says 1.2Kb was definitely lost ... all leaks should and will be fixed, but that's a ridiculously tiny amount.

comment:4 Changed 10 years ago by anonymous

Heh, and it's wrong anyway. e.g. the loss in DVBCam::SetPMT(), the pointer is assigned to PMTAddList or PMTList and deleted again when Stop() is called from the destructor. No leak there.

comment:5 Changed 10 years ago by udovdh@…

So now the leak is not worthy? There were 2 logiles if I am right. Please confirm. Over time (weeks) mythbackend grows to 100's of megs on my tiny Epia box. This new x86_64 setup is slightly more powerfull so it enables me to do a valgrind. Part of the reason to invest was this research.

comment:6 Changed 10 years ago by anonymous

Heh, and it's wrong anyway

So now valgrind is not trustworthy? You puzzle me. Comments about memory leaks, complete with graphs and ps aux dumps were ignored because we needed valgrind. Now we have valgrind and it is not reliable. This in the light that valgrind make a dual core 2.7 Ghz box have it's dvb buffers overflow because of valgrind. I.e.: still recordings are useless, despite the upgrade.

comment:7 Changed 10 years ago by cpinkham

Status: infoneeded_newnew
Ticket locked: set

Trac is not a discussion list.

Changed 10 years ago by udovdh@…

new log, with different valgrind options

comment:8 Changed 10 years ago by stuartm

(In [23893]) Backport [23892] to 0.22-fixes, fix a memory leak in ChannelBase?. Refs #8266

comment:9 Changed 10 years ago by stuartm

(In [23894]) Backport [23892] to 0.23-fixes, fix a memory leak in ChannelBase?. Refs #8266

comment:10 Changed 10 years ago by stuartm

Ticket locked: unset

comment:11 Changed 10 years ago by udovdh@…

Decided not to upgrade to 0.23 yet as it has not reached -fixes status yet. Instead I will gater some more numbers with 0.22-fixes to see what the impact of the recent leak fix is. I am using svn 23912.

comment:12 Changed 10 years ago by Janne Grunau

(In [23991]) fix leaking streamdata in the DVBRecorder. Refs #8266

This leak is caused by special handling of virtual functions in the destructor of the base class. Only functions of the base class are called. This seems to leak at least a couple of kBytes per recording.

comment:13 Changed 10 years ago by Janne Grunau

(In [24000]) cosmetic: rename member _mpeg_stream_data to _stream_data in firewirerecorder

prepares the move of stream_data handling to the base class DTVRecorder to fix the leak of _stream_data in various recorders. Refs #8266

comment:14 Changed 10 years ago by Janne Grunau

(In [24001]) cosmetic: use _stream_data instead of data in various SetStreamData? methods

Preparation for basic stream_data handling in the base class DTVRecorder. Refs #8266

comment:15 Changed 10 years ago by Janne Grunau

(In [24002]) move basic stream data handling to DTVRecorder

This fixes leaking stream data in various recorder on destruction. The SetStreamData?(NULL) call in ~DTVRecorder just calls its own SetStreamData?() which does nothing. Reverts the quick fix for the DVBRecorder in [23991]. Refs #8266

comment:16 Changed 10 years ago by udovdh@…

The first fix (using early figures) shows a slight decrease in RSS growth/day. In a few days we'll do a fresh build of 0.22-fixes to test this new leak fix.

comment:17 Changed 10 years ago by Janne Grunau

(In [24004]) backports [23991] from trunk and fix similar leaks in other decoders

the full fix/cleanup in [24002] in trunk is too large the change in mpegrecorder is probably not necessary but shouldn't harm Refs #8266

comment:18 Changed 10 years ago by udovdh@…

Runnign 0.22-fixes svn 24015. After a few days I see no real difference in memory size growth versus the previous build (pre-24000). It grows 10Megs a day or so. I'll try and do a valgrind run soon.

comment:19 Changed 10 years ago by stuartm

The recorder leak wasn't backported to 0.22-fixes, so I'm not surprised that you see no difference.

comment:20 in reply to:  19 Changed 10 years ago by udovdh@…

Replying to stuartm:

The recorder leak wasn't backported to 0.22-fixes, so I'm not surprised that you see no difference.

OK, so I upgraded to 0.23-fixes svn.24175.

comment:21 Changed 10 years ago by udovdh@…

The first few figures appear to indicate that 0.23-fixes ~ halves the leaked RSS memory amount versus 0.22-fixes. We're at less than 10 megs/day. So the 2400x changes really did change things? Congratulations! I'll keep running this 0.23-fixes instance to get some more numbers. Then we could do valgrind again.

comment:22 Changed 10 years ago by danielk

Resolution: fixed
Status: newclosed

Fixed in trunk:

On Thu, 2010-04-22 at 16:34 +0200, Udo van den Heuvel wrote:
%CPU %MEM    VSZ   RSS TTY      STAT START   TIME C
>  5.1  2.2 1706748 87208 ?       Ssl  Apr17  41:00 /usr/bin/myt
>  5.2  2.2 1706748 86804 ?       Ssl  Apr17 116:20 /usr/bin/myt
>  5.3  2.5 1706748 95612 ?       Ssl  Apr17 198:05 /usr/bin/myt
>  5.4  2.6 1706748 102480 ?      Ssl  Apr17 278:57 /usr/bin/myt
>  5.4  2.8 1706748 106516 ?      Ssl  Apr17 358:35 /usr/bin/myt

This sample shows zero growth in the memory requested by mythbackend.

It seems you are confused by the meaning of the numbers here so I'll explain. VSZ is the total size of the libraries, executable, heap, and stack. This has not grown at all, which means MythTV has not requested any memory while recording 358 hours of programming per tuner. RSS is the the resident set, this is the actual RAM that the MythTV process has access to. This is allocated by the operating system based on what else you have running. RSS will never grow beyond VSZ.

comment:23 Changed 10 years ago by udovdh@…

These figures are very similar to those I have been posting for months. Only difference is the increase in RSS per day. That is the actual memory used. So why is it fixed all of a sudden?

comment:24 Changed 10 years ago by danielk

Ticket locked: set

Heh, because you collected a valgrind log and stuartm fixed the leak. Then today I saw your log confirming the fix worked. So I closed the ticket.

Ideally, that's how it works :)

To be clear. No VSZ growth == no memory leak.

comment:25 Changed 10 years ago by Janne Grunau

(In [24255]) backports [24004] from release-0-23-fixes

fix memleaks in several mpegrecorder based recorders, Refs #8266

Note: See TracTickets for help on using tickets.