checkgeometry->bopcheck in background thread

Merged, abandoned or rejected pull requests are moved here to clear the main Pull Requests forum.
wmayer
Site Admin
Posts: 13910
Joined: Thu Feb 19, 2009 10:32 am

Re: checkgeometry->bopcheck in background thread

Postby wmayer » Sat Aug 11, 2018 8:52 am

Only one problem, it pegs CPU usage to 100% (compare to 25%).
That's not a problem, that's exactly what we wanted to have. When the CPU load is 25% then this only means that one core (that of the GUI thread) does its work and all other cores are sleeping. If all cores are on full load, i.e. 100% then the degree of capacity utilization is optimal.
TheMarkster
Posts: 546
Joined: Thu Apr 05, 2018 1:53 am

Re: checkgeometry->bopcheck in background thread

Postby TheMarkster » Sat Aug 11, 2018 7:02 pm

wmayer wrote:
Sat Aug 11, 2018 8:52 am
Only one problem, it pegs CPU usage to 100% (compare to 25%).
That's not a problem, that's exactly what we wanted to have. When the CPU load is 25% then this only means that one core (that of the GUI thread) does its work and all other cores are sleeping. If all cores are on full load, i.e. 100% then the degree of capacity utilization is optimal.
I removed the multi-threading stuff and just added the runparallel mode option. It looks like a background thread option for runbopcheck is going to be a no-go anyway, at least for the time being.

Relevant code:

Code: Select all

#if OCC_VERSION_HEX >= 0x060900
  BOPCheck.SetParallelMode(true); //this doesn't help for speed right now(occt 6.9.1).

/*new additional code begins*/
  ParameterGrp::handle group = App::GetApplication().GetUserParameter().
  GetGroup("BaseApp")->GetGroup("Preferences")->GetGroup("Mod")->GetGroup("Part");
  bool runParallelSignal = group->GetBool("RunParallelMode", true);
  //running in parallel mode improves performance, but also increases cpu usage,
  //which might lead to thermal issues
  //on some systems with inadequate cpu cooling
  //this parameter provides users a way to turn this feature off
  group->SetBool("RunParallelMode", runParallelSignal);
  if (runParallelSignal){
     BOPCheck.SetRunParallel(true);
  }
/*new additional code ends*/

  BOPCheck.TangentMode() = true; //these 4 new tests add about 5% processing time.
  BOPCheck.MergeVertexMode() = true;
  BOPCheck.CurveOnSurfaceMode() = true;
  BOPCheck.MergeEdgeMode() = true;
#endif
  
#ifdef FC_DEBUG
  Base::TimeInfo start_time;
#endif
This adds a new parameter in the Tools -> Edit Parameters -> Preferences -> Mod -> Part group called RunParallelMode, defaulting to true. It's not in the CheckGeometry group because I think parallel mode might also be useful in more general boolean operations. I'm not familar enough with FreeCAD code to know for sure on this last part. I just know according to a chart in an article I linked to in my previous post there is a list of boolean operations that can benefit from running in parallel mode, cuts and fusions listed among them. Should I put it in CheckGeometry group next to the RunBOPCheck option? Maybe something like RunBOPCheckInParallelMode instead or is the more general RunParallelMode option better? Should it default to false, as OCC has done? If it defaults to false then only users who know about the option will be able to use it, but we could put a warning in the wiki about how running in parallel mode will result in increased cpu usage and that users are advised to ensure their cpu cooling system is functioning properly before enabling it.

I installed Core Temp, a windows app to monitor cpu temps. According to an article I read 60C is about the highest temps you want to be running at for extended periods. (What amounts to 'extended periods', I'm not sure. Minutes? Hours? Days? Minutes fits the FreeCAD use case.) Core Temp shows each core (I have one of the first gen core i7 quad core processors) separately in the task bar next to the system clock. When I run a bopcheck with parallel mode on the temps reach the mid 60's for all 4 cores. (This is in debug mode inside the vm, with temps being monitored for the host os.) With parallel mode off (requires FreeCAD restart when changing) the temp on one core will be upper 50's and mid to upper 40's on the other 3, but it's being swapped around. In other words, the same core is not always at the high temp. The gui thread is being distributed equally among the cores.

I updated the pull request. Let me know if I need to do anything else.
User avatar
tanderson69
Posts: 1483
Joined: Thu Feb 18, 2010 1:07 am

Re: checkgeometry->bopcheck in background thread

Postby tanderson69 » Sat Aug 11, 2018 9:47 pm

TheMarkster wrote:
Sat Aug 11, 2018 7:02 pm
I removed the multi-threading stuff and just added the runparallel mode option. It looks like a background thread option for runbopcheck is going to be a no-go anyway, at least for the time being.
The 'BOPCheck.SetParallelMode(true)' call is not needed. That was me probably in a desperate act to speed it up. It also looks like we don't need to copy the shape anymore either (at least for occt 7.3).

IMHO: I would not add a parameter for controlling the multithread of the bopcheck. I would just enable it for the occt versions that have it and let it ride. Can always add it later if it is needed for some strange reason. I wouldn't worry about cpu temps. That is for hardware manufactures, system builders, and overclockers to worry about. As application developers, we should hold the accelerator to the floor and let the machine do what it can do.

This is great! Thanks for working on this. I took a test
from: 443.747654s wall, 443.210000s user + 0.320000s system = 443.530000s CPU (100.0%)
to: 151.161361s wall, 507.330000s user + 39.690000s system = 547.020000s CPU (361.9%)
TheMarkster
Posts: 546
Joined: Thu Apr 05, 2018 1:53 am

Re: checkgeometry->bopcheck in background thread

Postby TheMarkster » Sat Aug 11, 2018 11:45 pm

tanderson69 wrote:
Sat Aug 11, 2018 9:47 pm

The 'BOPCheck.SetParallelMode(true)' call is not needed. That was me probably in a desperate act to speed it up. It also looks like we don't need to copy the shape anymore either (at least for occt 7.3).

IMHO: I would not add a parameter for controlling the multithread of the bopcheck. I would just enable it for the occt versions that have it and let it ride. Can always add it later if it is needed for some strange reason. I wouldn't worry about cpu temps. That is for hardware manufactures, system builders, and overclockers to worry about. As application developers, we should hold the accelerator to the floor and let the machine do what it can do.

This is great! Thanks for working on this. I took a test
from: 443.747654s wall, 443.210000s user + 0.320000s system = 443.530000s CPU (100.0%)
to: 151.161361s wall, 507.330000s user + 39.690000s system = 547.020000s CPU (361.9%)
Thanks for the feedback. I'm not sure I understand those numbers, but the This is great! part, I assume, means you got better performance with it enabled.

You suggest removing the line BOPCheck.SetParallelMode(true)? Any harm in leaving it as is? What is the difference between the two settings (SetParallelMode and SetRunParallel)? Seems to me they should both do the same thing or perhaps work in tandem, but evidently that's not the case. One was listed as a static method while the other was inline. Maybe SetRunParallel is intended as a replacement down the road if it's more thread-safe? I'm trying to adopt the mindset of leaving any existing code alone if it's not necessary to change it.

As for the parameter, I'm of two minds about it. I'm okay with whatever Werner thinks on the subject. One angle to consider is we don't want to go unnecessarily scaring people away from using FreeCAD out of fear of damaging their CPU's. We also don't want to plant seeds of ideas that if something does happen to somebody's CPU maybe it was FreeCAD's fault. From that standpoint the wording should be something along the lines, enabling this option improves performance, but disabling reduces CPU usage, potentially lowering power consumption. (I'm not a lawyer, but I worked in the legal industry for about 5 years...)

I'm also thinking it would be better to make it specific to RunBOPCheck and put it right next to that parameter so when users enable RunBOPCheck they'll see it and be able to decide whether to enable or disable it, too. I don't know whether something similar can be also applied to other boolean operations (hopefully it can improve performance in those areas, too), but if it can and if it is deemed desirable to have a similar parameter for those operations, too, then another parameter can be created for them as the need arises, or perhaps a more general option for all boolean operations.

One argument in favor of the parameter is it makes it easy for users who would like to experiment with having it enabled or disabled to see what, if any, difference it makes. To that end, perhaps enabling the timer message (how long it took to run the test) for all builds, whether debug or not would be warranted?

There is another option, too, the one about ending the check on finding the first faulty that perhaps we should add a parameter for. If it speeds up the check, but at the expense of having fewer results, I think it would be worth doing. Perhaps running in parallel mode will make that option more useful.
User avatar
tanderson69
Posts: 1483
Joined: Thu Feb 18, 2010 1:07 am

Re: checkgeometry->bopcheck in background thread

Postby tanderson69 » Sun Aug 12, 2018 1:21 am

TheMarkster wrote:
Sat Aug 11, 2018 11:45 pm
Thanks for the feedback. I'm not sure I understand those numbers, but the This is great! part, I assume, means you got better performance with it enabled.
from 443 seconds down to 151 secoonds. :D




TheMarkster wrote:
Sat Aug 11, 2018 11:45 pm
You suggest removing the line BOPCheck.SetParallelMode(true)? Any harm in leaving it as is? What is the difference between the two settings (SetParallelMode and SetRunParallel)? Seems to me they should both do the same thing or perhaps work in tandem, but evidently that's not the case. One was listed as a static method while the other was inline. Maybe SetRunParallel is intended as a replacement down the road if it's more thread-safe? I'm trying to adopt the mindset of leaving any existing code alone if it's not necessary to change it.
I don't think there is any harm in leaving it there, but it never has done anything and it still isn't. The only reason it's there is to enable multi-threading and you have done it without it. So it is there to confuse the next person that looks at it. Your pull request do what you want.






TheMarkster wrote:
Sat Aug 11, 2018 11:45 pm
As for the parameter, I'm of two minds about it. I'm okay with whatever Werner thinks on the subject. One angle to consider is we don't want to go unnecessarily scaring people away from using FreeCAD out of fear of damaging their CPU's. We also don't want to plant seeds of ideas that if something does happen to somebody's CPU maybe it was FreeCAD's fault. From that standpoint the wording should be something along the lines, enabling this option improves performance, but disabling reduces CPU usage, potentially lowering power consumption. (I'm not a lawyer, but I worked in the legal industry for about 5 years...)
IMHO: this whole line of thinking is moot. Thousands of people fire up blender every day and hit render. Cycles then proceeds to push there machines to the max for hours at a time. People want a program to use all the horsepower that they have purchased, so time spent waiting for the machine is minimized.






TheMarkster wrote:
Sat Aug 11, 2018 11:45 pm
I'm also thinking it would be better to make it specific to RunBOPCheck and put it right next to that parameter so when users enable RunBOPCheck they'll see it and be able to decide whether to enable or disable it, too. I don't know whether something similar can be also applied to other boolean operations (hopefully it can improve performance in those areas, too), but if it can and if it is deemed desirable to have a similar parameter for those operations, too, then another parameter can be created for them as the need arises, or perhaps a more general option for all boolean operations.

One argument in favor of the parameter is it makes it easy for users who would like to experiment with having it enabled or disabled to see what, if any, difference it makes. To that end, perhaps enabling the timer message (how long it took to run the test) for all builds, whether debug or not would be warranted?
I expressed my opinion. Your pull request, do what you want.





TheMarkster wrote:
Sat Aug 11, 2018 11:45 pm
There is another option, too, the one about ending the check on finding the first faulty that perhaps we should add a parameter for. If it speeds up the check, but at the expense of having fewer results, I think it would be worth doing. Perhaps running in parallel mode will make that option more useful.
Have you tested that option?. As you can see by the note that it wasn't doing anything useful in the past.
GeneFC
Posts: 1000
Joined: Sat Mar 19, 2016 3:36 pm
Location: Punta Gorda, FL

Re: checkgeometry->bopcheck in background thread

Postby GeneFC » Sun Aug 12, 2018 1:41 am

TheMarkster wrote:
Sat Aug 11, 2018 11:45 pm
One angle to consider is we don't want to go unnecessarily scaring people away from using FreeCAD out of fear of damaging their CPU's. We also don't want to plant seeds of ideas that if something does happen to somebody's CPU maybe it was FreeCAD's fault. From that standpoint the wording should be something along the lines, enabling this option improves performance, but disabling reduces CPU usage, potentially lowering power consumption. (I'm not a lawyer, but I worked in the legal industry for about 5 years...)
I am not a lawyer either, but I spent a career in advanced semiconductor R&D, including high performance computer chips. The stress testing that is done during development is far greater than anything any user could do under any circumstances. As noted above the prominent CPU makers, Intel and AMD, have all sorts of measurement and control built in to prevent damage from user applications.

Warp speed.

Gene
TheMarkster
Posts: 546
Joined: Thu Apr 05, 2018 1:53 am

Re: checkgeometry->bopcheck in background thread

Postby TheMarkster » Sun Aug 12, 2018 2:39 am

Okay, guys. Thanks for the feedback. Full speed ahead it is.
wmayer
Site Admin
Posts: 13910
Joined: Thu Feb 19, 2009 10:32 am

Re: checkgeometry->bopcheck in background thread

Postby wmayer » Sun Aug 12, 2018 8:36 am

tanderson69 wrote:IMHO: I would not add a parameter for controlling the multithread of the bopcheck. I would just enable it for the occt versions that have it and let it ride. Can always add it later if it is needed for some strange reason.
I fully agree!. AFAIK some OCCT algorithms (e.g. BRepMesh_IncrementalMesh) are parallelized for some years now and we have enabled it there too by hard-coding it.
from 443 seconds down to 151 secoonds.
On how many cores? Probably 4?
User avatar
NormandC
Posts: 18461
Joined: Sat Feb 06, 2010 9:52 pm
Location: Québec, Canada

Re: checkgeometry->bopcheck in background thread

Postby NormandC » Sun Aug 12, 2018 9:51 am

GeneFC wrote:
Sun Aug 12, 2018 1:41 am
Warp speed.
Maximum warp that is, not a measly Warp 7. Think Borg transwarp conduit. :ugeek: :D
User avatar
tanderson69
Posts: 1483
Joined: Thu Feb 18, 2010 1:07 am

Re: checkgeometry->bopcheck in background thread

Postby tanderson69 » Sun Aug 12, 2018 12:49 pm

wmayer wrote:
Sun Aug 12, 2018 8:36 am
from 443 seconds down to 151 secoonds.
On how many cores? Probably 4?
Yes, 4. AMD A10-7850K Radeon R7