Move Indexed DB from dedicated thread to task scheduler

content/browser/storage_partition_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/storage_partition_impl.h"
 
 #include <stddef.h>
 
 #include <set>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/sequenced_task_runner.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/task_scheduler/post_task.h"
 #include "content/browser/blob_storage/chrome_blob_storage_context.h"
 #include "content/browser/browser_main_loop.h"
 #include "content/browser/browsing_data/storage_partition_http_cache_data_remover.h"
 #include "content/browser/fileapi/browser_file_system_helper.h"
 #include "content/browser/gpu/shader_cache_factory.h"
 #include "content/browser/notifications/platform_notification_context_impl.h"
 #include "content/common/dom_storage/dom_storage_types.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
@@ skipping 436 matching lines @@
   partition->database_tracker_ = new storage::DatabaseTracker(
       partition_path, in_memory, context->GetSpecialStoragePolicy(),
       quota_manager_proxy.get(),
       BrowserThread::GetTaskRunnerForThread(BrowserThread::FILE).get());
 
   partition->dom_storage_context_ = new DOMStorageContextWrapper(
       BrowserContext::GetConnectorFor(context),
       in_memory ? base::FilePath() : context->GetPath(),
       relative_partition_path, context->GetSpecialStoragePolicy());
 
+  // TODO(jsbell): Create task runner within IndexedDBContextImpl.
   // BrowserMainLoop may not be initialized in unit tests. Tests will
   // need to inject their own task runner into the IndexedDBContext.

[gab, 2017/06/12 15:13:07]

This is no longer true, so long as tests have a
base::test::ScopedTaskEnvironment (or content::TestBrowserThreadBundle which is
a superset) they can use post_task.h

I guess that's what your TODO above is about?

I'd move it below this comment as:

// TODO(jsbell): this is no longer true, update tests to provide use a
base::test::ScopedTaskEnvironment instead of injecting a test task runner.

See migration docs for more details:
https://chromium.googlesource.com/chromium/src/+/master/docs/task_scheduler_m...

Or even Testing specific section of new Threading&Tasks docs:
https://chromium.googlesource.com/chromium/src/+/master/docs/threading_and_ta...

[jsbell, 2017/06/12 16:19:02]

Yes...
Done.

(I'm working on a follow-up CL for this, BTW)

-  base::SequencedTaskRunner* idb_task_runner =
+  scoped_refptr<base::SequencedTaskRunner> idb_task_runner =
       BrowserThread::CurrentlyOn(BrowserThread::UI) &&
               BrowserMainLoop::GetInstance()
-          ? BrowserMainLoop::GetInstance()
-                ->indexed_db_thread()
-                ->task_runner()
-                .get()
-          : NULL;
+          ? base::CreateSequencedTaskRunnerWithTraits({
+                base::MayBlock(), base::WithBaseSyncPrimitives(),
+                base::TaskPriority::BACKGROUND,

[dmurph, 2017/06/09 19:15:07]

CAn you run the idb perf tests to see what the impact is?

[jsbell, 2017/06/09 20:50:30]

Thanks for the reminder! Done.

Summary: no significant difference. I did 3 runs of tip-of-tree and 3 with the
patch. The difference in average times between the two sets of runs was similar
to the standard deviation within each set.

[gab, 2017/06/12 15:13:08]

What is the IDB thread responsible for? Is none of it USER_VISIBLE? i.e. no
reads?

[jsbell, 2017/06/12 16:19:02]

It runs asynchronous database requests on behalf of scripts, e.g. get, put,
delete, etc.

Whether it's user visible or not depends on the script, but the calls are all
async. I noticed other storage implementations were using BACKGROUND and since
there's no perf difference it seemed like the appropriate choice.

[gab, 2017/06/12 16:49:11]

If it *can* be user visible then it should use TaskPriority::USER_VISIBLE or any
script for which it matters will get slow responses, even for foreground tabs,
when the machine is under heavy load (doesn't make a difference otherwise).
Ideally tasks will eventually also be labeled with the WebContents they were
requested for and we could better prioritize but at the moment we need to use
the highest common denominator.

[jsbell, 2017/06/12 17:45:31]

Okay - went with USER_VISIBLE

(The IndexedDB API also has the pesky behavior that a background tab could be
holding transaction locks that prevent a foreground tab from proceeding - scoped
to an origin and the requests are all async, of course. And yes, we're in touch
with the scheduler folks.)

[gab, 2017/06/12 17:59:05]

Right, remember that post-mortem, USER_VISIBLE sure sounds more correct.

+                base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN,

[gab, 2017/06/12 15:13:08]

Would it be safe to CONTINUE_ON_SHUTDOWN? Or do we need to complete tasks that
have already started to avoid corrupting the DB?

[jsbell, 2017/06/12 16:19:02]

Looking at the docs... CONTINUE has some scary caveats "...best to use this only
for slow but simple operations...". Ostensibly the database code us be resilient
to abnormal termination (i.e. crashes). However "...any singletons or global
objects you use may suddenly become invalid out from under you..." - the leveldb
environment code uses locks/condition variables to coordinate work across
threads (there's a background compaction thread).

We can try CONTINUE and watch for an uptick in corruption.

[gab, 2017/06/12 16:49:11]

"from under you" is referring to things that are registered with AtExitManager
or static objects deleted in static uninitialization at the end of main() on the
main thread (while other threads may still be processing CONTINUE_ON_SHUTDOWN
work).

CONTINUE_ON_SHUTDOWN prevents shutdown from hanging on these tasks, ultimately
the solution is to avoid any shutdown race by terminating process when writes
are done (we call this atomic shutdown but we're not there yet -- have
everything in TaskScheduler is a pre-req).

All that to say... if it doesn't feel safe to you, don't do it in this big
change IMO, we can assess later if shutdown hangs in IDB a lot.

[jsbell, 2017/06/12 17:45:31]

Okay - being "conservative" and going with SKIP; let's watch out for shutdown
hangs.

[gab, 2017/06/12 17:59:05]

Perfect, that way we can avoid this bigger change being reverted and we can keep
tweaks for later :)

+            })
+          : nullptr;
 
   base::FilePath path = in_memory ? base::FilePath() : partition_path;
   partition->indexed_db_context_ =
       new IndexedDBContextImpl(path, context->GetSpecialStoragePolicy(),
                                quota_manager_proxy.get(), idb_task_runner);
 
   partition->cache_storage_context_ = new CacheStorageContextImpl(context);
   partition->cache_storage_context_->Init(path, quota_manager_proxy);
 
   partition->service_worker_context_ = new ServiceWorkerContextWrapper(context);
@@ skipping 487 matching lines @@
   media_url_request_context_ = media_url_request_context;
 }
 
 void StoragePartitionImpl::GetQuotaSettings(
     const storage::OptionalQuotaSettingsCallback& callback) {
   GetContentClient()->browser()->GetQuotaSettings(browser_context_, this,
                                                   callback);
 }
 
 }  // namespace content