Reland: Lazily initialize MessageLoop for faster thread startup

Reland: Lazily initialize MessageLoop for faster thread startup

Original review: https://codereview.chromium.org/1011683002/

Reverted because it's suspected for following flakiness issues:
http://crbug.com/485157 - Windows race
http://crbug.com/485091 - Android ThreadWatcher
http://crbug.com/485178 - interactive_ui_tests Menu* tests

PS1 is the original patch set that gets reverted.

BUG=465458, 485157, 485091, 485178
TBR=jam

Committed: https://crrev.com/8b6133a69f16702a32a3c3104630c4d9ac393b7a
Cr-Commit-Position: refs/heads/master@{#330329}

[kinuko, 2015-05-07 14:31:26]

Patchset #1 (id:1) has been deleted

[Takashi Toyoshima, 2015-05-13 09:16:21]

toyoshim@chromium.org changed reviewers:
+ toyoshim@chromium.org

[Takashi Toyoshima, 2015-05-13 09:16:21]

I just look this incidentally.

https://codereview.chromium.org/1129953004/diff/40001/base/threading/platform...
File base/threading/platform_thread_win.cc (right):

https://codereview.chromium.org/1129953004/diff/40001/base/threading/platform...
base/threading/platform_thread_win.cc:280: out_thread_handle->handle_ =
CreateThread(
Windows does not guarantee CreateThread() returns before starting the new
thread, and even passing the out_thread_handle->id_ directly, it may not be set
before the new thread starts.

Using CREATE_SUSPENDED flag, then calling ResumeThread after the
PlatformThreadHandle initialized will work, but it may affect performance? I
didn't check the impact yet.

https://codereview.chromium.org/1129953004/diff/40001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1129953004/diff/40001/base/threading/thread.c...
base/threading/thread.cc:113: bool result = Start();
if (!Start())
  return false;
return WaitUntilThreadStarted();

will be simpler and easy to read?

[kinuko, 2015-05-13 09:24:19]

https://codereview.chromium.org/1129953004/diff/40001/base/threading/platform...
File base/threading/platform_thread_win.cc (right):

https://codereview.chromium.org/1129953004/diff/40001/base/threading/platform...
base/threading/platform_thread_win.cc:280: out_thread_handle->handle_ =
CreateThread(
On 2015/05/13 09:16:21, Takashi Toyoshima (chromium) wrote:
> Windows does not guarantee CreateThread() returns before starting the new
> thread, and even passing the out_thread_handle->id_ directly, it may not be
set
> before the new thread starts.

Yup, I'm aware of that. (This patch is not ready yet)

> Using CREATE_SUSPENDED flag, then calling ResumeThread after the
> PlatformThreadHandle initialized will work, but it may affect performance? I
> didn't check the impact yet.

I am/was thinking about using lock to enforce happens-before relationship, which
would effectively do same as create_suspended. I think that shouldn't spare
performance but will need some measurement.

[Takashi Toyoshima, 2015-05-13 09:36:25]

Yeah, and the second solution will also work for pthread_t :)

[kinuko, 2015-05-13 14:38:54]

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

[commit-bot: I haz the power, 2015-05-13 14:39:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129953004/80001

[kinuko, 2015-05-13 14:56:27]

The CQ bit was unchecked by kinuko@chromium.org

[kinuko, 2015-05-14 03:34:27]

kinuko@chromium.org changed reviewers:
+ thakis@chromium.org
- toyoshim@chromium.org

[kinuko, 2015-05-14 03:40:22]

kinuko@chromium.org changed reviewers:
- thakis@chromium.org

[kinuko, 2015-05-14 03:41:09]

(Deleting old patch sets PS1 and PS2)

[kinuko, 2015-05-14 03:41:21]

Patchset #1 (id:20001) has been deleted

[kinuko, 2015-05-14 03:41:32]

Patchset #1 (id:40001) has been deleted

[kinuko, 2015-05-14 03:42:22]

kinuko@chromium.org changed reviewers:
+ thakis@chromium.org

[kinuko, 2015-05-14 03:44:31]

Nico: this is a reland attempt for non-blocking thread startup patch, could you
take a look?
(Esp. diff for thread.{cc,h} between PS1 and PS2)

For some flakiness issues I can't repro (either on local or on try), I made some
plausible fixes but there may still be problems. I'm feeling like landing this
after branch cut.

[Nico, 2015-05-14 04:02:22]

lgtm if the answer to my second comment is "yes"

https://codereview.chromium.org/1129953004/diff/80001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1129953004/diff/80001/base/threading/thread.c...
base/threading/thread.cc:224: {
maybe put this in an

#if DCHECK_IS_ON()
#endif

block so that the new thread isn't blocked on its parent in release builds

https://codereview.chromium.org/1129953004/diff/100001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1129953004/diff/100001/base/threading/thread....
base/threading/thread.cc:121: return WaitUntilThreadStarted();
This is now just `return Start() && WaitUntilThreadStarted();`, right?

The only difference to before is that this now returns false if
WaitUntilThreadStarted() returns false, which only happens if start_event_ isn't
set, which right after Start() only happens if PlatformThread::Create() fails.
Does PlatformThread::Create() fail while the test suite runs?

[Takashi Toyoshima, 2015-05-14 04:21:49]

toyoshim@chromium.org changed reviewers:
+ toyoshim@chromium.org

[Takashi Toyoshima, 2015-05-14 04:21:50]

https://codereview.chromium.org/1129953004/diff/100001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1129953004/diff/100001/base/threading/thread....
base/threading/thread.cc:121: return WaitUntilThreadStarted();
Nico: You may look the difference between 2 and 3. Essential change looks
existing between 1 and 2 in StartWithOptions.

[Nico, 2015-05-14 04:33:31]

On 2015/05/14 04:21:50, Takashi Toyoshima (chromium) wrote:
>
https://codereview.chromium.org/1129953004/diff/100001/base/threading/thread.cc
> File base/threading/thread.cc (right):
> 
>
https://codereview.chromium.org/1129953004/diff/100001/base/threading/thread....
> base/threading/thread.cc:121: return WaitUntilThreadStarted();
> Nico: You may look the difference between 2 and 3. Essential change looks
> existing between 1 and 2 in StartWithOptions.

The change from 1 to 2 makes sense to me. But 2 to 3 changed this return, and I
was wondering why.

[kinuko, 2015-05-14 08:11:41]

Patchset #4 (id:120001) has been deleted

[kinuko, 2015-05-14 08:16:34]

Thanks, updated a bit. (I won't land this until next week so any of my comments
below sound suspicious please take another look)

https://codereview.chromium.org/1129953004/diff/80001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1129953004/diff/80001/base/threading/thread.c...
base/threading/thread.cc:224: {
On 2015/05/14 04:02:22, Nico wrote:
> maybe put this in an
> 
> #if DCHECK_IS_ON()
> #endif
> 
> block so that the new thread isn't blocked on its parent in release builds

Hmm. I want to make sure calling thread_id() is always safe on the new thread
too. But it doesn't necessarily happen here... moved this lock into thread_id()
instead (so it won't block until it's really necessary).  thread_id() doesn't
look very popular method so I assume it'll be fine.

https://codereview.chromium.org/1129953004/diff/100001/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/1129953004/diff/100001/base/threading/thread....
base/threading/thread.cc:121: return WaitUntilThreadStarted();
On 2015/05/14 04:02:22, Nico wrote:
> This is now just `return Start() && WaitUntilThreadStarted();`, right?
> 
> The only difference to before is that this now returns false if
> WaitUntilThreadStarted() returns false, which only happens if start_event_
isn't
> set, which right after Start() only happens if PlatformThread::Create() fails.
> Does PlatformThread::Create() fail while the test suite runs?

The change isn't really meant to change any behavior, just tried to shorten one
line.  Looking into this more WaitUntilThreadStarted() should always return true
here so changing this return may be actually a bit confusing. I reverted this.

[kinuko, 2015-05-15 04:31:46]

kinuko@chromium.org changed reviewers:
+ jam@chromium.org

[kinuko, 2015-05-18 04:58:16]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2015-05-18 04:58:17]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1129953004/#ps160001
(title: "build fix..")

[commit-bot: I haz the power, 2015-05-18 04:58:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129953004/160001

[kinuko, 2015-05-18 04:58:58]

On 2015/05/18 04:58:26, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1129953004/160001

(TBR-ing jam@ for content/ and net/ stuff for re-landing, no changes in those
dirs)

[commit-bot: I haz the power, 2015-05-18 06:10:06]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2015-05-18 11:33:43]

Patchset 5 (id:??) landed as
https://crrev.com/8b6133a69f16702a32a3c3104630c4d9ac393b7a
Cr-Commit-Position: refs/heads/master@{#330329}

[Alexander Potapenko, 2015-05-18 13:21:42]

Hi Kinuko,

I believe this patch has caused massive race reports on Linux:
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20TSan%20Tests...

I'm about to revert it.
Filed http://crbug.com/489263

[Alexander Potapenko, 2015-05-18 13:23:18]

A revert of this CL (patchset #5 id:160001) has been created in
https://codereview.chromium.org/1140363002/ by glider@chromium.org.

The reason for reverting is: Massive data race reports, see
https://crbug.com/489263.

[kinuko, 2015-05-18 13:29:32]

On 2015/05/18 13:23:18, Alexander Potapenko wrote:
> A revert of this CL (patchset #5 id:160001) has been created in
> https://codereview.chromium.org/1140363002/ by
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=glider@chromium.org.
> 
> The reason for reverting is: Massive data race reports, see
> https://crbug.com/489263.

:( thanks!

[kinuko, 2015-05-19 14:27:18]

Patchset #6 (id:180001) has been deleted