A new throttling policy with public APIs to suspend/resume #2640
Conversation
|
As a general note, could you please remove the old throttling scheduler (see #2568) and replace it with yours instead of adding yet another variation, please? |
| { | ||
| return pool_.get_sched(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why have you added this functionality, is it used anywhere?
There was a problem hiding this comment.
We need to know the number of disabled/suspended threads and it is used here:
https://github.com/allscale/allscale_runtime/blob/multi_objective_optimizer/src/components/scheduler_component.cpp#L237
There was a problem hiding this comment.
Ok, may I suggest not to abbreviate the function name, then?
Also, doesn't this mean that this functionality has to be exposed by all other schedulers as well?
There was a problem hiding this comment.
Sure, I will fix the naming.
Yes we can get access to any other scheduler via this API. But I think it will not require the other schedulers expose the same functionality (i.e. get_disabled_os_threads ), as the scheduler name can be checked via get_scheduler_name() before using get_disabled_os_threads.
| @@ -560,6 +562,11 @@ foreach(_scheduler ${HPX_WITH_THREAD_SCHEDULERS_UC}) | |||
| hpx_add_config_define(HPX_HAVE_PERIODIC_PRIORITY_SCHEDULER) | |||
| set(HPX_WITH_PERIODIC_PRIORITY_SCHEDULER ON CACHE INTERNAL "") | |||
| endif() | |||
| # The throttling scheduler has not been tested neither on Windows nor on Mac | |||
There was a problem hiding this comment.
Please enable this option only for the platforms you tested on. If it's untested (or unsupported) on Windows, Mac, etc. we should make sure the option produces a configuration error.
|
One thing to clarify regarding the changes in hpx/lcos/detail/future_data.hpp . I did this change because the compiler on the ppc64le (gcc (GCC) 6.3.1 20170110 (Advance-Toolchain-at10.0) IBM AT 10 branch, based on subversion id 244285. ) was not able to see where future_data defined. |
Yes, this very change has made it to master already. Thanks |
|
ibm-research-ireland@4f7f530 should cover both #2568 and #2640 |
|
@KhalidHasanov this can't be merged before you resolve the merge conflicts. Also, please address the comments made above. |
|
@hkaiser I think the comments made above already addressed. The issue with the merge is that I can't really see the details of the failed tests in order to fix them. Can only authorized people see that? |
Ok.
Just try to merge your branch to HEAD of master locally on your machine and you will see the conflicts. Also, inspect seems not to be happy with your changes: https://6836-4455628-gh.circle-artifacts.com/0/tmp/circle-artifacts.S9PMOfG/hpx_inspect_report.html |
I've finished up development of a new throttling policy after our discussion with @sithhell . Can you please have a look and see if it can be merged with the master branch.