Move IOThread initialization to happen asynchronously.

Move IOThread initialization to happen asynchronously.

Speeds up the main thread initialization by having the first task
on IOThread initialize itself instead of blocking the main thread for this.
This allows the start-up to happen faster.

BUG=258231

[Maria, 2013-07-09 22:45:35]

Hi,

I am moving the IO thread initialization to happen asynchronously as the first
task on the IO thread instead of having the main thread waiting for
initialization to complete (which today on Android takes a non-trivial amount of
time).

I would appreciate feedback on this change and in particular whether there are
some gotchas with this approach that I am not seeing. It seems to work when
running manually and passes the try bots.

[jam, 2013-07-10 01:10:48]

ah, this is a great idea. you're right, no need to block UI thread on the IO
thread's initialization. if we find anything that does depend on this, then it
should be fixed.

that said, i'm very surprised that code takes that long to execute. is that
release builds? is that on desktop or android?

lgtm

[mariakhomenko, 2013-07-10 01:21:45]

I am timing it on an android release build on Nexus S and it's taking
50-100ms there depending on the run. I would imagine it would be a fair bit
faster on better hardware, but I have not tried to measure it on desktop.


On Tue, Jul 9, 2013 at 6:10 PM, <jam@chromium.org> wrote:

> ah, this is a great idea. you're right, no need to block UI thread on the
> IO
> thread's initialization. if we find anything that does depend on this,
> then it
> should be fixed.
>
> that said, i'm very surprised that code takes that long to execute. is that
> release builds? is that on desktop or android?
>
> lgtm
>
>
https://codereview.chromium.**org/18405007/<https://codereview.chromium.org/1...
>

[willchan no longer on Chromium, 2013-07-10 07:50:54]

Does this work? I'd expect a few minor (fixable) threading gotchas. I'm
traveling right now so I can't take a look, but I'm in favor of something
like this. I'd ask mmenke to take a look.
On Jul 10, 2013 3:21 AM, "Maria Khomenko" <mariakhomenko@google.com> wrote:

> I am timing it on an android release build on Nexus S and it's taking
> 50-100ms there depending on the run. I would imagine it would be a fair bit
> faster on better hardware, but I have not tried to measure it on desktop.
>
>
> On Tue, Jul 9, 2013 at 6:10 PM, <jam@chromium.org> wrote:
>
>> ah, this is a great idea. you're right, no need to block UI thread on the
>> IO
>> thread's initialization. if we find anything that does depend on this,
>> then it
>> should be fixed.
>>
>> that said, i'm very surprised that code takes that long to execute. is
>> that
>> release builds? is that on desktop or android?
>>
>> lgtm
>>
>>
https://codereview.chromium.**org/18405007/<https://codereview.chromium.org/1...
>>
>
>

[awong, 2013-07-10 17:54:49]

looks not good to me

Thanks for trying to tackle this.  I have some concerns with this CL though.

The CL comments say "Speeds up the main thread initialization."  I'm assuming
"main thread" means UI thread.  However, this change doesn't affect UI thread
behavior at all so I think I'm confused at what the intent is.

Please see my comment in IOThread::Init() for details.

If we are indeed seeing this speed up android, and this code actually doesn't
crash like crazy, them I'm guessing it means the IOThread initialization itself
is too eager...

[awong, 2013-07-10 17:56:31]

not LGTM

(once more with the correct rietveld keywords :-/)

On 2013/07/10 17:54:49, awong wrote:
> looks not good to me
> 
> Thanks for trying to tackle this.  I have some concerns with this CL though.
> 
> The CL comments say "Speeds up the main thread initialization."  I'm assuming
> "main thread" means UI thread.  However, this change doesn't affect UI thread
> behavior at all so I think I'm confused at what the intent is.
> 
> Please see my comment in IOThread::Init() for details.
> 
> If we are indeed seeing this speed up android, and this code actually doesn't
> crash like crazy, them I'm guessing it means the IOThread initialization
itself
> is too eager...

[jam, 2013-07-10 17:58:43]

On 2013/07/10 17:54:49, awong wrote:
> looks not good to me
> 
> Thanks for trying to tackle this.  I have some concerns with this CL though.
> 
> The CL comments say "Speeds up the main thread initialization."  I'm assuming
> "main thread" means UI thread.  However, this change doesn't affect UI thread
> behavior at all so I think I'm confused at what the intent is.

It's a bit subtle to see, but when the main thread is starting the IO thread,
it's blocked until the IO thread's Init method returns. see the thread
implementation for details.

> 
> Please see my comment in IOThread::Init() for details.

link?

> 
> If we are indeed seeing this speed up android, and this code actually doesn't
> crash like crazy, them I'm guessing it means the IOThread initialization
itself
> is too eager...

[awong, 2013-07-10 18:45:47]

https://codereview.chromium.org/18405007/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/18405007/diff/1/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:618:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
This confuses me...

My understanding of the original code was:

(1) UI BrowserThread does "new IOThread()"
(2) UI BrowserThread does effectively "PostTask(UI, FROM_HERE,
Bind(&IOThread::Init)" via BrowserThreadDelegate.
(3) IO BrowserThread runs IOThread::Init.

Now we know that we can post tasks to the IO BrowserThread that assume the
global IOThread object is initialized.

In the modified code, we haven't changed the behavior of the UI thread at all.

What we do change is in (3) we now delay the initialization of the IOThread
object on the IO BrowserThread such that there is a gap in time where IOThread
is not initialized.  This seems wrong.

[mmenke, 2013-07-10 18:48:24]

https://codereview.chromium.org/18405007/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/18405007/diff/1/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:505:
.reset(CreateDefaultAuthHandlerFactory(globals_->host_resolver.get()));
nit:  For all of these, it's much more common across the chrome code base to put
the method on the same name as the class.  I don't think I've ever seen this
indentation style used in chrome, actually, except perhaps where it was
necessary due to variable name length.

https://codereview.chromium.org/18405007/diff/1/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:655: // Profile (and therefore the first
CookieMonster) is created.
After this CL, this can actually be done after the creation of the profile. 
Actually has to be done before URLRequestContexts are initialized on the IO
thread (Both the system one, and the per-profile ones), so we're still good. 
Comment is just more strict than what's needed.

[mmenke, 2013-07-10 18:51:04]

https://codereview.chromium.org/18405007/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/18405007/diff/1/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:505:
.reset(CreateDefaultAuthHandlerFactory(globals_->host_resolver.get()));
On 2013/07/10 18:48:24, mmenke wrote:
> nit:  For all of these, it's much more common across the chrome code base to
put
> the method on the same name as the class.  I don't think I've ever seen this
> indentation style used in chrome, actually, except perhaps where it was
> necessary due to variable name length.

"On the same line as the object name", rather.

[awong, 2013-07-10 19:01:07]

On 2013/07/10 17:58:43, jam wrote:
> On 2013/07/10 17:54:49, awong wrote:
> > looks not good to me
> > 
> > Thanks for trying to tackle this.  I have some concerns with this CL though.
> > 
> > The CL comments say "Speeds up the main thread initialization."  I'm
assuming
> > "main thread" means UI thread.  However, this change doesn't affect UI
thread
> > behavior at all so I think I'm confused at what the intent is.
> 
> It's a bit subtle to see, but when the main thread is starting the IO thread,
> it's blocked until the IO thread's Init method returns. see the thread
> implementation for details.

Ah, I see. Thread::StartWithOptions(const Options& options) does
startup_data_.Wait() which is only signaled after the
BrowserThreadDelegate::Init() call is done on the new thread.

This invalidates my comment a bit.

However, now I wonder whether we should change this "async" contract at the
BrowserThreadDelegate level.  Specifically, do
  if (delegate) {
    MessageLoop::current()->PostTask(FROM_HERE,
Bind(&BrowserThreadDelegate::Init, delegate));
  }

in content/browser/browser_thread_impl.cc:486.

The current CL is safe because an implicit assumption that the caller knows
exactly the state of the MessageLoop (there's nothing queued yet).  Thus it
seems better to pull the code up one level.

Doing a git grep, it seems like IOThread is the only user of
BrowserThreadDelegate so splatter effect of changing this API semantic would be
minimal.

> > 
> > Please see my comment in IOThread::Init() for details.
> 
> link?

sorry...sent it for real this time though given your previous comment this might
be invalid.
 
> > 
> > If we are indeed seeing this speed up android, and this code actually
doesn't
> > crash like crazy, them I'm guessing it means the IOThread initialization
> itself
> > is too eager...

[mariakhomenko, 2013-07-10 21:28:11]

Albert,

Re: changing the BrowserThreadDelegate contract. It seems like the other
user of BrowserThreadDelegate is AwUrlRequestContextGetter here:
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/br...
.
And there's a comment that it depends on being synchronous for
CookieMonster. Perhaps you can comment on this?

Matt,

I've got the indentation that I did by running clang formatter on the
function that I extracted. I saw a discussion on chromium-dev to the effect
that we want to move
towards uniformly formatted code. I thought clang formatter created
acceptable style, but I can revert that change if you prefer.



On Wed, Jul 10, 2013 at 12:01 PM, <ajwong@chromium.org> wrote:

> On 2013/07/10 17:58:43, jam wrote:
>
>> On 2013/07/10 17:54:49, awong wrote:
>> > looks not good to me
>> >
>> > Thanks for trying to tackle this.  I have some concerns with this CL
>> though.
>> >
>> > The CL comments say "Speeds up the main thread initialization."  I'm
>>
> assuming
>
>> > "main thread" means UI thread.  However, this change doesn't affect UI
>>
> thread
>
>> > behavior at all so I think I'm confused at what the intent is.
>>
>
>  It's a bit subtle to see, but when the main thread is starting the IO
>> thread,
>> it's blocked until the IO thread's Init method returns. see the thread
>> implementation for details.
>>
>
> Ah, I see. Thread::StartWithOptions(const Options& options) does
> startup_data_.Wait() which is only signaled after the
> BrowserThreadDelegate::Init() call is done on the new thread.
>
> This invalidates my comment a bit.
>
> However, now I wonder whether we should change this "async" contract at the
> BrowserThreadDelegate level.  Specifically, do
>   if (delegate) {
>     MessageLoop::current()->**PostTask(FROM_HERE,
> Bind(&BrowserThreadDelegate::**Init, delegate));
>   }
>
> in content/browser/browser_**thread_impl.cc:486.
>
> The current CL is safe because an implicit assumption that the caller knows
> exactly the state of the MessageLoop (there's nothing queued yet).  Thus it
> seems better to pull the code up one level.
>
> Doing a git grep, it seems like IOThread is the only user of
> BrowserThreadDelegate so splatter effect of changing this API semantic
> would be
> minimal.
>
>
>  >
>> > Please see my comment in IOThread::Init() for details.
>>
>
>  link?
>>
>
> sorry...sent it for real this time though given your previous comment this
> might
> be invalid.
>
>
>  >
>> > If we are indeed seeing this speed up android, and this code actually
>>
> doesn't
>
>> > crash like crazy, them I'm guessing it means the IOThread initialization
>> itself
>> > is too eager...
>>
>
>
https://codereview.chromium.**org/18405007/<https://codereview.chromium.org/1...
>

[awong, 2013-07-10 22:02:52]

On 2013/07/10 21:28:11, mariakhomenko_google.com wrote:
> Albert,
> 
> Re: changing the BrowserThreadDelegate contract. It seems like the other
> user of BrowserThreadDelegate is AwUrlRequestContextGetter here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/br...
> .
> And there's a comment that it depends on being synchronous for
> CookieMonster. Perhaps you can comment on this?

Interesting. I missed that call and it makes things a bit harder.

CookieMonster can actually be constructed on the UI thread. Conceivably we could
deserialize this relation by having the UI thread construct the CookieMonster,
maybe in PreMainMessageLoopRun().

Really, I wonder whether this code would be better off just done in
PreMainMessageLoopRun rather than as a BrowserThreadDelegate.  IIRC that's
closer to what ProfileImplIOData. (I don't believe Profile is created as part of
IOThread in desktop chrome).

Thoughts?

 
> Matt,
> 
> I've got the indentation that I did by running clang formatter on the
> function that I extracted. I saw a discussion on chromium-dev to the effect
> that we want to move
> towards uniformly formatted code. I thought clang formatter created
> acceptable style, but I can revert that change if you prefer.
> 
> 
> 
> On Wed, Jul 10, 2013 at 12:01 PM, <mailto:ajwong@chromium.org> wrote:
> 
> > On 2013/07/10 17:58:43, jam wrote:
> >
> >> On 2013/07/10 17:54:49, awong wrote:
> >> > looks not good to me
> >> >
> >> > Thanks for trying to tackle this.  I have some concerns with this CL
> >> though.
> >> >
> >> > The CL comments say "Speeds up the main thread initialization."  I'm
> >>
> > assuming
> >
> >> > "main thread" means UI thread.  However, this change doesn't affect UI
> >>
> > thread
> >
> >> > behavior at all so I think I'm confused at what the intent is.
> >>
> >
> >  It's a bit subtle to see, but when the main thread is starting the IO
> >> thread,
> >> it's blocked until the IO thread's Init method returns. see the thread
> >> implementation for details.
> >>
> >
> > Ah, I see. Thread::StartWithOptions(const Options& options) does
> > startup_data_.Wait() which is only signaled after the
> > BrowserThreadDelegate::Init() call is done on the new thread.
> >
> > This invalidates my comment a bit.
> >
> > However, now I wonder whether we should change this "async" contract at the
> > BrowserThreadDelegate level.  Specifically, do
> >   if (delegate) {
> >     MessageLoop::current()->**PostTask(FROM_HERE,
> > Bind(&BrowserThreadDelegate::**Init, delegate));
> >   }
> >
> > in content/browser/browser_**thread_impl.cc:486.
> >
> > The current CL is safe because an implicit assumption that the caller knows
> > exactly the state of the MessageLoop (there's nothing queued yet).  Thus it
> > seems better to pull the code up one level.
> >
> > Doing a git grep, it seems like IOThread is the only user of
> > BrowserThreadDelegate so splatter effect of changing this API semantic
> > would be
> > minimal.
> >
> >
> >  >
> >> > Please see my comment in IOThread::Init() for details.
> >>
> >
> >  link?
> >>
> >
> > sorry...sent it for real this time though given your previous comment this
> > might
> > be invalid.
> >
> >
> >  >
> >> > If we are indeed seeing this speed up android, and this code actually
> >>
> > doesn't
> >
> >> > crash like crazy, them I'm guessing it means the IOThread initialization
> >> itself
> >> > is too eager...
> >>
> >
> >
>
https://codereview.chromium.**org/18405007/%3Chttps://codereview.chromium.org...>
> >

[mmenke, 2013-07-11 14:38:41]

On 2013/07/10 21:28:11, mariakhomenko_google.com wrote:
> Albert,
> 
> Re: changing the BrowserThreadDelegate contract. It seems like the other
> user of BrowserThreadDelegate is AwUrlRequestContextGetter here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/br...
> .
> And there's a comment that it depends on being synchronous for
> CookieMonster. Perhaps you can comment on this?
> 
> Matt,
> 
> I've got the indentation that I did by running clang formatter on the
> function that I extracted. I saw a discussion on chromium-dev to the effect
> that we want to move
> towards uniformly formatted code. I thought clang formatter created
> acceptable style, but I can revert that change if you prefer.

I think this line breaking is sufficiently different from the rest of the code
base that it's worth manually fixing.