About LevelDB's implementation of `PosixEnv::Schedule`. I have a question: Why `Schedule` call `Signal` before it insert a new task to queue?

Question

I am trying to read the LevelDB source code. Some code below:

void PosixEnv::Schedule( void (*background_work_function)(void* background_work_arg), void* background_work_arg) {
  background_work_mutex_.Lock();

  // Start the background thread, if we haven't done so already.
  if (!started_background_thread_) {
    started_background_thread_ = true;
    std::thread background_thread(PosixEnv::BackgroundThreadEntryPoint, this);  
    background_thread.detach();
  }

  // If the queue is empty, the background thread may be waiting for work.
  if (background_work_queue_.empty()) {
    background_work_cv_.Signal();
  }

  background_work_queue_.emplace(background_work_function, background_work_arg);
  background_work_mutex_.Unlock();
}

void PosixEnv::BackgroundThreadMain() {
  while (true) {
    background_work_mutex_.Lock();

    // Wait until there is work to be done.
    while (background_work_queue_.empty()) {
      background_work_cv_.Wait();
    }

    assert(!background_work_queue_.empty());
    auto background_work_function = background_work_queue_.front().function;
    void* background_work_arg = background_work_queue_.front().arg;
    background_work_queue_.pop();

    background_work_mutex_.Unlock();
    background_work_function(background_work_arg);
  }
}

About the Schedule, I wonder why it calls background_work_cv_.Signal() when find queue is empty, before pushing a new task into the queue?

In my opinion, Schedule can be written as below to do the same thing:

void PosixEnv::Schedule( void (*background_work_function)(void* background_work_arg), void* background_work_arg) {
  background_work_mutex_.Lock();

  ...

  background_work_queue_.emplace(background_work_function, background_work_arg);
  background_work_mutex_.Unlock();
  background_work_cv_.Signal(); // cv.notify_one();
}

I would like to know if there is any difference between the two? Why does it have to be the first implementation? Is the second implementation flawed in some way, not taking into account a certain situation?

Answer 1

Yes, there is no harm in calling signal if there is no thread waiting, so your implementation would work fine. It's difficult to say why it was implemented this way. Perhaps the developers were afraid the redundant call to signal will slow down the code? If so, such things should be measured (and maybe they did measure it?) Basically, it is logical to call signal only if you know there's someone waiting, so hard to blame the developers. It might be just the first thing that came to their minds.

Also note, that in that code it doesn't matter whether you call signal before or after pushing something into the queue. The lock is still taken so the other thread cannot check the state of the queue until the lock is released.