Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment.

content/browser/browser_thread_impl.cc

 // Copyright (c) 2012 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 "content/browser/browser_thread_impl.h"
 
 #include <string.h>
 
 #include <string>
 
@@ skipping 82 matching lines @@
   }
 
   scoped_refptr<base::SingleThreadTaskRunner> proxies[BrowserThread::ID_COUNT];
 };
 
 base::LazyInstance<BrowserThreadTaskRunners>::Leaky g_task_runners =
     LAZY_INSTANCE_INITIALIZER;
 
 struct BrowserThreadGlobals {
   BrowserThreadGlobals()
-      : blocking_pool(new base::SequencedWorkerPool(3, "BrowserBlocking")) {
+      : blocking_pool(
+            new base::SequencedWorkerPool(3,
+                                          "BrowserBlocking",
+                                          base::TaskPriority::BACKGROUND)) {

[jam, 2016/06/22 20:17:37]

the blocking pool is for anything that needs to write to disk. some of those
tasks may be background, but not all.

how did you arrive at this priority?

[gab, 2016/06/22 21:08:56]

IMO anything that goes in the BlockingPool in the browser shouldn't expect any
kind of throughput. The critical disk access is on the CachePool, the
BlockingPool should only be used for background work.

Or are you aware of different types of work going into the BlockingPool?

[jam, 2016/06/22 21:38:44]

In general I agree. However it's not clear that this is shared by all consumers.
Have you audited the 355 callers of this method? the documentation states:
  // Returns the thread pool used for blocking file I/O. Use this object to
  // perform random blocking operations such as file writes or querying the
  // Windows registry.

It doesn't say that this is for things that "shouldn't expect any kind of
throughput". If you'd like to change the behavior, that's fine, but the onus
should be to first check existing callers.

[gab, 2016/06/27 21:29:32]

I don't think it's reasonable for any caller today to use the BlockingPool for
tasks that require responsiveness specifically because it documents itself as
being used to "perform random blocking operations" and as such even non-blocking
operations can be blocked behind other ones.

As such I don't think an audit is required as that would be a major undertaking
for an API which I honestly don't think should be used for the surveyed use
case.

[jam, 2016/06/30 22:45:53]

The documentation for it in content/public/browser is that it should be used for
blocking tasks such as file i/o or registry.

That is in contrast to base::TaskPriority::BACKGROUND which is "user won't
notice if this task takes an arbitrarily long time to complete."

These two descriptions are not the same.
There are 355 callers on
https://cs.chromium.org/chromium/src/content/public/browser/browser_thread.h?....

I skipped the first two (ash) and the third(blimp).
The first chrome one is drive_service_bridge.cc
  -this appears to affect the UI
The second chrome one is in_memory_url_index_factory.cc
  -this clearly affects UI

I stopped looking after.

[gab, 2016/07/05 13:53:44]

Thanks for the examples, you're right.

I guess that in the absence of a place for "foreground" file I/O (besides
FILE_USER_BLOCKING thread in truly blocking scenarios), the BlockingPool has
ended up mixing blocking background tasks with file tasks affecting UI elements.
So for now USER_VISIBLE is required and this will be properly separated when
deprecating the API in favor of explicit usage of TaskScheduler.

Done.

     memset(threads, 0, BrowserThread::ID_COUNT * sizeof(threads[0]));
     memset(thread_delegates, 0,
            BrowserThread::ID_COUNT * sizeof(thread_delegates[0]));
   }
 
   // This lock protects |threads|. Do not read or modify that array
   // without holding this lock. Do not block while holding this lock.
   base::Lock lock;
 
   // This array is protected by |lock|. The threads are not owned by this
@@ skipping 434 matching lines @@
   AtomicWord* storage = reinterpret_cast<AtomicWord*>(
       &globals.thread_delegates[identifier]);
   AtomicWord old_pointer = base::subtle::NoBarrier_AtomicExchange(
       storage, reinterpret_cast<AtomicWord>(delegate));
 
   // This catches registration when previously registered.
   DCHECK(!delegate || !old_pointer);
 }
 
 }  // namespace content