Synchronize memory in TaskTracker::Flush().

base/task_scheduler/task_tracker.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/task_scheduler/task_tracker.h"
 
 #include <limits>
 #include <string>
 
 #include "base/callback.h"
@@ skipping 197 matching lines @@
   PerformShutdown();
   DCHECK(IsShutdownComplete());
 
   // Unblock Flush() when shutdown completes.
   AutoSchedulerLock auto_lock(flush_lock_);
   flush_cv_->Signal();
 }
 
 void TaskTracker::Flush() {
   AutoSchedulerLock auto_lock(flush_lock_);
-  while (subtle::NoBarrier_Load(&num_pending_undelayed_tasks_) != 0 &&
+  while (subtle::Acquire_Load(&num_pending_undelayed_tasks_) != 0 &&

[robliao, 2017/05/02 14:23:20]

What was the failure that caused this?

[robliao, 2017/05/02 15:44:47]

Offline Discussion: The Acquire here is probably not necessary due to the
flush_lock_. We also aren't concerned about the effects of writes reordered
within this thread for visibility to other threads.

task_tracker.h will need to be updated too.
  // Number of undelayed tasks that haven't completed their execution. Is
  // incremented and decremented without a barrier. When it reaches zero,
  // |flush_lock_| is acquired (forcing memory synchronization) and |flush_cv_|
  // is signaled.
  subtle::Atomic32 num_pending_undelayed_tasks_ = 0;

[gab, 2017/05/02 20:26:27]

My thoughts (no action required):

So the problem is that it's possible that this sees 0 before |flush_lock_| is
acquired because it dropped it to 0 in DecrementNumPendingUndelayedTasks()

So what needs to happen is DecrementNumPendingUndelayedTasks() not only calls
flush_cv_->Signal() inside the lock but also sets another piece of state that
the conditional here will depend on (e.g "bool flushed_ = true;").

The atomic |num_pending_undelayed_tasks_| is still useful to make sure we only
grab the lock on sync points (0 -> 1 and 1 -> 0)...


But meh... overall the barriers are simpler I think...

PS: I do think we need the Acquire here as this is what forces this to
happen-after the Release semantics of the Barrier_AtomicIncrement.

[fdoray, 2017/05/03 13:17:28]

Updating comment in header file: Done.

          !IsShutdownComplete()) {
     flush_cv_->Wait();
   }
 }
 
 bool TaskTracker::WillPostTask(const Task* task) {
   DCHECK(task);
 
   if (!BeforePostTask(task->traits.shutdown_behavior()))
     return false;
@@ skipping 248 matching lines @@
 
   // This method can only be called after shutdown has started.
   DCHECK(state_->HasShutdownStarted());
   DCHECK(shutdown_event_);
 
   shutdown_event_->Signal();
 }
 
 void TaskTracker::DecrementNumPendingUndelayedTasks() {
   const auto new_num_pending_undelayed_tasks =
-      subtle::NoBarrier_AtomicIncrement(&num_pending_undelayed_tasks_, -1);
+      subtle::Barrier_AtomicIncrement(&num_pending_undelayed_tasks_, -1);

[robliao, 2017/05/02 14:23:20]

Is this barrier actually necessary? What other memory was impacted?

[robliao, 2017/05/02 15:44:48]

Offline discussion: The memory writes of the task finishing could be reordered
after this atomic decrement, which is why we need a barrier here so that when
another thread sees 0, it's guaranteed that all writes are visible.

   DCHECK_GE(new_num_pending_undelayed_tasks, 0);
   if (new_num_pending_undelayed_tasks == 0) {
     AutoSchedulerLock auto_lock(flush_lock_);
     flush_cv_->Signal();
   }
 }
 
 void TaskTracker::RecordTaskLatencyHistogram(Task* task) {
   const TimeDelta task_latency = TimeTicks::Now() - task->sequenced_time;
   task_latency_histograms_[static_cast<int>(task->traits.priority())]
                           [task->traits.may_block() ||
                                    task->traits.with_base_sync_primitives()
                                ? 1
                                : 0]
                               ->Add(task_latency.InMicroseconds());
 }
 
 }  // namespace internal
 }  // namespace base