checkgeometry->bopcheck in background thread

Merged, abandoned or rejected pull requests are moved here to clear the main Pull Requests forum.
TheMarkster
Posts: 544
Joined: Thu Apr 05, 2018 1:53 am

checkgeometry->bopcheck in background thread

Postby TheMarkster » Wed Aug 08, 2018 9:58 pm

https://github.com/FreeCAD/FreeCAD/pull/1588

Here is a screencast of it in action. Bear in mind a few things: this is running inside a virtual machine, in debug mode, while recording a screencast, so the performance will be a little sluggish.
geomcheck-backgrounded.gif
geomcheck-backgrounded.gif (524.66 KiB) Viewed 353 times
Advantages:

You can continue working in FreeCAD while the operation is ongoing. Feedback is in the form of a running time counter in the status bar. There is no progress information available to show a percentage complete, but the user can see that the process is still ongoing while waiting for it to finish.

Fewer users will figure FreeCAD has crashed and do forced kills via task manager because FreeCAD remains responsive during the operation.

Even if the user decides to exit FreeCAD rather than wait for the lengthy process to complete, he at least has the option of saving the document and exiting gracefully rather than doing a force kill.

Issues:

If you have another task/dialog open when the check geometry process completes it will not be able to display the results. You will get a warning message in the report view telling you there was another dialog open. But if there are errors found you will be notified in the report view that errors were found (just not which errors). (No notification if no errors found -- should notify in either case?)

There is no way to cancel/abort (except shutting down and restarting FreeCAD), but this could be added later, I think, but I'd prefer to keep it as simple as possible for now.

It probably (untested) takes longer to do the checkgeometry test in background mode than it does currently, but I think the tradeoff is well worth it, assuming backgrounding doesn't cause its own set of stability issues.

If anyone is willing, I would be grateful if you could compile this and test it on some other platforms. My only build environment that's working for me is on Ubuntu inside a virtual box vm. I'm also getting (unrelated) crashes running FreeCAD inside this vm (happens also when running the FreeCAD daily build, so I don't think it's related to this pull request).

You could clone from here:

https://github.com/mwganson/FreeCAD

and build, but since the only file I changed was src/Mod/Part/Gui/TaskCheckGeometry.cpp

https://github.com/mwganson/FreeCAD/blo ... ometry.cpp

an easier way would be just replace that file in your source folder and build it.
ickby
Posts: 2821
Joined: Wed Oct 05, 2011 7:36 am

Re: checkgeometry->bopcheck in background thread

Postby ickby » Thu Aug 09, 2018 7:46 am

Hello,

Interesting work! But are you sure bop-check is thread safe? With this change you allow to call OCC operations in parallel, e.g. if a user makes a new oepration on some shapes while bop-chaeck is running. This means the algorithms used (as well as many subalgorithms) need to be enabled for concurrent calls. To my knoweledge only few OCC algorithms are enabled for that, and I don't know if bopcheck is one of them. Also be aware that this function uses some parts of normal boolean algorithms, so it must also be compatible with boolean calls etc.

An alternative would be to open the task dialog anyway and give a progress update there. Than the user gets the feedback and you avoid the problem of concurrent operations (well , not completely, one can still make a occ operation via python or macros...)
wmayer
Site Admin
Posts: 13384
Joined: Thu Feb 19, 2009 10:32 am

Re: checkgeometry->bopcheck in background thread

Postby wmayer » Thu Aug 09, 2018 8:21 am

Using a while loop to always process pending events is probably very inefficient and makes the GUI thread to go on full CPU load. A better approach is to use a QFutureWatcher, connect its finished signal with the quit slot of a local QEventLoop and then start the local loop.

Then I don't think it's a good idea to display messages only using MainWindow::showMessage() because:
1. you don't know how many lines a message can have so most of it can be disappear in hyperspace when only showing it in the status bar
2. there can be many messages in short time so there is no chance to read them fast enough

You better leave the std::cout because internally we have implemented a redirector class that goes through Base::Console().Log and thus shows these messages in the report window, log file (if created) or the terminal if there is one.
TheMarkster
Posts: 544
Joined: Thu Apr 05, 2018 1:53 am

Re: checkgeometry->bopcheck in background thread

Postby TheMarkster » Thu Aug 09, 2018 7:09 pm

Thank you for your feedback. I have great respect for both of you and for your opinions. I think it's important for FreeCAD that we find a better way to handle some of these lengthy operations, something better than a wait cursor and an unresponsive FreeCAD and the user left wondering if it's off in an infinite loop.

Werner, you are right about the cpu usage. It's unacceptably high. I'll have to find another way. I'll look into your suggestions, but I'm new to Qt and my C++ is quite rusty.

Are you saying I should use std::cout or that I should not use it? I don't think it was working for me. Cerr was working, but not cout. The reason for using only the status bar was not to flood those other outputs. I don't see the harm if a few of the messages get lost, they're just there to remind/reassure the user the bopcheck is still ongoing.

Ickby, I did some tests, such as doing another geometry check while the current one was still ongoing. I also did some boolean operations simultaneously, even on the object that was currently being checked. I encountered no issues. It appears the objects getting checked are checked sequentially because results for a quick-running test come in after the results for a long-running test (but the dialog fails because another dialog is already open if the 2nd test finishes too quickly after the 1st one). More testing needs to be done, but I have to figure out the cpu usage issue first.

I'm thinking, assuming this concept is workable, we should add another parameter along with RunBOPCheck, something like RunBOPCheckInBackgroundThread (experimental) and only background it if the latter parameter is set to true, at least for the time being. That way people who enable it would know they are using something experimental and if it causes issues they can report it and/or disable the option.
wmayer
Site Admin
Posts: 13384
Joined: Thu Feb 19, 2009 10:32 am

Re: checkgeometry->bopcheck in background thread

Postby wmayer » Thu Aug 09, 2018 9:13 pm

I think it's important for FreeCAD that we find a better way to handle some of these lengthy operations, something better than a wait cursor and an unresponsive FreeCAD and the user left wondering if it's off in an infinite loop.
It's always nicer to have a responsive GUI while doing a lengthy operation. But in general this can make things much more difficult to implement because the user can do things he shouldn't do (deleting the object, closing the document, ...) while the thread is running and this can easily lead to segfaults. So, a lot of code must be written to either forbid certain things or react on them accordingly.
Werner, you are right about the cpu usage. It's unacceptably high. I'll have to find another way. I'll look into your suggestions, but I'm new to Qt and my C++ is quite rusty.
Something like this:

Code: Select all

QFuture<void> future = ...
QFutureWatcher<void> watcher;
watcher.setFuture(future);

// keep it responsive during computation
QEventLoop loop;
QObject::connect(&watcher, SIGNAL(finished()), &loop, SLOT(quit()));
loop.exec();
Are you saying I should use std::cout or that I should not use it?
Use it! The original code used it and you replaced most of it with a Gui::getMainWindow()->showMessage
I don't think it was working for me. Cerr was working, but not cout.
It works but doesn't print normal messages but log messages. See https://github.com/FreeCAD/FreeCAD/blob ... i.cpp#L281 and https://github.com/FreeCAD/FreeCAD/blob ... e.cpp#L906
In order to see the messages you have to activate the "Log message" option in the output window.
The reason for using only the status bar was not to flood those other outputs. I don't see the harm if a few of the messages get lost, they're just there to remind/reassure the user the bopcheck is still ongoing.
If a message might be overwritten too fast what is then the point of showing it? And as said if you don't have activated the "Log message" option (the default) it won't flood the output window.
Ickby, I did some tests, such as doing another geometry check while the current one was still ongoing. I also did some boolean operations simultaneously, even on the object that was currently being checked. I encountered no issues.
In the past OCCT used many global/static variables which made multi-threading impossible. Things have been improved a lot in the last years but unless explicitly written in the documentation you cannot be sure if an algorithm now is ready for multi-threading or not.
I'm thinking, assuming this concept is workable, we should add another parameter along with RunBOPCheck, something like RunBOPCheckInBackgroundThread (experimental) and only background it if the latter parameter is set to true, at least for the time being. That way people who enable it would know they are using something experimental and if it causes issues they can report it and/or disable the option.
That's a good idea.
UR_
Posts: 716
Joined: Tue Jan 03, 2017 8:42 pm

Re: checkgeometry->bopcheck in background thread

Postby UR_ » Fri Aug 10, 2018 6:35 pm

Thread's parent could at least show a modal messagebox, providing a cancel button.
Hopefully nothing gets hurt. :)
TheMarkster
Posts: 544
Joined: Thu Apr 05, 2018 1:53 am

Re: checkgeometry->bopcheck in background thread

Postby TheMarkster » Fri Aug 10, 2018 9:54 pm

UR_ wrote:
Fri Aug 10, 2018 6:35 pm
Thread's parent could at least show a modal messagebox, providing a cancel button.
Hopefully nothing gets hurt. :)
Good idea. I've been thinking along the same lines. Display a modal dialog with a cancel button. When bopcheck finishes, the modal gets closed and the results get shown. If the user presses cancel before bopcheck finishes, the bopcheck thread gets killed (and hopefully becomes the only casualty). But I don't think you can kill the thread that was started with QtConcurrent::run(). Maybe with map() you can. I need to do more research on it.
TheMarkster
Posts: 544
Joined: Thu Apr 05, 2018 1:53 am

Re: checkgeometry->bopcheck in background thread

Postby TheMarkster » Fri Aug 10, 2018 10:11 pm

wmayer wrote:
Thu Aug 09, 2018 9:13 pm

Something like this:

Code: Select all

QFuture<void> future = ...
QFutureWatcher<void> watcher;
watcher.setFuture(future);

// keep it responsive during computation
QEventLoop loop;
QObject::connect(&watcher, SIGNAL(finished()), &loop, SLOT(quit()));
loop.exec();
Yeah, I found this same code, more or less, browsing some of the FreeCAD source code. It works better than what I was doing before, but still hogs CPU resources at times (and not at other times). The first bopcheck I run during testing keeps the CPU usage at acceptable levels, but then if I run another bopcheck it pegs out at 100% again. I must be doing something wrong. I don't want to spend too much debugging this because I think UR_'s idea is a better way to do it anyway.


Here is the relevant section of TaskCheckGeometry.cpp:

Code: Select all

void runBOPCheckInBackground(BOPAlgo_ArgumentAnalyzer *bop){
    bop->Perform();
}

Code: Select all

#ifdef FC_DEBUG
  Base::TimeInfo start_time;
#endif

  //BOPCheck.Perform();

  QFuture <void> future = QtConcurrent::run(runBOPCheckInBackground,&BOPCheck);

  QFutureWatcher <void> watcher;
  watcher.setFuture(future);

  // keep it responsive during computation
  QEventLoop loop;
  QObject::connect(&watcher, SIGNAL(finished()), &loop, SLOT(quit()));

  loop.exec();

#ifdef FC_DEBUG
  float bopAlgoTime = Base::TimeInfo::diffTimeF(start_time,Base::TimeInfo());
  std::cout << std::endl << "BopAlgo check time is: " << bopAlgoTime << std::endl << std::endl;
#endif
Additionally, the line where the waitcursor is initialized is commented out, and a few more #include files at the top of the file.
wmayer
Site Admin
Posts: 13384
Joined: Thu Feb 19, 2009 10:32 am

Re: checkgeometry->bopcheck in background thread

Postby wmayer » Fri Aug 10, 2018 11:25 pm

If the user presses cancel before bopcheck finishes, the bopcheck thread gets killed (and hopefully becomes the only casualty). But I don't think you can kill the thread that was started with QtConcurrent::run(). Maybe with map() you can. I need to do more research on it.
It's in general a very bad idea trying to kill a thread because there is no chance to do any clean-up work like freeing memory or unlocking a mutex, ...
For more details have a look at QThread::terminate.

The QFuture has a cancel() method but it doesn't do anything when used with QtConcurrent::run(). The reason for this is quite simple: all the other QtConcurrent methods work an sequences where for one item the processing time is relatively short so that calling cancel() stops rather quickly. But with QtConcurrent::run() you started only one function and this can take a long time and QFuture::cancel() has no chance to break the execution.

But another idea is to check if the bop check algo accepts a progress indicator. This progress indicator and be combined with Qt's progress bar/dialog and when pressing cancel one can call a function of the progress indicator to stop execution.
TheMarkster
Posts: 544
Joined: Thu Apr 05, 2018 1:53 am

Re: checkgeometry->bopcheck in background thread

Postby TheMarkster » Sat Aug 11, 2018 5:42 am

wmayer wrote:
Fri Aug 10, 2018 11:25 pm
It's in general a very bad idea trying to kill a thread because there is no chance to do any clean-up work like freeing memory or unlocking a mutex, ...
For more details have a look at QThread::terminate.

The QFuture has a cancel() method but it doesn't do anything when used with QtConcurrent::run(). The reason for this is quite simple: all the other QtConcurrent methods work an sequences where for one item the processing time is relatively short so that calling cancel() stops rather quickly. But with QtConcurrent::run() you started only one function and this can take a long time and QFuture::cancel() has no chance to break the execution.
That makes sense, about run() versus map(). After reading some information on the subject I came to the same conclusion. It's a bad idea to force kill a thread. Doesn't seem to be any safe way to do it without having some cooperation from the thread being killed. So, there goes that.
wmayer wrote:
Fri Aug 10, 2018 11:25 pm
But another idea is to check if the bop check algo accepts a progress indicator. This progress indicator and be combined with Qt's progress bar/dialog and when pressing cancel one can call a function of the progress indicator to stop execution.
I'm looking through the documentation.
https://www.opencascade.com/doc/occt-7. ... ca4f50dd00

Came across this and found interesting enough to get sidetracked. There is a SetRunParallel() option that FreeCAD isn't currently using. It uses SetParallelMode(), but not SetRunParallel(). In an initial test, with both set to true, the time required to do a bopcheck went from 236 seconds to 75 seconds, another test 100 seconds. Only one problem, it pegs CPU usage to 100% (compare to 25%). Maybe that's why it wasn't already being used. I'm not comfortable with it, but maybe I'm being too conservative. I burned out an old obsolete CPU once playing around with overclocking it.

Discussion on the subject from 2013, no mention of CPU usage / overheating issues, but one wonders the real reason behind not making something the default setting when it gives such good performance improvements:
https://dev.opencascade.org/index.php?q=node/718

Seems there are a number of boolean operations that could reap performance gains using parallel mode, including boolean cuts and fusions, on the order of 2 to 3 times faster, which seem inline with my above initial test. See the chart at the bottom of the above linked page.

Interesting article on cpu throttling:
http://www.tomshardware.com/answers/id- ... ogies.html

CPU's will (at least Intel will, presumably AMD, too) reduce voltage when the core temp reaches a certain level (about 100 C) and actually shutdown at another point (around 130 C). How much of an actual danger it is, I'm not sure.

There is a SetProgressIndicator() method:

SetProgressIndicator()
void BOPAlgo_Options::SetProgressIndicator ( const Handle< Message_ProgressIndicator > & theObj )

BOPAlgo_ArgumentAnalyzer inherits from BOPAlgo_Algo inherits from BOPAlgo_Options
on that page they wrote: "The class provides the following options for the algorithms in Boolean Component:

Memory allocation tool - tool for memory allocations;
Error and warning reporting - allows recording warnings and errors occurred during the operation. Error means that the algorithm has failed.
Parallel processing mode - provides the possibility to perform operation in parallel mode;
Fuzzy tolerance - additional tolerance for the operation to detect touching or coinciding cases;
Progress indicator - provides interface to track the progress of operation and stop the operation by user's break.
Using the Oriented Bounding Boxes - Allows using the Oriented Bounding Boxes of the shapes for filtering the intersections."