kicad-developers team mailing list archive

[Tomasz Wlostowski <tomasz.wlostowski@xxxxxxx>]

On 04.08.2016 22:59, Michael Steinberg wrote:
> Hello all,
> 
> up to the most current version of the router code I could get hold of,
> it is leaking much memory (eventually resulting in std::bad_alloc) on
> iterative stuff like dragging. I was also made aware of bugs like
> "https://bugs.launchpad.net/kicad/+bug/1553804";. Now this stuff is a
> complex beast, and I'm nowhere any near fluent with it. But it's also
> very interesting to me algorithm-wise. I believe that the original devs
> at this point are the only ones that could really spot leaks and
> segfaults in it with a reasonable time effort, might be wrong though.
> 
> It is my experience though, that such bugs usually surface while
> transforming source code to a more rigid RAII source-style with clear
> and enforced ownership. I would be totally willing to fight myself
> through the sources, trying to transform it without change in behaviour,
> but getting that right will probably reveal misconceptions on my side
> and it will also touch a huge amount of lines in the end and take quite
> a while. I might even feel forced to ask people for support.
> 

Hi Michael,

I'm very glad that you want to tackle tackle the memory management
issues in the P&S code. When I started writing it, I didn't have a clear
idea of the router's design because I had never written a push-and-shove
router before... Also my C++ skills 3 years ago were probably a bit
worse, so there's a bit of technical debt in the code.

Just to start: the ownership rules go as follows:
- PNS_ITEM and derivatives are owned by a PNS_NODE they've been added
to. Calling PNS_NODE::Remove on an item does not release it's memory -
it has to be freed by the caller.
- PNS_LINE or PNS_JOINT never own PNS_ITEMs.
- PNS_NODEs that are not root (PNS_NODE::IsRoot() == false) belong to
the root PNS_NODE.
- world node (root) belongs to PNS_ROUTER.
- PNS_ROUTER and PNS_KICAD_IFACE belong to PNS_TOOL_BASE.
- PNS_ALGO_BASE derivatives belong to PNS_ROUTER.

The tricky bits are:
- node stack kept by the PNS_SHOVE algorithm for springback and their
interaction with PNS_OPTIMIZER. At some point I desperately wrote an
ugly "garbage collector" freeing all items allocated by PNS_SHOVE class
upon its destruction, later on I removed it and refactored the code a
bit but some leaks probably still remain.
- items passed outside the scope of the current algorithm (e.g. what
PNS_DRAGGER/PNS_SHOVE::Traces() returns for DisplayItems() ). I guess a
shared pointer might help here.

PS. I'm going to push patches to P&S tonight that fix some issues
reported on the bug tacker. You could start your work on top of these.

Many thanks again,
Tom