Switch to using the async DomStorage IPC messages and add a caching layer …

Fix the DomStorage is "wicked slow" bug by adding a renderer side cache and using a predominantly async IPC message protocol.

- use DomStorageCachedArea + DomStorageProxy
- an ipc message throttling mechanism to defend against misbehaving usage
- less chatty storage event propagation
- diable sudden termination when domstorage messages are pending to allow changes to be flushed thru to the backend on page unload
- deleted the obsolete sync message types and handlers

BUG=94382, 128482
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=139602

[Pan, 2012-05-15 05:34:13]

About this CL, actually we have got similar ideas to optimize webstorage
performance by cache + async before, however we didn't do that, one concern is
the behavior might be changed when concurrent 'set' and 'get' occurred among
renderers in the same origin.
example scenario:
In sync mode:
Time1: renderer R1 setItem('K','V1');
Time2: renderer R2 setItem('K','V2');
Time3: renderer R1 getItem('K'); here, it got the value V2.
In cache + async mode:
Time1: renderer R1 setItem('K','V1');
Time2: renderer R2 setItem('K','V2');
Time3: renderer R1 getItem('K'); here, if DomStorageMsg_Event and
DOMStorageMsg_AsyncOperationComplete of the operations in Time1 and Time2 are
all in browser process's IO message queue, R1 will get the value V1 from its
cache, right?

I mean even in the cache mode, when renderer process getItem, it have to ask
browser process to know who is the current storage holder.

[michaeln, 2012-05-16 02:30:25]

In general, yes there is behavior change that goes along with this new strategy.
That behavior change you singled out actually fixes a bug in the existing impl
by providing domStorage read stability (whereas before there was none).

I think by "current storage holder" you may be referring to the owner of the
origin's storage mutex? We're have no plans to implement the storage mutex, or
more clearly, we explicitly plan to not implement that. It goes against some
strongly held principals around, and its addition would preclude making an order
of magnitude improvement in performance in our multi-process browser (as is
possible with the addition of a caching layer, see comment 21 in
crbug.com/94382)

I'm a little distracted from this CL for the time being, but I'll get back to
this when I've got a better handle on this related P1 crasher that showed up on
my plate, http://code.google.com/p/chromium/issues/detail?id=127919.

[michaeln, 2012-05-16 03:08:43]

> we have got similar ideas

Also, who is "we"? I'm glad you have an interest in this stuff and welcome the
efforts to help makes things better, just curious as to who you are and where
you're coming from.

[Pan, 2012-05-16 13:27:23]

On 2012/05/16 02:30:25, michaeln wrote:
> In general, yes there is behavior change that goes along with this new
strategy.
> That behavior change you singled out actually fixes a bug in the existing impl
> by providing domStorage read stability (whereas before there was none).
> 
> I think by "current storage holder" you may be referring to the owner of the
> origin's storage mutex? We're have no plans to implement the storage mutex, or
> more clearly, we explicitly plan to not implement that. It goes against some
> strongly held principals around, and its addition would preclude making an
order
> of magnitude improvement in performance in our multi-process browser (as is
> possible with the addition of a caching layer, see comment 21 in
> crbug.com/94382)
> 
> I'm a little distracted from this CL for the time being, but I'll get back to
> this when I've got a better handle on this related P1 crasher that showed up
on
> my plate, http://code.google.com/p/chromium/issues/detail?id=127919.

Thanks, I mean renderer A maybe return dirty values from its cache because at
that time the value in browser process has been updated by another renderer B,
but the DomStorageEvent of the update hasn't arrived renderer A yet. To avoid
that, I think an IPC from renderer A to channel B to check if value has been
updated is needed(getItem will not get a good performance).
I agree that a mutex is not a good choice. 
My another raw idea is, can we distinguish single same-origin renderer case and
multiple same-origin renderer cases? the former one can be improved great,
although its complex and not beautiful. :)

[Pan, 2012-05-16 13:36:42]

On 2012/05/16 03:08:43, michaeln wrote:
> > we have got similar ideas
> 
> Also, who is "we"? I'm glad you have an interest in this stuff and welcome the
> efforts to help makes things better, just curious as to who you are and where
> you're coming from.

yeah, thanks :) I'm very glad if I can help to make it better.
As it may take some time to check in this new strategy, could you please
consider http://codereview.chromium.org/10389067/ check in first?
it is helpful although not huge speed up :)

[michaeln, 2012-05-16 19:16:49]

I expect to be committing this change in a week or so. Actually i'll probably
split it into two part.
  part 1: DomStorageCachedArea + unittests
  part 2: the rest of it that wires that class into things
What remains to be done are those unittests and some rate-limiting on the async
messages (as mentioned in a TODO in there). That shouldn't take too long to wrap
up.

> Thanks, I mean renderer A maybe return dirty values from its cache because at
> that time the value in browser process has been updated by another renderer B,
> but the DomStorageEvent of the update hasn't arrived renderer A yet. To avoid
> that, I think an IPC from renderer A to channel B to check if value has been
> updated is needed(getItem will not get a good performance).
> I agree that a mutex is not a good choice. 

Yes, renderer A may read a old value that renderer B has updated but whose
updated value has not yet made its way into renderer A. That's fine,
intentional, and actually necessary to provide read stability. Here's what i
mean by read stability.

  // In renderer A
  var value = localStorage.getItem("foo");  // occurs at time t1
  AssertEqual(value, localStorage.getItem("foo"));  // occurs at time t3

That assert should always succeed, thats a feature. Now suppose, concurrently in
a different renderer.

  // In renderer B
  localStorage.setItem("foo", "bar");  // occurs at time t2

In order for the assert in renderer A to succeed, it should not see the updated
value set by renderer B. Our current implementation is buggy in this regard
since "bar" will seen in renderer A given this sequence of events. This CL fixes
that bug (in addition to making things much faster).

We here veto any mutex around an origin's storage area :)

> My another raw idea is, can we distinguish single same-origin renderer case
and
> multiple same-origin renderer cases? the former one can be improved great,
> although its complex and not beautiful. :)

This is a really good idea that completely lines up with what we're thinking too
(the event propagation is way too chatty). I was just talking to darin about
exactly this yesterday. This is probably the next step to take (after adding the
caching layer). I think its not all that out of reach really, and could be
reasonably straight forward. I'll shoot you an email about that separately
because if you'd like to contribute to this, that would be an awesome
contribution to make.

[michaeln, 2012-05-17 03:15:24]

I opened a bug for this instead of sending that mail.
https://code.google.com/p/chromium/issues/detail?id=128482

> This is a really good idea that completely lines up with what we're thinking
too
> (the event propagation is way too chatty). I was just talking to darin about
> exactly this yesterday. This is probably the next step to take (after adding
the
> caching layer). I think its not all that out of reach really, and could be
> reasonably straight forward. I'll shoot you an email about that separately
> because if you'd like to contribute to this, that would be an awesome
> contribution to make.

[michaeln, 2012-05-21 23:34:54]

hi eric and josh

i think this dom storage work is ready for review. this CL is a bit much in one
chunk, so i'll be breaking into small pieces and putting those up individually,
but this larger CL shows how things fit together + an additional webkit patch...
https://bugs.webkit.org/show_bug.cgi?id=87031
... which is needed for the less chattiness aspect of things.

[michaeln, 2012-05-22 00:37:54]

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/browser/do...
File content/browser/dom_storage/dom_storage_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/browser/do...
content/browser/dom_storage/dom_storage_message_filter.cc:200:
host_->HasAreaOpen(area->namespace_id(), area->origin())) {
The HasAreaOpen(...) call is the chattiness reducing part.

[ericu, 2012-05-22 23:23:35]

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/browser/do...
File content/browser/dom_storage/dom_storage_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/browser/do...
content/browser/dom_storage/dom_storage_message_filter.cc:116: Send(new
DOMStorageMsg_AsyncOperationComplete(true));
Won't your MessageThrottlingFilter count this message and get confused because
you didn't request this through it?

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/renderer/d...
File content/renderer/dom_storage/dom_storage_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/renderer/d...
content/renderer/dom_storage/dom_storage_dispatcher.cc:75: DomStorageCachedArea*
LookupCachedArea(
Why does this return a raw pointer, whereas OpenCachedArea returns a
scoped_refptr?

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/renderer/d...
content/renderer/dom_storage/dom_storage_dispatcher.cc:151:
DCHECK_EQ(holder->area_.get(), area);
DCHECK_GT(holder->open_count_, 0) ?

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/renderer/d...
content/renderer/dom_storage/dom_storage_dispatcher.cc:183: connection_id,
values));
Nit: It would be kind of nice to use the throttling_filter_ for every message,
and have it do the right thing, if that's not too messy.  It would keep you from
accidentally sending through the wrong sender when you change the code some day.

https://chromiumcodereview.appspot.com/10383123/diff/11027/webkit/dom_storage...
File webkit/dom_storage/dom_storage_cached_area.cc (right):

https://chromiumcodereview.appspot.com/10383123/diff/11027/webkit/dom_storage...
webkit/dom_storage/dom_storage_cached_area.cc:43: // A quick check to reject
obvsiously overbudget items to avoid
Typo: obviously.

https://chromiumcodereview.appspot.com/10383123/diff/11027/webkit/dom_storage...
webkit/dom_storage/dom_storage_cached_area.cc:83: ignore_all_mutations_ = true;
What if we call clear twice before the first OnClearComplete comes back?

https://chromiumcodereview.appspot.com/10383123/diff/11027/webkit/dom_storage...
webkit/dom_storage/dom_storage_cached_area.cc:154: ignore_all_mutations_ =
false;
Here, too, what if we called clear before this came back?

https://chromiumcodereview.appspot.com/10383123/diff/11027/webkit/dom_storage...
File webkit/dom_storage/dom_storage_types.h (right):

https://chromiumcodereview.appspot.com/10383123/diff/11027/webkit/dom_storage...
webkit/dom_storage/dom_storage_types.h:21: // In the browser process we allow
some overrage to
typo: overage

[michaeln, 2012-05-23 00:38:51]

thnx for looking... no new snapshot yet

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/browser/do...
File content/browser/dom_storage/dom_storage_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/browser/do...
content/browser/dom_storage/dom_storage_message_filter.cc:116: Send(new
DOMStorageMsg_AsyncOperationComplete(true));
On 2012/05/22 23:23:35, ericu wrote:
> Won't your MessageThrottlingFilter count this message and get confused because
> you didn't request this through it?

Yes it sure will get confused, thnx for spotting this.

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/renderer/d...
File content/renderer/dom_storage/dom_storage_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/renderer/d...
content/renderer/dom_storage/dom_storage_dispatcher.cc:75: DomStorageCachedArea*
LookupCachedArea(
On 2012/05/22 23:23:35, ericu wrote:
> Why does this return a raw pointer, whereas OpenCachedArea returns a
> scoped_refptr?


The open method may create a new instance whereas the lookup pointer will never
do that. The different return types is a (probably poor) attempt to make a
distinction between those two cases.

Given that this class actually holds a reference to the newly created instance,
and that this method is only accessible by DomStorageDispatcher which just
passes it up as a return value... i'll switch to raw ptrs for the both of them.

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/renderer/d...
content/renderer/dom_storage/dom_storage_dispatcher.cc:183: connection_id,
values));
On 2012/05/22 23:23:35, ericu wrote:
> Nit: It would be kind of nice to use the throttling_filter_ for every message,
> and have it do the right thing, if that's not too messy.  It would keep you
from
> accidentally sending through the wrong sender when you change the code some
day.

sgtm, should be simple enough to determine 'needsThrottling' based on
msg->type() or msg->isSync() or something

https://chromiumcodereview.appspot.com/10383123/diff/11027/webkit/dom_storage...
File webkit/dom_storage/dom_storage_cached_area.cc (right):

https://chromiumcodereview.appspot.com/10383123/diff/11027/webkit/dom_storage...
webkit/dom_storage/dom_storage_cached_area.cc:83: ignore_all_mutations_ = true;
On 2012/05/22 23:23:35, ericu wrote:
> What if we call clear twice before the first OnClearComplete comes back?

Reset() kills all pending callbacks by virtue of invalidating outstanding weak
ptrs. If we call clear twice, in the second call the first callback is
effectively cancelled and will never be received, only the callback associated
with the second call proxy_->ClearArea() will happen.

https://chromiumcodereview.appspot.com/10383123/diff/11027/webkit/dom_storage...
webkit/dom_storage/dom_storage_cached_area.cc:154: ignore_all_mutations_ =
false;
On 2012/05/22 23:23:35, ericu wrote:
> Here, too, what if we called clear before this came back?

If we called clear before this came back... it will never come back.

[michaeln, 2012-05-23 22:37:28]

new snapshot, i'm still working on unittests (not included here)

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/browser/do...
File content/browser/dom_storage/dom_storage_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/browser/do...
content/browser/dom_storage/dom_storage_message_filter.cc:116: Send(new
DOMStorageMsg_AsyncOperationComplete(true));
On 2012/05/23 00:38:51, michaeln wrote:
> On 2012/05/22 23:23:35, ericu wrote:
> > Won't your MessageThrottlingFilter count this message and get confused
because
> > you didn't request this through it?
> 
> Yes it sure will get confused, thnx for spotting this.

Done. Fixed by sending this message through the throttling filter.

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/renderer/d...
File content/renderer/dom_storage/dom_storage_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/renderer/d...
content/renderer/dom_storage/dom_storage_dispatcher.cc:75: DomStorageCachedArea*
LookupCachedArea(
On 2012/05/23 00:38:51, michaeln wrote:
> On 2012/05/22 23:23:35, ericu wrote:
> > Why does this return a raw pointer, whereas OpenCachedArea returns a
> > scoped_refptr?
> 
> 
> The open method may create a new instance whereas the lookup pointer will
never
> do that. The different return types is a (probably poor) attempt to make a
> distinction between those two cases.
> 
> Given that this class actually holds a reference to the newly created
instance,
> and that this method is only accessible by DomStorageDispatcher which just
> passes it up as a return value... i'll switch to raw ptrs for the both of
them.

Done.

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/renderer/d...
content/renderer/dom_storage/dom_storage_dispatcher.cc:151:
DCHECK_EQ(holder->area_.get(), area);
On 2012/05/22 23:23:35, ericu wrote:
> DCHECK_GT(holder->open_count_, 0) ?

Done.

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/renderer/d...
content/renderer/dom_storage/dom_storage_dispatcher.cc:183: connection_id,
values));
On 2012/05/23 00:38:51, michaeln wrote:
> On 2012/05/22 23:23:35, ericu wrote:
> > Nit: It would be kind of nice to use the throttling_filter_ for every
message,
> > and have it do the right thing, if that's not too messy.  It would keep you
> from
> > accidentally sending through the wrong sender when you change the code some
> day.
> 
> sgtm, should be simple enough to determine 'needsThrottling' based on
> msg->type() or msg->isSync() or something

Done... kindof...
* added DCHECKs in SendThrottled to assert that only messages that will be ack'd
are sent thru it
* also added DCHECKs to assert that pending_count never dips below zero (this
should catch the mistake of neglecting to send a message that will be ack'd thru
the throttler

https://chromiumcodereview.appspot.com/10383123/diff/11027/webkit/dom_storage...
File webkit/dom_storage/dom_storage_cached_area.cc (right):

https://chromiumcodereview.appspot.com/10383123/diff/11027/webkit/dom_storage...
webkit/dom_storage/dom_storage_cached_area.cc:43: // A quick check to reject
obvsiously overbudget items to avoid
On 2012/05/22 23:23:35, ericu wrote:
> Typo: obviously.

Done.

https://chromiumcodereview.appspot.com/10383123/diff/11027/webkit/dom_storage...
File webkit/dom_storage/dom_storage_types.h (right):

https://chromiumcodereview.appspot.com/10383123/diff/11027/webkit/dom_storage...
webkit/dom_storage/dom_storage_types.h:21: // In the browser process we allow
some overrage to
On 2012/05/22 23:23:35, ericu wrote:
> typo: overage

Done.

[michaeln, 2012-05-25 01:07:46]

Darin brought up a good point about having to ensure that changes get flushed
during shutdown, so I'll be poking at the DomStorageDispatcher some more to see
to that.

[michaeln, 2012-05-25 01:59:59]

Added renderer side flushing on shutdown logic to DomStorageDispatcher in #18,
still have to add some to the browser side to get it all the way to disk.

[michaeln, 2012-05-25 19:04:32]

ptal

[jsbell, 2012-05-25 22:38:03]

lgtm but my review was fairly cursor (gardening distractions)

[michaeln, 2012-05-25 23:38:09]

On 2012/05/25 22:38:03, jsbell wrote:
> lgtm but my review was fairly cursor (gardening distractions)

i could use a good close look at this one. i'm in no rush to commit before the
long weekend since i want to be around for if/when anything in here hoses trunk.
eric did a good pass at it once already and i think i responded to his
comments/questions, but since then i've added the flush on close logic to
things.

[michaeln, 2012-05-29 19:07:21]

I've split out the changes to DomStorageTaskRunner to another CL because i'm
going to see about merging that part to m20.
https://chromiumcodereview.appspot.com/10444065/

[ericu, 2012-05-30 00:44:11]

Looks fine apart from this one lifetime issue.

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/renderer/d...
File content/renderer/dom_storage/dom_storage_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10383123/diff/11027/content/renderer/d...
content/renderer/dom_storage/dom_storage_dispatcher.cc:75: DomStorageCachedArea*
LookupCachedArea(
On 2012/05/23 00:38:51, michaeln wrote:
> On 2012/05/22 23:23:35, ericu wrote:
> > Why does this return a raw pointer, whereas OpenCachedArea returns a
> > scoped_refptr?
> 
> 
> The open method may create a new instance whereas the lookup pointer will
never
> do that. The different return types is a (probably poor) attempt to make a
> distinction between those two cases.
> 
> Given that this class actually holds a reference to the newly created
instance,
> and that this method is only accessible by DomStorageDispatcher which just
> passes it up as a return value... i'll switch to raw ptrs for the both of
them.

I'd probably have gone the other way, and switched both to refptrs.  At least
there should be documentation of the lifetime of a DOMStorageCachedArea.  In
your current usage, the WebStorageAreaImpl's keeping a refptr, so it's safe, but
it's also not referencing the object after calling CloseCachedArea, so it'd be
safe with a raw pointer as well.  It would make sense to require [or at least
return] refptrs, and not let on that there's an extra reference held internally.
 It would also make sense instead to require that callers not use the pointer
after calling CloseCachedArea.  But the current state's a little weird.

What do you think?

[michaeln, 2012-05-30 01:06:24]

> What do you think?

I think if the most notable thing is whether a return value s/b a
scoped_refptr<T> or a T*, well... great :) Fine by me to switch
DomStorageDispatcher::OpenCachedArea to return a scopted_refptr. Done.

[ericu, 2012-05-30 01:34:31]

;'>

LGTM