fix resource handling in parsimony

[westram]

• parsimony leaks nodes and edges fixed with [13321]
  • does not really affect usability, because this happens only under rare conditions (e.g. remove/add nodes) and does not affect optimization itself
  • but this hinders checking parsimony with memory debuggers like valgrind/leak sanitizer
• one cause for #620 seems to be an initialization problem: didnt fix anything
  • when new nodes get inserted into it is impossible to push their (not-inserted) state onto the undo-stack (as insertion is performed by base class which does not know anything about undo-stacks)
  • the node-factory can perform these stack operations, but w/o correct resource handling it appears hard to verify whether that fix does work as expected
• the method moveNextTo (in ​AP_tree and ​AP_tree_nlen) is really a PITA compared to what it effectively does (remove a node and re-add it to another part of the ). Debugging/reading that code is quite useless.
  With proper resource handling at hand, it would be possible to replace the whole thing by remove+insert - i do not expect that to generate more combines.

still TODO

• provoke create+destroy on same stack frame ⇒ instant delete possible when stack frame gets removed (true for accept and revert) by [13380:13382]
• top-level FrameStackData should not store anything. That data is outside of any stack-frame ⇒ state will always get accepted ⇒ handle resources like accept does
  by [13379]
• use sorted vectors instead of sets for resource management (only need to check for common nodes/edges when a stack-frame is dropped)

[westram]

basically fixed by [13321]

[westram]

Replying to description:

• one cause for #620 seems to be an initialization problem:
  • when new nodes get inserted into it is impossible to push their (not-inserted) state onto the undo-stack (as insertion is performed by base class which does not know anything about undo-stacks)
  • the node-factory can perform these stack operations, but w/o correct resource handling it appears hard to verify whether that fix does work as expected

Tested initializing new nodes with [13341]: had no effect on restore bugs

[westram]

attempt to replace moveNextTo by simpler implementation (use old method; compare with new)

[westram]

like moveNextTo_original.patch, but use new, compare with old (changes test results)

[westram]

interdiff between both methods

[westram]

Replying to westram:

• the method moveNextTo (in ​AP_tree and ​AP_tree_nlen) is really a PITA compared to what it effectively does (remove a node and re-add it to another part of the ). Debugging/reading that code is quite useless.
  With proper resource handling at hand, it would be possible to replace the whole thing by remove+insert - i do not expect that to generate more combines.

I give up on that issue: wasn't able to figure out why the new method​ changes the test results. Changes are caused by running NNI after quick-add (which uses moveNextTo to find best position). The NNI behaves slightly different when the new method is used, but i could not detect the reason why. Topologies (before NNI) are identical for both versions of moveNextTo().

[westram]

mark changes that got fixed after arb 6.0.x

[westram]

Replying to westram:

I give up on that issue: wasn't able to figure out why the new method​ changes the test results. Changes are caused by running NNI after quick-add (which uses moveNextTo to find best position). The NNI behaves slightly different when the new method is used, but i could not detect the reason why. Topologies (before NNI) are identical for both versions of moveNextTo().

Again stumbled across that problem: the use of moveNextTo() apparently causes edges to be moved around in the . quick-add processes an EdgeChain attempting to visit all possible insert-positions. as a result some insert positions are not visited at all, others are visited twice.

[westram]

Milestone renamed