Re: Memory leak from list

[Scott Kostyshak]

On Sun, Feb 23, 2020 at 05:00:43PM -0500, Richard Kimberly Heck wrote:
> On 2/23/20 4:11 PM, Scott Kostyshak wrote:
> > On Sun, Feb 23, 2020 at 03:54:06PM -0500, Richard Kimberly Heck wrote:
> >> On 2/23/20 2:31 PM, Scott Kostyshak wrote:
> >>> On Sun, Feb 23, 2020 at 12:50:42PM -0500, Richard Kimberly Heck wrote:
>  On 2/23/20 8:23 AM, Scott Kostyshak wrote:
> > On Tue, Feb 18, 2020 at 08:28:33PM -0500, Scott Kostyshak wrote:
> >> On Tue, Feb 18, 2020 at 07:33:39PM -0500, Richard Kimberly Heck wrote:
> >>> On 2/18/20 6:07 PM, Scott Kostyshak wrote:
>  Valgrind gave me the following error:
> 
>    ==732== 112 (72 direct, 40 indirect) bytes in 1 blocks are 
>  definitely lost in loss record 5,165 of 5,862
>    ==732==at 0x483AE63: operator new(unsigned long) (in 
>  /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
>    ==732==by 0x103A62D: lyx::Buffer::cloneBufferOnly() const 
>  (Buffer.cpp:661)
>    ==732==by 0x11E583C: lyx::(anonymous 
>  namespace)::copyToTempBuffer(lyx::ParagraphList const&, 
>  std::shared_ptr) (CutAndPaste.cpp:582)
>    ==732==by 0x11E5C6A: lyx::(anonymous 
>  namespace)::putClipboard(lyx::ParagraphList const&, 
>  std::shared_ptr, 
>  std::__cxx11::basic_string, 
>  std::allocator > const&, lyx::BufferParams) 
>  (CutAndPaste.cpp:613)
>    ==732==by 0x11E910B: lyx::cap::copySelection(lyx::Cursor 
>  const&, std::__cxx11::basic_string  std::char_traits, std::allocator > const&) 
>  (CutAndPaste.cpp:1123)
>    ==732==by 0x11E84F8: lyx::cap::copySelection(lyx::Cursor 
>  const&) (CutAndPaste.cpp:1024)
>    ==732==by 0x13ECDAB: lyx::Text::dispatch(lyx::Cursor&, 
>  lyx::FuncRequest&) (Text3.cpp:1593)
>    ==732==by 0x1767E04: lyx::InsetText::doDispatch(lyx::Cursor&, 
>  lyx::FuncRequest&) (InsetText.cpp:339)
>    ==732==by 0x15FFC39: lyx::Inset::dispatch(lyx::Cursor&, 
>  lyx::FuncRequest&) (Inset.cpp:325)
>    ==732==by 0x11D1B13: lyx::Cursor::dispatch(lyx::FuncRequest 
>  const&) (Cursor.cpp:825)
>    ==732==by 0x1816F4F: 
>  lyx::frontend::GuiView::dispatchToBufferView(lyx::FuncRequest 
>  const&, lyx::DispatchResult&) (GuiView.cpp:3878)
>    ==732==by 0x181B959: 
>  lyx::frontend::GuiView::dispatch(lyx::FuncRequest const&, 
>  lyx::DispatchResult&) (GuiView.cpp:4569)
> 
>  It comes from the following line (Buffer.cpp:661):
> 
>    cloned_buffers.push_back(new CloneList);
> 
>  Currently cloned_buffers is a list. Would it make sense 
>  to make it a list of *smart* pointers instead? Alternatively we 
>  could make a class and then make a custom destructor that would free 
>  the CloneLists that the list elements point to?
> >>> This is some kind of thinko, probably on my part. The code at line 549
> >>> was supposed to be cleaning this up, but it actually only removes the
> >>> entry from the list.
> >>>
> >>> If it works to make it a smart pointer of some kind, then that would 
> >>> be
> >>> simplest. But I think we could just do something like:
> >>>
> >>> else {
> >>>     delete(*it);
> >>>     cloned_buffers.erase(it);
> >>> }
> >> Ah that makes sense.
> > Riki, I propose that you commit. Thanks for the fix.
>  Just to check: You've verified this fixes the problem?
> >>> I just tried to reproduce the original error (without the fix) and could
> >>> not. I originally got the message from copying something and compiling.
> >>> I just tried doing that with the Customization and Embedded Objects
> >>> manuals, but I could not trigger the error.
> >> Committed, then.
> > Thanks for the fix. I'll continue testing LyX with Valgrind at some
> > point in the future. Right now I'm tired of the waiting. I might look
> > into the suggestions that Neven gave.
> 
> Actually, that is all wrong, and now I'm very confused. The patch causes
> a double delete exception here, so I have reverted it. The line
> 
>     delete d->clone_list_;
> 
> already does this deletion, since *it just is d->clone_list_.
> 
> I've committed another patch with a shared_ptr. That has at least to help.

OK thanks for your work on this. I'll keep an eye out when testing with
Valgrind to see if something related comes up.

Scott


signature.asc
Description: PGP signature
-- 
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

Re: Memory leak from list

[Richard Kimberly Heck]

On 2/23/20 4:11 PM, Scott Kostyshak wrote:
> On Sun, Feb 23, 2020 at 03:54:06PM -0500, Richard Kimberly Heck wrote:
>> On 2/23/20 2:31 PM, Scott Kostyshak wrote:
>>> On Sun, Feb 23, 2020 at 12:50:42PM -0500, Richard Kimberly Heck wrote:
 On 2/23/20 8:23 AM, Scott Kostyshak wrote:
> On Tue, Feb 18, 2020 at 08:28:33PM -0500, Scott Kostyshak wrote:
>> On Tue, Feb 18, 2020 at 07:33:39PM -0500, Richard Kimberly Heck wrote:
>>> On 2/18/20 6:07 PM, Scott Kostyshak wrote:
 Valgrind gave me the following error:

   ==732== 112 (72 direct, 40 indirect) bytes in 1 blocks are 
 definitely lost in loss record 5,165 of 5,862
   ==732==at 0x483AE63: operator new(unsigned long) (in 
 /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
   ==732==by 0x103A62D: lyx::Buffer::cloneBufferOnly() const 
 (Buffer.cpp:661)
   ==732==by 0x11E583C: lyx::(anonymous 
 namespace)::copyToTempBuffer(lyx::ParagraphList const&, 
 std::shared_ptr) (CutAndPaste.cpp:582)
   ==732==by 0x11E5C6A: lyx::(anonymous 
 namespace)::putClipboard(lyx::ParagraphList const&, 
 std::shared_ptr, 
 std::__cxx11::basic_string, 
 std::allocator > const&, lyx::BufferParams) 
 (CutAndPaste.cpp:613)
   ==732==by 0x11E910B: lyx::cap::copySelection(lyx::Cursor const&, 
 std::__cxx11::basic_string, 
 std::allocator > const&) (CutAndPaste.cpp:1123)
   ==732==by 0x11E84F8: lyx::cap::copySelection(lyx::Cursor const&) 
 (CutAndPaste.cpp:1024)
   ==732==by 0x13ECDAB: lyx::Text::dispatch(lyx::Cursor&, 
 lyx::FuncRequest&) (Text3.cpp:1593)
   ==732==by 0x1767E04: lyx::InsetText::doDispatch(lyx::Cursor&, 
 lyx::FuncRequest&) (InsetText.cpp:339)
   ==732==by 0x15FFC39: lyx::Inset::dispatch(lyx::Cursor&, 
 lyx::FuncRequest&) (Inset.cpp:325)
   ==732==by 0x11D1B13: lyx::Cursor::dispatch(lyx::FuncRequest 
 const&) (Cursor.cpp:825)
   ==732==by 0x1816F4F: 
 lyx::frontend::GuiView::dispatchToBufferView(lyx::FuncRequest const&, 
 lyx::DispatchResult&) (GuiView.cpp:3878)
   ==732==by 0x181B959: 
 lyx::frontend::GuiView::dispatch(lyx::FuncRequest const&, 
 lyx::DispatchResult&) (GuiView.cpp:4569)

 It comes from the following line (Buffer.cpp:661):

   cloned_buffers.push_back(new CloneList);

 Currently cloned_buffers is a list. Would it make sense 
 to make it a list of *smart* pointers instead? Alternatively we could 
 make a class and then make a custom destructor that would free the 
 CloneLists that the list elements point to?
>>> This is some kind of thinko, probably on my part. The code at line 549
>>> was supposed to be cleaning this up, but it actually only removes the
>>> entry from the list.
>>>
>>> If it works to make it a smart pointer of some kind, then that would be
>>> simplest. But I think we could just do something like:
>>>
>>> else {
>>>     delete(*it);
>>>     cloned_buffers.erase(it);
>>> }
>> Ah that makes sense.
> Riki, I propose that you commit. Thanks for the fix.
 Just to check: You've verified this fixes the problem?
>>> I just tried to reproduce the original error (without the fix) and could
>>> not. I originally got the message from copying something and compiling.
>>> I just tried doing that with the Customization and Embedded Objects
>>> manuals, but I could not trigger the error.
>> Committed, then.
> Thanks for the fix. I'll continue testing LyX with Valgrind at some
> point in the future. Right now I'm tired of the waiting. I might look
> into the suggestions that Neven gave.

Actually, that is all wrong, and now I'm very confused. The patch causes
a double delete exception here, so I have reverted it. The line

    delete d->clone_list_;

already does this deletion, since *it just is d->clone_list_.

I've committed another patch with a shared_ptr. That has at least to help.

Riki


-- 
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

Re: Memory leak from list

[Scott Kostyshak]

On Sun, Feb 23, 2020 at 03:54:06PM -0500, Richard Kimberly Heck wrote:
> On 2/23/20 2:31 PM, Scott Kostyshak wrote:
> > On Sun, Feb 23, 2020 at 12:50:42PM -0500, Richard Kimberly Heck wrote:
> >> On 2/23/20 8:23 AM, Scott Kostyshak wrote:
> >>> On Tue, Feb 18, 2020 at 08:28:33PM -0500, Scott Kostyshak wrote:
>  On Tue, Feb 18, 2020 at 07:33:39PM -0500, Richard Kimberly Heck wrote:
> > On 2/18/20 6:07 PM, Scott Kostyshak wrote:
> >> Valgrind gave me the following error:
> >>
> >>   ==732== 112 (72 direct, 40 indirect) bytes in 1 blocks are 
> >> definitely lost in loss record 5,165 of 5,862
> >>   ==732==at 0x483AE63: operator new(unsigned long) (in 
> >> /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
> >>   ==732==by 0x103A62D: lyx::Buffer::cloneBufferOnly() const 
> >> (Buffer.cpp:661)
> >>   ==732==by 0x11E583C: lyx::(anonymous 
> >> namespace)::copyToTempBuffer(lyx::ParagraphList const&, 
> >> std::shared_ptr) (CutAndPaste.cpp:582)
> >>   ==732==by 0x11E5C6A: lyx::(anonymous 
> >> namespace)::putClipboard(lyx::ParagraphList const&, 
> >> std::shared_ptr, 
> >> std::__cxx11::basic_string, 
> >> std::allocator > const&, lyx::BufferParams) 
> >> (CutAndPaste.cpp:613)
> >>   ==732==by 0x11E910B: lyx::cap::copySelection(lyx::Cursor const&, 
> >> std::__cxx11::basic_string, 
> >> std::allocator > const&) (CutAndPaste.cpp:1123)
> >>   ==732==by 0x11E84F8: lyx::cap::copySelection(lyx::Cursor const&) 
> >> (CutAndPaste.cpp:1024)
> >>   ==732==by 0x13ECDAB: lyx::Text::dispatch(lyx::Cursor&, 
> >> lyx::FuncRequest&) (Text3.cpp:1593)
> >>   ==732==by 0x1767E04: lyx::InsetText::doDispatch(lyx::Cursor&, 
> >> lyx::FuncRequest&) (InsetText.cpp:339)
> >>   ==732==by 0x15FFC39: lyx::Inset::dispatch(lyx::Cursor&, 
> >> lyx::FuncRequest&) (Inset.cpp:325)
> >>   ==732==by 0x11D1B13: lyx::Cursor::dispatch(lyx::FuncRequest 
> >> const&) (Cursor.cpp:825)
> >>   ==732==by 0x1816F4F: 
> >> lyx::frontend::GuiView::dispatchToBufferView(lyx::FuncRequest const&, 
> >> lyx::DispatchResult&) (GuiView.cpp:3878)
> >>   ==732==by 0x181B959: 
> >> lyx::frontend::GuiView::dispatch(lyx::FuncRequest const&, 
> >> lyx::DispatchResult&) (GuiView.cpp:4569)
> >>
> >> It comes from the following line (Buffer.cpp:661):
> >>
> >>   cloned_buffers.push_back(new CloneList);
> >>
> >> Currently cloned_buffers is a list. Would it make sense 
> >> to make it a list of *smart* pointers instead? Alternatively we could 
> >> make a class and then make a custom destructor that would free the 
> >> CloneLists that the list elements point to?
> > This is some kind of thinko, probably on my part. The code at line 549
> > was supposed to be cleaning this up, but it actually only removes the
> > entry from the list.
> >
> > If it works to make it a smart pointer of some kind, then that would be
> > simplest. But I think we could just do something like:
> >
> > else {
> >     delete(*it);
> >     cloned_buffers.erase(it);
> > }
>  Ah that makes sense.
> >>> Riki, I propose that you commit. Thanks for the fix.
> >> Just to check: You've verified this fixes the problem?
> > I just tried to reproduce the original error (without the fix) and could
> > not. I originally got the message from copying something and compiling.
> > I just tried doing that with the Customization and Embedded Objects
> > manuals, but I could not trigger the error.
> 
> Committed, then.

Thanks for the fix. I'll continue testing LyX with Valgrind at some
point in the future. Right now I'm tired of the waiting. I might look
into the suggestions that Neven gave.

Scott


signature.asc
Description: PGP signature
-- 
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

Re: Memory leak from list

[Richard Kimberly Heck]

On 2/23/20 2:31 PM, Scott Kostyshak wrote:
> On Sun, Feb 23, 2020 at 12:50:42PM -0500, Richard Kimberly Heck wrote:
>> On 2/23/20 8:23 AM, Scott Kostyshak wrote:
>>> On Tue, Feb 18, 2020 at 08:28:33PM -0500, Scott Kostyshak wrote:
 On Tue, Feb 18, 2020 at 07:33:39PM -0500, Richard Kimberly Heck wrote:
> On 2/18/20 6:07 PM, Scott Kostyshak wrote:
>> Valgrind gave me the following error:
>>
>>   ==732== 112 (72 direct, 40 indirect) bytes in 1 blocks are definitely 
>> lost in loss record 5,165 of 5,862
>>   ==732==at 0x483AE63: operator new(unsigned long) (in 
>> /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
>>   ==732==by 0x103A62D: lyx::Buffer::cloneBufferOnly() const 
>> (Buffer.cpp:661)
>>   ==732==by 0x11E583C: lyx::(anonymous 
>> namespace)::copyToTempBuffer(lyx::ParagraphList const&, 
>> std::shared_ptr) (CutAndPaste.cpp:582)
>>   ==732==by 0x11E5C6A: lyx::(anonymous 
>> namespace)::putClipboard(lyx::ParagraphList const&, 
>> std::shared_ptr, 
>> std::__cxx11::basic_string, 
>> std::allocator > const&, lyx::BufferParams) 
>> (CutAndPaste.cpp:613)
>>   ==732==by 0x11E910B: lyx::cap::copySelection(lyx::Cursor const&, 
>> std::__cxx11::basic_string, 
>> std::allocator > const&) (CutAndPaste.cpp:1123)
>>   ==732==by 0x11E84F8: lyx::cap::copySelection(lyx::Cursor const&) 
>> (CutAndPaste.cpp:1024)
>>   ==732==by 0x13ECDAB: lyx::Text::dispatch(lyx::Cursor&, 
>> lyx::FuncRequest&) (Text3.cpp:1593)
>>   ==732==by 0x1767E04: lyx::InsetText::doDispatch(lyx::Cursor&, 
>> lyx::FuncRequest&) (InsetText.cpp:339)
>>   ==732==by 0x15FFC39: lyx::Inset::dispatch(lyx::Cursor&, 
>> lyx::FuncRequest&) (Inset.cpp:325)
>>   ==732==by 0x11D1B13: lyx::Cursor::dispatch(lyx::FuncRequest 
>> const&) (Cursor.cpp:825)
>>   ==732==by 0x1816F4F: 
>> lyx::frontend::GuiView::dispatchToBufferView(lyx::FuncRequest const&, 
>> lyx::DispatchResult&) (GuiView.cpp:3878)
>>   ==732==by 0x181B959: 
>> lyx::frontend::GuiView::dispatch(lyx::FuncRequest const&, 
>> lyx::DispatchResult&) (GuiView.cpp:4569)
>>
>> It comes from the following line (Buffer.cpp:661):
>>
>>   cloned_buffers.push_back(new CloneList);
>>
>> Currently cloned_buffers is a list. Would it make sense to 
>> make it a list of *smart* pointers instead? Alternatively we could make 
>> a class and then make a custom destructor that would free the CloneLists 
>> that the list elements point to?
> This is some kind of thinko, probably on my part. The code at line 549
> was supposed to be cleaning this up, but it actually only removes the
> entry from the list.
>
> If it works to make it a smart pointer of some kind, then that would be
> simplest. But I think we could just do something like:
>
> else {
>     delete(*it);
>     cloned_buffers.erase(it);
> }
 Ah that makes sense.
>>> Riki, I propose that you commit. Thanks for the fix.
>> Just to check: You've verified this fixes the problem?
> I just tried to reproduce the original error (without the fix) and could
> not. I originally got the message from copying something and compiling.
> I just tried doing that with the Customization and Embedded Objects
> manuals, but I could not trigger the error.

Committed, then.

Riki


-- 
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

Re: Memory leak from list

[Scott Kostyshak]

On Sun, Feb 23, 2020 at 12:50:42PM -0500, Richard Kimberly Heck wrote:
> On 2/23/20 8:23 AM, Scott Kostyshak wrote:
> > On Tue, Feb 18, 2020 at 08:28:33PM -0500, Scott Kostyshak wrote:
> >> On Tue, Feb 18, 2020 at 07:33:39PM -0500, Richard Kimberly Heck wrote:
> >>> On 2/18/20 6:07 PM, Scott Kostyshak wrote:
>  Valgrind gave me the following error:
> 
>    ==732== 112 (72 direct, 40 indirect) bytes in 1 blocks are definitely 
>  lost in loss record 5,165 of 5,862
>    ==732==at 0x483AE63: operator new(unsigned long) (in 
>  /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
>    ==732==by 0x103A62D: lyx::Buffer::cloneBufferOnly() const 
>  (Buffer.cpp:661)
>    ==732==by 0x11E583C: lyx::(anonymous 
>  namespace)::copyToTempBuffer(lyx::ParagraphList const&, 
>  std::shared_ptr) (CutAndPaste.cpp:582)
>    ==732==by 0x11E5C6A: lyx::(anonymous 
>  namespace)::putClipboard(lyx::ParagraphList const&, 
>  std::shared_ptr, 
>  std::__cxx11::basic_string, 
>  std::allocator > const&, lyx::BufferParams) 
>  (CutAndPaste.cpp:613)
>    ==732==by 0x11E910B: lyx::cap::copySelection(lyx::Cursor const&, 
>  std::__cxx11::basic_string, 
>  std::allocator > const&) (CutAndPaste.cpp:1123)
>    ==732==by 0x11E84F8: lyx::cap::copySelection(lyx::Cursor const&) 
>  (CutAndPaste.cpp:1024)
>    ==732==by 0x13ECDAB: lyx::Text::dispatch(lyx::Cursor&, 
>  lyx::FuncRequest&) (Text3.cpp:1593)
>    ==732==by 0x1767E04: lyx::InsetText::doDispatch(lyx::Cursor&, 
>  lyx::FuncRequest&) (InsetText.cpp:339)
>    ==732==by 0x15FFC39: lyx::Inset::dispatch(lyx::Cursor&, 
>  lyx::FuncRequest&) (Inset.cpp:325)
>    ==732==by 0x11D1B13: lyx::Cursor::dispatch(lyx::FuncRequest 
>  const&) (Cursor.cpp:825)
>    ==732==by 0x1816F4F: 
>  lyx::frontend::GuiView::dispatchToBufferView(lyx::FuncRequest const&, 
>  lyx::DispatchResult&) (GuiView.cpp:3878)
>    ==732==by 0x181B959: 
>  lyx::frontend::GuiView::dispatch(lyx::FuncRequest const&, 
>  lyx::DispatchResult&) (GuiView.cpp:4569)
> 
>  It comes from the following line (Buffer.cpp:661):
> 
>    cloned_buffers.push_back(new CloneList);
> 
>  Currently cloned_buffers is a list. Would it make sense to 
>  make it a list of *smart* pointers instead? Alternatively we could make 
>  a class and then make a custom destructor that would free the CloneLists 
>  that the list elements point to?
> >>> This is some kind of thinko, probably on my part. The code at line 549
> >>> was supposed to be cleaning this up, but it actually only removes the
> >>> entry from the list.
> >>>
> >>> If it works to make it a smart pointer of some kind, then that would be
> >>> simplest. But I think we could just do something like:
> >>>
> >>> else {
> >>>     delete(*it);
> >>>     cloned_buffers.erase(it);
> >>> }
> >> Ah that makes sense.
> > Riki, I propose that you commit. Thanks for the fix.
> 
> Just to check: You've verified this fixes the problem?

I just tried to reproduce the original error (without the fix) and could
not. I originally got the message from copying something and compiling.
I just tried doing that with the Customization and Embedded Objects
manuals, but I could not trigger the error.

Scott


signature.asc
Description: PGP signature
-- 
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

Re: Memory leak from list

[Richard Kimberly Heck]

On 2/23/20 8:23 AM, Scott Kostyshak wrote:
> On Tue, Feb 18, 2020 at 08:28:33PM -0500, Scott Kostyshak wrote:
>> On Tue, Feb 18, 2020 at 07:33:39PM -0500, Richard Kimberly Heck wrote:
>>> On 2/18/20 6:07 PM, Scott Kostyshak wrote:
 Valgrind gave me the following error:

   ==732== 112 (72 direct, 40 indirect) bytes in 1 blocks are definitely 
 lost in loss record 5,165 of 5,862
   ==732==at 0x483AE63: operator new(unsigned long) (in 
 /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
   ==732==by 0x103A62D: lyx::Buffer::cloneBufferOnly() const 
 (Buffer.cpp:661)
   ==732==by 0x11E583C: lyx::(anonymous 
 namespace)::copyToTempBuffer(lyx::ParagraphList const&, 
 std::shared_ptr) (CutAndPaste.cpp:582)
   ==732==by 0x11E5C6A: lyx::(anonymous 
 namespace)::putClipboard(lyx::ParagraphList const&, 
 std::shared_ptr, 
 std::__cxx11::basic_string, 
 std::allocator > const&, lyx::BufferParams) (CutAndPaste.cpp:613)
   ==732==by 0x11E910B: lyx::cap::copySelection(lyx::Cursor const&, 
 std::__cxx11::basic_string, 
 std::allocator > const&) (CutAndPaste.cpp:1123)
   ==732==by 0x11E84F8: lyx::cap::copySelection(lyx::Cursor const&) 
 (CutAndPaste.cpp:1024)
   ==732==by 0x13ECDAB: lyx::Text::dispatch(lyx::Cursor&, 
 lyx::FuncRequest&) (Text3.cpp:1593)
   ==732==by 0x1767E04: lyx::InsetText::doDispatch(lyx::Cursor&, 
 lyx::FuncRequest&) (InsetText.cpp:339)
   ==732==by 0x15FFC39: lyx::Inset::dispatch(lyx::Cursor&, 
 lyx::FuncRequest&) (Inset.cpp:325)
   ==732==by 0x11D1B13: lyx::Cursor::dispatch(lyx::FuncRequest const&) 
 (Cursor.cpp:825)
   ==732==by 0x1816F4F: 
 lyx::frontend::GuiView::dispatchToBufferView(lyx::FuncRequest const&, 
 lyx::DispatchResult&) (GuiView.cpp:3878)
   ==732==by 0x181B959: 
 lyx::frontend::GuiView::dispatch(lyx::FuncRequest const&, 
 lyx::DispatchResult&) (GuiView.cpp:4569)

 It comes from the following line (Buffer.cpp:661):

   cloned_buffers.push_back(new CloneList);

 Currently cloned_buffers is a list. Would it make sense to 
 make it a list of *smart* pointers instead? Alternatively we could make a 
 class and then make a custom destructor that would free the CloneLists 
 that the list elements point to?
>>> This is some kind of thinko, probably on my part. The code at line 549
>>> was supposed to be cleaning this up, but it actually only removes the
>>> entry from the list.
>>>
>>> If it works to make it a smart pointer of some kind, then that would be
>>> simplest. But I think we could just do something like:
>>>
>>> else {
>>>     delete(*it);
>>>     cloned_buffers.erase(it);
>>> }
>> Ah that makes sense.
> Riki, I propose that you commit. Thanks for the fix.

Just to check: You've verified this fixes the problem?

Riki


-- 
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

Re: Memory leak from list

[Scott Kostyshak]

On Tue, Feb 18, 2020 at 08:28:33PM -0500, Scott Kostyshak wrote:
> On Tue, Feb 18, 2020 at 07:33:39PM -0500, Richard Kimberly Heck wrote:
> > On 2/18/20 6:07 PM, Scott Kostyshak wrote:
> > > Valgrind gave me the following error:
> > >
> > >   ==732== 112 (72 direct, 40 indirect) bytes in 1 blocks are definitely 
> > > lost in loss record 5,165 of 5,862
> > >   ==732==at 0x483AE63: operator new(unsigned long) (in 
> > > /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
> > >   ==732==by 0x103A62D: lyx::Buffer::cloneBufferOnly() const 
> > > (Buffer.cpp:661)
> > >   ==732==by 0x11E583C: lyx::(anonymous 
> > > namespace)::copyToTempBuffer(lyx::ParagraphList const&, 
> > > std::shared_ptr) (CutAndPaste.cpp:582)
> > >   ==732==by 0x11E5C6A: lyx::(anonymous 
> > > namespace)::putClipboard(lyx::ParagraphList const&, 
> > > std::shared_ptr, 
> > > std::__cxx11::basic_string, 
> > > std::allocator > const&, lyx::BufferParams) (CutAndPaste.cpp:613)
> > >   ==732==by 0x11E910B: lyx::cap::copySelection(lyx::Cursor const&, 
> > > std::__cxx11::basic_string, 
> > > std::allocator > const&) (CutAndPaste.cpp:1123)
> > >   ==732==by 0x11E84F8: lyx::cap::copySelection(lyx::Cursor const&) 
> > > (CutAndPaste.cpp:1024)
> > >   ==732==by 0x13ECDAB: lyx::Text::dispatch(lyx::Cursor&, 
> > > lyx::FuncRequest&) (Text3.cpp:1593)
> > >   ==732==by 0x1767E04: lyx::InsetText::doDispatch(lyx::Cursor&, 
> > > lyx::FuncRequest&) (InsetText.cpp:339)
> > >   ==732==by 0x15FFC39: lyx::Inset::dispatch(lyx::Cursor&, 
> > > lyx::FuncRequest&) (Inset.cpp:325)
> > >   ==732==by 0x11D1B13: lyx::Cursor::dispatch(lyx::FuncRequest const&) 
> > > (Cursor.cpp:825)
> > >   ==732==by 0x1816F4F: 
> > > lyx::frontend::GuiView::dispatchToBufferView(lyx::FuncRequest const&, 
> > > lyx::DispatchResult&) (GuiView.cpp:3878)
> > >   ==732==by 0x181B959: 
> > > lyx::frontend::GuiView::dispatch(lyx::FuncRequest const&, 
> > > lyx::DispatchResult&) (GuiView.cpp:4569)
> > >
> > > It comes from the following line (Buffer.cpp:661):
> > >
> > >   cloned_buffers.push_back(new CloneList);
> > >
> > > Currently cloned_buffers is a list. Would it make sense to 
> > > make it a list of *smart* pointers instead? Alternatively we could make a 
> > > class and then make a custom destructor that would free the CloneLists 
> > > that the list elements point to?
> > 
> > This is some kind of thinko, probably on my part. The code at line 549
> > was supposed to be cleaning this up, but it actually only removes the
> > entry from the list.
> > 
> > If it works to make it a smart pointer of some kind, then that would be
> > simplest. But I think we could just do something like:
> > 
> > else {
> >     delete(*it);
> >     cloned_buffers.erase(it);
> > }
> 
> Ah that makes sense.

Riki, I propose that you commit. Thanks for the fix.

Scott


signature.asc
Description: PGP signature
-- 
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

Re: [RFC][PATCH] Fix for potential memory leak in GuiLyXFiles::updateContents

[Stephan Witt]

Am 20.02.2020 um 13:22 schrieb Enrico Forestieri :
> 
> On Thu, Feb 20, 2020 at 09:37:12AM +0100, Stephan Witt wrote:
>> Hi all,
>> 
>> the last one of the potential memory leaks is a little bit trickier.
>> 
>> The loop logic for subcatItem if cats contains catsave doesn’t guarantee
>> a match so a check for the match is needed after all.
>> 
>> This makes me unsure what should be done in this case.
>> Ignore it or warn about it or try something else?
> 
> Please, see suggestion below.

Oh, yes. That’s pretty good. Thanks!

New patch attached.

Stephan

> 
>> 
>> Stephan
>> 
> 
>> [-- mutt.octet.filter file type: "diff output text" --]
>> 
>> diff --git a/src/frontends/qt/GuiLyXFiles.cpp 
>> b/src/frontends/qt/GuiLyXFiles.cpp
>> index 096a390d64..d6f283c397 100644
>> --- a/src/frontends/qt/GuiLyXFiles.cpp
>> +++ b/src/frontends/qt/GuiLyXFiles.cpp
>> @@ -445,7 +445,7 @@ void GuiLyXFiles::updateContents()
>>  if (subcat.isEmpty())
>>  catItem->addChild(item);
>>  else {
>> -QTreeWidgetItem * subcatItem = new QTreeWidgetItem();
>> +QTreeWidgetItem * subcatItem = 0;
> 
> These days nullptr is preferred to an unadorned 0.
> 
>>  if (cats.contains(catsave)) {
>>  QList pcats = 
>> filesLW->findItems(cat, Qt::MatchExactly);
>>  for (int iit = 0; iit < pcats.size(); ++iit) {
>> @@ -457,12 +457,15 @@ void GuiLyXFiles::updateContents()
>>  }
>>  }
> 
> Change this line
>>  } else {
> 
> with
>   }
>   if (!subcatItem) {
>> +subcatItem = new QTreeWidgetItem();
>>  subcatItem->setText(0, subcat);
>>  subcatItem->setIcon(0, file_icon);
>>  cats << catsave;
>>  }
> 
> Drop the following change
> 
>> -subcatItem->addChild(item);
>> -catItem->addChild(subcatItem);
>> +if (subcatItem) {
>> +subcatItem->addChild(item);
>> +catItem->addChild(subcatItem);
>> +}
>>  }
>>  ++it;
>>  }
> 
> 
> -- 
> Enrico
> -- 
> lyx-devel mailing list
> lyx-devel@lists.lyx.org
> http://lists.lyx.org/mailman/listinfo/lyx-devel


GuiLyXFiles-subcatItem-2.patch
Description: Binary data
-- 
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

Re: [RFC][PATCH] Fix for potential memory leak in GuiLyXFiles::updateContents

[Enrico Forestieri]

On Thu, Feb 20, 2020 at 09:37:12AM +0100, Stephan Witt wrote:
> Hi all,
> 
> the last one of the potential memory leaks is a little bit trickier.
> 
> The loop logic for subcatItem if cats contains catsave doesn’t guarantee
> a match so a check for the match is needed after all.
> 
> This makes me unsure what should be done in this case.
> Ignore it or warn about it or try something else?

Please, see suggestion below.

> 
> Stephan
>  

> [-- mutt.octet.filter file type: "diff output text" --]
> 
> diff --git a/src/frontends/qt/GuiLyXFiles.cpp 
> b/src/frontends/qt/GuiLyXFiles.cpp
> index 096a390d64..d6f283c397 100644
> --- a/src/frontends/qt/GuiLyXFiles.cpp
> +++ b/src/frontends/qt/GuiLyXFiles.cpp
> @@ -445,7 +445,7 @@ void GuiLyXFiles::updateContents()
>   if (subcat.isEmpty())
>   catItem->addChild(item);
>   else {
> - QTreeWidgetItem * subcatItem = new QTreeWidgetItem();
> + QTreeWidgetItem * subcatItem = 0;

These days nullptr is preferred to an unadorned 0.

>   if (cats.contains(catsave)) {
>   QList pcats = 
> filesLW->findItems(cat, Qt::MatchExactly);
>   for (int iit = 0; iit < pcats.size(); ++iit) {
> @@ -457,12 +457,15 @@ void GuiLyXFiles::updateContents()
>   }
>   }

Change this line
>   } else {

with
}
if (!subcatItem) {
> + subcatItem = new QTreeWidgetItem();
>   subcatItem->setText(0, subcat);
>   subcatItem->setIcon(0, file_icon);
>   cats << catsave;
>   }

Drop the following change

> - subcatItem->addChild(item);
> - catItem->addChild(subcatItem);
> + if (subcatItem) {
> + subcatItem->addChild(item);
> + catItem->addChild(subcatItem);
> + }
>   }
>   ++it;
>   }


-- 
Enrico
-- 
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

[RFC][PATCH] Fix for potential memory leak in GuiLyXFiles::updateContents

[Stephan Witt]

Hi all,

the last one of the potential memory leaks is a little bit trickier.

The loop logic for subcatItem if cats contains catsave doesn’t guarantee
a match so a check for the match is needed after all.

This makes me unsure what should be done in this case.
Ignore it or warn about it or try something else?

Stephan
 


GuiLyXFiles-subcatItem.patch
Description: Binary data
-- 
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

Re: Memory leak from list

[Scott Kostyshak]

On Wed, Feb 19, 2020 at 08:52:18AM +, Neven Sajko wrote:
> > Well, actually the hardest part is waiting because LyX is very slow when 
> > run under valgrind.
> 
> Try sanitizers instead. They are instrumentation that GCC or Clang can
> include in executables. They do basically the same thing as Valgrind,
> but should be much faster and since you are already compiling your own
> Lyx and Qt, the sanitizers should be a more appropriate tool. There

I do not always compile Qt. On my last attempt to compile Qt I came
across an error and just used the pre-built libraries (and I think the
corresponding -dbg packages). I did not initially realize that compiling
Qt with the address sanitizers would make a big difference, but now I
think I see the intuition: LyX might be responsible for a sanitizer
error at the Qt level, and we would only catch that if we compile Qt
with the address sanitizers.

> are four sanitizers that you may want to compile with: asan, tsan,
> msan, ubsan; each for a different class of bugs.
> 
> Instructions: include -fsanitize=asan (replace asan with wanted
> sanitizer if you want another one) to both compilation and linking
> commands (GCC or Clang) of all libraries and code. It is also a good
> idea to disable some compiler optimizations, in particular one should
> use -fno-omit-frame-pointer. See
> https://github.com/google/sanitizers/wiki for more information.
> 
> By default the error reports will be output to stderr, but it is
> possible to output them to a file by setting an environment variable
> (search for log_path in
> https://github.com/google/sanitizers/wiki/AddressSanitizerFlags if in
> need of that option).
> 
> I hope that was helpful.

Thanks, Neven! This is helpful. I've been meaning to look into tools
like this for a while, and this might be just the push I needed.
Eventually I would like to add these options to the building and testing
scripts at https://github.com/scottkosty/lyx-tester. It would be
interesting to build with a sanitizer and then run our big suite of
automated tests. I made an issue to remind me to look into this,
although I probably won't look into it soon (just because of time
reasons and other pending tasks):

  https://github.com/scottkosty/lyx-tester/issues/1

Scott


signature.asc
Description: PGP signature
-- 
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

Re: Memory leak from list

[Neven Sajko]

My instructions for the C compiler and linker command line were wrong:
instead of -fsanitize=asan , use -fsanitize=address or
-fsanitize=thread or -fsanitize=undefined or -fsanitize=memory .

And, of course, include debugging symbols with "-g".

Regards,
Neven Sajko
-- 
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

Re: Memory leak from list

[Neven Sajko]

> Well, actually the hardest part is waiting because LyX is very slow when run 
> under valgrind.

Try sanitizers instead. They are instrumentation that GCC or Clang can
include in executables. They do basically the same thing as Valgrind,
but should be much faster and since you are already compiling your own
Lyx and Qt, the sanitizers should be a more appropriate tool. There
are four sanitizers that you may want to compile with: asan, tsan,
msan, ubsan; each for a different class of bugs.

Instructions: include -fsanitize=asan (replace asan with wanted
sanitizer if you want another one) to both compilation and linking
commands (GCC or Clang) of all libraries and code. It is also a good
idea to disable some compiler optimizations, in particular one should
use -fno-omit-frame-pointer. See
https://github.com/google/sanitizers/wiki for more information.

By default the error reports will be output to stderr, but it is
possible to output them to a file by setting an environment variable
(search for log_path in
https://github.com/google/sanitizers/wiki/AddressSanitizerFlags if in
need of that option).

I hope that was helpful.

Regards,
Neven Sajko
-- 
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

Re: Memory leak from list

[Scott Kostyshak]

On Tue, Feb 18, 2020 at 07:33:39PM -0500, Richard Kimberly Heck wrote:
> On 2/18/20 6:07 PM, Scott Kostyshak wrote:
> > Valgrind gave me the following error:
> >
> >   ==732== 112 (72 direct, 40 indirect) bytes in 1 blocks are definitely 
> > lost in loss record 5,165 of 5,862
> >   ==732==at 0x483AE63: operator new(unsigned long) (in 
> > /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
> >   ==732==by 0x103A62D: lyx::Buffer::cloneBufferOnly() const 
> > (Buffer.cpp:661)
> >   ==732==by 0x11E583C: lyx::(anonymous 
> > namespace)::copyToTempBuffer(lyx::ParagraphList const&, 
> > std::shared_ptr) (CutAndPaste.cpp:582)
> >   ==732==by 0x11E5C6A: lyx::(anonymous 
> > namespace)::putClipboard(lyx::ParagraphList const&, 
> > std::shared_ptr, 
> > std::__cxx11::basic_string, 
> > std::allocator > const&, lyx::BufferParams) (CutAndPaste.cpp:613)
> >   ==732==by 0x11E910B: lyx::cap::copySelection(lyx::Cursor const&, 
> > std::__cxx11::basic_string, 
> > std::allocator > const&) (CutAndPaste.cpp:1123)
> >   ==732==by 0x11E84F8: lyx::cap::copySelection(lyx::Cursor const&) 
> > (CutAndPaste.cpp:1024)
> >   ==732==by 0x13ECDAB: lyx::Text::dispatch(lyx::Cursor&, 
> > lyx::FuncRequest&) (Text3.cpp:1593)
> >   ==732==by 0x1767E04: lyx::InsetText::doDispatch(lyx::Cursor&, 
> > lyx::FuncRequest&) (InsetText.cpp:339)
> >   ==732==by 0x15FFC39: lyx::Inset::dispatch(lyx::Cursor&, 
> > lyx::FuncRequest&) (Inset.cpp:325)
> >   ==732==by 0x11D1B13: lyx::Cursor::dispatch(lyx::FuncRequest const&) 
> > (Cursor.cpp:825)
> >   ==732==by 0x1816F4F: 
> > lyx::frontend::GuiView::dispatchToBufferView(lyx::FuncRequest const&, 
> > lyx::DispatchResult&) (GuiView.cpp:3878)
> >   ==732==by 0x181B959: 
> > lyx::frontend::GuiView::dispatch(lyx::FuncRequest const&, 
> > lyx::DispatchResult&) (GuiView.cpp:4569)
> >
> > It comes from the following line (Buffer.cpp:661):
> >
> >   cloned_buffers.push_back(new CloneList);
> >
> > Currently cloned_buffers is a list. Would it make sense to 
> > make it a list of *smart* pointers instead? Alternatively we could make a 
> > class and then make a custom destructor that would free the CloneLists that 
> > the list elements point to?
> 
> This is some kind of thinko, probably on my part. The code at line 549
> was supposed to be cleaning this up, but it actually only removes the
> entry from the list.
> 
> If it works to make it a smart pointer of some kind, then that would be
> simplest. But I think we could just do something like:
> 
> else {
>     delete(*it);
>     cloned_buffers.erase(it);
> }

Ah that makes sense.

> I should learn how to use valgrind. Then I could solve these problems
> myself

The hard part (for me) is interpreting the results. Well, actually the
hardest part is waiting because LyX is very slow when run under
valgrind. Probably there are some options to mess with to speed things
up a bit. The command I run is this:

  valgrind --leak-check=full --track-origins=yes --log-file=valgrind.log 
~/lyxbuilds/master/CMakeBuild/bin/lyx -userdir ~/lyxbuilds/master/user-dir

Scott


signature.asc
Description: PGP signature
-- 
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

Re: Memory leak from list

[Richard Kimberly Heck]

On 2/18/20 6:07 PM, Scott Kostyshak wrote:
> Valgrind gave me the following error:
>
>   ==732== 112 (72 direct, 40 indirect) bytes in 1 blocks are definitely lost 
> in loss record 5,165 of 5,862
>   ==732==at 0x483AE63: operator new(unsigned long) (in 
> /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
>   ==732==by 0x103A62D: lyx::Buffer::cloneBufferOnly() const 
> (Buffer.cpp:661)
>   ==732==by 0x11E583C: lyx::(anonymous 
> namespace)::copyToTempBuffer(lyx::ParagraphList const&, 
> std::shared_ptr) (CutAndPaste.cpp:582)
>   ==732==by 0x11E5C6A: lyx::(anonymous 
> namespace)::putClipboard(lyx::ParagraphList const&, 
> std::shared_ptr, 
> std::__cxx11::basic_string, 
> std::allocator > const&, lyx::BufferParams) (CutAndPaste.cpp:613)
>   ==732==by 0x11E910B: lyx::cap::copySelection(lyx::Cursor const&, 
> std::__cxx11::basic_string, 
> std::allocator > const&) (CutAndPaste.cpp:1123)
>   ==732==by 0x11E84F8: lyx::cap::copySelection(lyx::Cursor const&) 
> (CutAndPaste.cpp:1024)
>   ==732==by 0x13ECDAB: lyx::Text::dispatch(lyx::Cursor&, 
> lyx::FuncRequest&) (Text3.cpp:1593)
>   ==732==by 0x1767E04: lyx::InsetText::doDispatch(lyx::Cursor&, 
> lyx::FuncRequest&) (InsetText.cpp:339)
>   ==732==by 0x15FFC39: lyx::Inset::dispatch(lyx::Cursor&, 
> lyx::FuncRequest&) (Inset.cpp:325)
>   ==732==by 0x11D1B13: lyx::Cursor::dispatch(lyx::FuncRequest const&) 
> (Cursor.cpp:825)
>   ==732==by 0x1816F4F: 
> lyx::frontend::GuiView::dispatchToBufferView(lyx::FuncRequest const&, 
> lyx::DispatchResult&) (GuiView.cpp:3878)
>   ==732==by 0x181B959: lyx::frontend::GuiView::dispatch(lyx::FuncRequest 
> const&, lyx::DispatchResult&) (GuiView.cpp:4569)
>
> It comes from the following line (Buffer.cpp:661):
>
>   cloned_buffers.push_back(new CloneList);
>
> Currently cloned_buffers is a list. Would it make sense to make 
> it a list of *smart* pointers instead? Alternatively we could make a class 
> and then make a custom destructor that would free the CloneLists that the 
> list elements point to?

This is some kind of thinko, probably on my part. The code at line 549
was supposed to be cleaning this up, but it actually only removes the
entry from the list.

If it works to make it a smart pointer of some kind, then that would be
simplest. But I think we could just do something like:

else {
    delete(*it);
    cloned_buffers.erase(it);
}

I should learn how to use valgrind. Then I could solve these problems
myself

Riki



-- 
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

Memory leak from list

[Scott Kostyshak]

Valgrind gave me the following error:

  ==732== 112 (72 direct, 40 indirect) bytes in 1 blocks are definitely lost in 
loss record 5,165 of 5,862
  ==732==at 0x483AE63: operator new(unsigned long) (in 
/usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==732==by 0x103A62D: lyx::Buffer::cloneBufferOnly() const (Buffer.cpp:661)
  ==732==by 0x11E583C: lyx::(anonymous 
namespace)::copyToTempBuffer(lyx::ParagraphList const&, 
std::shared_ptr) (CutAndPaste.cpp:582)
  ==732==by 0x11E5C6A: lyx::(anonymous 
namespace)::putClipboard(lyx::ParagraphList const&, 
std::shared_ptr, std::__cxx11::basic_string, std::allocator > const&, lyx::BufferParams) 
(CutAndPaste.cpp:613)
  ==732==by 0x11E910B: lyx::cap::copySelection(lyx::Cursor const&, 
std::__cxx11::basic_string, 
std::allocator > const&) (CutAndPaste.cpp:1123)
  ==732==by 0x11E84F8: lyx::cap::copySelection(lyx::Cursor const&) 
(CutAndPaste.cpp:1024)
  ==732==by 0x13ECDAB: lyx::Text::dispatch(lyx::Cursor&, lyx::FuncRequest&) 
(Text3.cpp:1593)
  ==732==by 0x1767E04: lyx::InsetText::doDispatch(lyx::Cursor&, 
lyx::FuncRequest&) (InsetText.cpp:339)
  ==732==by 0x15FFC39: lyx::Inset::dispatch(lyx::Cursor&, 
lyx::FuncRequest&) (Inset.cpp:325)
  ==732==by 0x11D1B13: lyx::Cursor::dispatch(lyx::FuncRequest const&) 
(Cursor.cpp:825)
  ==732==by 0x1816F4F: 
lyx::frontend::GuiView::dispatchToBufferView(lyx::FuncRequest const&, 
lyx::DispatchResult&) (GuiView.cpp:3878)
  ==732==by 0x181B959: lyx::frontend::GuiView::dispatch(lyx::FuncRequest 
const&, lyx::DispatchResult&) (GuiView.cpp:4569)

It comes from the following line (Buffer.cpp:661):

  cloned_buffers.push_back(new CloneList);

Currently cloned_buffers is a list. Would it make sense to make it
a list of *smart* pointers instead? Alternatively we could make a class and
then make a custom destructor that would free the CloneLists that the list
elements point to?

Scott


signature.asc
Description: PGP signature
-- 
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

Re: Memory leak in master?

[Jean-Marc Lasgouttes]

Hi Guillaume,

I thought about it again, and you are right, of course.

JMarc

Le 24 février 2017 22:10:17 GMT+01:00, Guillaume Munch  a écrit :
>Le 22/02/2017 à 18:03, Jean-Marc Lasgouttes a écrit :

>> It would be nice to hide these subtleties in the Cache
>implementation,
>> if possible.
>>
>I am not sure what you mean with "hide these subtleties". The
>specification is simple: if the key does not exist in the cache, you
>get
>the default object. 

Re: Memory leak in master?

[Guillaume Munch]

Le 22/02/2017 à 18:03, Jean-Marc Lasgouttes a écrit :

Le 21/02/2017 à 20:13, Guillaume Munch a écrit :


BTW, why don't you use Cache::contains in getLayout like you do for
other cache uses?



This is explained in the documentation of Cache::object. It is enough to
check for a null pointer in that case.


It would be nice to hide these subtleties in the Cache implementation,
if possible.



Hi Jean-Marc,

I am not sure what you mean with "hide these subtleties". The
specification is simple: if the key does not exist in the cache, you get
the default object. I thought about using boost::optional at first but
for the main use I had in mind (shared_ptr) I found that the code would
be heavier (you end up having to dereference twice) for little clarity gain.

Guillaume

Re: Memory leak in master?

[Jean-Marc Lasgouttes]

Le 23/02/2017 à 18:05, Richard Heck a écrit :


Here is for reference the updated patch (even with comment
clarification). Richard?


OK. I'll test it over the next couple weeks, and if all is well move
toward 2.2.3.

rh

Done at 998c3e7c8ef. There is no status.22x entry, since this is a fix 
over a new feature.


JMarc

Re: Memory leak in master?

[Richard Heck]

On 02/23/2017 04:46 AM, Jean-Marc Lasgouttes wrote:
> Le 22/02/2017 à 18:03, Jean-Marc Lasgouttes a écrit :
>> Indeed. Richard, I think this patch should go in 2.2.3, because the
>> caching code that is in stable causes bad memory leaks with Qt5.
>
> Here is for reference the updated patch (even with comment
> clarification). Richard?

OK. I'll test it over the next couple weeks, and if all is well move
toward 2.2.3.

rh

Re: Memory leak in master?

[Richard Heck]

On 02/23/2017 04:46 AM, Jean-Marc Lasgouttes wrote:
> Le 22/02/2017 à 18:03, Jean-Marc Lasgouttes a écrit :
>> Indeed. Richard, I think this patch should go in 2.2.3, because the
>> caching code that is in stable causes bad memory leaks with Qt5.
>
> Here is for reference the updated patch (even with comment
> clarification). Richard?

OK. I'll test it over the next couple weeks.

rh

Re: Memory leak in master?

[Jean-Marc Lasgouttes]

Le 22/02/2017 à 18:03, Jean-Marc Lasgouttes a écrit :

Indeed. Richard, I think this patch should go in 2.2.3, because the
caching code that is in stable causes bad memory leaks with Qt5.


Here is for reference the updated patch (even with comment 
clarification). Richard?


JMarc


From 8e04248b2fea58a7edcf16fb3318c302b04c66e8 Mon Sep 17 00:00:00 2001
From: Guillaume Munch <g...@lyx.org>
Date: Mon, 20 Feb 2017 23:59:24 +0100
Subject: [PATCH] Introduce support/Cache.h

Useful to cache copies of objects, including shared_ptrs. No risks of dangling
pointer, and avoid naked pointers in the source.

Fix memory leak when compiling with Qt5.

As part as the backport to stable, this code has been change to work
with C++98.

(cherry picked from commit 33b696c8acf2e64b44d449180781de6dbc203709)
(cherry picked from commit e04079aa528ecbf4a8e39ed2b19c3cb50174e151)
(cherry picked from commit 5211ca52cac2ad7a6669d15c39f2cee172d18323)
(cherry picked from commit 8353a53cc38fe364bee516e86a08251e4ae974fc)
---
 src/frontends/qt4/GuiFontMetrics.cpp |  126 +++---
 src/frontends/qt4/GuiFontMetrics.h   |   46 +++--
 src/frontends/qt4/GuiPainter.cpp |3 +-
 src/support/Cache.h  |   89 
 src/support/Makefile.am  |1 +
 5 files changed, 160 insertions(+), 105 deletions(-)
 create mode 100644 src/support/Cache.h

diff --git a/src/frontends/qt4/GuiFontMetrics.cpp b/src/frontends/qt4/GuiFontMetrics.cpp
index 5bb99aa..80ff819 100644
--- a/src/frontends/qt4/GuiFontMetrics.cpp
+++ b/src/frontends/qt4/GuiFontMetrics.cpp
@@ -24,14 +24,11 @@
 #include "support/convert.h"
 #include "support/lassert.h"
 
-#ifdef CACHE_SOME_METRICS
 #include 
-#endif
 
 using namespace std;
 using namespace lyx::support;
 
-#ifdef CACHE_SOME_METRICS
 namespace std {
 
 /*
@@ -46,11 +43,28 @@ uint qHash(lyx::docstring const & s)
 }
 
 }
-#endif
 
 namespace lyx {
 namespace frontend {
 
+
+/*
+ * Limit (strwidth|breakat)_cache_ size to 512kB of string data.
+ * Limit qtextlayout_cache_ size to 500 elements (we do not know the
+ * size of the QTextLayout objects anyway).
+ * Note that all these numbers are arbitrary.
+ * Also, setting size to 0 is tantamount to disabling the cache.
+ */
+int cache_metrics_width_size = 1 << 19;
+int cache_metrics_breakat_size = 1 << 19;
+// Qt 5.x already has its own caching of QTextLayout objects
+#if (QT_VERSION < 0x05)
+int cache_metrics_qtextlayout_size = 500;
+#else
+int cache_metrics_qtextlayout_size = 0;
+#endif
+
+
 namespace {
 /**
  * Convert a UCS4 character into a QChar.
@@ -73,23 +87,11 @@ inline QChar const ucs4_to_qchar(char_type const ucs4)
 } // anon namespace
 
 
-/*
- * Limit (strwidth|breakat)_cache_ size to 512kB of string data.
- * Limit qtextlayout_cache_ size to 500 elements (we do not know the
- * size of the QTextLayout objects anyway).
- * Note that all these numbers are arbitrary.
- */
 GuiFontMetrics::GuiFontMetrics(QFont const & font)
-	: font_(font), metrics_(font, 0)
-#ifdef CACHE_METRICS_WIDTH
-	, strwidth_cache_(1 << 19)
-#endif
-#ifdef CACHE_METRICS_BREAKAT
-	, breakat_cache_(1 << 19)
-#endif
-#ifdef CACHE_METRICS_QTEXTLAYOUT
-	,  qtextlayout_cache_(500)
-#endif
+	: font_(font), metrics_(font, 0),
+	  strwidth_cache_(cache_metrics_width_size),
+	  breakat_cache_(cache_metrics_breakat_size),
+	  qtextlayout_cache_(cache_metrics_qtextlayout_size)
 {
 }
 
@@ -174,11 +176,8 @@ int GuiFontMetrics::rbearing(char_type c) const
 
 int GuiFontMetrics::width(docstring const & s) const
 {
-#ifdef CACHE_METRICS_WIDTH
-	int * pw = strwidth_cache_[s];
-	if (pw)
-		return *pw;
-#endif
+	if (strwidth_cache_.contains(s))
+		return strwidth_cache_[s];
 	/* For some reason QMetrics::width returns a wrong value with Qt5
 	 * with some arabic text. OTOH, QTextLayout is broken for single
 	 * characters with null width (like \not in mathed). Also, as a
@@ -200,9 +199,7 @@ int GuiFontMetrics::width(docstring const & s) const
 		tl.endLayout();
 		w = int(line.naturalTextWidth());
 	}
-#ifdef CACHE_METRICS_WIDTH
-	strwidth_cache_.insert(s, new int(w), s.size() * sizeof(char_type));
-#endif
+	strwidth_cache_.insert(s, w, s.size() * sizeof(char_type));
 	return w;
 }
 
@@ -225,31 +222,26 @@ int GuiFontMetrics::signedWidth(docstring const & s) const
 }
 
 
-QTextLayout const *
+shared_ptr
 GuiFontMetrics::getTextLayout(docstring const & s, bool const rtl,
   double const wordspacing) const
 {
-	QTextLayout * ptl;
-#ifdef CACHE_METRICS_QTEXTLAYOUT
-	docstring const s_cache = s + (rtl ? "r" : "l") + convert(wordspacing);
-	ptl = qtextlayout_cache_[s_cache];
-	if (!ptl) {
-#endif
-		ptl = new QTextLayout();
-		ptl->setCacheEnabled(true);
-		ptl->setText(toqstr(s));
-		QFont copy = font_;
-		copy.setWordSpacing(wordspacing);
-		ptl->setFont(copy);
-		// Note that both setFlags and the 

Re: Memory leak in master?

[Jean-Marc Lasgouttes]

Le 21/02/2017 à 20:13, Guillaume Munch a écrit :

Thanks for doing this. I thought there was some bigger adaptation to the
code to do but indeed it only looks like a matter of C++98 conversion.


You mean it was a trap? It did not work %-p


I think it's all good except:

class Cache : private QCache {
-#if !(defined(__GNUC__) && (__GNUC__ == 4) && (__GNUC_MINOR__ == 6))
+#if defined(LYX_USE_CXX11) && !(defined(__GNUC__) && (__GNUC__ == 4) &&
(__GNUC_MINOR__ == 6))


Indeed. Richard, I think this patch should go in 2.2.3, because the 
caching code that is in stable causes bad memory leaks with Qt5.



BTW, why don't you use Cache::contains in getLayout like you do for
other cache uses?



This is explained in the documentation of Cache::object. It is enough to
check for a null pointer in that case.


It would be nice to hide these subtleties in the Cache implementation, 
if possible.


JMarc

Re: Memory leak in master?

[Guillaume Munch]

Le 21/02/2017 à 07:19, Jean-Marc Lasgouttes a écrit :

Le 21/02/2017 à 00:08, Guillaume Munch a écrit :


Could you do the backport to stable? :)


What about that? Please check especially the code related to
LYX_USE_CXX11 in Cache.h. For the caching, I re-read the code to make
sure that my hand-merging was correct, I hope I did not miss anything.


Thanks for doing this. I thought there was some bigger adaptation to the
code to do but indeed it only looks like a matter of C++98 conversion.

I think it's all good except:

class Cache : private QCache {
-#if !(defined(__GNUC__) && (__GNUC__ == 4) && (__GNUC_MINOR__ == 6))
+#if defined(LYX_USE_CXX11) && !(defined(__GNUC__) && (__GNUC__ == 4) && 
(__GNUC_MINOR__ == 6))





BTW, why don't you use Cache::contains in getLayout like you do for
other cache uses?



This is explained in the documentation of Cache::object. It is enough to
check for a null pointer in that case.

Guillaume

Re: Memory leak in master?

[Guillaume Munch]

Le 20/02/2017 à 18:25, Jean-Marc Lasgouttes a écrit :

Le 10/02/2017 à 17:58, Guillaume Munch a écrit :

There's more data, but I am not sure how to interpret the results that
massif-visualizer shows.


If you send the file or make it available, I can take a look.


Here it is. But if it is like callgrind, it might lack the symbols.


Interestingly, the massif output that you sent also points to
MathMacro::clone(). Besides the obvious getTextLayout leak that needs to
be plugged, would you have an idea on why it is specifically macros that
appear here (with an increasing memory use, like the other)? I do not
see other math objects doing that.

Note that there are other sources that point to MathAtom::MathAtom, as
the drop in percentage shows.



Interesting, massif-visualizer shows the results differently. With it
one sees that MathAtom quickly reaches its final space usage and even
decreases at some point. It does not look similar to the slow creep of
the memory leak. Again, I am not sure that I know how to interpret the
results.

Guillaume

Re: Memory leak in master?

[Jean-Marc Lasgouttes]

Le 21/02/2017 à 00:08, Guillaume Munch a écrit :


Could you do the backport to stable? :)


What about that? Please check especially the code related to 
LYX_USE_CXX11 in Cache.h. For the caching, I re-read the code to make 
sure that my hand-merging was correct, I hope I did not miss anything.


BTW, why don't you use Cache::contains in getLayout like you do for 
other cache uses?


JMarc



From 30b62647ba2dc111aeb4425292e92994cf96f931 Mon Sep 17 00:00:00 2001
From: Guillaume Munch <g...@lyx.org>
Date: Mon, 20 Feb 2017 23:59:24 +0100
Subject: [PATCH] Introduce support/Cache.h

Useful to cache copies of objects, including shared_ptrs. No risks of dangling
pointer, and avoid naked pointers in the source.

Fix memory leak when compiling with Qt5.

As part as the backport to stable, this code has been change to work
with C++98.

(cherry picked from commit 33b696c8acf2e64b44d449180781de6dbc203709)
(cherry picked from commit e04079aa528ecbf4a8e39ed2b19c3cb50174e151)
(cherry picked from commit 5211ca52cac2ad7a6669d15c39f2cee172d18323)
---
 src/frontends/qt4/GuiFontMetrics.cpp |  126 +++---
 src/frontends/qt4/GuiFontMetrics.h   |   46 +++--
 src/frontends/qt4/GuiPainter.cpp |3 +-
 src/support/Cache.h  |   88 
 src/support/Makefile.am  |1 +
 5 files changed, 159 insertions(+), 105 deletions(-)
 create mode 100644 src/support/Cache.h

diff --git a/src/frontends/qt4/GuiFontMetrics.cpp b/src/frontends/qt4/GuiFontMetrics.cpp
index 5bb99aa..80ff819 100644
--- a/src/frontends/qt4/GuiFontMetrics.cpp
+++ b/src/frontends/qt4/GuiFontMetrics.cpp
@@ -24,14 +24,11 @@
 #include "support/convert.h"
 #include "support/lassert.h"
 
-#ifdef CACHE_SOME_METRICS
 #include 
-#endif
 
 using namespace std;
 using namespace lyx::support;
 
-#ifdef CACHE_SOME_METRICS
 namespace std {
 
 /*
@@ -46,11 +43,28 @@ uint qHash(lyx::docstring const & s)
 }
 
 }
-#endif
 
 namespace lyx {
 namespace frontend {
 
+
+/*
+ * Limit (strwidth|breakat)_cache_ size to 512kB of string data.
+ * Limit qtextlayout_cache_ size to 500 elements (we do not know the
+ * size of the QTextLayout objects anyway).
+ * Note that all these numbers are arbitrary.
+ * Also, setting size to 0 is tantamount to disabling the cache.
+ */
+int cache_metrics_width_size = 1 << 19;
+int cache_metrics_breakat_size = 1 << 19;
+// Qt 5.x already has its own caching of QTextLayout objects
+#if (QT_VERSION < 0x05)
+int cache_metrics_qtextlayout_size = 500;
+#else
+int cache_metrics_qtextlayout_size = 0;
+#endif
+
+
 namespace {
 /**
  * Convert a UCS4 character into a QChar.
@@ -73,23 +87,11 @@ inline QChar const ucs4_to_qchar(char_type const ucs4)
 } // anon namespace
 
 
-/*
- * Limit (strwidth|breakat)_cache_ size to 512kB of string data.
- * Limit qtextlayout_cache_ size to 500 elements (we do not know the
- * size of the QTextLayout objects anyway).
- * Note that all these numbers are arbitrary.
- */
 GuiFontMetrics::GuiFontMetrics(QFont const & font)
-	: font_(font), metrics_(font, 0)
-#ifdef CACHE_METRICS_WIDTH
-	, strwidth_cache_(1 << 19)
-#endif
-#ifdef CACHE_METRICS_BREAKAT
-	, breakat_cache_(1 << 19)
-#endif
-#ifdef CACHE_METRICS_QTEXTLAYOUT
-	,  qtextlayout_cache_(500)
-#endif
+	: font_(font), metrics_(font, 0),
+	  strwidth_cache_(cache_metrics_width_size),
+	  breakat_cache_(cache_metrics_breakat_size),
+	  qtextlayout_cache_(cache_metrics_qtextlayout_size)
 {
 }
 
@@ -174,11 +176,8 @@ int GuiFontMetrics::rbearing(char_type c) const
 
 int GuiFontMetrics::width(docstring const & s) const
 {
-#ifdef CACHE_METRICS_WIDTH
-	int * pw = strwidth_cache_[s];
-	if (pw)
-		return *pw;
-#endif
+	if (strwidth_cache_.contains(s))
+		return strwidth_cache_[s];
 	/* For some reason QMetrics::width returns a wrong value with Qt5
 	 * with some arabic text. OTOH, QTextLayout is broken for single
 	 * characters with null width (like \not in mathed). Also, as a
@@ -200,9 +199,7 @@ int GuiFontMetrics::width(docstring const & s) const
 		tl.endLayout();
 		w = int(line.naturalTextWidth());
 	}
-#ifdef CACHE_METRICS_WIDTH
-	strwidth_cache_.insert(s, new int(w), s.size() * sizeof(char_type));
-#endif
+	strwidth_cache_.insert(s, w, s.size() * sizeof(char_type));
 	return w;
 }
 
@@ -225,31 +222,26 @@ int GuiFontMetrics::signedWidth(docstring const & s) const
 }
 
 
-QTextLayout const *
+shared_ptr
 GuiFontMetrics::getTextLayout(docstring const & s, bool const rtl,
   double const wordspacing) const
 {
-	QTextLayout * ptl;
-#ifdef CACHE_METRICS_QTEXTLAYOUT
-	docstring const s_cache = s + (rtl ? "r" : "l") + convert(wordspacing);
-	ptl = qtextlayout_cache_[s_cache];
-	if (!ptl) {
-#endif
-		ptl = new QTextLayout();
-		ptl->setCacheEnabled(true);
-		ptl->setText(toqstr(s));
-		QFont copy = font_;
-		copy.setWordSpacing(wordspacing);
-		ptl

Re: Memory leak in master?

[Jean-Marc Lasgouttes]

Le 21/02/2017 à 00:08, Guillaume Munch a écrit :

Le 20/02/2017 à 18:27, Jean-Marc Lasgouttes a écrit :

Le 13/02/2017 à 20:55, Jean-Marc Lasgouttes a écrit :

What I mean is that you can easily force
QCache into shared ownership by replacing

QCache

with

QCache

and create the appropriate wrappers for insertion and retrieval.


Could you have a go at this solution?



Could you do the backport to stable? :)


You mean the version that does not require C++11? Mmm, I can always have 
a go at it.


JMarc

Re: Memory leak in master?

[Guillaume Munch]

Le 20/02/2017 à 18:27, Jean-Marc Lasgouttes a écrit :

Le 13/02/2017 à 20:55, Jean-Marc Lasgouttes a écrit :

What I mean is that you can easily force
QCache into shared ownership by replacing

QCache

with

QCache

and create the appropriate wrappers for insertion and retrieval.


Could you have a go at this solution?



Could you do the backport to stable? :)

Re: Memory leak in master?

[Jean-Marc Lasgouttes]

Le 13/02/2017 à 20:55, Jean-Marc Lasgouttes a écrit :

What I mean is that you can easily force
QCache into shared ownership by replacing

QCache

with

QCache

and create the appropriate wrappers for insertion and retrieval.


Could you have a go at this solution?

JMarc

Re: Memory leak in master?

[Jean-Marc Lasgouttes]

Le 10/02/2017 à 17:58, Guillaume Munch a écrit :

There's more data, but I am not sure how to interpret the results that
massif-visualizer shows.


If you send the file or make it available, I can take a look.


Here it is. But if it is like callgrind, it might lack the symbols.


Interestingly, the massif output that you sent also points to 
MathMacro::clone(). Besides the obvious getTextLayout leak that needs to 
be plugged, would you have an idea on why it is specifically macros that 
appear here (with an increasing memory use, like the other)? I do not 
see other math objects doing that.


Note that there are other sources that point to MathAtom::MathAtom, as 
the drop in percentage shows.


JMarc

->09.75% (18,139,776B) 0x8DA8B5: 
lyx::MathMacro::MathMacro(lyx::MathMacro const&) (MathMacro.cpp:299)
| ->09.75% (18,139,776B) 0x8DB03F: lyx::MathMacro::clone() const 
(MathMacro.cpp:394)
|   ->09.75% (18,139,776B) 0x8AD26D: 
lyx::MathAtom::MathAtom(lyx::MathAtom const&) (MathAtom.cpp:35)
| ->03.42% (6,356,216B) 0x86ADFE: 
std::vector::operator=(std::vector const&) 
(stl_construct.h:75)
| | ->01.16% (2,158,056B) 0x8EF837: lyx::(anonymous 
namespace)::Parser::parse(lyx::MathData&, unsigned int, 
lyx::Inset::mode_type) (vector:111)
| | | ->01.16% (2,158,056B) 0x8EFC99: 
lyx::mathed_parse_cell(lyx::MathData&, lyx::docstring const&, 
lyx::Parse::flags) (MathParser.cpp:2150)
| | | | ->01.16% (2,158,056B) 0x9079EB: lyx::asArray(lyx::docstring 
const&, lyx::MathData&, lyx::Parse::flags) (MathSupport.cpp:985)
| | | |   ->01.16% (2,158,056B) 0x8DBBE8: 
lyx::MathMacro::updateRepresentation(lyx::Cursor*, lyx::MacroContext 
const&, lyx::UpdateType, int) (MathMacro.cpp:699)

Re: Memory leak with WordList

[Jean-Marc Lasgouttes]

Le 31/12/2016 à 15:42, Guillaume Munch a écrit :

Snippet of Valgrind output for stable below. The WordList object created
in theWordList() never gets deleted, although QThreadStorage is supposed
to be automagic.



According to the docs, the GlobalWordLists were deleted with the
QApplication, that is, before the call to cleanUpWordLists.


Thanks for fixing this.

JMarc

Re: Memory leak with WordList

[Guillaume Munch]

Le 31/12/2016 à 13:16, Jean-Marc Lasgouttes a écrit :

WordList leaks all its contents on exit, which is very impolite (even
though the memory will be reclaimed anyway). This hides real memory
leaks that may happen elsewhere.

Snippet of Valgrind output for stable below. The WordList object created
in theWordList() never gets deleted, although QThreadStorage is supposed
to be automagic.



According to the docs, the GlobalWordLists were deleted with the 
QApplication, that is, before the call to cleanUpWordLists.

Memory leak with WordList

[Jean-Marc Lasgouttes]

WordList leaks all its contents on exit, which is very impolite (even 
though the memory will be reclaimed anyway). This hides real memory 
leaks that may happen elsewhere.


Snippet of Valgrind output for stable below. The WordList object created 
in theWordList() never gets deleted, although QThreadStorage is supposed 
to be automagic.


Actually, this message was prepared during The Great LyX Shutdown, and I 
repost now. A little search turned this:

http://marc.info/?t=13041632374=1=2

I do not think that this lead to anything interesting, but since then 
the situation has been made better by not storing full Language objects 
(f8da042312cf9a56).


But still I think this needs to be addressed.

JMarc

==2403== 707,096 (16 direct, 707,080 indirect) bytes in 2 blocks are 
definitely lost in loss record 10,474 of 10,475
==2403==at 0x4C2D1AF: operator new(unsigned long) (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2403==by 0x54867B: 
lyx::theWordList(std::__cxx11::basic_string const&) (WordList.cpp:46)
==2403==by 0x47CC24: lyx::Paragraph::registerWords() 
(Paragraph.cpp:3783)

==2403==by 0x48C606: lyx::Paragraph::updateWords() (Paragraph.cpp:3796)

Re: Memory leak ?

[Stephan Witt]

Am 22.01.2012 um 21:58 schrieb Jean-Marc Lasgouttes:

 Le 22/01/12 02:09, Lars Gullik Bjønnes a écrit :
 My testing so far says the opposite. We are continually using more and
 more memory. Not especially fast, but it is there.
 
 A good tool to know where memory goes is the massif tool of valgrind.

I've tried to use Instruments Leak-Detector on Mac.

1. It doesn't find any suspicious leaks - the growing memory is not lost.
2. The growing memory is allocated in TextClass constructor - about 40k per 
autosave operation.
3. Call Stack:

Total % Bytes   Library Symbol Name
...
11.41.65 MB lyx::frontend::GuiView::autoSave()
11.41.64 MB lyx::Buffer::cloneBufferOnly() const
11.41.64 MB lyx::Buffer::Buffer(std::string const, bool, lyx::Buffer 
const*)
11.41.64 MB lyx::Buffer::Impl::Impl(lyx::Buffer*, lyx::support::FileName 
const, bool, lyx::Buffer const*)
11.41.64 MB lyx::BufferParams::BufferParams()
11.41.64 MB lyx::BufferParams::makeDocumentClass()
11.41.64 MB lyx::DocumentClassBundle::makeDocumentClass(lyx::LayoutFile 
const, lyx::LayoutModuleList const)
11.41.64 MB lyx::DocumentClassBundle::newClass(lyx::LayoutFile const)
11.21.61 MB lyx::DocumentClass::DocumentClass(lyx::LayoutFile const)
11.21.61 MB lyx::TextClass::TextClass(lyx::TextClass const)

1 minute later

Total % Bytes   Library Symbol Name
...
11.71.69 MB lyx::DocumentClassBundle::newClass(lyx::LayoutFile const)
11.41.65 MB lyx::TextClass::TextClass(lyx::TextClass const)

Stephan

Re: Memory leak ?

[Jean-Marc Lasgouttes]

Le 23/01/2012 10:56, Lars Gullik Bjønnes a écrit :


| A good tool to know where memory goes is the massif tool of valgrind.

This is the result from a ~6hrs run.
I am not quite sure how to read it.


Like what Stephan reported, we see:

fantomas: ms_print massif.out.24204|grep TextClass::TextClass
-01.02% (303,120B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const) (new_allocator.h:92)
-03.16% (972,720B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const) (new_allocator.h:92)
-03.06% (1,262,880B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const) (new_allocator.h:92)
-03.56% (1,486,080B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const) (new_allocator.h:92)
-05.12% (1,642,320B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const) (new_allocator.h:92)
-04.06% (1,709,280B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const) (new_allocator.h:92)
-05.74% (1,865,520B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const) (new_allocator.h:92)
-04.54% (1,932,480B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const) (new_allocator.h:92)


We see that the number of allocated TextClass objects increses. I think 
that we never release buffers cloned by autosave.


JMarc

Re: Memory leak ?

[Richard Heck]

On 01/23/2012 05:53 AM, Jean-Marc Lasgouttes wrote:

Le 23/01/2012 10:56, Lars Gullik Bjønnes a écrit :


| A good tool to know where memory goes is the massif tool of valgrind.

This is the result from a ~6hrs run.
I am not quite sure how to read it.


Like what Stephan reported, we see:

fantomas: ms_print massif.out.24204|grep TextClass::TextClass
-01.02% (303,120B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const) (new_allocator.h:92)
-03.16% (972,720B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const) (new_allocator.h:92)
-03.06% (1,262,880B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-03.56% (1,486,080B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-05.12% (1,642,320B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-04.06% (1,709,280B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-05.74% (1,865,520B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-04.54% (1,932,480B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)


We see that the number of allocated TextClass objects increses. I 
think that we never release buffers cloned by autosave.


I think I understand what's going on here. We release the Buffers, but 
we do not, it is true, release the TextClass objects that are associated 
with them. There's actually a good reason for that where ordinary 
Buffers are concerned---there could be something on the cut stack that 
still has a reference to the TextClass in question---but in the case of 
AutoSave, there is no reason we need to keep these around. See the 
comments at the end of TextClass.h, concerning the DocumentClassBundle.


I'll try to have a look shortly, but classes start again on Wednesday. 
If anyone else had ideas, I'd be happy to hear them.


Richard

Re: Memory leak ?

[Vincent van Ravesteijn]

Op 23-1-2012 15:54, Richard Heck schreef:

On 01/23/2012 05:53 AM, Jean-Marc Lasgouttes wrote:

Le 23/01/2012 10:56, Lars Gullik Bjønnes a écrit :


| A good tool to know where memory goes is the massif tool of valgrind.

This is the result from a ~6hrs run.
I am not quite sure how to read it.


Like what Stephan reported, we see:

fantomas: ms_print massif.out.24204|grep TextClass::TextClass
-01.02% (303,120B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-03.16% (972,720B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-03.06% (1,262,880B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-03.56% (1,486,080B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-05.12% (1,642,320B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-04.06% (1,709,280B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-05.74% (1,865,520B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-04.54% (1,932,480B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)


We see that the number of allocated TextClass objects increses. I 
think that we never release buffers cloned by autosave.


I think I understand what's going on here. We release the Buffers, but 
we do not, it is true, release the TextClass objects that are 
associated with them. There's actually a good reason for that where 
ordinary Buffers are concerned---there could be something on the cut 
stack that still has a reference to the TextClass in question---but in 
the case of AutoSave, there is no reason we need to keep these around. 
See the comments at the end of TextClass.h, concerning the 
DocumentClassBundle.


I'll try to have a look shortly, but classes start again on Wednesday. 
If anyone else had ideas, I'd be happy to hear them.


Richard



We don't need a new TextClass everytime. If we already have one for the 
documentclass/layout, we can reuse it again.


This also means that with every preview we lose memory.

Vincent

Re: Memory leak ?

[Richard Heck]

On 01/23/2012 12:04 PM, Vincent van Ravesteijn wrote:

Op 23-1-2012 15:54, Richard Heck schreef:

On 01/23/2012 05:53 AM, Jean-Marc Lasgouttes wrote:

Le 23/01/2012 10:56, Lars Gullik Bjønnes a écrit :

| A good tool to know where memory goes is the massif tool of 
valgrind.


This is the result from a ~6hrs run.
I am not quite sure how to read it.


Like what Stephan reported, we see:

fantomas: ms_print massif.out.24204|grep TextClass::TextClass
-01.02% (303,120B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-03.16% (972,720B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-03.06% (1,262,880B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-03.56% (1,486,080B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-05.12% (1,642,320B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-04.06% (1,709,280B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-05.74% (1,865,520B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)
-04.54% (1,932,480B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const) (new_allocator.h:92)


We see that the number of allocated TextClass objects increses. I 
think that we never release buffers cloned by autosave.


I think I understand what's going on here. We release the Buffers, 
but we do not, it is true, release the TextClass objects that are 
associated with them. There's actually a good reason for that where 
ordinary Buffers are concerned---there could be something on the cut 
stack that still has a reference to the TextClass in question---but 
in the case of AutoSave, there is no reason we need to keep these 
around. See the comments at the end of TextClass.h, concerning the 
DocumentClassBundle.


I'll try to have a look shortly, but classes start again on 
Wednesday. If anyone else had ideas, I'd be happy to hear them.


Richard



We don't need a new TextClass everytime. If we already have one for 
the documentclass/layout, we can reuse it again.


I'm afraid it's more complicated than that. I mean, in principle we 
could do that, but the new TextClass (actually DocumentClass) object is 
created in the BufferParams constructor. I think I have a good idea what 
to do here, though. Minimally, we could delete the clone's DocumentClass 
on destruction since we know that no-one else will have a reference to it.



This also means that with every preview we lose memory.

Yes, this is all an unforeseen consequence of the cloning business. 
(Enrico strikes again!) But there are other places we create these 
objects and never get rid of them.


Richard

Re: Memory leak ?

[Stephan Witt]

Am 22.01.2012 um 21:58 schrieb Jean-Marc Lasgouttes:

> Le 22/01/12 02:09, Lars Gullik Bjønnes a écrit :
>> My testing so far says the opposite. We are continually using more and
>> more memory. Not especially fast, but it is there.
> 
> A good tool to know where memory goes is the massif tool of valgrind.

I've tried to use Instruments Leak-Detector on Mac.

1. It doesn't find any suspicious leaks - the growing memory is not lost.
2. The growing memory is allocated in TextClass constructor - about 40k per 
autosave operation.
3. Call Stack:

Total % Bytes   Library Symbol Name
...
11.41.65 MB lyx::frontend::GuiView::autoSave()
11.41.64 MB lyx::Buffer::cloneBufferOnly() const
11.41.64 MB lyx::Buffer::Buffer(std::string const&, bool, lyx::Buffer 
const*)
11.41.64 MB lyx::Buffer::Impl::Impl(lyx::Buffer*, lyx::support::FileName 
const&, bool, lyx::Buffer const*)
11.41.64 MB lyx::BufferParams::BufferParams()
11.41.64 MB lyx::BufferParams::makeDocumentClass()
11.41.64 MB lyx::DocumentClassBundle::makeDocumentClass(lyx::LayoutFile 
const&, lyx::LayoutModuleList const&)
11.41.64 MB lyx::DocumentClassBundle::newClass(lyx::LayoutFile const&)
11.21.61 MB lyx::DocumentClass::DocumentClass(lyx::LayoutFile const&)
11.21.61 MB lyx::TextClass::TextClass(lyx::TextClass const&)

1 minute later

Total % Bytes   Library Symbol Name
...
11.71.69 MB lyx::DocumentClassBundle::newClass(lyx::LayoutFile const&)
11.41.65 MB lyx::TextClass::TextClass(lyx::TextClass const&)

Stephan

Re: Memory leak ?

[Jean-Marc Lasgouttes]

Le 23/01/2012 10:56, Lars Gullik Bjønnes a écrit :


| A good tool to know where memory goes is the massif tool of valgrind.

This is the result from a ~6hrs run.
I am not quite sure how to read it.


Like what Stephan reported, we see:

fantomas: ms_print massif.out.24204|grep TextClass::TextClass
->01.02% (303,120B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const&) (new_allocator.h:92)
->03.16% (972,720B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const&) (new_allocator.h:92)
->03.06% (1,262,880B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const&) (new_allocator.h:92)
->03.56% (1,486,080B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const&) (new_allocator.h:92)
->05.12% (1,642,320B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const&) (new_allocator.h:92)
->04.06% (1,709,280B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const&) (new_allocator.h:92)
->05.74% (1,865,520B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const&) (new_allocator.h:92)
->04.54% (1,932,480B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const&) (new_allocator.h:92)


We see that the number of allocated TextClass objects increses. I think 
that we never release buffers cloned by autosave.


JMarc

Re: Memory leak ?

[Richard Heck]

On 01/23/2012 05:53 AM, Jean-Marc Lasgouttes wrote:

Le 23/01/2012 10:56, Lars Gullik Bjønnes a écrit :


| A good tool to know where memory goes is the massif tool of valgrind.

This is the result from a ~6hrs run.
I am not quite sure how to read it.


Like what Stephan reported, we see:

fantomas: ms_print massif.out.24204|grep TextClass::TextClass
->01.02% (303,120B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const&) (new_allocator.h:92)
->03.16% (972,720B) 0x58C82F: lyx::TextClass::TextClass(lyx::TextClass 
const&) (new_allocator.h:92)
->03.06% (1,262,880B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->03.56% (1,486,080B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->05.12% (1,642,320B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->04.06% (1,709,280B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->05.74% (1,865,520B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->04.54% (1,932,480B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)


We see that the number of allocated TextClass objects increses. I 
think that we never release buffers cloned by autosave.


I think I understand what's going on here. We release the Buffers, but 
we do not, it is true, release the TextClass objects that are associated 
with them. There's actually a good reason for that where ordinary 
Buffers are concerned---there could be something on the cut stack that 
still has a reference to the TextClass in question---but in the case of 
AutoSave, there is no reason we need to keep these around. See the 
comments at the end of TextClass.h, concerning the DocumentClassBundle.


I'll try to have a look shortly, but classes start again on Wednesday. 
If anyone else had ideas, I'd be happy to hear them.


Richard

Re: Memory leak ?

[Vincent van Ravesteijn]

Op 23-1-2012 15:54, Richard Heck schreef:

On 01/23/2012 05:53 AM, Jean-Marc Lasgouttes wrote:

Le 23/01/2012 10:56, Lars Gullik Bjønnes a écrit :


| A good tool to know where memory goes is the massif tool of valgrind.

This is the result from a ~6hrs run.
I am not quite sure how to read it.


Like what Stephan reported, we see:

fantomas: ms_print massif.out.24204|grep TextClass::TextClass
->01.02% (303,120B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->03.16% (972,720B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->03.06% (1,262,880B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->03.56% (1,486,080B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->05.12% (1,642,320B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->04.06% (1,709,280B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->05.74% (1,865,520B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->04.54% (1,932,480B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)


We see that the number of allocated TextClass objects increses. I 
think that we never release buffers cloned by autosave.


I think I understand what's going on here. We release the Buffers, but 
we do not, it is true, release the TextClass objects that are 
associated with them. There's actually a good reason for that where 
ordinary Buffers are concerned---there could be something on the cut 
stack that still has a reference to the TextClass in question---but in 
the case of AutoSave, there is no reason we need to keep these around. 
See the comments at the end of TextClass.h, concerning the 
DocumentClassBundle.


I'll try to have a look shortly, but classes start again on Wednesday. 
If anyone else had ideas, I'd be happy to hear them.


Richard



We don't need a new TextClass everytime. If we already have one for the 
documentclass/layout, we can reuse it again.


This also means that with every preview we lose memory.

Vincent

Re: Memory leak ?

[Richard Heck]

On 01/23/2012 12:04 PM, Vincent van Ravesteijn wrote:

Op 23-1-2012 15:54, Richard Heck schreef:

On 01/23/2012 05:53 AM, Jean-Marc Lasgouttes wrote:

Le 23/01/2012 10:56, Lars Gullik Bjønnes a écrit :

| A good tool to know where memory goes is the massif tool of 
valgrind.


This is the result from a ~6hrs run.
I am not quite sure how to read it.


Like what Stephan reported, we see:

fantomas: ms_print massif.out.24204|grep TextClass::TextClass
->01.02% (303,120B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->03.16% (972,720B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->03.06% (1,262,880B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->03.56% (1,486,080B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->05.12% (1,642,320B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->04.06% (1,709,280B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->05.74% (1,865,520B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)
->04.54% (1,932,480B) 0x58C82F: 
lyx::TextClass::TextClass(lyx::TextClass const&) (new_allocator.h:92)


We see that the number of allocated TextClass objects increses. I 
think that we never release buffers cloned by autosave.


I think I understand what's going on here. We release the Buffers, 
but we do not, it is true, release the TextClass objects that are 
associated with them. There's actually a good reason for that where 
ordinary Buffers are concerned---there could be something on the cut 
stack that still has a reference to the TextClass in question---but 
in the case of AutoSave, there is no reason we need to keep these 
around. See the comments at the end of TextClass.h, concerning the 
DocumentClassBundle.


I'll try to have a look shortly, but classes start again on 
Wednesday. If anyone else had ideas, I'd be happy to hear them.


Richard



We don't need a new TextClass everytime. If we already have one for 
the documentclass/layout, we can reuse it again.


I'm afraid it's more complicated than that. I mean, in principle we 
could do that, but the new TextClass (actually DocumentClass) object is 
created in the BufferParams constructor. I think I have a good idea what 
to do here, though. Minimally, we could delete the clone's DocumentClass 
on destruction since we know that no-one else will have a reference to it.



This also means that with every preview we lose memory.

Yes, this is all an unforeseen consequence of the cloning business. 
(Enrico strikes again!) But there are other places we create these 
objects and never get rid of them.


Richard

Re: Memory leak ?

[Lars Gullik Bjønnes]

Richard Heck rgh...@comcast.net writes:

| On 01/21/2012 08:09 PM, Lars Gullik Bjønnes wrote:
 Richard Heckrgh...@comcast.net  writes:

 | On 01/21/2012 05:36 AM, Jean-Pierre Chrétien wrote:
 Hello,

 A French user tells me privately that he finds a big memeory leak in
 LyX-2.0.2 on an Archlinux LyX distribution (about 1.5Mo/mn).

 On my Debian Squeeze and a simple document, I find about 1.7ko/mn.

 Testing with the Userguide, I find around 19ko/mn with the following
 recipe:
   - open LyX-2.0.2 (English locale)
   - open the UserGuide
   - minimize the window
   - activate a crontab every minute with command ps -o rss= -p
 process_id

 | Below is a log of a test on Fedora 16, Qt 4.8.0, with 2.0.3svn,
 | compiled as for release. As you'll see, there's a jump when the first
 | autosave happens---we may be loading some Qt libraries we weren't
 | otherwise using---but after that, there's not much change.

 My testing so far says the opposite. We are continually using more and
 more memory. Not especially fast, but it is there.

 I see a steady increase.

 Plot the attached with gnuplot
 (plot lyx-leak.dat using 1:2)

 and see for yourself. Snarfed with

 let a=0; while true; do echo -n $a   lyx-leak.dat; ps -o rss=,vsz= -p 
 32013  lyx-leak.dat ; let ++a; sleep 1 ; done
| Hard to know what the difference is here. Was this with branch or trunk?

Trunk.

-- 
   Lgb

Re: Memory leak ?

[Jean-Marc Lasgouttes]

Le 22/01/12 02:09, Lars Gullik Bjønnes a écrit :

My testing so far says the opposite. We are continually using more and
more memory. Not especially fast, but it is there.


A good tool to know where memory goes is the massif tool of valgrind.

JMarc

Re: Memory leak ?

[Lars Gullik Bjønnes]

Richard Heck  writes:

| On 01/21/2012 08:09 PM, Lars Gullik Bjønnes wrote:
>> Richard Heck  writes:
>>
>> | On 01/21/2012 05:36 AM, Jean-Pierre Chrétien wrote:
 Hello,

 A French user tells me privately that he finds a big memeory leak in
 LyX-2.0.2 on an Archlinux LyX distribution (about 1.5Mo/mn).

 On my Debian Squeeze and a simple document, I find about 1.7ko/mn.

 Testing with the Userguide, I find around 19ko/mn with the following
 recipe:
   - open LyX-2.0.2 (English locale)
   - open the UserGuide
   - minimize the window
   - activate a crontab every minute with command ps -o rss= -p
 

>> | Below is a log of a test on Fedora 16, Qt 4.8.0, with 2.0.3svn,
>> | compiled as for release. As you'll see, there's a jump when the first
>> | autosave happens---we may be loading some Qt libraries we weren't
>> | otherwise using---but after that, there's not much change.
>>
>> My testing so far says the opposite. We are continually using more and
>> more memory. Not especially fast, but it is there.
>>
>> I see a steady increase.
>>
>> Plot the attached with gnuplot
>> (plot "lyx-leak.dat" using 1:2)
>>
>> and see for yourself. Snarfed with
>>
>> let a=0; while true; do echo -n "$a ">>  lyx-leak.dat; ps -o rss=,vsz= -p 
>> 32013>>  lyx-leak.dat ; let ++a; sleep 1 ; done
| Hard to know what the difference is here. Was this with branch or trunk?

Trunk.

-- 
   Lgb

Re: Memory leak ?

[Jean-Marc Lasgouttes]

Le 22/01/12 02:09, Lars Gullik Bjønnes a écrit :

My testing so far says the opposite. We are continually using more and
more memory. Not especially fast, but it is there.


A good tool to know where memory goes is the massif tool of valgrind.

JMarc

Memory leak ?

[Jean-Pierre Chrétien]

Hello,

A French user tells me privately that he finds a big memeory leak in LyX-2.0.2 
on an Archlinux LyX distribution (about 1.5Mo/mn).


On my Debian Squeeze and a simple document, I find about 1.7ko/mn.

Testing with the Userguide, I find around 19ko/mn with the following recipe:
 - open LyX-2.0.2 (English locale)
 - open the UserGuide
 - minimize the window
 - activate a crontab every minute with command ps -o rss= -p process_id

I find no leak with Lyx-1.6.10, and an much smaller one (identical for each) of 
0.28ko/mn with Lyx-2.0.2 and 2.0.1.


Is this a real leak, a misleading procedure, or a new behaviour with 2.0 (hidden 
tasks like auto-update or something) ?


Jean-Pierre

On ArchLinux:
% lyx --version
LyX 2.0.2 (2011-11-26)
Built on Dec 31 2011, 06:13:09
Configuration
  Host type:x86_64-unknown-linux-gnu
  Special build flags:  build=release use-aspell use-enchant
use-hunspell
  C   Compiler: gcc
  C   Compiler LyX flags:
  C   Compiler flags:-march=x86-64 -mtune=generic -O2
-pipe -fstack-protector --param=ssp-buffer-size=4 -D_FORTIFY_SOURCE=2
  C++ Compiler: g++ (4.6.2)
  C++ Compiler LyX flags:
  C++ Compiler flags:-march=x86-64 -mtune=generic -O2
-pipe -fstack-protector --param=ssp-buffer-size=4 -D_FORTIFY_SOURCE=2
-fpermissive
  Linker flags:
  Linker user flags:
-Wl,-O1,--sort-common,--as-needed,-z,relro,--hash-style=gnu
  Qt 4 Frontend:
  Qt 4 version: 4.8.0
  Packaging:posix
  LyX binary dir:   /usr/bin
  LyX files dir:/usr/share/lyx

On Debian Squeeze:
$ lyx-2.0.2 --version
LyX 2.0.2 (2011-11-25)
Built on Nov 26 2011, 17:09:16
Configuration
  Host type:i686-pc-linux-gnu
  Special build flags:  build=release use-aspell
  C   Compiler: gcc
  C   Compiler LyX flags:
  C   Compiler flags:-O2
  C++ Compiler: g++ (4.4.5)
  C++ Compiler LyX flags:
  C++ Compiler flags:-O2
  Linker flags:
  Linker user flags:
  Qt 4 Frontend:
  Qt 4 version: 4.6.3
  Packaging:posix
  LyX binary dir:   /usr/local/bin
  LyX files dir:/usr/local/share/lyx-2.0.2

Re: Memory leak ?

[Jean-Pierre Chrétien]

Jean-Pierre Chrétien jeanpierre.chretien at free.fr writes:

 I find no leak with Lyx-1.6.10, and an much smaller one (identical for each) 
 of  0.28ko/mn with Lyx-2.0.2 and 2.0.1.

I meant  Lyx-2.0.0 and 2.0.1, sorry.

-- 
Jean-Pierre

Re: Memory leak ?

[Lars Gullik Bjønnes]

Jean-Pierre Chrétien jeanpierre.chret...@free.fr writes:

| Hello,

| A French user tells me privately that he finds a big memeory leak in
| LyX-2.0.2 on an Archlinux LyX distribution (about 1.5Mo/mn).

| On my Debian Squeeze and a simple document, I find about 1.7ko/mn.

| Testing with the Userguide, I find around 19ko/mn with the following recipe:
|  - open LyX-2.0.2 (English locale)
|  - open the UserGuide
|  - minimize the window
|  - activate a crontab every minute with command ps -o rss= -p process_id

| I find no leak with Lyx-1.6.10, and an much smaller one (identical for
| each) of 0.28ko/mn with Lyx-2.0.2 and 2.0.1.

| Is this a real leak, a misleading procedure, or a new behaviour with
| 2.0 (hidden tasks like auto-update or something) ?

I ran valgrind on trunk I couldn't see anything special. I also left lyx
arunning for a long time no valgrind reports.

So either the leak you see does not exist on trunk, or it is not a leak
in the strict sense. That however does not mean that what you see is
wrong.

I used this suppression file when running whith valgrind. Some of the
suppressions should be looked into, but they are very minor leaks.



lyx.supp
Description: Binary data

-- 
   Lgb

Re: Memory leak ?

[Richard Heck]

On 01/21/2012 05:36 AM, Jean-Pierre Chrétien wrote:

Hello,

A French user tells me privately that he finds a big memeory leak in 
LyX-2.0.2 on an Archlinux LyX distribution (about 1.5Mo/mn).


On my Debian Squeeze and a simple document, I find about 1.7ko/mn.

Testing with the Userguide, I find around 19ko/mn with the following 
recipe:

 - open LyX-2.0.2 (English locale)
 - open the UserGuide
 - minimize the window
 - activate a crontab every minute with command ps -o rss= -p 
process_id


Below is a log of a test on Fedora 16, Qt 4.8.0, with 2.0.3svn, compiled 
as for release. As you'll see, there's a jump when the first autosave 
happens---we may be loading some Qt libraries we weren't otherwise 
using---but after that, there's not much change.


Richard

==

/home/rgheck/  while [ ss ]; do ps -o rss= -p 3483; sleep 60; done
63512
63416
63416
63416
63416
63416
Now I will make the document dirty.
63516
63516
Autosave is set to 10 minutes
63628
63628
73444
/tmp/  ll *User*
-rw-rw-r--. 1 rgheck rgheck 795K Jan 21 12:17 #UserGuide.lyx#
-rw-rw-r--. 1 rgheck rgheck 795K Jan 21 12:03 UserGuide.lyx
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73480
/tmp/  ll *User*
-rw-rw-r--. 1 rgheck rgheck 795K Jan 21 12:37 #UserGuide.lyx#
-rw-rw-r--. 1 rgheck rgheck 795K Jan 21 12:03 UserGuide.lyx
73464
73464
73704
73704
74392
74392
74392
74392
74392
74568
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
74568

Re: Memory leak ?

[Richard Heck]

On 01/21/2012 08:09 PM, Lars Gullik Bjønnes wrote:

Richard Heckrgh...@comcast.net  writes:

| On 01/21/2012 05:36 AM, Jean-Pierre Chrétien wrote:

Hello,

A French user tells me privately that he finds a big memeory leak in
LyX-2.0.2 on an Archlinux LyX distribution (about 1.5Mo/mn).

On my Debian Squeeze and a simple document, I find about 1.7ko/mn.

Testing with the Userguide, I find around 19ko/mn with the following
recipe:
  - open LyX-2.0.2 (English locale)
  - open the UserGuide
  - minimize the window
  - activate a crontab every minute with command ps -o rss= -p
process_id


| Below is a log of a test on Fedora 16, Qt 4.8.0, with 2.0.3svn,
| compiled as for release. As you'll see, there's a jump when the first
| autosave happens---we may be loading some Qt libraries we weren't
| otherwise using---but after that, there's not much change.

My testing so far says the opposite. We are continually using more and
more memory. Not especially fast, but it is there.

I see a steady increase.

Plot the attached with gnuplot
(plot lyx-leak.dat using 1:2)

and see for yourself. Snarfed with

let a=0; while true; do echo -n $a   lyx-leak.dat; ps -o rss=,vsz= -p 
32013  lyx-leak.dat ; let ++a; sleep 1 ; done

Hard to know what the difference is here. Was this with branch or trunk?

Richard

Memory leak ?

[Jean-Pierre Chrétien]

Hello,

A French user tells me privately that he finds a big memeory leak in LyX-2.0.2 
on an Archlinux LyX distribution (about 1.5Mo/mn).


On my Debian Squeeze and a simple document, I find about 1.7ko/mn.

Testing with the Userguide, I find around 19ko/mn with the following recipe:
 - open LyX-2.0.2 (English locale)
 - open the UserGuide
 - minimize the window
 - activate a crontab every minute with command ps -o rss= -p 

I find no leak with Lyx-1.6.10, and an much smaller one (identical for each) of 
0.28ko/mn with Lyx-2.0.2 and 2.0.1.


Is this a real leak, a misleading procedure, or a new behaviour with 2.0 (hidden 
tasks like auto-update or something) ?


Jean-Pierre

On ArchLinux:
% lyx --version
LyX 2.0.2 (2011-11-26)
Built on Dec 31 2011, 06:13:09
Configuration
  Host type:x86_64-unknown-linux-gnu
  Special build flags:  build=release use-aspell use-enchant
use-hunspell
  C   Compiler: gcc
  C   Compiler LyX flags:
  C   Compiler flags:-march=x86-64 -mtune=generic -O2
-pipe -fstack-protector --param=ssp-buffer-size=4 -D_FORTIFY_SOURCE=2
  C++ Compiler: g++ (4.6.2)
  C++ Compiler LyX flags:
  C++ Compiler flags:-march=x86-64 -mtune=generic -O2
-pipe -fstack-protector --param=ssp-buffer-size=4 -D_FORTIFY_SOURCE=2
-fpermissive
  Linker flags:
  Linker user flags:
-Wl,-O1,--sort-common,--as-needed,-z,relro,--hash-style=gnu
  Qt 4 Frontend:
  Qt 4 version: 4.8.0
  Packaging:posix
  LyX binary dir:   /usr/bin
  LyX files dir:/usr/share/lyx

On Debian Squeeze:
$ lyx-2.0.2 --version
LyX 2.0.2 (2011-11-25)
Built on Nov 26 2011, 17:09:16
Configuration
  Host type:i686-pc-linux-gnu
  Special build flags:  build=release use-aspell
  C   Compiler: gcc
  C   Compiler LyX flags:
  C   Compiler flags:-O2
  C++ Compiler: g++ (4.4.5)
  C++ Compiler LyX flags:
  C++ Compiler flags:-O2
  Linker flags:
  Linker user flags:
  Qt 4 Frontend:
  Qt 4 version: 4.6.3
  Packaging:posix
  LyX binary dir:   /usr/local/bin
  LyX files dir:/usr/local/share/lyx-2.0.2

Re: Memory leak ?

[Jean-Pierre Chrétien]

Jean-Pierre Chrétien  free.fr> writes:

> I find no leak with Lyx-1.6.10, and an much smaller one (identical for each) 
> of  0.28ko/mn with Lyx-2.0.2 and 2.0.1.

I meant  Lyx-2.0.0 and 2.0.1, sorry.

-- 
Jean-Pierre

Re: Memory leak ?

[Lars Gullik Bjønnes]

Jean-Pierre Chrétien  writes:

| Hello,
>
| A French user tells me privately that he finds a big memeory leak in
| LyX-2.0.2 on an Archlinux LyX distribution (about 1.5Mo/mn).
>
| On my Debian Squeeze and a simple document, I find about 1.7ko/mn.
>
| Testing with the Userguide, I find around 19ko/mn with the following recipe:
|  - open LyX-2.0.2 (English locale)
|  - open the UserGuide
|  - minimize the window
|  - activate a crontab every minute with command ps -o rss= -p 
>
| I find no leak with Lyx-1.6.10, and an much smaller one (identical for
| each) of 0.28ko/mn with Lyx-2.0.2 and 2.0.1.
>
| Is this a real leak, a misleading procedure, or a new behaviour with
| 2.0 (hidden tasks like auto-update or something) ?

I ran valgrind on trunk I couldn't see anything special. I also left lyx
arunning for a long time no valgrind reports.

So either the leak you see does not exist on trunk, or it is not a leak
in the strict sense. That however does not mean that what you see is
wrong.

I used this suppression file when running whith valgrind. Some of the
suppressions should be looked into, but they are very minor leaks.



lyx.supp
Description: Binary data

-- 
   Lgb

Re: Memory leak ?

[Richard Heck]

On 01/21/2012 05:36 AM, Jean-Pierre Chrétien wrote:

Hello,

A French user tells me privately that he finds a big memeory leak in 
LyX-2.0.2 on an Archlinux LyX distribution (about 1.5Mo/mn).


On my Debian Squeeze and a simple document, I find about 1.7ko/mn.

Testing with the Userguide, I find around 19ko/mn with the following 
recipe:

 - open LyX-2.0.2 (English locale)
 - open the UserGuide
 - minimize the window
 - activate a crontab every minute with command ps -o rss= -p 



Below is a log of a test on Fedora 16, Qt 4.8.0, with 2.0.3svn, compiled 
as for release. As you'll see, there's a jump when the first autosave 
happens---we may be loading some Qt libraries we weren't otherwise 
using---but after that, there's not much change.


Richard

==

/home/rgheck/ > while [ "ss" ]; do ps -o rss= -p 3483; sleep 60; done
63512
63416
63416
63416
63416
63416
Now I will make the document dirty.
63516
63516
Autosave is set to 10 minutes
63628
63628
73444
/tmp/ > ll *User*
-rw-rw-r--. 1 rgheck rgheck 795K Jan 21 12:17 #UserGuide.lyx#
-rw-rw-r--. 1 rgheck rgheck 795K Jan 21 12:03 UserGuide.lyx
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73476
73480
/tmp/ > ll *User*
-rw-rw-r--. 1 rgheck rgheck 795K Jan 21 12:37 #UserGuide.lyx#
-rw-rw-r--. 1 rgheck rgheck 795K Jan 21 12:03 UserGuide.lyx
73464
73464
73704
73704
74392
74392
74392
74392
74392
74568
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
73888
74568

Re: Memory leak ?

[Richard Heck]

On 01/21/2012 08:09 PM, Lars Gullik Bjønnes wrote:

Richard Heck  writes:

| On 01/21/2012 05:36 AM, Jean-Pierre Chrétien wrote:

Hello,

A French user tells me privately that he finds a big memeory leak in
LyX-2.0.2 on an Archlinux LyX distribution (about 1.5Mo/mn).

On my Debian Squeeze and a simple document, I find about 1.7ko/mn.

Testing with the Userguide, I find around 19ko/mn with the following
recipe:
  - open LyX-2.0.2 (English locale)
  - open the UserGuide
  - minimize the window
  - activate a crontab every minute with command ps -o rss= -p



| Below is a log of a test on Fedora 16, Qt 4.8.0, with 2.0.3svn,
| compiled as for release. As you'll see, there's a jump when the first
| autosave happens---we may be loading some Qt libraries we weren't
| otherwise using---but after that, there's not much change.

My testing so far says the opposite. We are continually using more and
more memory. Not especially fast, but it is there.

I see a steady increase.

Plot the attached with gnuplot
(plot "lyx-leak.dat" using 1:2)

and see for yourself. Snarfed with

let a=0; while true; do echo -n "$a ">>  lyx-leak.dat; ps -o rss=,vsz= -p 
32013>>  lyx-leak.dat ; let ++a; sleep 1 ; done

Hard to know what the difference is here. Was this with branch or trunk?

Richard

[PATCH] Fix Autosave Memory Leak

[Richard Heck]

In discussion of Pavel's crash, Vincent asked whether we were
unnecessarily cloning all the children of a given Buffer when we
autosave it. The answer is that we were and, worse, that, since changes
I made to fix other bugs, we were starting the cloning process from the
master Buffer and then, even worse, not deleting the master but only the
original Buffer when we were done. In the right setting, this would leak
a lot of memory.

The attached patch addresses this issue. Basically, it just clones the
single Buffer we are trying to autosave. Comments welcome, especially on
whether we should put this into 2.0.2. It looks pretty safe to me, and
the memory leak, as I said, could be pretty bad otherwise.

Richard


From 1c0f114c36983d72af27ec0f26f643eb2e7845ba Mon Sep 17 00:00:00 2001
From: Richard Heck rgh...@lyx.org
Date: Tue, 15 Nov 2011 11:08:50 -0500
Subject: [PATCH] Don't clone all the children on autosave.

---
 src/Buffer.cpp|   16 
 src/Buffer.h  |9 ++---
 src/frontends/qt4/GuiView.cpp |4 ++--
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index 625c0ff..a1aada2 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -430,17 +430,17 @@ Buffer::~Buffer()
 }
 
 
-Buffer * Buffer::clone() const
+Buffer * Buffer::cloneFromMaster() const
 {
 	BufferMap bufmap;
-	masterBuffer()-clone(bufmap);
+	masterBuffer()-cloneWithChildren(bufmap);
 	BufferMap::iterator it = bufmap.find(this);
 	LASSERT(it != bufmap.end(), return 0);
 	return it-second;
 }
 
 
-void Buffer::clone(BufferMap  bufmap) const
+void Buffer::cloneWithChildren(BufferMap  bufmap) const
 {
 	// have we already been cloned?
 	if (bufmap.find(this) != bufmap.end())
@@ -461,7 +461,7 @@ void Buffer::clone(BufferMap  bufmap) const
 		dit.setBuffer(buffer_clone);
 		Buffer * child = const_castBuffer *(it-second.second);
 
-		child-clone(bufmap);
+		child-cloneWithChildren(bufmap);
 		BufferMap::iterator const bit = bufmap.find(child);
 		LASSERT(bit != bufmap.end(), continue);
 		Buffer * child_clone = bit-second;
@@ -479,6 +479,14 @@ void Buffer::clone(BufferMap  bufmap) const
 }
 
 
+Buffer * Buffer::cloneBufferOnly() const {
+	Buffer * buffer_clone = new Buffer(fileName().absFileName(), false, this);
+	// we won't be cloning the children
+	buffer_clone-d-children_positions.clear();
+	return buffer_clone;
+}
+
+
 bool Buffer::isClone() const
 {
 	return d-cloned_buffer_;
diff --git a/src/Buffer.h b/src/Buffer.h
index bf2a9ea..463ff66 100644
--- a/src/Buffer.h
+++ b/src/Buffer.h
@@ -166,8 +166,11 @@ public:
 	/// Destructor
 	~Buffer();
 
-	///
-	Buffer * clone() const;
+	/// Clones the entire structure of which this Buffer is part, starting
+	/// with the master and cloning all the children, too.
+	Buffer * cloneFromMaster() const;
+	/// Just clones this single Buffer. For autosave.
+	Buffer * cloneBufferOnly() const;
 	///
 	bool isClone() const;
 
@@ -230,7 +233,7 @@ private:
 	///
 	typedef std::mapBuffer const *, Buffer * BufferMap;
 	///
-	void clone(BufferMap ) const;
+	void cloneWithChildren(BufferMap ) const;
 	/// save timestamp and checksum of the given file.
 	void saveCheckSum() const;	
 	/// read a new file
diff --git a/src/frontends/qt4/GuiView.cpp b/src/frontends/qt4/GuiView.cpp
index 2409823..48bbc05 100644
--- a/src/frontends/qt4/GuiView.cpp
+++ b/src/frontends/qt4/GuiView.cpp
@@ -1575,7 +1575,7 @@ void GuiView::autoSave()
 #if (QT_VERSION = 0x040400)
 	GuiViewPrivate::busyBuffers.insert(buffer);
 	QFuturedocstring f = QtConcurrent::run(GuiViewPrivate::autosaveAndDestroy,
-		buffer, buffer-clone());
+		buffer, buffer-cloneBufferOnly());
 	d.autosave_watcher_.setFuture(f);
 #else
 	buffer-autoSave();
@@ -3122,7 +3122,7 @@ bool GuiView::GuiViewPrivate::asyncBufferProcessing(
 	QFutureBuffer::ExportStatus f = QtConcurrent::run(
 asyncFunc,
 used_buffer,
-used_buffer-clone(),
+used_buffer-cloneFromMaster(),
 format);
 	setPreviewFuture(f);
 	last_export_format = used_buffer-params().bufferFormat();
-- 
1.7.4.4

[PATCH] Fix Autosave Memory Leak

[Richard Heck]

In discussion of Pavel's crash, Vincent asked whether we were
unnecessarily cloning all the children of a given Buffer when we
autosave it. The answer is that we were and, worse, that, since changes
I made to fix other bugs, we were starting the cloning process from the
master Buffer and then, even worse, not deleting the master but only the
original Buffer when we were done. In the right setting, this would leak
a lot of memory.

The attached patch addresses this issue. Basically, it just clones the
single Buffer we are trying to autosave. Comments welcome, especially on
whether we should put this into 2.0.2. It looks pretty safe to me, and
the memory leak, as I said, could be pretty bad otherwise.

Richard


>From 1c0f114c36983d72af27ec0f26f643eb2e7845ba Mon Sep 17 00:00:00 2001
From: Richard Heck <rgh...@lyx.org>
Date: Tue, 15 Nov 2011 11:08:50 -0500
Subject: [PATCH] Don't clone all the children on autosave.

---
 src/Buffer.cpp|   16 
 src/Buffer.h  |9 ++---
 src/frontends/qt4/GuiView.cpp |4 ++--
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index 625c0ff..a1aada2 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -430,17 +430,17 @@ Buffer::~Buffer()
 }
 
 
-Buffer * Buffer::clone() const
+Buffer * Buffer::cloneFromMaster() const
 {
 	BufferMap bufmap;
-	masterBuffer()->clone(bufmap);
+	masterBuffer()->cloneWithChildren(bufmap);
 	BufferMap::iterator it = bufmap.find(this);
 	LASSERT(it != bufmap.end(), return 0);
 	return it->second;
 }
 
 
-void Buffer::clone(BufferMap & bufmap) const
+void Buffer::cloneWithChildren(BufferMap & bufmap) const
 {
 	// have we already been cloned?
 	if (bufmap.find(this) != bufmap.end())
@@ -461,7 +461,7 @@ void Buffer::clone(BufferMap & bufmap) const
 		dit.setBuffer(buffer_clone);
 		Buffer * child = const_cast(it->second.second);
 
-		child->clone(bufmap);
+		child->cloneWithChildren(bufmap);
 		BufferMap::iterator const bit = bufmap.find(child);
 		LASSERT(bit != bufmap.end(), continue);
 		Buffer * child_clone = bit->second;
@@ -479,6 +479,14 @@ void Buffer::clone(BufferMap & bufmap) const
 }
 
 
+Buffer * Buffer::cloneBufferOnly() const {
+	Buffer * buffer_clone = new Buffer(fileName().absFileName(), false, this);
+	// we won't be cloning the children
+	buffer_clone->d->children_positions.clear();
+	return buffer_clone;
+}
+
+
 bool Buffer::isClone() const
 {
 	return d->cloned_buffer_;
diff --git a/src/Buffer.h b/src/Buffer.h
index bf2a9ea..463ff66 100644
--- a/src/Buffer.h
+++ b/src/Buffer.h
@@ -166,8 +166,11 @@ public:
 	/// Destructor
 	~Buffer();
 
-	///
-	Buffer * clone() const;
+	/// Clones the entire structure of which this Buffer is part, starting
+	/// with the master and cloning all the children, too.
+	Buffer * cloneFromMaster() const;
+	/// Just clones this single Buffer. For autosave.
+	Buffer * cloneBufferOnly() const;
 	///
 	bool isClone() const;
 
@@ -230,7 +233,7 @@ private:
 	///
 	typedef std::map BufferMap;
 	///
-	void clone(BufferMap &) const;
+	void cloneWithChildren(BufferMap &) const;
 	/// save timestamp and checksum of the given file.
 	void saveCheckSum() const;	
 	/// read a new file
diff --git a/src/frontends/qt4/GuiView.cpp b/src/frontends/qt4/GuiView.cpp
index 2409823..48bbc05 100644
--- a/src/frontends/qt4/GuiView.cpp
+++ b/src/frontends/qt4/GuiView.cpp
@@ -1575,7 +1575,7 @@ void GuiView::autoSave()
 #if (QT_VERSION >= 0x040400)
 	GuiViewPrivate::busyBuffers.insert(buffer);
 	QFuture f = QtConcurrent::run(GuiViewPrivate::autosaveAndDestroy,
-		buffer, buffer->clone());
+		buffer, buffer->cloneBufferOnly());
 	d.autosave_watcher_.setFuture(f);
 #else
 	buffer->autoSave();
@@ -3122,7 +3122,7 @@ bool GuiView::GuiViewPrivate::asyncBufferProcessing(
 	QFuture f = QtConcurrent::run(
 asyncFunc,
 used_buffer,
-used_buffer->clone(),
+used_buffer->cloneFromMaster(),
 format);
 	setPreviewFuture(f);
 	last_export_format = used_buffer->params().bufferFormat();
-- 
1.7.4.4

Re: Memory leak?

[Abdelrazak Younes]

rgheck wrote:

On 08/10/2009 05:01 PM, Abdelrazak Younes wrote:

On 10/08/2009 22:46, rgheck wrote:

On 08/10/2009 04:21 PM, Abdelrazak Younes wrote:
QPixmap are implicitly shared. So this means that when you do a = 
b, a will be a shadow copy of b up until it is modified, thus 
occupying zero additional memory. This is the same concept as 
shared pointer, the object, and thus the memory, is not cleared up 
until the last use of it is destroyed.


OK, so here's an idea. The problem lies more or less in 
Loader::Impl::createPixmap(), here:


image_.reset(cached_item_-image()-clone());

bool const success = image_-setPixmap(params_);


The first line clones the original image, if I'm not mistaken, and 
all the transformations then happen in the call to 
GuiImage::setPixmap(). But the cached original remains. So can we 
clear that once this has happened?


We can, if you manage to give access to the original image inside 
GuiImage::setPixmap(). Without major surgery I am sure this is 
possible. Or at least that was the conclusion I reached last time I 
had the same idea ;-)


My thought was more simpleminded, but I don't know this code. Why do 
we need a cache of the original image once the pixmap has been set? If 
the original is changed, then presumably we want to reload. But if it 
doesn't change, we don't need access to it any more, do we?


Correct.

So, if setPixmap() returns true, then can't we do something like 
reset() the cached_item_ and just get rid if that QImage?


You can try. Last time I did, I failed. But I was so upset about this 
piece of code that maybe the solution was much simpler than I thought...


Abdel.

Re: Memory leak?

[rgheck]

On 08/11/2009 03:40 AM, Abdelrazak Younes wrote:


So, if setPixmap() returns true, then can't we do something like 
reset() the cached_item_ and just get rid if that QImage?


You can try. Last time I did, I failed. But I was so upset about this 
piece of code that maybe the solution was much simpler than I thought...


Hmm. That's a little too brute force. We need the cache item, for 
monitoring and the like. What we don't need is to have the image itself 
still there. Does that seem right? (Sorry, I don't know this code and am 
just poking around.)


rh

Re: Memory leak?

[Abdelrazak Younes]

rgheck wrote:

On 08/10/2009 05:01 PM, Abdelrazak Younes wrote:

On 10/08/2009 22:46, rgheck wrote:

On 08/10/2009 04:21 PM, Abdelrazak Younes wrote:
QPixmap are implicitly shared. So this means that when you do a = 
b, a will be a shadow copy of b up until it is modified, thus 
occupying zero additional memory. This is the same concept as 
shared pointer, the object, and thus the memory, is not cleared up 
until the last use of it is destroyed.


OK, so here's an idea. The problem lies more or less in 
Loader::Impl::createPixmap(), here:


image_.reset(cached_item_->image()->clone());

bool const success = image_->setPixmap(params_);


The first line clones the original image, if I'm not mistaken, and 
all the transformations then happen in the call to 
GuiImage::setPixmap(). But the cached original remains. So can we 
clear that once this has happened?


We can, if you manage to give access to the original image inside 
GuiImage::setPixmap(). Without major surgery I am sure this is 
possible. Or at least that was the conclusion I reached last time I 
had the same idea ;-)


My thought was more simpleminded, but I don't know this code. Why do 
we need a cache of the original image once the pixmap has been set? If 
the original is changed, then presumably we want to reload. But if it 
doesn't change, we don't need access to it any more, do we?


Correct.

So, if setPixmap() returns true, then can't we do something like 
reset() the cached_item_ and just get rid if that QImage?


You can try. Last time I did, I failed. But I was so upset about this 
piece of code that maybe the solution was much simpler than I thought...


Abdel.

Re: Memory leak?

[rgheck]

On 08/11/2009 03:40 AM, Abdelrazak Younes wrote:


So, if setPixmap() returns true, then can't we do something like 
reset() the cached_item_ and just get rid if that QImage?


You can try. Last time I did, I failed. But I was so upset about this 
piece of code that maybe the solution was much simpler than I thought...


Hmm. That's a little too brute force. We need the cache item, for 
monitoring and the like. What we don't need is to have the image itself 
still there. Does that seem right? (Sorry, I don't know this code and am 
just poking around.)


rh

Memory leak?

[Pavel Sanda]

hi,
for the third time my system get frozen because of
bug http://www.lyx.org/trac/ticket/5002.

it seems when we load eg. jpg image to cache and rescale it, the
original image is not forgoten so even small scale is of no help
and more print-ready image are able to knock system down.

the code looks like as we intend to free this memory, but it
wont happen:

@@ -108,9 +108,12 @@ bool GuiImage::setPixmap(Params const  params)
// Clear the pixmap to save some memory.
if (is_transformed_)
original_ = QImage();

this code should work? if i manually add original_.~QImage(); memory is
rescued and no swapping happens. however i get some crash in lyx quit then;
aparently graphics cache is more intricated stuff...

pavel

Re: Memory leak?

[Abdelrazak Younes]

On 10/08/2009 21:34, Pavel Sanda wrote:

hi,
for the third time my system get frozen because of
bug http://www.lyx.org/trac/ticket/5002.

it seems when we load eg. jpg image to cache and rescale it, the
original image is not forgoten so even small scale is of no help
and more print-ready image are able to knock system down.

the code looks like as we intend to free this memory, but it
wont happen:

@@ -108,9 +108,12 @@ bool GuiImage::setPixmap(Params const  params)
// Clear the pixmap to save some memory.
if (is_transformed_)
original_ = QImage();

this code should work? if i manually add original_.~QImage(); memory is
rescued and no swapping happens. however i get some crash in lyx quit then;
aparently graphics cache is more intricated stuff...
   


Very intricate!

The problem is not coming from Qt but from our own caching mechanism 
which is heavy and is redundant with Qt own caching system. Last time I 
tried to clean up this mess I stopped at this issue with a big 
headache... The only sane solution is to get rid of this complicated 
caching engine and to rewrite everything with Qt only.


Abdel.

Re: Memory leak?

[Abdelrazak Younes]

On 10/08/2009 21:39, Abdelrazak Younes wrote:

On 10/08/2009 21:34, Pavel Sanda wrote:

hi,
for the third time my system get frozen because of
bug http://www.lyx.org/trac/ticket/5002.

it seems when we load eg. jpg image to cache and rescale it, the
original image is not forgoten so even small scale is of no help
and more print-ready image are able to knock system down.

the code looks like as we intend to free this memory, but it
wont happen:

@@ -108,9 +108,12 @@ bool GuiImage::setPixmap(Params const  params)
// Clear the pixmap to save some memory.
if (is_transformed_)
original_ = QImage();

this code should work? if i manually add original_.~QImage(); memory is
rescued and no swapping happens. however i get some crash in lyx quit 
then;

aparently graphics cache is more intricated stuff...


Very intricate!

The problem is not coming from Qt but from our own caching mechanism 
which is heavy and is redundant with Qt own caching system. Last time 
I tried to clean up this mess I stopped at this issue with a big 
headache... The only sane solution is to get rid of this complicated 
caching engine and to rewrite everything with Qt only.


Now that I read my comments in the bug, I see that I reached to the same 
conclusion one year ago :-)


Abdel.

Re: Memory leak?

[Pavel Sanda]

Abdelrazak Younes wrote:
 Very intricate!

 The problem is not coming from Qt but from our own caching mechanism which 
 is heavy and is redundant with Qt own caching system. Last time I tried to 
 clean up this mess I stopped at this issue with a big headache... The only 
 sane solution is to get rid of this complicated caching engine and to 
 rewrite everything with Qt only.

but my question is simpler now:
why should original_ = QImage(); release the memory as the comment above says?
if not what would be correct way of releasing it?

pavel

Re: Memory leak?

[Abdelrazak Younes]

On 10/08/2009 21:46, Pavel Sanda wrote:

Abdelrazak Younes wrote:
   

Very intricate!

The problem is not coming from Qt but from our own caching mechanism which
is heavy and is redundant with Qt own caching system. Last time I tried to
clean up this mess I stopped at this issue with a big headache... The only
sane solution is to get rid of this complicated caching engine and to
rewrite everything with Qt only.
 


but my question is simpler now:
why should original_ = QImage(); release the memory as the comment above says?
if not what would be correct way of releasing it?
   


This code works and is not the problem. I don't remember the details but 
the original image is also stored somewhere else in the graphics cache, 
AFAIR.


Abdel.

Re: Memory leak?

[Pavel Sanda]

Abdelrazak Younes wrote:
 but my question is simpler now:
 why should original_ = QImage(); release the memory as the comment above 
 says?
 if not what would be correct way of releasing it?


 This code works and is not the problem. I don't remember the details but 
 the original image is also stored somewhere else in the graphics cache, 
 AFAIR.

which doesn't help me to understand what '// Clear the pixmap to save some 
memory'
means :)

pavel

Re: Memory leak?

[Abdelrazak Younes]

On 10/08/2009 21:55, Pavel Sanda wrote:

Abdelrazak Younes wrote:
   

but my question is simpler now:
why should original_ = QImage(); release the memory as the comment above
says?
if not what would be correct way of releasing it?

   

This code works and is not the problem. I don't remember the details but
the original image is also stored somewhere else in the graphics cache,
AFAIR.
 


which doesn't help me to understand what '// Clear the pixmap to save some 
memory'
means :)

   
It means Here we clear the pixmap 'original_' to save the memory that 
it occupies; I don't know to express it in a better way...


Abdel.

Re: Memory leak?

[Pavel Sanda]

Abdelrazak Younes wrote:
 This code works and is not the problem. I don't remember the details but
 the original image is also stored somewhere else in the graphics cache,
 AFAIR.
  

 which doesn't help me to understand what '// Clear the pixmap to save some 
 memory'
 means :)


 It means Here we clear the pixmap 'original_' to save the memory that it 
 occupies; I don't know to express it in a better way...

maybe its just too late for me today and missing something obvious... but
why i see huge memory difference for the code
if (is_transformed_)
{
original_.~QImage();
original_ = QImage();
}

and for 

if (is_transformed_)
original_ = QImage();

if the second one is intended to clear that pixmap?
pavel

Re: Memory leak?

[Abdelrazak Younes]

On 10/08/2009 22:11, Pavel Sanda wrote:

Abdelrazak Younes wrote:
   

This code works and is not the problem. I don't remember the details but
the original image is also stored somewhere else in the graphics cache,
AFAIR.

 

which doesn't help me to understand what '// Clear the pixmap to save some
memory'
means :)


   

It means Here we clear the pixmap 'original_' to save the memory that it
occupies; I don't know to express it in a better way...
 


maybe its just too late for me today and missing something obvious... but
why i see huge memory difference for the code
if (is_transformed_)
{
 original_.~QImage();
 original_ = QImage();
}

and for

if (is_transformed_)
original_ = QImage();

if the second one is intended to clear that pixmap?
   


Because the original_ pixmap has a shadow copy somewhere else deep 
inside our graphics system. So deep that it's basically a nightmare to 
retrieve it at this point. When you call the destructor this shadow copy 
is of course erased at the same time. This explain why you get a crash 
at exit.


Abdel.

Re: Memory leak?

[rgheck]

On 08/10/2009 04:16 PM, Abdelrazak Younes wrote:

On 10/08/2009 22:11, Pavel Sanda wrote:

Abdelrazak Younes wrote:
This code works and is not the problem. I don't remember the 
details but
the original image is also stored somewhere else in the graphics 
cache,

AFAIR.

which doesn't help me to understand what '// Clear the pixmap to 
save some

memory'
means :)


It means Here we clear the pixmap 'original_' to save the memory 
that it

occupies; I don't know to express it in a better way...


maybe its just too late for me today and missing something obvious... 
but

why i see huge memory difference for the code
if (is_transformed_)
{
 original_.~QImage();
 original_ = QImage();
}

and for

if (is_transformed_)
original_ = QImage();

if the second one is intended to clear that pixmap?


Because the original_ pixmap has a shadow copy somewhere else deep 
inside our graphics system. So deep that it's basically a nightmare to 
retrieve it at this point. When you call the destructor this shadow 
copy is of course erased at the same time. This explain why you get a 
crash at exit.


Can you explain what you mean by shadow copy here? Do you mean there 
is some other variable that points to it, or contains a reference to it, 
or something of the sort?


rh

Re: Memory leak?

[Abdelrazak Younes]

On 10/08/2009 22:18, rgheck wrote:

On 08/10/2009 04:16 PM, Abdelrazak Younes wrote:

On 10/08/2009 22:11, Pavel Sanda wrote:

Abdelrazak Younes wrote:
This code works and is not the problem. I don't remember the 
details but
the original image is also stored somewhere else in the graphics 
cache,

AFAIR.

which doesn't help me to understand what '// Clear the pixmap to 
save some

memory'
means :)


It means Here we clear the pixmap 'original_' to save the memory 
that it

occupies; I don't know to express it in a better way...


maybe its just too late for me today and missing something 
obvious... but

why i see huge memory difference for the code
if (is_transformed_)
{
 original_.~QImage();
 original_ = QImage();
}

and for

if (is_transformed_)
original_ = QImage();

if the second one is intended to clear that pixmap?


Because the original_ pixmap has a shadow copy somewhere else deep 
inside our graphics system. So deep that it's basically a nightmare 
to retrieve it at this point. When you call the destructor this 
shadow copy is of course erased at the same time. This explain why 
you get a crash at exit.


Can you explain what you mean by shadow copy here? Do you mean there 
is some other variable that points to it, or contains a reference to 
it, or something of the sort?


QPixmap are implicitly shared. So this means that when you do a = b, a 
will be a shadow copy of b up until it is modified, thus occupying zero 
additional memory. This is the same concept as shared pointer, the 
object, and thus the memory, is not cleared up until the last use of it 
is destroyed.


Abdel.

Re: Memory leak?

[Andre Poenitz]

On Mon, Aug 10, 2009 at 09:34:00PM +0200, Pavel Sanda wrote:
 hi,
 for the third time my system get frozen because of
 bug http://www.lyx.org/trac/ticket/5002.
 
 it seems when we load eg. jpg image to cache and rescale it, the
 original image is not forgoten so even small scale is of no help
 and more print-ready image are able to knock system down.
 
 the code looks like as we intend to free this memory, but it
 wont happen:
 
 @@ -108,9 +108,12 @@ bool GuiImage::setPixmap(Params const  params)
   // Clear the pixmap to save some memory.
   if (is_transformed_)
   original_ = QImage();
   
 this code should work?

Yes.

 if i manually add original_.~QImage(); memory is rescued and no
 swapping happens. however i get some crash in lyx quit then; aparently
 graphics cache is more intricated stuff...

Probably because the destructor runs twice: once called explicitly by
you and the second time wheneven the object containing original_ is
destroyed.

Andre'

Re: Memory leak?

[Pavel Sanda]

Abdelrazak Younes wrote:
 QPixmap are implicitly shared.

this was the missing brick.. thanks
pavel

Re: Memory leak?

[rgheck]

On 08/10/2009 04:21 PM, Abdelrazak Younes wrote:
QPixmap are implicitly shared. So this means that when you do a = b, a 
will be a shadow copy of b up until it is modified, thus occupying 
zero additional memory. This is the same concept as shared pointer, 
the object, and thus the memory, is not cleared up until the last use 
of it is destroyed.


OK, so here's an idea. The problem lies more or less in 
Loader::Impl::createPixmap(), here:


image_.reset(cached_item_-image()-clone());

bool const success = image_-setPixmap(params_);


The first line clones the original image, if I'm not mistaken, and all 
the transformations then happen in the call to GuiImage::setPixmap(). 
But the cached original remains. So can we clear that once this has 
happened?


Richard

Re: Memory leak?

[Abdelrazak Younes]

On 10/08/2009 22:46, rgheck wrote:

On 08/10/2009 04:21 PM, Abdelrazak Younes wrote:
QPixmap are implicitly shared. So this means that when you do a = b, 
a will be a shadow copy of b up until it is modified, thus occupying 
zero additional memory. This is the same concept as shared pointer, 
the object, and thus the memory, is not cleared up until the last use 
of it is destroyed.


OK, so here's an idea. The problem lies more or less in 
Loader::Impl::createPixmap(), here:


image_.reset(cached_item_-image()-clone());

bool const success = image_-setPixmap(params_);


The first line clones the original image, if I'm not mistaken, and all 
the transformations then happen in the call to GuiImage::setPixmap(). 
But the cached original remains. So can we clear that once this has 
happened?


We can, if you manage to give access to the original image inside 
GuiImage::setPixmap(). Without major surgery I am sure this is possible. 
Or at least that was the conclusion I reached last time I had the same 
idea ;-)


Abdel.

Re: Memory leak?

[rgheck]

On 08/10/2009 05:01 PM, Abdelrazak Younes wrote:

On 10/08/2009 22:46, rgheck wrote:

On 08/10/2009 04:21 PM, Abdelrazak Younes wrote:
QPixmap are implicitly shared. So this means that when you do a = b, 
a will be a shadow copy of b up until it is modified, thus occupying 
zero additional memory. This is the same concept as shared pointer, 
the object, and thus the memory, is not cleared up until the last 
use of it is destroyed.


OK, so here's an idea. The problem lies more or less in 
Loader::Impl::createPixmap(), here:


image_.reset(cached_item_-image()-clone());

bool const success = image_-setPixmap(params_);


The first line clones the original image, if I'm not mistaken, and 
all the transformations then happen in the call to 
GuiImage::setPixmap(). But the cached original remains. So can we 
clear that once this has happened?


We can, if you manage to give access to the original image inside 
GuiImage::setPixmap(). Without major surgery I am sure this is 
possible. Or at least that was the conclusion I reached last time I 
had the same idea ;-)


My thought was more simpleminded, but I don't know this code. Why do we 
need a cache of the original image once the pixmap has been set? If the 
original is changed, then presumably we want to reload. But if it 
doesn't change, we don't need access to it any more, do we? So, if 
setPixmap() returns true, then can't we do something like reset() the 
cached_item_ and just get rid if that QImage?


rh


Abdel.

Re: Memory leak?

[Pavel Sanda]

Richard Heck wrote:
 My thought was more simpleminded, but I don't know this code. Why do we 
 need a cache of the original image once the pixmap has been set? If the 
 original is changed, then presumably we want to reload. But if it doesn't 
 change, we don't need access to it any more, do we? So, if setPixmap() 
 returns true, then can't we do something like reset() the cached_item_ and 
 just get rid if that QImage?

maybe we want it when user want eg rescale to a different value?
pavel

Re: Memory leak?

[rgheck]

On 08/10/2009 06:23 PM, Pavel Sanda wrote:

Richard Heck wrote:
   

My thought was more simpleminded, but I don't know this code. Why do we
need a cache of the original image once the pixmap has been set? If the
original is changed, then presumably we want to reload. But if it doesn't
change, we don't need access to it any more, do we? So, if setPixmap()
returns true, then can't we do something like reset() the cached_item_ and
just get rid if that QImage?
 


maybe we want it when user want eg rescale to a different value?

   
Though then you have it, and so get the memory issues you're seeing. So 
we have to let it go, I think. In any event, the rescale will happen 
little enough that we can re-read from the disk then.


rh


pavel
   

Memory leak?

[Pavel Sanda]

hi,
for the third time my system get frozen because of
bug http://www.lyx.org/trac/ticket/5002.

it seems when we load eg. jpg image to cache and rescale it, the
original image is not forgoten so even small scale is of no help
and more print-ready image are able to knock system down.

the code looks like as we intend to free this memory, but it
wont happen:

@@ -108,9 +108,12 @@ bool GuiImage::setPixmap(Params const & params)
// Clear the pixmap to save some memory.
if (is_transformed_)
original_ = QImage();

this code should work? if i manually add original_.~QImage(); memory is
rescued and no swapping happens. however i get some crash in lyx quit then;
aparently graphics cache is more intricated stuff...

pavel

Re: Memory leak?

[Abdelrazak Younes]

On 10/08/2009 21:34, Pavel Sanda wrote:

hi,
for the third time my system get frozen because of
bug http://www.lyx.org/trac/ticket/5002.

it seems when we load eg. jpg image to cache and rescale it, the
original image is not forgoten so even small scale is of no help
and more print-ready image are able to knock system down.

the code looks like as we intend to free this memory, but it
wont happen:

@@ -108,9 +108,12 @@ bool GuiImage::setPixmap(Params const&  params)
// Clear the pixmap to save some memory.
if (is_transformed_)
original_ = QImage();

this code should work? if i manually add original_.~QImage(); memory is
rescued and no swapping happens. however i get some crash in lyx quit then;
aparently graphics cache is more intricated stuff...
   


Very intricate!

The problem is not coming from Qt but from our own caching mechanism 
which is heavy and is redundant with Qt own caching system. Last time I 
tried to clean up this mess I stopped at this issue with a big 
headache... The only sane solution is to get rid of this complicated 
caching engine and to rewrite everything with Qt only.


Abdel.

Re: Memory leak?

[Abdelrazak Younes]

On 10/08/2009 21:39, Abdelrazak Younes wrote:

On 10/08/2009 21:34, Pavel Sanda wrote:

hi,
for the third time my system get frozen because of
bug http://www.lyx.org/trac/ticket/5002.

it seems when we load eg. jpg image to cache and rescale it, the
original image is not forgoten so even small scale is of no help
and more print-ready image are able to knock system down.

the code looks like as we intend to free this memory, but it
wont happen:

@@ -108,9 +108,12 @@ bool GuiImage::setPixmap(Params const&  params)
// Clear the pixmap to save some memory.
if (is_transformed_)
original_ = QImage();

this code should work? if i manually add original_.~QImage(); memory is
rescued and no swapping happens. however i get some crash in lyx quit 
then;

aparently graphics cache is more intricated stuff...


Very intricate!

The problem is not coming from Qt but from our own caching mechanism 
which is heavy and is redundant with Qt own caching system. Last time 
I tried to clean up this mess I stopped at this issue with a big 
headache... The only sane solution is to get rid of this complicated 
caching engine and to rewrite everything with Qt only.


Now that I read my comments in the bug, I see that I reached to the same 
conclusion one year ago :-)


Abdel.

Re: Memory leak?

[Pavel Sanda]

Abdelrazak Younes wrote:
> Very intricate!
>
> The problem is not coming from Qt but from our own caching mechanism which 
> is heavy and is redundant with Qt own caching system. Last time I tried to 
> clean up this mess I stopped at this issue with a big headache... The only 
> sane solution is to get rid of this complicated caching engine and to 
> rewrite everything with Qt only.

but my question is simpler now:
why should original_ = QImage(); release the memory as the comment above says?
if not what would be correct way of releasing it?

pavel

Re: Memory leak?

[Abdelrazak Younes]

On 10/08/2009 21:46, Pavel Sanda wrote:

Abdelrazak Younes wrote:
   

Very intricate!

The problem is not coming from Qt but from our own caching mechanism which
is heavy and is redundant with Qt own caching system. Last time I tried to
clean up this mess I stopped at this issue with a big headache... The only
sane solution is to get rid of this complicated caching engine and to
rewrite everything with Qt only.
 


but my question is simpler now:
why should original_ = QImage(); release the memory as the comment above says?
if not what would be correct way of releasing it?
   


This code works and is not the problem. I don't remember the details but 
the original image is also stored somewhere else in the graphics cache, 
AFAIR.


Abdel.

Re: Memory leak?

[Pavel Sanda]

Abdelrazak Younes wrote:
>> but my question is simpler now:
>> why should original_ = QImage(); release the memory as the comment above 
>> says?
>> if not what would be correct way of releasing it?
>>
>
> This code works and is not the problem. I don't remember the details but 
> the original image is also stored somewhere else in the graphics cache, 
> AFAIR.

which doesn't help me to understand what '// Clear the pixmap to save some 
memory'
means :)

pavel

Re: Memory leak?

[Abdelrazak Younes]

On 10/08/2009 21:55, Pavel Sanda wrote:

Abdelrazak Younes wrote:
   

but my question is simpler now:
why should original_ = QImage(); release the memory as the comment above
says?
if not what would be correct way of releasing it?

   

This code works and is not the problem. I don't remember the details but
the original image is also stored somewhere else in the graphics cache,
AFAIR.
 


which doesn't help me to understand what '// Clear the pixmap to save some 
memory'
means :)

   
It means "Here we clear the pixmap 'original_' to save the memory that 
it occupies"; I don't know to express it in a better way...


Abdel.

Re: Memory leak?

[Pavel Sanda]

Abdelrazak Younes wrote:
>>> This code works and is not the problem. I don't remember the details but
>>> the original image is also stored somewhere else in the graphics cache,
>>> AFAIR.
>>>  
>>
>> which doesn't help me to understand what '// Clear the pixmap to save some 
>> memory'
>> means :)
>>
>>
> It means "Here we clear the pixmap 'original_' to save the memory that it 
> occupies"; I don't know to express it in a better way...

maybe its just too late for me today and missing something obvious... but
why i see huge memory difference for the code
if (is_transformed_)
{
original_.~QImage();
original_ = QImage();
}

and for 

if (is_transformed_)
original_ = QImage();

if the second one is intended to clear that pixmap?
pavel

Re: Memory leak?

[Abdelrazak Younes]

On 10/08/2009 22:11, Pavel Sanda wrote:

Abdelrazak Younes wrote:
   

This code works and is not the problem. I don't remember the details but
the original image is also stored somewhere else in the graphics cache,
AFAIR.

 

which doesn't help me to understand what '// Clear the pixmap to save some
memory'
means :)


   

It means "Here we clear the pixmap 'original_' to save the memory that it
occupies"; I don't know to express it in a better way...
 


maybe its just too late for me today and missing something obvious... but
why i see huge memory difference for the code
if (is_transformed_)
{
 original_.~QImage();
 original_ = QImage();
}

and for

if (is_transformed_)
original_ = QImage();

if the second one is intended to clear that pixmap?
   


Because the original_ pixmap has a shadow copy somewhere else deep 
inside our graphics system. So deep that it's basically a nightmare to 
retrieve it at this point. When you call the destructor this shadow copy 
is of course erased at the same time. This explain why you get a crash 
at exit.


Abdel.

Re: Memory leak?

[rgheck]

On 08/10/2009 04:16 PM, Abdelrazak Younes wrote:

On 10/08/2009 22:11, Pavel Sanda wrote:

Abdelrazak Younes wrote:
This code works and is not the problem. I don't remember the 
details but
the original image is also stored somewhere else in the graphics 
cache,

AFAIR.

which doesn't help me to understand what '// Clear the pixmap to 
save some

memory'
means :)


It means "Here we clear the pixmap 'original_' to save the memory 
that it

occupies"; I don't know to express it in a better way...


maybe its just too late for me today and missing something obvious... 
but

why i see huge memory difference for the code
if (is_transformed_)
{
 original_.~QImage();
 original_ = QImage();
}

and for

if (is_transformed_)
original_ = QImage();

if the second one is intended to clear that pixmap?


Because the original_ pixmap has a shadow copy somewhere else deep 
inside our graphics system. So deep that it's basically a nightmare to 
retrieve it at this point. When you call the destructor this shadow 
copy is of course erased at the same time. This explain why you get a 
crash at exit.


Can you explain what you mean by "shadow copy" here? Do you mean there 
is some other variable that points to it, or contains a reference to it, 
or something of the sort?


rh

Re: Memory leak?

[Abdelrazak Younes]

On 10/08/2009 22:18, rgheck wrote:

On 08/10/2009 04:16 PM, Abdelrazak Younes wrote:

On 10/08/2009 22:11, Pavel Sanda wrote:

Abdelrazak Younes wrote:
This code works and is not the problem. I don't remember the 
details but
the original image is also stored somewhere else in the graphics 
cache,

AFAIR.

which doesn't help me to understand what '// Clear the pixmap to 
save some

memory'
means :)


It means "Here we clear the pixmap 'original_' to save the memory 
that it

occupies"; I don't know to express it in a better way...


maybe its just too late for me today and missing something 
obvious... but

why i see huge memory difference for the code
if (is_transformed_)
{
 original_.~QImage();
 original_ = QImage();
}

and for

if (is_transformed_)
original_ = QImage();

if the second one is intended to clear that pixmap?


Because the original_ pixmap has a shadow copy somewhere else deep 
inside our graphics system. So deep that it's basically a nightmare 
to retrieve it at this point. When you call the destructor this 
shadow copy is of course erased at the same time. This explain why 
you get a crash at exit.


Can you explain what you mean by "shadow copy" here? Do you mean there 
is some other variable that points to it, or contains a reference to 
it, or something of the sort?


QPixmap are implicitly shared. So this means that when you do a = b, a 
will be a shadow copy of b up until it is modified, thus occupying zero 
additional memory. This is the same concept as shared pointer, the 
object, and thus the memory, is not cleared up until the last use of it 
is destroyed.


Abdel.

Re: Memory leak?

[Andre Poenitz]

On Mon, Aug 10, 2009 at 09:34:00PM +0200, Pavel Sanda wrote:
> hi,
> for the third time my system get frozen because of
> bug http://www.lyx.org/trac/ticket/5002.
> 
> it seems when we load eg. jpg image to cache and rescale it, the
> original image is not forgoten so even small scale is of no help
> and more print-ready image are able to knock system down.
> 
> the code looks like as we intend to free this memory, but it
> wont happen:
> 
> @@ -108,9 +108,12 @@ bool GuiImage::setPixmap(Params const & params)
>   // Clear the pixmap to save some memory.
>   if (is_transformed_)
>   original_ = QImage();
>   
> this code should work?

Yes.

> if i manually add original_.~QImage(); memory is rescued and no
> swapping happens. however i get some crash in lyx quit then; aparently
> graphics cache is more intricated stuff...

Probably because the destructor runs twice: once called explicitly by
you and the second time wheneven the object containing original_ is
destroyed.

Andre'

Re: Memory leak?

[Pavel Sanda]

Abdelrazak Younes wrote:
> QPixmap are implicitly shared.

this was the missing brick.. thanks
pavel

Re: Memory leak?

[rgheck]

On 08/10/2009 04:21 PM, Abdelrazak Younes wrote:
QPixmap are implicitly shared. So this means that when you do a = b, a 
will be a shadow copy of b up until it is modified, thus occupying 
zero additional memory. This is the same concept as shared pointer, 
the object, and thus the memory, is not cleared up until the last use 
of it is destroyed.


OK, so here's an idea. The problem lies more or less in 
Loader::Impl::createPixmap(), here:


image_.reset(cached_item_->image()->clone());

bool const success = image_->setPixmap(params_);


The first line clones the original image, if I'm not mistaken, and all 
the transformations then happen in the call to GuiImage::setPixmap(). 
But the cached original remains. So can we clear that once this has 
happened?


Richard

Re: Memory leak?

[Abdelrazak Younes]

On 10/08/2009 22:46, rgheck wrote:

On 08/10/2009 04:21 PM, Abdelrazak Younes wrote:
QPixmap are implicitly shared. So this means that when you do a = b, 
a will be a shadow copy of b up until it is modified, thus occupying 
zero additional memory. This is the same concept as shared pointer, 
the object, and thus the memory, is not cleared up until the last use 
of it is destroyed.


OK, so here's an idea. The problem lies more or less in 
Loader::Impl::createPixmap(), here:


image_.reset(cached_item_->image()->clone());

bool const success = image_->setPixmap(params_);


The first line clones the original image, if I'm not mistaken, and all 
the transformations then happen in the call to GuiImage::setPixmap(). 
But the cached original remains. So can we clear that once this has 
happened?


We can, if you manage to give access to the original image inside 
GuiImage::setPixmap(). Without major surgery I am sure this is possible. 
Or at least that was the conclusion I reached last time I had the same 
idea ;-)


Abdel.

Re: Memory leak?

[rgheck]

On 08/10/2009 05:01 PM, Abdelrazak Younes wrote:

On 10/08/2009 22:46, rgheck wrote:

On 08/10/2009 04:21 PM, Abdelrazak Younes wrote:
QPixmap are implicitly shared. So this means that when you do a = b, 
a will be a shadow copy of b up until it is modified, thus occupying 
zero additional memory. This is the same concept as shared pointer, 
the object, and thus the memory, is not cleared up until the last 
use of it is destroyed.


OK, so here's an idea. The problem lies more or less in 
Loader::Impl::createPixmap(), here:


image_.reset(cached_item_->image()->clone());

bool const success = image_->setPixmap(params_);


The first line clones the original image, if I'm not mistaken, and 
all the transformations then happen in the call to 
GuiImage::setPixmap(). But the cached original remains. So can we 
clear that once this has happened?


We can, if you manage to give access to the original image inside 
GuiImage::setPixmap(). Without major surgery I am sure this is 
possible. Or at least that was the conclusion I reached last time I 
had the same idea ;-)


My thought was more simpleminded, but I don't know this code. Why do we 
need a cache of the original image once the pixmap has been set? If the 
original is changed, then presumably we want to reload. But if it 
doesn't change, we don't need access to it any more, do we? So, if 
setPixmap() returns true, then can't we do something like reset() the 
cached_item_ and just get rid if that QImage?


rh


Abdel.

Re: Memory leak?

[Pavel Sanda]

Richard Heck wrote:
> My thought was more simpleminded, but I don't know this code. Why do we 
> need a cache of the original image once the pixmap has been set? If the 
> original is changed, then presumably we want to reload. But if it doesn't 
> change, we don't need access to it any more, do we? So, if setPixmap() 
> returns true, then can't we do something like reset() the cached_item_ and 
> just get rid if that QImage?

maybe we want it when user want eg rescale to a different value?
pavel

Re: Memory leak?

[rgheck]

On 08/10/2009 06:23 PM, Pavel Sanda wrote:

Richard Heck wrote:
   

My thought was more simpleminded, but I don't know this code. Why do we
need a cache of the original image once the pixmap has been set? If the
original is changed, then presumably we want to reload. But if it doesn't
change, we don't need access to it any more, do we? So, if setPixmap()
returns true, then can't we do something like reset() the cached_item_ and
just get rid if that QImage?
 


maybe we want it when user want eg rescale to a different value?

   
Though then you have it, and so get the memory issues you're seeing. So 
we have to let it go, I think. In any event, the rescale will happen 
little enough that we can re-read from the disk then.


rh


pavel
   

Re: memory leak in Qt?

[Peter Kümmel]

Andre Poenitz wrote:

On Mon, Dec 03, 2007 at 11:00:00PM +0100, Peter Kümmel wrote:

Playing around with vld I came to the conclusion
that there is a memory leak in Qt's plugin system.

When I trace the allocations of Qt
(see ForceIncludeModules in the vld.ini file)
I get attached output after just opening and closing lyx.


It traces down to the macro in src/corelib/plugin/qplugin.h
(macro from 4.4, similar in 4.3.2):

#define Q_PLUGIN_INSTANCE(IMPLEMENTATION) \
{ \
static 
QT_PREPEND_NAMESPACE(QPointer)QT_PREPEND_NAMESPACE(QObject) _instance; \

if (!_instance)  \
_instance = new IMPLEMENTATION; \
return _instance; \
}


Which plugin is this?


One of the image plugins.



Q_PLUGIN_INSTANCE is documented not to delete the instance, so this 
seems to  'unlucky, but documented behaviour'...


Any reason not to delete it?

--
Peter Kümmel