#619 Made TreeView stable

Merged, abandoned or rejected pull requests are moved here to clear the main Pull Requests forum.
realthunder
Posts: 151
Joined: Tue Jan 03, 2017 10:55 am

Re: #619 Made TreeView stable

Postby realthunder » Wed Mar 15, 2017 4:52 pm

DeepSOIC wrote:I caught the crash in debugger.

Yep, stack frame 26, Tree.cpp:1006, the assertion failure. I've removed that assertion. Please sync and try again.
DeepSOIC
Posts: 4681
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: #619 Made TreeView stable

Postby DeepSOIC » Wed Mar 15, 2017 5:06 pm

realthunder wrote:Yep, stack frame 26, Tree.cpp:1006, the assertion failure. I've removed that assertion. Please sync and try again.
Done. No crashes so far...
DeepSOIC
Posts: 4681
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: #619 Made TreeView stable

Postby DeepSOIC » Wed Mar 15, 2017 5:14 pm

Good news. I lowered PoM's timer interval from 300 to 30 ms. This made it hit the assert every time. With your latest change, no asserts, and no crashing. Works fine!
DeepSOIC
Posts: 4681
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: #619 Made TreeView stable

Postby DeepSOIC » Wed Mar 15, 2017 7:06 pm

This causes an endless loop:

Code: Select all

class proxy(object):
    def __init__(self, obj):
        obj.Proxy = self

class vpproxy(object):
    def __init__(self,vobj):
        vobj.Proxy = self

    def attach(self, vobj):
        self.ViewObject = vobj
        self.Object = vobj.Object

    def claimChildren(self):
        return [self.Object]

if not App.ActiveDocument:
    App.newDocument()

obj = App.ActiveDocument.addObject("App::FeaturePython", "chicken_or_egg")
proxy(obj)
vpproxy(obj.ViewObject)

I was quite surprised to find out it doesn't eat up more and more memory. Just hangs.
DeepSOIC
Posts: 4681
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: #619 Made TreeView stable

Postby DeepSOIC » Wed Mar 15, 2017 7:14 pm

And another example with same result, that can actually happen for real.

Code: Select all

if not App.ActiveDocument:
    App.newDocument()

import Part
chicken = App.ActiveDocument.addObject("Part::Extrusion", "chicken")
egg = App.ActiveDocument.addObject("Part::Extrusion", "egg")
chicken.Base = egg
egg.Base = chicken

Hangs, too.
realthunder
Posts: 151
Joined: Tue Jan 03, 2017 10:55 am

Re: #619 Made TreeView stable

Postby realthunder » Thu Mar 16, 2017 4:14 am

DeepSOIC wrote:And another example with same result, that can actually happen for real.

Hmm... It's easy for me to fix this up by extra checking, and just not create item for this kind of scenario. But it can't prevent cyclic dependency, and it is still there. There bound to be other problems, e.g. if DAGView is enabled, it doesn't freeze the GUI, but spits error message endlessly. To be fair, FC without my patch will lock up too.

It is actually a deeper problem than it appears. It is reasonable to demand that FC shall guarantee no cyclic dependency when creating models using GUI, but what about using python? None of the programming language that I know of prevent you from locking yourself up with infinite loop. Is it worth the trouble to detect such thing, and to what extent? Anyway, if checking is desirable, then it is better done in App namespace, before it reaches GUI, probably in link property assignment.
DeepSOIC
Posts: 4681
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: #619 Made TreeView stable

Postby DeepSOIC » Thu Mar 16, 2017 9:51 am

realthunder wrote:To be fair, FC without my patch will lock up too.
Indeed it does, I haven't checked this :oops: . But it shouldn't. I was using an artificial circular dependency in Lattice2 as a method to temporarily block recomputes...

People arrive to cyclic dependencies in projects quite often, and it should be possible to fix the loop. Also, the loop may just be a temporarily state when some operation is in progress, like OpenSCAD ReplaceObject.

I'll have a look at what's going on, because I now have a feeling it's not your code that hangs.
realthunder
Posts: 151
Joined: Tue Jan 03, 2017 10:55 am

Re: #619 Made TreeView stable

Postby realthunder » Thu Mar 16, 2017 10:00 am

DeepSOIC wrote:I'll have a look at what's going on, because I now have a feeling it's not your code that hangs.

It is kinda of. It actually hangs inside Qt::TreeWidgetItem.addChild. They don't bother checking cyclic reference either. And for a good reason, as it may be a potentially expensive operation.

I added the check nevertheless. We'll see how it affects FC performance. Please sync the branch. As I mentioned, it may cause other problems. If you enable DAGView, you'll be spammed with endless error message.
DeepSOIC
Posts: 4681
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: #619 Made TreeView stable

Postby DeepSOIC » Thu Mar 16, 2017 10:20 am

I checked down to:
OS: Windows 8
Word size of OS: 64-bit
Word size of FreeCAD: 64-bit
Version: 0.16.6219 (Git)
Build type: Release
Branch: FreeCAD-DeepSOIC6
Hash: 1aff8d612e303501d401891e75aae8680b8c130f
Python version: 2.7.8
Qt version: 4.8.6
Coin version: 4.0.0a
OCC version: 6.7.1
And it hangs too. So we either were lucky and cyclic dependencies made so far never involved cyclic tree. Or they were, but people couldn't reproduce it, so didn't bother reporting.

realthunder wrote:If you enable DAGView, you'll be spammed with endless error message.
Well, at least it doesn't block one from fixing the loop ;)

realthunder wrote:I added the check nevertheless. We'll see how it affects FC performance. Please sync the branch.
It may, as it looks like tree update is fired for every change to a property... So maybe just leave it alone for now?
realthunder
Posts: 151
Joined: Tue Jan 03, 2017 10:55 am

Re: #619 Made TreeView stable

Postby realthunder » Thu Mar 16, 2017 10:31 am

DeepSOIC wrote:It may, as it looks like tree update is fired for every change to a property... So maybe just leave it alone for now?

The cyclic check only happens when a free item (i.e. an item at document root) is newly claimed, which already has children. The check needs to visit all its children. That's the only case requiring a check. We'd be creating new items (without children obviously) for all other cases. So, I think the performance impact should be limited.