Re: [Kicad-developers] [Patch] Fix some memory leaks

[Seth Hillbrand]

On 2019-08-13 10:50, Wayne Stambaugh wrote:

On 8/13/19 2:13 AM, jp charras wrote:

Le 12/08/2019 à 21:54, Wayne Stambaugh a écrit :
Sounds like a plan.  If there are not bug reports against this over 
the

next month, I'll will cherry-pick it into 5.1.

Wayne


I am not sure this issue exists in 5.1.

AFAIK it comes from moving code from our DLIST to std::deque, only in
master branch.

Our DLIST dtor manages (when it has the ownership) the items deletion.
But std::deque does not delete the items, so the code using std::deque
has to delete these items.


Maybe we should have used boost::ptr_deque instead.  You get heap
allocation cleanup for free.


We can adjust between containers now if desired as long as they obey the 
std::random_access_iterator constraints.  The major work in moving from 
DLIST was removing all the places where we assumed the element was 
itself a list.


For now, I prefer keeping the single list cleanup in the board dtor 
until we have a standardized python interface that doesn't expose our 
internals.  After we abstract the python interface, I see no reason why 
we wouldn't utilize std::unique_ptr as Simon suggested.


-Seth

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] [Patch] Fix some memory leaks

[Thomas Pointhuber]

Indeed,

there is actually a plan to improve this in the future. Personally, I
even wrote down a very rough idea myself:

https://github.com/pointhi/kicad-python/wiki/swig-interface-idea

I was able to compile eeschema with swig binding
(https://github.com/pointhi/kicad-source-mirror/commits/eeschema_swig),
but it looks like I need to wait until the unified coordinate system is
integrated, to start with the real work. Otherwise, sharing a common
library between eeschema and pcbnew could require a unnecessary number
of hacks.

Regards, Thomas

Am 13.08.19 um 18:41 schrieb Simon Richter:
> Hi,
> 
> On Tue, Aug 13, 2019 at 05:43:02PM +0200, Ian McInerney wrote:
> 
>> Smart pointers would definitely have been nicer to use, but the issue we
>> have with the board objects is they are passed out through SWIG to Python
>> currently, and SWIG doesn't seem to support unique_ptr, so I am not sure
>> how it would react if we gave it a deque of unique_ptrs. Anyone know if
>> that would break anything?
> 
> That would break horribly, indeed.
> 
> It would probably make sense to have a higher-level interface for
> scripting, and SWIG that instead of exposing the internals, breaking all
> the scripts everytime we improve something. :/
> 
>Simon
> 
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
> 



signature.asc
Description: OpenPGP digital signature
___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] [Patch] Fix some memory leaks

[Simon Richter]

Hi,

On Tue, Aug 13, 2019 at 05:43:02PM +0200, Ian McInerney wrote:

> Smart pointers would definitely have been nicer to use, but the issue we
> have with the board objects is they are passed out through SWIG to Python
> currently, and SWIG doesn't seem to support unique_ptr, so I am not sure
> how it would react if we gave it a deque of unique_ptrs. Anyone know if
> that would break anything?

That would break horribly, indeed.

It would probably make sense to have a higher-level interface for
scripting, and SWIG that instead of exposing the internals, breaking all
the scripts everytime we improve something. :/

   Simon

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] [Patch] Fix some memory leaks

[Ian McInerney]

Smart pointers would definitely have been nicer to use, but the issue we
have with the board objects is they are passed out through SWIG to Python
currently, and SWIG doesn't seem to support unique_ptr, so I am not sure
how it would react if we gave it a deque of unique_ptrs. Anyone know if
that would break anything?

-Ian

On Tue, Aug 13, 2019 at 5:20 PM Simon Richter 
wrote:

> Hi,
>
> On Tue, Aug 13, 2019 at 10:50:10AM -0400, Wayne Stambaugh wrote:
>
> > Maybe we should have used boost::ptr_deque instead.  You get heap
> > allocation cleanup for free.
>
> Or a deque of smart pointers. Without range-based for, that was annoying to
> use because you needed the double dereference
>
> for(std::deque >::const_iterator i =
> objects.begin;
> i != object.end(); ++i)
> (*i)->function();
>
> which was the main reason boost::ptr_container exists. Today, we can just
>
> for(auto const  : objects)
> p->function();
>
> which is not perfect, but probably good enough.
>
> I'm not entirely convinced we need lists of polymorphic objects that much.
> The construct where VIA is a subclass of TRACK (which is also used
> directly) could possibly be changed into abstract TRACK as a base, TRACE
> and VIA as (final) most derived classes, and objects stored in deque
> and deque without fear of slicing. That should cut down on the number
> of allocations required, and also allow introducing a pool allocator into
> the deque type as a further optimization.
>
>Simon
>
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>
___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] [Patch] Fix some memory leaks

[Simon Richter]

Hi,

On Tue, Aug 13, 2019 at 10:50:10AM -0400, Wayne Stambaugh wrote:

> Maybe we should have used boost::ptr_deque instead.  You get heap
> allocation cleanup for free.

Or a deque of smart pointers. Without range-based for, that was annoying to
use because you needed the double dereference

for(std::deque >::const_iterator i = objects.begin;
i != object.end(); ++i)
(*i)->function();

which was the main reason boost::ptr_container exists. Today, we can just

for(auto const  : objects)
p->function();

which is not perfect, but probably good enough.

I'm not entirely convinced we need lists of polymorphic objects that much.
The construct where VIA is a subclass of TRACK (which is also used
directly) could possibly be changed into abstract TRACK as a base, TRACE
and VIA as (final) most derived classes, and objects stored in deque
and deque without fear of slicing. That should cut down on the number
of allocations required, and also allow introducing a pool allocator into
the deque type as a further optimization.

   Simon

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] [Patch] Fix some memory leaks

[Wayne Stambaugh]

On 8/13/19 2:13 AM, jp charras wrote:
> Le 12/08/2019 à 21:54, Wayne Stambaugh a écrit :
>> Sounds like a plan.  If there are not bug reports against this over the
>> next month, I'll will cherry-pick it into 5.1.
>>
>> Wayne
> 
> I am not sure this issue exists in 5.1.
> 
> AFAIK it comes from moving code from our DLIST to std::deque, only in
> master branch.
> 
> Our DLIST dtor manages (when it has the ownership) the items deletion.
> But std::deque does not delete the items, so the code using std::deque
> has to delete these items.

Maybe we should have used boost::ptr_deque instead.  You get heap
allocation cleanup for free.

> 
>>
>> On 8/12/19 3:47 PM, Ian McInerney wrote:
>>> Wayne, lets let this settle in master for a while to make sure that no
>>> issues due to object lifetime surface.
>>>
>>> -Ian
>>>
>>> On Mon, Aug 12, 2019 at 9:20 PM Wayne Stambaugh >> > wrote:
>>>
>>> Ian,
>>>
>>> I merged your patch.  I'm guessing this should be cherry-picked into the
>>> 5.1 branch.
>>>
>>> Thanks,
>>>
>>> Wayne
>>>
>>> On 8/11/19 4:42 PM, Ian McInerney wrote:
>>> > I was noticing there were some memory leaks inside the board/module
>>> > classes that got somewhat extreme in some cases (I saw ~300MB leaked
>>> > from opening and closing cvpcb in Eeschema when run without a project
>>> > manager). This patch adds some deletion to the destructors of the
>>> > board/module classes, so they now will delete their sub items.
>>> >
>>> > I believe these classes are the respective owners of those pointers to
>>> > the sub items, and my testing doesn't show any problems with this, but
>>> > if anyone can see a case where deleting these sub items on destruction
>>> > might be an issue, let me know.
>>> >
>>> > -Ian
>>> >
>>> > ___
>>> > Mailing list: https://launchpad.net/~kicad-developers
>>> > Post to     : kicad-developers@lists.launchpad.net
>>> 
>>> > Unsubscribe : https://launchpad.net/~kicad-developers
>>> > More help   : https://help.launchpad.net/ListHelp
>>> >
>>>
>>> ___
>>> Mailing list: https://launchpad.net/~kicad-developers
>>> Post to     : kicad-developers@lists.launchpad.net
>>> 
>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>> More help   : https://help.launchpad.net/ListHelp
>>>
>>
>> ___
>> Mailing list: https://launchpad.net/~kicad-developers
>> Post to : kicad-developers@lists.launchpad.net
>> Unsubscribe : https://launchpad.net/~kicad-developers
>> More help   : https://help.launchpad.net/ListHelp
>>
> 
> 

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] [Patch] Fix some memory leaks

[jp charras]

Le 12/08/2019 à 21:54, Wayne Stambaugh a écrit :
> Sounds like a plan.  If there are not bug reports against this over the
> next month, I'll will cherry-pick it into 5.1.
> 
> Wayne

I am not sure this issue exists in 5.1.

AFAIK it comes from moving code from our DLIST to std::deque, only in
master branch.

Our DLIST dtor manages (when it has the ownership) the items deletion.
But std::deque does not delete the items, so the code using std::deque
has to delete these items.

> 
> On 8/12/19 3:47 PM, Ian McInerney wrote:
>> Wayne, lets let this settle in master for a while to make sure that no
>> issues due to object lifetime surface.
>>
>> -Ian
>>
>> On Mon, Aug 12, 2019 at 9:20 PM Wayne Stambaugh > > wrote:
>>
>> Ian,
>>
>> I merged your patch.  I'm guessing this should be cherry-picked into the
>> 5.1 branch.
>>
>> Thanks,
>>
>> Wayne
>>
>> On 8/11/19 4:42 PM, Ian McInerney wrote:
>> > I was noticing there were some memory leaks inside the board/module
>> > classes that got somewhat extreme in some cases (I saw ~300MB leaked
>> > from opening and closing cvpcb in Eeschema when run without a project
>> > manager). This patch adds some deletion to the destructors of the
>> > board/module classes, so they now will delete their sub items.
>> >
>> > I believe these classes are the respective owners of those pointers to
>> > the sub items, and my testing doesn't show any problems with this, but
>> > if anyone can see a case where deleting these sub items on destruction
>> > might be an issue, let me know.
>> >
>> > -Ian
>> >
>> > ___
>> > Mailing list: https://launchpad.net/~kicad-developers
>> > Post to     : kicad-developers@lists.launchpad.net
>> 
>> > Unsubscribe : https://launchpad.net/~kicad-developers
>> > More help   : https://help.launchpad.net/ListHelp
>> >
>>
>> ___
>> Mailing list: https://launchpad.net/~kicad-developers
>> Post to     : kicad-developers@lists.launchpad.net
>> 
>> Unsubscribe : https://launchpad.net/~kicad-developers
>> More help   : https://help.launchpad.net/ListHelp
>>
> 
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
> 


-- 
Jean-Pierre CHARRAS

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] [Patch] Fix some memory leaks

[Wayne Stambaugh]

Sounds like a plan.  If there are not bug reports against this over the
next month, I'll will cherry-pick it into 5.1.

Wayne

On 8/12/19 3:47 PM, Ian McInerney wrote:
> Wayne, lets let this settle in master for a while to make sure that no
> issues due to object lifetime surface.
> 
> -Ian
> 
> On Mon, Aug 12, 2019 at 9:20 PM Wayne Stambaugh  > wrote:
> 
> Ian,
> 
> I merged your patch.  I'm guessing this should be cherry-picked into the
> 5.1 branch.
> 
> Thanks,
> 
> Wayne
> 
> On 8/11/19 4:42 PM, Ian McInerney wrote:
> > I was noticing there were some memory leaks inside the board/module
> > classes that got somewhat extreme in some cases (I saw ~300MB leaked
> > from opening and closing cvpcb in Eeschema when run without a project
> > manager). This patch adds some deletion to the destructors of the
> > board/module classes, so they now will delete their sub items.
> >
> > I believe these classes are the respective owners of those pointers to
> > the sub items, and my testing doesn't show any problems with this, but
> > if anyone can see a case where deleting these sub items on destruction
> > might be an issue, let me know.
> >
> > -Ian
> >
> > ___
> > Mailing list: https://launchpad.net/~kicad-developers
> > Post to     : kicad-developers@lists.launchpad.net
> 
> > Unsubscribe : https://launchpad.net/~kicad-developers
> > More help   : https://help.launchpad.net/ListHelp
> >
> 
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@lists.launchpad.net
> 
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
> 

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] [Patch] Fix some memory leaks

[Ian McInerney]

Wayne, lets let this settle in master for a while to make sure that no
issues due to object lifetime surface.

-Ian

On Mon, Aug 12, 2019 at 9:20 PM Wayne Stambaugh 
wrote:

> Ian,
>
> I merged your patch.  I'm guessing this should be cherry-picked into the
> 5.1 branch.
>
> Thanks,
>
> Wayne
>
> On 8/11/19 4:42 PM, Ian McInerney wrote:
> > I was noticing there were some memory leaks inside the board/module
> > classes that got somewhat extreme in some cases (I saw ~300MB leaked
> > from opening and closing cvpcb in Eeschema when run without a project
> > manager). This patch adds some deletion to the destructors of the
> > board/module classes, so they now will delete their sub items.
> >
> > I believe these classes are the respective owners of those pointers to
> > the sub items, and my testing doesn't show any problems with this, but
> > if anyone can see a case where deleting these sub items on destruction
> > might be an issue, let me know.
> >
> > -Ian
> >
> > ___
> > Mailing list: https://launchpad.net/~kicad-developers
> > Post to : kicad-developers@lists.launchpad.net
> > Unsubscribe : https://launchpad.net/~kicad-developers
> > More help   : https://help.launchpad.net/ListHelp
> >
>
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>
___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] [Patch] Fix some memory leaks

[Wayne Stambaugh]

Ian,

I merged your patch.  I'm guessing this should be cherry-picked into the
5.1 branch.

Thanks,

Wayne

On 8/11/19 4:42 PM, Ian McInerney wrote:
> I was noticing there were some memory leaks inside the board/module
> classes that got somewhat extreme in some cases (I saw ~300MB leaked
> from opening and closing cvpcb in Eeschema when run without a project
> manager). This patch adds some deletion to the destructors of the
> board/module classes, so they now will delete their sub items.
> 
> I believe these classes are the respective owners of those pointers to
> the sub items, and my testing doesn't show any problems with this, but
> if anyone can see a case where deleting these sub items on destruction
> might be an issue, let me know.
> 
> -Ian
> 
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
> 

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

[Kicad-developers] [Patch] Fix some memory leaks

[Ian McInerney]

I was noticing there were some memory leaks inside the board/module classes
that got somewhat extreme in some cases (I saw ~300MB leaked from opening
and closing cvpcb in Eeschema when run without a project manager). This
patch adds some deletion to the destructors of the board/module classes, so
they now will delete their sub items.

I believe these classes are the respective owners of those pointers to the
sub items, and my testing doesn't show any problems with this, but if
anyone can see a case where deleting these sub items on destruction might
be an issue, let me know.

-Ian
From 93959c0c51f7a19ceb45c63d274888feb0107e2a Mon Sep 17 00:00:00 2001
From: Ian McInerney 
Date: Sun, 11 Aug 2019 22:30:33 +0200
Subject: [PATCH] Fix some memory leaks

Some elements of modules and boards were not deleted, so memory
was being leaked on some library loads and single-instance
pcbnew usage.
---
 kicad/kicad_manager_frame.cpp |  7 +++
 pcbnew/class_board.cpp| 19 +++
 pcbnew/class_module.cpp   | 11 +++
 3 files changed, 37 insertions(+)

diff --git a/kicad/kicad_manager_frame.cpp b/kicad/kicad_manager_frame.cpp
index 2ff4df65e..9ea194849 100644
--- a/kicad/kicad_manager_frame.cpp
+++ b/kicad/kicad_manager_frame.cpp
@@ -165,6 +165,13 @@ KICAD_MANAGER_FRAME::KICAD_MANAGER_FRAME( wxWindow* parent, const wxString& titl
 
 KICAD_MANAGER_FRAME::~KICAD_MANAGER_FRAME()
 {
+// Ensure there are no active tools
+if( m_toolManager )
+m_toolManager->DeactivateTool();
+
+delete m_actions;
+delete m_toolManager;
+
 m_auimgr.UnInit();
 }
 
diff --git a/pcbnew/class_board.cpp b/pcbnew/class_board.cpp
index bc640f218..0dbe59020 100644
--- a/pcbnew/class_board.cpp
+++ b/pcbnew/class_board.cpp
@@ -146,9 +146,28 @@ BOARD::~BOARD()
 Delete( area_to_remove );
 }
 
+// Clean up the owned elements
 DeleteMARKERs();
 DeleteZONEOutlines();
 
+// Delete the modules
+for( auto m : m_modules )
+delete m;
+
+m_modules.clear();
+
+// Delete the tracks
+for( auto t : m_tracks )
+delete t;
+
+m_tracks.clear();
+
+// Delete the drawings
+for (auto d : m_drawings )
+delete d;
+
+m_drawings.clear();
+
 delete m_CurrentZoneContour;
 m_CurrentZoneContour = NULL;
 }
diff --git a/pcbnew/class_module.cpp b/pcbnew/class_module.cpp
index 9fa8405eb..84326ef64 100644
--- a/pcbnew/class_module.cpp
+++ b/pcbnew/class_module.cpp
@@ -142,9 +142,20 @@ MODULE::MODULE( const MODULE& aModule ) :
 
 MODULE::~MODULE()
 {
+// Clean up the owned elements
 delete m_Reference;
 delete m_Value;
 delete m_initial_comments;
+
+for( auto p : m_pads )
+delete p;
+
+m_pads.clear();
+
+for( auto d : m_drawings )
+delete d;
+
+m_drawings.clear();
 }
 
 
-- 
2.21.0

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp