boostorg / thread

Boost.org thread module
http://boost.org/libs/thread
203 stars 162 forks source link

loop_executor should block on it's work_queue instead of polling #117

Closed Oberon00 closed 7 years ago

Oberon00 commented 7 years ago

loop_executor::loop is currently:

    void loop()
    {
      while (!closed())
      {
        schedule_one_or_yield();
      }
      while (try_executing_one())
      {
      }
    }

The first loop repeatedly calls schedule_one_or_yield() which is simply

    void schedule_one_or_yield()
    {
        if ( ! try_executing_one())
        {
          this_thread::yield();
        }
    }

The current implementation uses unnecessary CPU cycles (esp. if all other threads are idle) by busy waiting. As concurrent::sync_queue<work> work_queue already supports a blocking wait_pull_front , it would be better to use a (private?) wait_executing_one that behaves just like try_executing_one but calls work_queue.wait_pull_front instead of try_pull_front.

IMHO, the current implementation is an outright performance bug.

viboes commented 7 years ago

Le 27/02/2017 à 19:42, Christian N. a écrit :

|loop_executor::loop| is currently:

 void  loop()
 {
   while  (!closed())
   {
     schedule_one_or_yield();
   }
   while  (try_executing_one())
   {
   }
 }

The first loop repeatedly calls |schedule_one_or_yield()| which is simply

 void  schedule_one_or_yield()
 {
     if  ( !try_executing_one())
     {
       this_thread::yield();
     }
 }

The current implementation uses unnecessary CPU cycles (esp. if all other threads are idle) by busy waiting. As |concurrent::sync_queue work_queue| already supports a blocking |wait_pull_front| , it would be better to use a (private?) |wait_executing_one| that behaves just like |try_executing_one| but calls |work_queue.wait_pull_front| instead of |try_pull_front|.

IMHO, the current implementation is an outright performance bug.

Hi,

you are surely right.

Would you mind to add a github issue? What about adding a pull request PR?

Vicente

viboes commented 7 years ago

Sorry. I didn't saw this was already a github issue. What about adding a pull request PR?

Oberon00 commented 7 years ago

Ok, will do (but not before tomorrow).