IndexedDB: Keep the utility process open.

IndexedDB: Keep the utility process open.

Lazily create the process when it is first needed, and keep it running.
This significantly speeds up data insertion when there is a key path.

BUG=60138
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=65942

[hans, 2010-11-08 17:40:22]

This speeds things up.

Will go look at getting a performance test for this into the page cyclers.

[hans, 2010-11-09 11:05:29]

On 2010/11/08 17:40:22, hans wrote:
> This speeds things up.
> 
> Will go look at getting a performance test for this into the page cyclers.

Marcus asked me yesterday whether I had tested that shutdown works while the
utility process is running.

It does work when it is running idle. When trying to shut down while it is doing
IDBKeysFromValuesAndKeyPath, it fails silently. This happens on trunk too, so
it's not new but I'm trying to figure out how to fix it.

[jorlow, 2010-11-09 11:09:17]

I don't see any problems, but Marcus needs to sign off on it.  Marcus, if you're
not 100% sure, maybe rope in other reviewers as well?

[bulach, 2010-11-09 12:19:56]

the change itself seems fine, but I'm a bit uncomfortable with changing the
UtilityProcess behavior. I'd prefer to hook up the shutdown with something more
specific to indexeddb (something like IndexedDBDispatcherHost::ShutDown), or
perhaps observing RESOURCE_MESSAGE_FILTER_SHUTDOWN?

what do you think?

http://codereview.chromium.org/4678002/diff/2001/3001
File chrome/browser/in_process_webkit/browser_webkitclient_impl.cc (right):

http://codereview.chromium.org/4678002/diff/2001/3001#newcode23
chrome/browser/in_process_webkit/browser_webkitclient_impl.cc:23:
BrowserWebKitClientImpl::~BrowserWebKitClientImpl() {
(see more below on UtilityProcessHost)
I'd add:

if (indexed_db_key_utility_client_.get())
  indexed_db_key_utility_client_->EndUilityProcess();


however, iirc, the gotcha here is that this will be deleted during the WebKit
thread shutdown, which happens after the IO thread has gone, so no IPC (and I
think somewhere higher up would complain that the utility process is still
around..).

I think that we should try to add some plumbing and hook up this shutdown with
some indexed-db call rather than relying on shutdown order.

http://codereview.chromium.org/4678002/diff/2001/3002
File chrome/browser/in_process_webkit/browser_webkitclient_impl.h (right):

http://codereview.chromium.org/4678002/diff/2001/3002#newcode18
chrome/browser/in_process_webkit/browser_webkitclient_impl.h:18:
~BrowserWebKitClientImpl();
maybe virtual?

http://codereview.chromium.org/4678002/diff/2001/3002#newcode59
chrome/browser/in_process_webkit/browser_webkitclient_impl.h:59:
scoped_refptr<IndexedDBKeyUtilityClient> indexed_db_key_utility_client;
s/client;/client_;/

http://codereview.chromium.org/4678002/diff/2001/3003
File chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc (left):

http://codereview.chromium.org/4678002/diff/2001/3003#oldcode21
chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:21:
DCHECK(!client_.get());
see below, I think it'd be good to keep these checks here.

http://codereview.chromium.org/4678002/diff/2001/3004
File chrome/browser/utility_process_host.cc (right):

http://codereview.chromium.org/4678002/diff/2001/3004#newcode30
chrome/browser/utility_process_host.cc:30: EndBatchMode();
hmm, I think this would break the original API though. afaict, UtilityProcess is
either a one-shot / fire-and-forget "run in a separate thread", or a controlled
batch mode.

whilst this may be useful for safety / cleanup, I think relying on it may not be
desirable. once we're done with IDB (possibly with a lingering timer if we
really want), there's no much point in keeping this process around much longer.

[hans, 2010-11-09 16:44:15]

On 2010/11/09 12:19:56, bulach wrote:
> the change itself seems fine, but I'm a bit uncomfortable with changing the
> UtilityProcess behavior. I'd prefer to hook up the shutdown with something
more
> specific to indexeddb (something like IndexedDBDispatcherHost::ShutDown), or
> perhaps observing RESOURCE_MESSAGE_FILTER_SHUTDOWN?
> 
> what do you think?
Sounds like a good idea. I suppose since we call StartBatchMode(), it would be
decent of us to also call EndBatchMode(), rather than just letting that happen
implicitly through shutdown.

I'm trying to figure out how to set this up, though. Starting from
IndexedDBDispatcherHost::ShutDown sounds good, but I suppose we then have to go
through webkit to end up in BrowserWebKitClientImpl, and perhaps call some
IndexedDBIsShuttingDown() method on that? Input is welcome here.

> 
> http://codereview.chromium.org/4678002/diff/2001/3001
> File chrome/browser/in_process_webkit/browser_webkitclient_impl.cc (right):
> 
> http://codereview.chromium.org/4678002/diff/2001/3001#newcode23
> chrome/browser/in_process_webkit/browser_webkitclient_impl.cc:23:
> BrowserWebKitClientImpl::~BrowserWebKitClientImpl() {
> (see more below on UtilityProcessHost)
> I'd add:
> 
> if (indexed_db_key_utility_client_.get())
>   indexed_db_key_utility_client_->EndUilityProcess();
I tried that :)

Like you note below, the IO thread is already gone at this stage, and it turns
out that the UtilityProcessHost is destructed as part of the IO thread shutdown,
so by this time it is already gone. (Yes, that had me puzzled for a while.)

> 
> 
> however, iirc, the gotcha here is that this will be deleted during the WebKit
> thread shutdown, which happens after the IO thread has gone, so no IPC (and I
> think somewhere higher up would complain that the utility process is still
> around..).
> 
> I think that we should try to add some plumbing and hook up this shutdown with
> some indexed-db call rather than relying on shutdown order.
> 
> http://codereview.chromium.org/4678002/diff/2001/3002
> File chrome/browser/in_process_webkit/browser_webkitclient_impl.h (right):
> 
> http://codereview.chromium.org/4678002/diff/2001/3002#newcode18
> chrome/browser/in_process_webkit/browser_webkitclient_impl.h:18:
> ~BrowserWebKitClientImpl();
> maybe virtual?
> 
> http://codereview.chromium.org/4678002/diff/2001/3002#newcode59
> chrome/browser/in_process_webkit/browser_webkitclient_impl.h:59:
> scoped_refptr<IndexedDBKeyUtilityClient> indexed_db_key_utility_client;
> s/client;/client_;/
> 
> http://codereview.chromium.org/4678002/diff/2001/3003
> File chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc (left):
> 
> http://codereview.chromium.org/4678002/diff/2001/3003#oldcode21
> chrome/browser/in_process_webkit/indexed_db_key_utility_client.cc:21:
> DCHECK(!client_.get());
> see below, I think it'd be good to keep these checks here.
> 
> http://codereview.chromium.org/4678002/diff/2001/3004
> File chrome/browser/utility_process_host.cc (right):
> 
> http://codereview.chromium.org/4678002/diff/2001/3004#newcode30
> chrome/browser/utility_process_host.cc:30: EndBatchMode();
> hmm, I think this would break the original API though. afaict, UtilityProcess
is
> either a one-shot / fire-and-forget "run in a separate thread", or a
controlled
> batch mode.
> 
> whilst this may be useful for safety / cleanup, I think relying on it may not
be
> desirable. once we're done with IDB (possibly with a lingering timer if we
> really want), there's no much point in keeping this process around much
longer.

You're right. Maybe we shouldn't rely on it, but it still needs to be fixed. As
it is now, if a user tries to shut down the browser while the UtilityProcess is
used for extracting keys, the program will not exit.

I'll address the nits when I've figured out how to do the rest.

[andreip3000, 2010-11-09 18:31:42]

http://codereview.chromium.org/4678002/diff/2001/3001
File chrome/browser/in_process_webkit/browser_webkitclient_impl.cc (right):

http://codereview.chromium.org/4678002/diff/2001/3001#newcode23
chrome/browser/in_process_webkit/browser_webkitclient_impl.cc:23:
BrowserWebKitClientImpl::~BrowserWebKitClientImpl() {
On 2010/11/09 12:19:56, bulach wrote:
> (see more below on UtilityProcessHost)
> I'd add:
> 
> if (indexed_db_key_utility_client_.get())
>   indexed_db_key_utility_client_->EndUilityProcess();
> 
> 
> however, iirc, the gotcha here is that this will be deleted during the WebKit
> thread shutdown, which happens after the IO thread has gone, so no IPC (and I
> think somewhere higher up would complain that the utility process is still
> around..).
> 
> I think that we should try to add some plumbing and hook up this shutdown with
> some indexed-db call rather than relying on shutdown order.

How about doing that in the IDBFactoryBackendInterface dtor? We already have a
Chromium-specific version of that file that in storage/chromium, so it'd be a
matter of adding the dtor implementation there and making a call to
ChromiumBridge.

[hans, 2010-11-10 11:43:30]

On 2010/11/09 18:31:42, andreip3000 wrote:
> http://codereview.chromium.org/4678002/diff/2001/3001
> File chrome/browser/in_process_webkit/browser_webkitclient_impl.cc (right):
> 
> http://codereview.chromium.org/4678002/diff/2001/3001#newcode23
> chrome/browser/in_process_webkit/browser_webkitclient_impl.cc:23:
> BrowserWebKitClientImpl::~BrowserWebKitClientImpl() {
> On 2010/11/09 12:19:56, bulach wrote:
> > (see more below on UtilityProcessHost)
> > I'd add:
> > 
> > if (indexed_db_key_utility_client_.get())
> >   indexed_db_key_utility_client_->EndUilityProcess();
> > 
> > 
> > however, iirc, the gotcha here is that this will be deleted during the
WebKit
> > thread shutdown, which happens after the IO thread has gone, so no IPC (and
I
> > think somewhere higher up would complain that the utility process is still
> > around..).
> > 
> > I think that we should try to add some plumbing and hook up this shutdown
with
> > some indexed-db call rather than relying on shutdown order.
> 
> How about doing that in the IDBFactoryBackendInterface dtor? We already have a
> Chromium-specific version of that file that in storage/chromium, so it'd be a
> matter of adding the dtor implementation there and making a call to
> ChromiumBridge.

Sounds good. Uploaded new patch. WebKit side is here:
https://bugs.webkit.org/show_bug.cgi?id=49313

[andreip3000, 2010-11-10 12:09:38]

LGTM

[bulach, 2010-11-10 12:14:10]

LGTM

brilliant, thanks!