Getting rid of unnecessary recomputes

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
jrheinlaender
Posts: 554
Joined: Sat Apr 07, 2012 2:42 am

Getting rid of unnecessary recomputes

Postby jrheinlaender » Wed Aug 15, 2012 4:59 pm

Hi,

as far as I can see there are three recomputes whenever I double-click a feature in the model tree to edit it:

1. When I double-click and the task dialog opens (unnecessary because nothing was changed yet)
2. Whenever I do an edit in the dialog (OK)
3. When I finish editing with the OK button (unnecessary because all changes were already handled in 2.)

By checking for mustExecute() == true in the feature's execute() method it seems I can avoid 1. but what about 3.? And would everybody agree to add a line

Code: Select all

if (!mustExecute())
        return App::DocumentObject::StdReturn;
at the beginning of all feature's execute() methods?

Jan
wmayer
Site Admin
Posts: 15724
Joined: Thu Feb 19, 2009 10:32 am

Re: Getting rid of unnecessary recomputes

Postby wmayer » Thu Aug 16, 2012 8:55 am

1. When I double-click and the task dialog opens (unnecessary because nothing was changed yet)
Might be indeed a useless call here.
3. When I finish editing with the OK button (unnecessary because all changes were already handled in 2.)
Not all view providers open a task dialog when editing them. So, a recompute must be done here.
By checking for mustExecute() == true in the feature's execute() method it seems I can avoid 1. but what about 3.? And would everybody agree to add a line
No, this is not needed because this is already checked in Document::recompute(). Furthermore, having this inside the execute() method is even dangerous because it will cause a feature A that depends on another feature B to be never recomputed if the other feature B has recomputed. In Document::recompute() we handle this case, i.e. a feature gets recomputed if its mustExecute() returns 1 or if a feature it depends on has been recomputed.

However, we can maybe improve Document::recompute() to continue only if at least one feature's mustExecute() returns 1.
jrheinlaender
Posts: 554
Joined: Sat Apr 07, 2012 2:42 am

Re: Getting rid of unnecessary recomputes

Postby jrheinlaender » Thu Aug 16, 2012 1:55 pm

wmayer wrote:
Not all view providers open a task dialog when editing them. So, a recompute must be done here.
So in other words for the ViewProviders of the features which do open a task dialog (all of the PartDesign ones, I think) I can remove the recompute() call from the TaskDlg...Parameters::accept() method?
wmayer
Site Admin
Posts: 15724
Joined: Thu Feb 19, 2009 10:32 am

Re: Getting rid of unnecessary recomputes

Postby wmayer » Fri Aug 17, 2012 8:09 am

So in other words for the ViewProviders of the features which do open a task dialog (all of the PartDesign ones, I think) I can remove the recompute() call from the TaskDlg...Parameters::accept() method?
I quickly checked how the pad feature works. It calls the recomputeFeature() method which only updates this single object but not the whole document. So, removing the document's recompute from the accept() method may cause some problems here and there. I really think it's better to first check all features inside recompute() if one is touched because this check is very fast.
User avatar
jriegel
Site Admin
Posts: 3369
Joined: Sun Feb 15, 2009 5:29 pm
Location: Ulm, Germany
Contact:

Re: Getting rid of unnecessary recomputes

Postby jriegel » Fri Aug 17, 2012 9:25 am

I agree with Werner, the recompute should be do nothing if not necessary.
Mean, if no property of a feature was change, the frequent call of recompute should do nothing (no execute() is called).
Stop whining - start coding!
jrheinlaender
Posts: 554
Joined: Sat Apr 07, 2012 2:42 am

Re: Getting rid of unnecessary recomputes

Postby jrheinlaender » Fri Aug 17, 2012 9:57 am

I found the culprit is actually not Document::recompute() which as far as I can see checks carefully for the mustExecute() flag before recomputing anything.

But in all the PartDesignGui::TaskDlg...Parameters::accept() methods, when the user clicks OK, all the feature properties are set with calls to the Python interface, e.g. for Pad

Code: Select all

Gui::Command::doCommand(Gui::Command::Doc,"App.ActiveDocument.%s.Length = %f",name.c_str(),parameter->getLength());
IMHO this is unnecessary because all properties are kept up-to-date as soon as the user enters the data. If I take out these calls the unnecessary recompute goes away.
wmayer
Site Admin
Posts: 15724
Joined: Thu Feb 19, 2009 10:32 am

Re: Getting rid of unnecessary recomputes

Postby wmayer » Fri Aug 17, 2012 10:17 am

IMHO this is unnecessary because all properties are kept up-to-date as soon as the user enters the data. If I take out these calls the unnecessary recompute goes away.
Not really because these calls are needed for the macro recording. When changing the values while the dialog is open we do everything directly in C++ and thus it's very fast. But for macro recording we once have to set the values via Python which of course is somewhat slower.

I just wonder is this a real bottleneck or is it more an academic issue to avoid the some extra calls? In the first case we can think of a solution to just macro record the code and don't let it go through the Python interpreter.
jrheinlaender
Posts: 554
Joined: Sat Apr 07, 2012 2:42 am

Re: Getting rid of unnecessary recomputes

Postby jrheinlaender » Fri Aug 17, 2012 10:48 am

I just wonder is this a real bottleneck
It's a nuisance when working with big patterns because the boolean operation takes so long. Also, Pocket/Pad up to a non-planar surface can also take a significant amount of computationg time.
wmayer
Site Admin
Posts: 15724
Joined: Thu Feb 19, 2009 10:32 am

Re: Getting rid of unnecessary recomputes

Postby wmayer » Fri Aug 17, 2012 11:27 am

It's a nuisance when working with big patterns because the boolean operation takes so long. Also, Pocket/Pad up to a non-planar surface can also take a significant amount of computationg time.
But this also means that whenever you change a value in the dialog the application takes some time to show the result which makes the "real time effect" impossible. Wouldn't make it sense to have a check box called "Update" so that the dialog doesn't do anything if it's off?
jrheinlaender
Posts: 554
Joined: Sat Apr 07, 2012 2:42 am

Re: Getting rid of unnecessary recomputes

Postby jrheinlaender » Fri Aug 17, 2012 12:16 pm

Wouldn't make it sense to have a check box called "Update" so that the dialog doesn't do anything if it's off?
Sounds like a good idea. Is there any place to implement this globally for all PartDesign features so that I don't have to repeat the code in every single feature?

Nevertheless, as a user it would annoy me if I just clicked on a feature to have a peek at its parameters, decided not to change anything, and then have the feature and all its dependents recompute. In a complex part this could take minutes!