DomStorage commit task sequencing.

DomStorage commit task sequencing. Use two workerpool task sequences (primary vs commit) instead of one to avoid DomStorage access queuing up behind commits.
* Initialization, shutdown, and administrative tasks are performed as shutdown-blocking primary sequence tasks.
* Methods that return values to the java-scriptable interface are performed as non-shutdown-blocking primary sequence tasks.
* Internal tasks related to committing changes to disk are performed as shutdown-blocking commit sequence tasks.

BUG=106763
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=127981

[michaeln, 2012-03-19 00:49:13]

hi ben, not sure if you're still around, but if you are would be great if you
could ptal, this is along the lines of what we've chatted about with having two
task 'sequences'

it's not quite ready yet (no tests, and blocked behind
https://chromiumcodereview.appspot.com/9695013/ landing)

[benm (inactive), 2012-03-19 14:22:53]

Awesome - this looks pretty good to me! Just a few nits/suggestions.

Cheers, Ben

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.cc:164: DomStorageArea::CommitBatch*
DomStorageArea::GetCommitBatch() {
As this method is more than a getter (it may kick off the timer to commit
changes) I think the name should reflect that extra behavior. How about
CommitBatchIfNeeded or something like that?

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.cc:165: DCHECK(is_shutdown_);
!is_shutdown?

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.cc:176: base::TimeDelta::FromSeconds(1));
might be worth putting a constant in dom_storage_types for the time delta as
we're using it in a couple of places now.

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.cc:191: // a task for immediate exeuction on
the 'write' sequence.
execution

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.cc:199: // This method executes on the
'write' sequence.
Can we DCHECK that this is the case?

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_area.h (right):

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.h:30: static FilePath
DatabaseFileNameFromOrigin(const GURL& origin);
Why is this public now?

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_task_runner.cc (right):

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_task_runner.cc:97: bool
DomStorageWorkerPoolTaskRunner::PostShutdownBlockingTask(
Maybe rather than having two methods have one that takes an enum parameter for
which sequence to post to?

[michaeln, 2012-03-19 16:04:42]

no new shapshot yet

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.cc:165: DCHECK(is_shutdown_);
On 2012/03/19 14:22:53, benm wrote:
> !is_shutdown?

Done.

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.cc:176: base::TimeDelta::FromSeconds(1));
On 2012/03/19 14:22:53, benm wrote:
> might be worth putting a constant in dom_storage_types for the time delta as
> we're using it in a couple of places now.

Done.

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.cc:191: // a task for immediate exeuction on
the 'write' sequence.
On 2012/03/19 14:22:53, benm wrote:
> execution

Done.

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.cc:199: // This method executes on the
'write' sequence.
> Can we DCHECK that this is the case?

I wish we could, but right now there is no way to assert that since
SequencedWorkerPool doesn't support it yet.

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_area.h (right):

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.h:30: static FilePath
DatabaseFileNameFromOrigin(const GURL& origin);
> Why is this public now?

This change really belongs in https://chromiumcodereview.appspot.com/9695013/

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_task_runner.cc (right):

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_task_runner.cc:97: bool
DomStorageWorkerPoolTaskRunner::PostShutdownBlockingTask(
On 2012/03/19 14:22:53, benm wrote:
> Maybe rather than having two methods have one that takes an enum parameter for
> which sequence to post to?

sgtm... PostShutdownBlockingTask(sequence_id, ...)

[michaeln, 2012-03-19 16:26:51]

i'll clean up the new method names and such and upload a new snapshot later
today, and also take a look at decluttering the DomStorageTaskRunner interface
(seems like deriving from SequencedTaskRunner just forced the addition of
methods that dont really need to be there)

https://chromiumcodereview.appspot.com/9718029/diff/2004/webkit/dom_storage/d...
File webkit/dom_storage/dom_storage_area.cc (right):

https://chromiumcodereview.appspot.com/9718029/diff/2004/webkit/dom_storage/d...
webkit/dom_storage/dom_storage_area.cc:164: DomStorageArea::CommitBatch*
DomStorageArea::GetCommitBatch() {
On 2012/03/19 14:22:53, benm wrote:
> As this method is more than a getter (it may kick off the timer to commit
> changes) I think the name should reflect that extra behavior. How about
> CommitBatchIfNeeded or something like that?

maybe StartCommitBatchIfNeeded()?

[benm (inactive), 2012-03-19 17:08:45]

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9718029/diff/2004/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.cc:164: DomStorageArea::CommitBatch*
DomStorageArea::GetCommitBatch() {
sg, or maybe Schedule instead of Start, you decide :)

[michaeln, 2012-03-19 20:35:32]

new snapshot

[michaeln, 2012-03-20 03:24:03]

I have to update the 'area' unittests but otherwise i think this CL is about
code complete. Here's my todo list for what remains in the CL

- update 'area' unittests
- better CL description
- add some doc comments for the public shutdown methods
- worsmith the taskRunner class comment to better described what functions occur
on the PRIMARY vs COMMIT sequences

[benm (inactive), 2012-03-20 13:39:16]

A few suggestions for comments and with the edits you mentioned in the last
message to tests etc, a pre-emptive lgtm as I'm away from tomorrow.

http://codereview.chromium.org/9718029/diff/19002/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9718029/diff/19002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:195: // This method executes on the
'read' sequence, we schedule
s/read/primary

http://codereview.chromium.org/9718029/diff/19002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:196: // a task for immediate execution on
the 'write' sequence.
s/write/commit

http://codereview.chromium.org/9718029/diff/19002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:217: void
DomStorageArea::OnCommitComplete() {
mention we're back on the primary sequence?

http://codereview.chromium.org/9718029/diff/19002/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_context.cc (right):

http://codereview.chromium.org/9718029/diff/19002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:92: // We can only purge memory from
the local storage namespace
did you mean to add these changes in this CL?

http://codereview.chromium.org/9718029/diff/19002/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_task_runner.h (right):

http://codereview.chromium.org/9718029/diff/19002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_task_runner.h:37: // a non shutdown blocking
READ_SEQUENCE task to run immediately.
s/READ_/PRIMARY_/ and below

http://codereview.chromium.org/9718029/diff/19002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_task_runner.h:68: // A derived class that utlizes
the SequenceWorkerPool under a
remove "a"

[michaeln, 2012-03-20 23:28:10]

@jsbell, could you take a look at the unit tests?

[jsbell, 2012-03-21 00:23:43]

lgtm

lgtm