Move Indexed DB from dedicated thread to task scheduler

Move Indexed DB from dedicated thread to task scheduler

Back in the mists of time, Indexed DB in the browser
ran on a "webkit thread". We slimmed that down to being
a dedicated IDB thread, but it's now possible to ditch
that and use the task scheduler APIs, e.g. to avoid
thrashing.

BUG=552552
Review-Url: https://codereview.chromium.org/1504033007
Cr-Commit-Position: refs/heads/master@{#478726}
Committed: https://chromium.googlesource.com/chromium/src/+/db76a97fd74c4ee0e5f4c379a1289ee0f44ed7ea

[jsbell, 2017-06-06 00:31:29]

https://codereview.chromium.org/1504033007/diff/20001/content/browser/storage...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/1504033007/diff/20001/content/browser/storage...
content/browser/storage_partition_impl.cc:477:
scoped_refptr<base::SequencedTaskRunner> idb_task_runner =
It would be nice to push this into the context itself. A naive change ended up
dchecking in MemoryDumpProvider (memory_dump_manager.cc(408)) when running the
unit tests, so probably best to leave for a follow-up.

[jsbell, 2017-06-06 00:31:42]

Description was changed from

==========
Move Indexed DB from dedicated thread to blocking worker pool

WORK IN PROGRESS - DO NOT REVIEW OR LAND

BUG=552552
==========

to

==========
Move Indexed DB from dedicated thread to task scheduler

WORK IN PROGRESS - DO NOT REVIEW OR LAND

BUG=552552
==========

[jsbell, 2017-06-08 20:28:22]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 20:28:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2017-06-08 20:32:39]

jsbell@chromium.org changed reviewers:
+ dmurph@chromium.org, gab@chromium.org, jam@chromium.org

[jsbell, 2017-06-08 20:32:39]

gab@ - could you please sanity check the thread->sequence changes?

dmurph@ - can you please review indexed_db ?

jam@ - can you please review the content/browser/main_loop.* and
storage_partition_impl.cc changes?

[commit-bot: I haz the power, 2017-06-08 21:34:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-08 21:34:50]

Dry run: This issue passed the CQ dry run.

[jsbell, 2017-06-08 22:11:47]

Description was changed from

==========
Move Indexed DB from dedicated thread to task scheduler

WORK IN PROGRESS - DO NOT REVIEW OR LAND

BUG=552552
==========

to

==========
Move Indexed DB from dedicated thread to task scheduler

Back in the mists of time, Indexed DB in the browser
ran on a "webkit thread". We slimmed that down to being
a dedicated IDB thread, but it's now possible to ditch
that and use the task scheduler APIs, e.g. to avoid
thrashing.

BUG=552552
==========

[jam, 2017-06-09 06:08:53]

lgtm, nice

[dmurph, 2017-06-09 19:15:08]

https://codereview.chromium.org/1504033007/diff/40001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_context_impl.cc (right):

https://codereview.chromium.org/1504033007/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_context_impl.cc:117:
task_runner_(task_runner) {
std::move(task_runner)

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:483: base::TaskPriority::BACKGROUND,
CAn you run the idb perf tests to see what the impact is?

[jsbell, 2017-06-09 20:50:31]

https://codereview.chromium.org/1504033007/diff/40001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_context_impl.cc (right):

https://codereview.chromium.org/1504033007/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_context_impl.cc:117:
task_runner_(task_runner) {
On 2017/06/09 19:15:07, dmurph wrote:
> std::move(task_runner)

Done.

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:483: base::TaskPriority::BACKGROUND,
On 2017/06/09 19:15:07, dmurph wrote:
> CAn you run the idb perf tests to see what the impact is?

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.

[dmurph, 2017-06-09 20:51:20]

On 2017/06/09 20:50:31, jsbell wrote:
>
https://codereview.chromium.org/1504033007/diff/40001/content/browser/indexed...
> File content/browser/indexed_db/indexed_db_context_impl.cc (right):
> 
>
https://codereview.chromium.org/1504033007/diff/40001/content/browser/indexed...
> content/browser/indexed_db/indexed_db_context_impl.cc:117:
> task_runner_(task_runner) {
> On 2017/06/09 19:15:07, dmurph wrote:
> > std::move(task_runner)
> 
> Done.
> 
>
https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
> File content/browser/storage_partition_impl.cc (right):
> 
>
https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
> content/browser/storage_partition_impl.cc:483: base::TaskPriority::BACKGROUND,
> On 2017/06/09 19:15:07, dmurph wrote:
> > CAn you run the idb perf tests to see what the impact is?
> 
> 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.

Awesome. lgtm

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

Nice \o/!!

https://codereview.chromium.org/1504033007/diff/40001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_context_impl.cc (right):

https://codereview.chromium.org/1504033007/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_context_impl.cc:117:
task_runner_(task_runner) {
On 2017/06/09 20:50:30, jsbell wrote:
> On 2017/06/09 19:15:07, dmurph wrote:
> > std::move(task_runner)
> 
> Done.

Didn't upload?

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:477: // need to inject their own task
runner into the IndexedDBContext.
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...

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:483: base::TaskPriority::BACKGROUND,
On 2017/06/09 20:50:30, jsbell wrote:
> On 2017/06/09 19:15:07, dmurph wrote:
> > CAn you run the idb perf tests to see what the impact is?
> 
> 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.

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

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:484:
base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN,
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:18:59]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[jsbell, 2017-06-12 16:19:03]

https://codereview.chromium.org/1504033007/diff/40001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_context_impl.cc (right):

https://codereview.chromium.org/1504033007/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_context_impl.cc:117:
task_runner_(task_runner) {
On 2017/06/12 15:13:07, gab wrote:
> On 2017/06/09 20:50:30, jsbell wrote:
> > On 2017/06/09 19:15:07, dmurph wrote:
> > > std::move(task_runner)
> > 
> > Done.
> 
> Didn't upload?

Apparently. :P

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:477: // need to inject their own task
runner into the IndexedDBContext.
On 2017/06/12 15:13:07, gab wrote:
> 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?

Yes...

> 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.

Done.

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

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:483: base::TaskPriority::BACKGROUND,
On 2017/06/12 15:13:08, gab wrote:
> What is the IDB thread responsible for? Is none of it USER_VISIBLE? i.e. no
> reads?

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.

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:484:
base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN,
On 2017/06/12 15:13:08, gab wrote:
> Would it be safe to CONTINUE_ON_SHUTDOWN? Or do we need to complete tasks that
> have already started to avoid corrupting the DB?

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.

[commit-bot: I haz the power, 2017-06-12 16:19:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-06-12 16:49:12]

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:483: base::TaskPriority::BACKGROUND,
On 2017/06/12 16:19:02, jsbell wrote:
> On 2017/06/12 15:13:08, gab wrote:
> > What is the IDB thread responsible for? Is none of it USER_VISIBLE? i.e. no
> > reads?
> 
> 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.
> 

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.

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:484:
base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN,
On 2017/06/12 16:19:02, jsbell wrote:
> On 2017/06/12 15:13:08, gab wrote:
> > Would it be safe to CONTINUE_ON_SHUTDOWN? Or do we need to complete tasks
that
> > have already started to avoid corrupting the DB?
> 
> 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.

"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.

[commit-bot: I haz the power, 2017-06-12 16:52:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-12 16:52:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jsbell, 2017-06-12 17:42:13]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 17:42:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Thanks gab!

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:483: base::TaskPriority::BACKGROUND,
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.)

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:484:
base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN,
On 2017/06/12 16:49:11, gab wrote:
> 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.

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

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

lgtm

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:483: base::TaskPriority::BACKGROUND,
On 2017/06/12 17:45:31, jsbell wrote:
> 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.)

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

https://codereview.chromium.org/1504033007/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:484:
base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN,
On 2017/06/12 17:45:31, jsbell wrote:
> On 2017/06/12 16:49:11, gab wrote:
> > 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.
> 
> Okay - being "conservative" and going with SKIP; let's watch out for shutdown
> hangs.

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

[commit-bot: I haz the power, 2017-06-12 18:58:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-12 18:58:08]

Dry run: This issue passed the CQ dry run.

[jsbell, 2017-06-12 19:47:43]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2017-06-12 19:47:44]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
dmurph@chromium.org
Link to the patchset: https://codereview.chromium.org/1504033007/#ps80001
(title: "Tweak traits")

[commit-bot: I haz the power, 2017-06-12 19:48:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 19:53:35]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497296863518570,
"parent_rev": "4c99eb040348ae35f9736fc11c7279fa95d1d9ee", "commit_rev":
"db76a97fd74c4ee0e5f4c379a1289ee0f44ed7ea"}

[commit-bot: I haz the power, 2017-06-12 19:53:48]

Description was changed from

==========
Move Indexed DB from dedicated thread to task scheduler

Back in the mists of time, Indexed DB in the browser
ran on a "webkit thread". We slimmed that down to being
a dedicated IDB thread, but it's now possible to ditch
that and use the task scheduler APIs, e.g. to avoid
thrashing.

BUG=552552
==========

to

==========
Move Indexed DB from dedicated thread to task scheduler

Back in the mists of time, Indexed DB in the browser
ran on a "webkit thread". We slimmed that down to being
a dedicated IDB thread, but it's now possible to ditch
that and use the task scheduler APIs, e.g. to avoid
thrashing.

BUG=552552

Review-Url: https://codereview.chromium.org/1504033007
Cr-Commit-Position: refs/heads/master@{#478726}
Committed:
https://chromium.googlesource.com/chromium/src/+/db76a97fd74c4ee0e5f4c379a128...
==========

[commit-bot: I haz the power, 2017-06-12 19:53:51]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/db76a97fd74c4ee0e5f4c379a128...