Revert 210434 "Revert 210433 "Revert 210423 "base: Make Sequence..."

The SequencedWorkerPool change broke CrOs (daisy) builder due to unused variable:

  threading/sequenced_worker_pool.cc:682:13: error: variable 'shutdown_wait_begin'

reverting until that is fixed.


> Revert 210433 "Revert 210423 "base: Make SequencedWorkerPool iss..."
> 
> Undoing speculative revert. Still failed after revert.
> 
> > Revert 210423 "base: Make SequencedWorkerPool issue globally uni..."
> > 
> > Speculative revert for failing PageCyclerTest.FailProvisionalLoads on
> > linux_clang.
> > 
> > > base: Make SequencedWorkerPool issue globally unique SequenceTokens.
> > > 
> > > SequencedWorkerPool currently issues SequenceTokens out of an internal member counter. This means that two different SequencedWorkerPool instances can issue identical SequenceTokens, which mucks up any attempt to distinguish sequences using only SequenceTokens.
> > > 
> > > This change makes the SequenceTokens issued from an StaticAtomicSequenceNumber, which is globally shared amongst all SequencedWorkerPools.
> > > 
> > > This change also makes the SequencedWorkerPool included in the nacl_untrusted builds, as it is needed for SequenceChecker and WeakPtr to work correctly. It previously was excluded because it used base/metrics. I've #ifdefed the base/metrics usage out for nacl.
> > > 
> > > This issue is a spinoff and pre-requisite of issue 18501008: Make WeakPtr use SequenceChecker instead of ThreadChecker.
> > > 
> > > R=akalin,darin
> > > BUG=165590
> > > 
> > > Review URL: https://chromiumcodereview.appspot.com/18650006
> > 
> > TBR=tommycli@chromium.org
> > 
> > Review URL: https://codereview.chromium.org/18271011
> 
> TBR=scottmg@chromium.org
> 
> Review URL: https://codereview.chromium.org/18242008

TBR=scottmg@chromium.org

Review URL: https://codereview.chromium.org/18861008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@210462 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 13 insertions, 51 deletions

diff --git a/base/base.gypi b/base/base.gypi
index fc7cf04..f3b16d8 100644
--- a/base/base.gypi
+++ b/base/base.gypi
@@ -701,6 +701,7 @@
                'scoped_native_library.cc',
                'files/scoped_temp_dir.cc',
                'sys_info_posix.cc',
+               'threading/sequenced_worker_pool.cc',
                'third_party/dynamic_annotations/dynamic_annotations.c',
             ],
             'sources/': [
diff --git a/base/threading/sequenced_worker_pool.cc b/base/threading/sequenced_worker_pool.cc
index ba9a7a3..f98b23d 100644
--- a/base/threading/sequenced_worker_pool.cc
+++ b/base/threading/sequenced_worker_pool.cc
@@ -10,22 +10,21 @@
 #include <utility>
 #include <vector>
 
-#include "base/atomic_sequence_num.h"
+#include "base/atomicops.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/critical_closure.h"
 #include "base/debug/trace_event.h"
-#include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/linked_ptr.h"
 #include "base/message_loop/message_loop_proxy.h"
+#include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/simple_thread.h"
-#include "base/threading/thread_local.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 #include "base/tracked_objects.h"
@@ -34,10 +33,6 @@
 #include "base/mac/scoped_nsautorelease_pool.h"
 #endif
 
-#if !defined(OS_NACL)
-#include "base/metrics/histogram.h"
-#endif
-
 namespace base {
 
 namespace {
@@ -218,10 +213,6 @@ uint64 GetTaskTraceID(const SequencedTask& task,
          static_cast<uint64>(reinterpret_cast<intptr_t>(pool));
 }
 
-base::LazyInstance<base::ThreadLocalPointer<
-    SequencedWorkerPool::SequenceToken> > g_lazy_tls_ptr =
-        LAZY_INSTANCE_INITIALIZER;
-
 }  // namespace
 
 // Worker ---------------------------------------------------------------------
@@ -391,9 +382,8 @@ class SequencedWorkerPool::Inner {
 
   // The last sequence number used. Managed by GetSequenceToken, since this
   // only does threadsafe increment operations, you do not need to hold the
-  // lock. This is class-static to make SequenceTokens issued by
-  // GetSequenceToken unique across SequencedWorkerPool instances.
-  static base::StaticAtomicSequenceNumber g_last_sequence_number_;
+  // lock.
+  volatile subtle::Atomic32 last_sequence_number_;
 
   // This lock protects |everything in this class|. Do not read or modify
   // anything without holding this lock. Do not block while holding this
@@ -489,10 +479,6 @@ SequencedWorkerPool::Worker::~Worker() {
 }
 
 void SequencedWorkerPool::Worker::Run() {
-  // Store a pointer to the running sequence in thread local storage for
-  // static function access.
-  g_lazy_tls_ptr.Get().Set(&running_sequence_);
-
   // Just jump back to the Inner object to run the thread, since it has all the
   // tracking information and queues. It might be more natural to implement
   // using DelegateSimpleThread and have Inner implement the Delegate to avoid
@@ -511,6 +497,7 @@ SequencedWorkerPool::Inner::Inner(
     const std::string& thread_name_prefix,
     TestingObserver* observer)
     : worker_pool_(worker_pool),
+      last_sequence_number_(0),
       lock_(),
       has_work_cv_(&lock_),
       can_shutdown_cv_(&lock_),
@@ -545,9 +532,9 @@ SequencedWorkerPool::Inner::~Inner() {
 
 SequencedWorkerPool::SequenceToken
 SequencedWorkerPool::Inner::GetSequenceToken() {
-  // Need to add one because StaticAtomicSequenceNumber starts at zero, which
-  // is used as a sentinel value in SequenceTokens.
-  return SequenceToken(g_last_sequence_number_.GetNext() + 1);
+  subtle::Atomic32 result =
+      subtle::NoBarrier_AtomicIncrement(&last_sequence_number_, 1);
+  return SequenceToken(static_cast<int>(result));
 }
 
 SequencedWorkerPool::SequenceToken
@@ -628,7 +615,7 @@ bool SequencedWorkerPool::Inner::IsRunningSequenceOnCurrentThread(
   ThreadMap::const_iterator found = threads_.find(PlatformThread::CurrentId());
   if (found == threads_.end())
     return false;
-  return sequence_token.Equals(found->second->running_sequence());
+  return found->second->running_sequence().Equals(sequence_token);
 }
 
 // See https://code.google.com/p/chromium/issues/detail?id=168415
@@ -687,10 +674,8 @@ void SequencedWorkerPool::Inner::Shutdown(
     while (!CanShutdown())
       can_shutdown_cv_.Wait();
   }
-#if !defined(OS_NACL)
   UMA_HISTOGRAM_TIMES("SequencedWorkerPool.ShutdownDelayTime",
                       TimeTicks::Now() - shutdown_wait_begin);
-#endif
 }
 
 void SequencedWorkerPool::Inner::ThreadLoop(Worker* this_worker) {
@@ -887,10 +872,8 @@ SequencedWorkerPool::Inner::GetWorkStatus SequencedWorkerPool::Inner::GetWork(
     std::vector<Closure>* delete_these_outside_lock) {
   lock_.AssertAcquired();
 
-#if !defined(OS_NACL)
   UMA_HISTOGRAM_COUNTS_100("SequencedWorkerPool.TaskCount",
                            static_cast<int>(pending_tasks_.size()));
-#endif
 
   // Find the next task with a sequence token that's not currently in use.
   // If the token is in use, that means another thread is running something
@@ -975,10 +958,8 @@ SequencedWorkerPool::Inner::GetWorkStatus SequencedWorkerPool::Inner::GetWork(
   // Track the number of tasks we had to skip over to see if we should be
   // making this more efficient. If this number ever becomes large or is
   // frequently "some", we should consider the optimization above.
-#if !defined(OS_NACL)
   UMA_HISTOGRAM_COUNTS_100("SequencedWorkerPool.UnrunnableTaskCount",
                            unrunnable_tasks);
-#endif
   return status;
 }
 
@@ -1107,20 +1088,8 @@ bool SequencedWorkerPool::Inner::CanShutdown() const {
          blocking_shutdown_pending_task_count_ == 0;
 }
 
-base::StaticAtomicSequenceNumber
-SequencedWorkerPool::Inner::g_last_sequence_number_;
-
 // SequencedWorkerPool --------------------------------------------------------
 
-// static
-SequencedWorkerPool::SequenceToken
-SequencedWorkerPool::GetSequenceTokenForCurrentThread() {
-  SequencedWorkerPool::SequenceToken* token = g_lazy_tls_ptr.Get().Get();
-  if (!token)
-    return SequenceToken();
-  return *token;
-}
-
 SequencedWorkerPool::SequencedWorkerPool(
     size_t max_threads,
     const std::string& thread_name_prefix)
diff --git a/base/threading/sequenced_worker_pool.h b/base/threading/sequenced_worker_pool.h
index 9e3dd5d..7e04b07 100644
--- a/base/threading/sequenced_worker_pool.h
+++ b/base/threading/sequenced_worker_pool.h
@@ -126,11 +126,6 @@ class BASE_EXPORT SequencedWorkerPool : public TaskRunner {
       return id_ == other.id_;
     }
 
-    // Returns false if current thread is executing an unsequenced task.
-    bool IsValid() const {
-      return id_ != 0;
-    }
-
    private:
     friend class SequencedWorkerPool;
 
@@ -148,11 +143,6 @@ class BASE_EXPORT SequencedWorkerPool : public TaskRunner {
     virtual void OnDestruct() = 0;
   };
 
-  // Gets the SequencedToken of the current thread.
-  // If current thread is not a SequencedWorkerPool worker thread or is running
-  // an unsequenced task, returns an invalid SequenceToken.
-  static SequenceToken GetSequenceTokenForCurrentThread();
-
   // When constructing a SequencedWorkerPool, there must be a
   // MessageLoop on the current thread unless you plan to deliberately
   // leak it.
@@ -169,7 +159,7 @@ class BASE_EXPORT SequencedWorkerPool : public TaskRunner {
                       TestingObserver* observer);
 
   // Returns a unique token that can be used to sequence tasks posted to
-  // PostSequencedWorkerTask(). Valid tokens are always nonzero.
+  // PostSequencedWorkerTask(). Valid tokens are alwys nonzero.
   SequenceToken GetSequenceToken();
 
   // Returns the sequence token associated with the given name. Calling this
diff --git a/base/threading/sequenced_worker_pool_unittest.cc b/base/threading/sequenced_worker_pool_unittest.cc
index eb63058..f1d7ed3 100644
--- a/base/threading/sequenced_worker_pool_unittest.cc
+++ b/base/threading/sequenced_worker_pool_unittest.cc
@@ -728,6 +728,8 @@ TEST_F(SequencedWorkerPoolTest, IsRunningOnCurrentThread) {
 
   scoped_refptr<SequencedWorkerPool> unused_pool =
       new SequencedWorkerPool(2, "unused_pool");
+  EXPECT_TRUE(token1.Equals(unused_pool->GetSequenceToken()));
+  EXPECT_TRUE(token2.Equals(unused_pool->GetSequenceToken()));
 
   EXPECT_FALSE(pool()->RunsTasksOnCurrentThread());
   EXPECT_FALSE(pool()->IsRunningSequenceOnCurrentThread(token1));