I'm back with questions - AutoCorrections tool memory usage and resource leaks

[Matthias Welwarsky]

Hello all,

I've not been able to do much work on Digikam for quite some time but I still use it regularely and follow the development. Recently I have
noticed that the AutoCorrections tool seems to drive up memory consumption of digikam a lot and also seems to leak memory since I've
been getting strange crashes of Digikam recently that came from failed memory allocations during DImg construction.

I've looked into it and found two reasons. First is in the constructor of each of the correction filters:

NormalizeFilter::NormalizeFilter(DImg* orgImage, DImg* refImage, QObject* parent)
               : DImgThreadedFilter(orgImage, parent, "NormalizeFilter")
{
    m_refImage = refImage->copy();
    initFilter();
}

Each filter does a deep copy of the reference image. For 12MP 16bit images this amounts to 100MB for each filter. The PreviewList has
one filter instance for each effect, so that's 5 times the memory of the image. Yet the filters don't change the reference image at all, so
detaching from the original data is not necessary. My current quick hack is to change DImg m_refImage to const DImg* m_pRefImage
and correct all the const'ness down the chain but it violates the ImageIface paradigm to never store the DImg* acquired through
ImageIface::getOriginalImage(). Since DImg copies are refcounted it should be possible by other means especially since all uses of the
refImage are const.

The first problem itself does not create a mem leak, it just largely exaggerates the memory usage. The second problem, which leads to
the mem leak is the undefined lifetime of the effect filters, especially those in the PreviewList.

The filter instances are never explicitely destroyed. They are either tied to the lifetime of the AutoCorrectionTool instance or the lifetime of
the PreviewList. But the PreviewList is created and tied to the _parent_ object of the AutoCorrectionTool, which is unfortunately the
ImagePlugin_Core instance, which has near infinite lifetime. I think you get the picture ;)

I hope I didn't annoy you with this long storytelling, but it teaches to be careful with the lifetime of objects, especially since memory
leaks like this don't show up in valgrind, due to the parent/child relationship the parent QObject will always hold a pointer to all its
children and valgrind will not notice the leak.

So, the patch for the PreviewList is trivial, but I would like your advice for fixing the image copying in the effect filters.

best regards,
matthias

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[Gilles Caulier-4]

Hi Mathias,

Yes, you have right. All recent changes in svn trunk come from me.


2010/3/4 Matthias Welwarsky <[hidden email]>:

> Hello all,
>
> I've not been able to do much work on Digikam for quite some time but I still use it regularely and follow the development. Recently I have
> noticed that the AutoCorrections tool seems to drive up memory consumption of digikam a lot and also seems to leak memory since I've
> been getting strange crashes of Digikam recently that came from failed memory allocations during DImg construction.
>
> I've looked into it and found two reasons. First is in the constructor of each of the correction filters:
>
> NormalizeFilter::NormalizeFilter(DImg* orgImage, DImg* refImage, QObject* parent)
>               : DImgThreadedFilter(orgImage, parent, "NormalizeFilter")
> {
>    m_refImage = refImage->copy();
>    initFilter();
> }
>
> Each filter does a deep copy of the reference image. For 12MP 16bit images this amounts to 100MB for each filter. The PreviewList has
> one filter instance for each effect, so that's 5 times the memory of the image. Yet the filters don't change the reference image at all, so
> detaching from the original data is not necessary. My current quick hack is to change DImg m_refImage to const DImg* m_pRefImage
> and correct all the const'ness down the chain but it violates the ImageIface paradigm to never store the DImg* acquired through
> ImageIface::getOriginalImage(). Since DImg copies are refcounted it should be possible by other means especially since all uses of the
> refImage are const.
>
> The first problem itself does not create a mem leak, it just largely exaggerates the memory usage.

The deep copy refimage is not necessary here of course. it can be removed.

> The second problem, which leads to
> the mem leak is the undefined lifetime of the effect filters, especially those in the PreviewList.
>
> The filter instances are never explicitely destroyed. They are either tied to the lifetime of the AutoCorrectionTool instance or the lifetime of
> the PreviewList. But the PreviewList is created and tied to the _parent_ object of the AutoCorrectionTool, which is unfortunately the
> ImagePlugin_Core instance, which has near infinite lifetime. I think you get the picture ;)
>

What the real solution here ? To pass a widget object as parent
instead image_plugin core instance ?

> I hope I didn't annoy you with this long storytelling, but it teaches to be careful with the lifetime of objects, especially since memory
> leaks like this don't show up in valgrind, due to the parent/child relationship the parent QObject will always hold a pointer to all its
> children and valgrind will not notice the leak.
>
> So, the patch for the PreviewList is trivial, but I would like your advice for fixing the image copying in the >effect filters.

Well, i'm waiting your patch...

Gilles
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[Matthias Welwarsky]

Hi Gilles,

On Wednesday 03 March 2010 11:06:01 you wrote:
> Hi Mathias,
>
> Yes, you have right. All recent changes in svn trunk come from me.
> The deep copy refimage is not necessary here of course. it can be removed.

OK, how would the code look like then?

m_refImage = *refImage?

>
> > The second problem, which leads to
> > the mem leak is the undefined lifetime of the effect filters, especially
> > those in the PreviewList.
> >
> > The filter instances are never explicitely destroyed. They are either
> > tied to the lifetime of the AutoCorrectionTool instance or the lifetime
> > of the PreviewList. But the PreviewList is created and tied to the
> > _parent_ object of the AutoCorrectionTool, which is unfortunately the
> > ImagePlugin_Core instance, which has near infinite lifetime. I think you
> > get the picture ;)
>
> What the real solution here ? To pass a widget object as parent
> instead image_plugin core instance ?

I tried that, by creating PreviewList as with "this" as the parent object, but
I get a crash on application shutdown. For now my solution is to explicitly
delete the PreviewList in the AutoCorrectionTool destructor. It's a bit
strange, I thought that PreviewList would be destroyed together with
AutoCorrectionTool due to the parent/child relation, but it's not the case, I
see the AutoCorrectionTool destructor execute but not the PreviewList
destructor. PreviewList is destructed on application shutdown only and there I
get a crash.

>
> > I hope I didn't annoy you with this long storytelling, but it teaches to
> > be careful with the lifetime of objects, especially since memory leaks
> > like this don't show up in valgrind, due to the parent/child relationship
> > the parent QObject will always hold a pointer to all its children and
> > valgrind will not notice the leak.
> >
> > So, the patch for the PreviewList is trivial, but I would like your
> > advice for fixing the image copying in the >effect filters.
>
> Well, i'm waiting your patch...

OK. Will post something tonight.

regards,
matthias

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[Gilles Caulier-4]

2010/3/3 Matthias Welwarsky <[hidden email]>:

> Hi Gilles,
>
> On Wednesday 03 March 2010 11:06:01 you wrote:
>> Hi Mathias,
>>
>> Yes, you have right. All recent changes in svn trunk come from me.
>> The deep copy refimage is not necessary here of course. it can be removed.
>
> OK, how would the code look like then?
>
> m_refImage = *refImage?

yes, it's look fine.

>
>>
>> > The second problem, which leads to
>> > the mem leak is the undefined lifetime of the effect filters, especially
>> > those in the PreviewList.
>> >
>> > The filter instances are never explicitely destroyed. They are either
>> > tied to the lifetime of the AutoCorrectionTool instance or the lifetime
>> > of the PreviewList. But the PreviewList is created and tied to the
>> > _parent_ object of the AutoCorrectionTool, which is unfortunately the
>> > ImagePlugin_Core instance, which has near infinite lifetime. I think you
>> > get the picture ;)
>>
>> What the real solution here ? To pass a widget object as parent
>> instead image_plugin core instance ?
>
> I tried that, by creating PreviewList as with "this" as the parent object, but
> I get a crash on application shutdown. For now my solution is to explicitly
> delete the PreviewList in the AutoCorrectionTool destructor. It's a bit
> strange, I thought that PreviewList would be destroyed together with
> AutoCorrectionTool due to the parent/child relation, but it's not the case, I
> see the AutoCorrectionTool destructor execute but not the PreviewList
> destructor. PreviewList is destructed on application shutdown only and there I
> get a crash.
>

It's not due to use deleteLater() ?

http://lxr.kde.org/source/extragear/graphics/digikam/utilities/imageeditor/widgets/previewlist.cpp#131

Gilles
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[Matthias Welwarsky]

Hi Gilles,

On Wednesday 03 March 2010 11:06:01 Gilles Caulier wrote:
> Well, i'm waiting your patch...

I've committed my changes. It seems to work, but it's difficult to evaluate
the effect. You can use the "pmap" tool to monitor the memory map of the
process. At least big allocations like whole images show up in the list as
individual "[anon]" mappings.

Interestingly I still see large memory blocks allocated even after closing the
image editor, but they don't seem to become more and more.

regards,
matthias


> _______________________________________________
> Digikam-devel mailing list
> [hidden email]
> https://mail.kde.org/mailman/listinfo/digikam-devel
>

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel