PlatformThreadHandle: remove public id() interface

PlatformThreadHandle: remove public id() interface

Remove PlatformThreadHandle.id() interface.
Also remove id_ member that is not needed any more.

BUG=468793
TEST=./out/Debug/base_unittests
TEST=git cl try

Committed: https://crrev.com/c41261e9491c3a763c472fe35737ec36200d6310
Cr-Commit-Position: refs/heads/master@{#340242}

[Takashi Toyoshima, 2015-06-26 12:43:37]

Patchset #3 (id:40001) has been deleted

[Takashi Toyoshima, 2015-06-29 16:23:31]

Patchset #5 (id:100001) has been deleted

[Takashi Toyoshima, 2015-06-30 03:37:24]

Patchset #1 (id:1) has been deleted

[Takashi Toyoshima, 2015-06-30 03:37:31]

Patchset #1 (id:20001) has been deleted

[Takashi Toyoshima, 2015-06-30 03:37:39]

Patchset #1 (id:60001) has been deleted

[Takashi Toyoshima, 2015-06-30 03:37:46]

Patchset #1 (id:80001) has been deleted

[Takashi Toyoshima, 2015-06-30 03:41:05]

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

[Takashi Toyoshima, 2015-06-30 03:42:39]

gab@, Can you take a look? This is a CL to remove id() from PlatformThread that
we discussed in a previous review.
Maybe I'd split video_renderer_impl.cc from this CL.

[Takashi Toyoshima, 2015-06-30 11:27:19]

video_renderer_impl.cc was fixed in a separate CL,
https://codereview.chromium.org/1217943003/ and already landed :)

[gab, 2015-06-30 13:57:35]

https://codereview.chromium.org/1207823004/diff/140001/base/threading/platfor...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/1207823004/diff/140001/base/threading/platfor...
base/threading/platform_thread.h:91: friend class ThreadIdNameManager;
I don't like using friend and I don't think I mind that the const
platform_handle() getter be usable outside of these classes (it's not really
useful there anyways).

Can we initially focus on getting rid of only id() and perhaps think about this
use case independently after if the exposure of the public const call is really
an issue?

[Takashi Toyoshima, 2015-07-02 06:24:23]

Patchset #3 (id:150011) has been deleted

[Takashi Toyoshima, 2015-07-02 06:24:32]

Patchset #3 (id:170001) has been deleted

[Takashi Toyoshima, 2015-07-02 06:34:28]

PTAL Patch Set 4.

https://codereview.chromium.org/1207823004/diff/140001/base/threading/platfor...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/1207823004/diff/140001/base/threading/platfor...
base/threading/platform_thread.h:91: friend class ThreadIdNameManager;
OK, let me upload another patch set.

[gab, 2015-07-02 12:00:52]

https://codereview.chromium.org/1207823004/diff/210001/base/threading/platfor...
File base/threading/platform_thread_posix.cc (left):

https://codereview.chromium.org/1207823004/diff/210001/base/threading/platfor...
base/threading/platform_thread_posix.cc:67: PlatformThread::CurrentId());
Need to fix platform_thread_win.cc similarly.

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

https://codereview.chromium.org/1207823004/diff/210001/base/threading/thread....
base/threading/thread.cc:211: id_ = PlatformThread::CurrentId();
Need to lock to make sure this change is flushed to other threads I think (i.e.
this is not like a constructor on one thread which guarantees the initial value
is visible to all threads).

https://codereview.chromium.org/1207823004/diff/210001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1207823004/diff/210001/base/threading/thread....
base/threading/thread.h:173: // The thread ID.
// The thread ID. Only valid after this Thread was launched.

https://codereview.chromium.org/1207823004/diff/210001/base/threading/thread....
base/threading/thread.h:224: // The thread's id.
// The thread's id once it was started.

[Takashi Toyoshima, 2015-07-02 13:54:58]

https://codereview.chromium.org/1207823004/diff/210001/base/threading/platfor...
File base/threading/platform_thread_posix.cc (left):

https://codereview.chromium.org/1207823004/diff/210001/base/threading/platfor...
base/threading/platform_thread_posix.cc:67: PlatformThread::CurrentId());
I may misunderstand your comment, but I think I already applied a right fix for
Win32.
Note that caller thread blocks until this PlatformThreadHandle is set and
following handle_set is signaled at the next line. I will remove this
restriction in the next CL https://codereview.chromium.org/1150563002/

[gab, 2015-07-02 14:32:34]

https://codereview.chromium.org/1207823004/diff/210001/base/threading/platfor...
File base/threading/platform_thread_posix.cc (left):

https://codereview.chromium.org/1207823004/diff/210001/base/threading/platfor...
base/threading/platform_thread_posix.cc:67: PlatformThread::CurrentId());
On 2015/07/02 13:54:58, Takashi Toyoshima (chromium) wrote:
> I may misunderstand your comment, but I think I already applied a right fix
for
> Win32.
> Note that caller thread blocks until this PlatformThreadHandle is set and
> following handle_set is signaled at the next line. I will remove this
> restriction in the next CL https://codereview.chromium.org/1150563002/

Oh oops, no that's not what I meant, but I'd somehow missed the change to
platform_thread_win.cc in this CL (and that is what I thought was missing...).

Are there tests for Thread::id()? To confirm this change doesn't break anything
cross-platform (I don't see anything in thread_unittest.cc)

[Takashi Toyoshima, 2015-07-03 04:22:56]

Added a test and defensive CHECK in thread_id(). Let me see the try results.

https://codereview.chromium.org/1207823004/diff/210001/base/threading/platfor...
File base/threading/platform_thread_posix.cc (left):

https://codereview.chromium.org/1207823004/diff/210001/base/threading/platfor...
base/threading/platform_thread_posix.cc:67: PlatformThread::CurrentId());
ThreadIdNameManagerTest seems to depend on base::Thread.thread_id(), but it
would be better to have a simple test in thread_unittest.cc. I'd try adding some
tests.

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

https://codereview.chromium.org/1207823004/diff/210001/base/threading/thread....
base/threading/thread.cc:211: id_ = PlatformThread::CurrentId();
On 2015/07/02 12:00:52, gab (OOO) wrote:
> Need to lock to make sure this change is flushed to other threads I think
(i.e.
> this is not like a constructor on one thread which guarantees the initial
value
> is visible to all threads).

Done.

https://codereview.chromium.org/1207823004/diff/210001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1207823004/diff/210001/base/threading/thread....
base/threading/thread.h:173: // The thread ID.
Ah, that's the thing I should explain here.
By the way, I think we use "start" here and there instead of "launch", so if you
do not have a special meaning on "launch", I prefer "start". WDYT?

https://codereview.chromium.org/1207823004/diff/210001/base/threading/thread....
base/threading/thread.h:224: // The thread's id.
On 2015/07/02 12:00:52, gab (OOO) wrote:
> // The thread's id once it was started.

Done.

https://codereview.chromium.org/1207823004/diff/210001/base/threading/thread....
base/threading/thread.h:227: mutable base::Lock thread_lock_;  // Protects
|thread_| and |id_|.
Oops, I only updated the comment, but did nothing for protecting id_ :(

[gab, 2015-07-08 18:35:21]

Looks like some kind of rebase is required, the diff includes unexpected
changes.

https://codereview.chromium.org/1207823004/diff/210001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1207823004/diff/210001/base/threading/thread....
base/threading/thread.h:173: // The thread ID.
On 2015/07/03 04:22:56, Takashi Toyoshima (chromium) wrote:
> Ah, that's the thing I should explain here.
> By the way, I think we use "start" here and there instead of "launch", so if
you
> do not have a special meaning on "launch", I prefer "start". WDYT?

"Started" SGTM.

https://codereview.chromium.org/1207823004/diff/230001/base/threading/platfor...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/1207823004/diff/230001/base/threading/platfor...
base/threading/platform_thread.h:183: static void
SetCurrentThreadPriority(ThreadPriority priority);
I'm confused, why has the change from SetThreadPriority() to
SetCurrentThreadPriority() leaked into this CL's diff? Wrong upstream for
upload?

[Takashi Toyoshima, 2015-07-09 03:39:22]

https://codereview.chromium.org/1207823004/diff/230001/base/threading/platfor...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/1207823004/diff/230001/base/threading/platfor...
base/threading/platform_thread.h:183: static void
SetCurrentThreadPriority(ThreadPriority priority);
This patch set 5 contains the CL for SetCurrentThreadPriority in order to run
try. Patch set 6 is the diff from the previous CL.

[gab, 2015-07-09 18:10:19]

On 2015/07/09 03:39:22, Takashi Toyoshima (chromium) wrote:
>
https://codereview.chromium.org/1207823004/diff/230001/base/threading/platfor...
> File base/threading/platform_thread.h (right):
> 
>
https://codereview.chromium.org/1207823004/diff/230001/base/threading/platfor...
> base/threading/platform_thread.h:183: static void
> SetCurrentThreadPriority(ThreadPriority priority);
> This patch set 5 contains the CL for SetCurrentThreadPriority in order to run
> try. Patch set 6 is the diff from the previous CL.

Ah I see, in the past I'd used "git try" for this which only cares about your
local upstream not the uploaded one like "git cl try" does, you could do "git cl
upload && git br -u master && git try && git br -u real_upstream" and it would
send a try job with the diff from master but still attach the results to the
right CL which only has the incremental diff uploaded.

Not sure if this works anymore... otherwise perhaps use a separate issue to
upload the full diff + tries (toggling back and forth with "git cl issue
1207823004"), as-is this is really confusing for reviewers.

Anyways, looking.

[gab, 2015-07-09 18:19:00]

lg, a few comments below

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

https://codereview.chromium.org/1207823004/diff/250001/base/threading/thread....
base/threading/thread.cc:181: CHECK_NE(kInvalidThreadId, id_);
Is this really a security issue? i.e. is it worth forcing a crash right here in
official builds?

Ref:
https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...

https://codereview.chromium.org/1207823004/diff/250001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1207823004/diff/250001/base/threading/thread....
base/threading/thread.h:227: mutable base::Lock id_lock_;  // Protects |id_|.
I'm not convinced there is enough contention to justify multiple locks, one lock
for this class is fine I think.

https://codereview.chromium.org/1207823004/diff/250001/base/trace_event/trace...
File base/trace_event/trace_event_unittest.cc (right):

https://codereview.chromium.org/1207823004/diff/250001/base/trace_event/trace...
base/trace_event/trace_event_unittest.cc:1550:
threads[i]->StartAndWaitForTesting();
Why is this related to this CL?

[Takashi Toyoshima, 2015-07-14 08:05:53]

toyoshim@chromium.org changed reviewers:
+ thakis@chromium.org, thestig@chromium.org

[Takashi Toyoshima, 2015-07-14 08:05:54]

+thestig@, thakis@ since they review a CL on which this change depends.

[Takashi Toyoshima, 2015-07-14 10:47:29]

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

https://codereview.chromium.org/1207823004/diff/250001/base/threading/thread....
base/threading/thread.cc:181: CHECK_NE(kInvalidThreadId, id_);
I see, DCHECK_NE looks better here.

https://codereview.chromium.org/1207823004/diff/250001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1207823004/diff/250001/base/threading/thread....
base/threading/thread.h:227: mutable base::Lock id_lock_;  // Protects |id_|.
On 2015/07/09 18:19:00, gab wrote:
> I'm not convinced there is enough contention to justify multiple locks, one
lock
> for this class is fine I think.

Done.

https://codereview.chromium.org/1207823004/diff/250001/base/trace_event/trace...
File base/trace_event/trace_event_unittest.cc (right):

https://codereview.chromium.org/1207823004/diff/250001/base/trace_event/trace...
base/trace_event/trace_event_unittest.cc:1550:
threads[i]->StartAndWaitForTesting();
This is because now thread_id() can return a valid value only after the thread
actually starts running.

[Takashi Toyoshima, 2015-07-14 14:08:27]

Hum..., some tests look broken somewhere after the patch set 3.
I'll ping again once these problems are fixed.

[kinuko, 2015-07-16 00:50:36]

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

[kinuko, 2015-07-16 00:50:37]

Drive-by.

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

https://codereview.chromium.org/1207823004/diff/330001/base/threading/thread....
base/threading/thread.cc:173: DCHECK_NE(kInvalidThreadId, id_);
Assume that we could fix all the failing tests, how we could make sure this
change doesn't introduce flaky failure on production code?

https://codereview.chromium.org/1207823004/diff/330001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1207823004/diff/330001/base/threading/thread....
base/threading/thread.h:228: // Protects |running_|, |thread_|, and |id_|.
nit: This no longer protects thread_?

nit: Unless the protected fields have close relationships or we have very severe
memory limit I prefer using separate locks for each field.

[Takashi Toyoshima, 2015-07-16 05:07:31]

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

https://codereview.chromium.org/1207823004/diff/330001/base/threading/thread....
base/threading/thread.cc:173: DCHECK_NE(kInvalidThreadId, id_);
Thanks. I'm doing trial and error here
https://codereview.chromium.org/1239623003/
This was one big reason of test failures, and I will change this to call
WaitUntilThreadStarted() here.
Once I get a good try result, I'll update patch set here.

https://codereview.chromium.org/1207823004/diff/330001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1207823004/diff/330001/base/threading/thread....
base/threading/thread.h:228: // Protects |running_|, |thread_|, and |id_|.
Ah, good catch. It's true from the last patch set.

I changed to share one lock because gab@ suggested in the previous review, but
I'd follow kinuko's suggestion in the next patch set unless base/ owner object
it. Since kinuko is a content/ owner that is one of the most authoritative part
in chrome.

[kinuko, 2015-07-16 06:01:05]

On 2015/07/16 05:07:31, Takashi Toyoshima (chromium) wrote:
>
https://codereview.chromium.org/1207823004/diff/330001/base/threading/thread.cc
> File base/threading/thread.cc (right):
> 
>
https://codereview.chromium.org/1207823004/diff/330001/base/threading/thread....
> base/threading/thread.cc:173: DCHECK_NE(kInvalidThreadId, id_);
> Thanks. I'm doing trial and error here
> https://codereview.chromium.org/1239623003/
> This was one big reason of test failures, and I will change this to call
> WaitUntilThreadStarted() here.
> Once I get a good try result, I'll update patch set here.
> 
> https://codereview.chromium.org/1207823004/diff/330001/base/threading/thread.h
> File base/threading/thread.h (right):
> 
>
https://codereview.chromium.org/1207823004/diff/330001/base/threading/thread....
> base/threading/thread.h:228: // Protects |running_|, |thread_|, and |id_|.
> Ah, good catch. It's true from the last patch set.
> 
> I changed to share one lock because gab@ suggested in the previous review, but
> I'd follow kinuko's suggestion in the next patch set unless base/ owner object
> it. Since kinuko is a content/ owner that is one of the most authoritative
part
> in chrome.

Oh, it's not about authority, please follow base/ owners' comments and use your
judgement, if there's a good reason I'm fine with either code :)

[Takashi Toyoshima, 2015-07-16 08:12:38]

PTAL Patch Set 11.
I find another problem of my CL. Thread seems to support multiple Start() and
Stop(), and sandbox_linux_unittests relies on it. thread_unittest also contains
a test calling Start() and Stop() multiple times, so I'd continue to support it.

https://codereview.chromium.org/1207823004/diff/330001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1207823004/diff/330001/base/threading/thread....
base/threading/thread.h:228: // Protects |running_|, |thread_|, and |id_|.
So one reason to use a separate lock for id_ is that thread_id() is called from
many places for DCHECK and so on. So, sharing lock can easily introduce
recursive lock on changing this class.

[Takashi Toyoshima, 2015-07-16 10:55:57]

undo PTAL :(

[gab, 2015-07-16 17:19:34]

Seems this CL is now doing a lot more than removing id() from the interface.
Please strip out other changes to other CLs (i.e. this CL should only have to
be: remove the field/getter/constructor and fix users of those to get it some
other way).

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

https://codereview.chromium.org/1207823004/diff/370001/base/threading/thread....
base/threading/thread.cc:163: DCHECK_NE(thread_id(),
PlatformThread::CurrentId());
Why move this? DCHECKs enforcing the method's contract typically go at the top.

https://codereview.chromium.org/1207823004/diff/370001/base/threading/thread....
base/threading/thread.cc:234: id_ = PlatformThread::CurrentId();
You still need a lock to protect this even if all the reads are after the only
write.

Well, you don't actually need a "lock", you need a "memory barrier" to force
other threads to see the new value (even if they never read it explicitly they
could have cached a page that contained it) -- and a "lock" is the simplest
version of such a barrier in our codebase (plus in non-contentious scenarios,
which you are creating here, there is pretty much zero overhead to a lock).

[gab, 2015-07-16 17:28:24]

https://codereview.chromium.org/1207823004/diff/250001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1207823004/diff/250001/base/threading/thread....
base/threading/thread.h:227: mutable base::Lock id_lock_;  // Protects |id_|.
On 2015/07/14 10:47:28, Takashi Toyoshima (chromium) wrote:
> On 2015/07/09 18:19:00, gab wrote:
> > I'm not convinced there is enough contention to justify multiple locks, one
> lock
> > for this class is fine I think.
> 
> Done.

To be clear, I didn't mean to say lock-less, just to use a single lock for all
lock operations.

[Takashi Toyoshima, 2015-07-16 19:51:11]

Hum... I do not think this contains unrelated changes, and everything are needed
to make tests pass without race or deadlock. Anyway, this still needs some
modifications to pass all tests.

Anyway, strategy could be changed to solve remaining problems. So please do not
review this patch set for now.

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

https://codereview.chromium.org/1207823004/diff/370001/base/threading/thread....
base/threading/thread.cc:163: DCHECK_NE(thread_id(),
PlatformThread::CurrentId());
Because without this fix, this assertion fires in a test that calls Start() and
Stop() repeatedly. But I have another right fix in the newer patch set, and this
is already reverted.

https://codereview.chromium.org/1207823004/diff/370001/base/threading/thread....
base/threading/thread.cc:234: id_ = PlatformThread::CurrentId();
Yeah, I understand memory consistency. As I commented in the header,
start_event_->Signal() implicitly performs membar operation. So this should
work. But the problem here is the original code does not protect start_event_.
scoped_ptr<WaitableEvent> must need a lock to access the internal pointer, but
we set and reset the pointer on multiple threads without any lock.

The reason why I use a event instead of lock is simple. id_ could not be
available until the created thread actually starts running. So reading id_ needs
to wait for the start_event_ signaled anyway.

[Takashi Toyoshima, 2015-07-17 13:20:22]

PTAL Patch Set 14.

https://codereview.chromium.org/1207823004/diff/410001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1207823004/diff/410001/base/threading/thread....
base/threading/thread.h:174: // call.  Keeps on returning the same ID even after
a Stop() call. The next
Note: many tests rely on this behavior to hold the thread ID even after Stop().

https://codereview.chromium.org/1207823004/diff/410001/base/threading/thread....
base/threading/thread.h:244: scoped_ptr<WaitableEvent> start_event_;
start_event_ should not be scoped_ptr too, but I do not fix this in this patch
set.

[gab, 2015-07-17 20:12:25]

lgtm but I think a Lock would be simpler than a WaitableEvent (which itself has
a Lock and more).

[Takashi Toyoshima, 2015-07-18 07:06:51]

We can not use a Lock here because the lock period starts and ends on different
threads. WaitableEvent is exactly designed to use in such cases.

[Takashi Toyoshima, 2015-07-18 07:11:25]

Lei, can you take a look for OWNERS approval?

[gab, 2015-07-19 23:39:32]

On 2015/07/18 07:06:51, Takashi Toyoshima (chromium) wrote:
> We can not use a Lock here because the lock period starts and ends on
different
> threads. WaitableEvent is exactly designed to use in such cases.

Ah I see, given you want to make thread_id() callable whether the thread is
already started or not.

But then I'm not a fan of this ScopedAllowWait. Maybe we should have a distinct
API for people that truly want to wait rather than potentially get an invalid
ID? Otherwise we risk some people making starting a thread janky by
re-introducing the synchronous wait unintentionally.

[Takashi Toyoshima, 2015-07-20 13:37:46]

For now, posix implementation blocks in Start*() until the newly created thread
starts running. So, practically we never block in thread_id() now.
With my next patch https://chromiumcodereview.appspot.com/1150563002/, I will
remove the blocking code and ScopedAllowWait from Start*(). Until then,
thread_id()'s wait could not be a problem. Anyway, we can remove jankiness on
thread creation in most cases with this and next patches.

I do not want to introduce maybe_thread_id() like method until it turns out that
this wait call is really performance sensitive and maybe_thread_id() can solve
the issue. I will introduce measurement code in the next patch.

[Lei Zhang, 2015-07-20 21:27:32]

Looking good.

Can you add a unit test that reads thread_id() from a different thread? So TSAN
will find races if |id_| isn't correctly protected.

https://codereview.chromium.org/1207823004/diff/410001/base/threading/platfor...
File base/threading/platform_thread_win.cc (right):

https://codereview.chromium.org/1207823004/diff/410001/base/threading/platfor...
base/threading/platform_thread_win.cc:117: PlatformThreadId thread_id;
No longer used on line 126, so pass a nullptr to ::CreateThread() ?

https://codereview.chromium.org/1207823004/diff/410001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1207823004/diff/410001/base/threading/thread....
base/threading/thread.h:227: // The thread's id once it was started.
was -> has

https://codereview.chromium.org/1207823004/diff/410001/base/threading/thread....
base/threading/thread.h:229: 
nit: no line separator to better indicate relationship with |id_event_|, and for
consistency with the locks aboves.

[Takashi Toyoshima, 2015-07-21 16:05:37]

PTAL Patch Set 16.

https://chromiumcodereview.appspot.com/1207823004/diff/410001/base/threading/...
File base/threading/platform_thread_win.cc (right):

https://chromiumcodereview.appspot.com/1207823004/diff/410001/base/threading/...
base/threading/platform_thread_win.cc:117: PlatformThreadId thread_id;
On 2015/07/20 21:27:32, Lei Zhang wrote:
> No longer used on line 126, so pass a nullptr to ::CreateThread() ?

Done.

https://chromiumcodereview.appspot.com/1207823004/diff/410001/base/threading/...
File base/threading/thread.h (right):

https://chromiumcodereview.appspot.com/1207823004/diff/410001/base/threading/...
base/threading/thread.h:227: // The thread's id once it was started.
On 2015/07/20 21:27:32, Lei Zhang wrote:
> was -> has

Done.

https://chromiumcodereview.appspot.com/1207823004/diff/410001/base/threading/...
base/threading/thread.h:229: 
On 2015/07/20 21:27:32, Lei Zhang wrote:
> nit: no line separator to better indicate relationship with |id_event_|, and
for
> consistency with the locks aboves.

Done.

[gab, 2015-07-21 16:06:31]

On 2015/07/20 13:37:46, Takashi Toyoshima (chromium) wrote:
> For now, posix implementation blocks in Start*() until the newly created
thread
> starts running. So, practically we never block in thread_id() now.
> With my next patch https://chromiumcodereview.appspot.com/1150563002/, I will
> remove the blocking code and ScopedAllowWait from Start*(). Until then,
> thread_id()'s wait could not be a problem. Anyway, we can remove jankiness on
> thread creation in most cases with this and next patches.
> 
> I do not want to introduce maybe_thread_id() like method until it turns out
that
> this wait call is really performance sensitive and maybe_thread_id() can solve
> the issue. I will introduce measurement code in the next patch.

I think this should DCHECK that waiting is allowed and the caller should then
make the explicit decision to ScopedAllowWait where it makes sense. I don't like
the hidden wait (i.e. some callers may not call this if they knew it could
hang).

[Takashi Toyoshima, 2015-07-21 17:42:02]

Here is a simple example.

1: Thread thread;
2: thread.Start();
3: // some other tasks
4: thread.thread_id();

For now, the code blocks at line 2 until the created thread starts running. This
behavior is not changed even after this patch.
Once my next patch is submitted, the code does not block at line 2, and blocks
at line 4 until the created thread starts running.
There are no performance regression here, and practically the thread started
while running thread.Start() and some other tasks before reaching to thread_id()
at line 4. So, this hidden wait does not matter typically. Otherwise, it should
matter NOW because thread.Start() internally blocks in all cases.

[gab, 2015-07-21 17:55:27]

On 2015/07/21 17:42:02, Takashi Toyoshima (chromium) wrote:
> Here is a simple example.
> 
> 1: Thread thread;
> 2: thread.Start();
> 3: // some other tasks
> 4: thread.thread_id();
> 
> For now, the code blocks at line 2 until the created thread starts running.
This
> behavior is not changed even after this patch.
> Once my next patch is submitted, the code does not block at line 2, and blocks
> at line 4 until the created thread starts running.
> There are no performance regression here, and practically the thread started
> while running thread.Start() and some other tasks before reaching to
thread_id()
> at line 4. So, this hidden wait does not matter typically. Otherwise, it
should
> matter NOW because thread.Start() internally blocks in all cases.

It doesn't matter now indeed because we block on Start() anyways, *but* my point
is that your upcoming improvement is about making Start() NOT block and that it
would be very unfortunate if callers were not getting the improvement just
because they query the id(), not knowing that this may block.

I think it's preferable to force callers to declare that they're okay with
waiting rather than implicitly force callers into a hidden wait.

[Takashi Toyoshima, 2015-07-21 18:15:06]

Is there any reason to think this case so seriously? What is different from
mutex lock here?

[Takashi Toyoshima, 2015-07-21 18:17:00]

I prefer to be realist rather than idealist here, but I will follow base owner's
suggestion here.

[gab, 2015-07-21 19:01:55]

On 2015/07/21 18:15:06, Takashi Toyoshima (chromium) wrote:
> Is there any reason to think this case so seriously? What is different from
> mutex lock here?

A mutex lock only ensures there is no race on the read/write, a wait actually
blocks until the other thread starts (which defeats the purpose of attempting to
make Start() non-blocking IMO).

I could imagine a caller that does:
thread.Start()
id_ = thread.thread_id();

Just wanting to cache the |id_| for whatever reason for later, not realizing
that this actually results in blocking, at least if you force the caller to
specify ScopedAllowWait instead of implicitly specifying it for them, then they
have to decide whether blocking is okay for them.

Doing the ScopedAllowWait in the base/ API's impl would be like adding a
ScopedAllowIO on ReadFileToString IMO. Callers should dictate what they're okay
with, the API shouldn't allow it for them.

Lei WDYT?

[Lei Zhang, 2015-07-22 01:37:51]

This is just my opinion. Feel free to disagree. :)

On 2015/07/21 19:01:55, gab wrote:
> I could imagine a caller that does:
> thread.Start()
> id_ = thread.thread_id();
> 
> Just wanting to cache the |id_| for whatever reason for later, not realizing
> that this actually results in blocking, at least if you force the caller to
> specify ScopedAllowWait instead of implicitly specifying it for them, then
they
> have to decide whether blocking is okay for them.

How often is this going to happen though? As is, thread_id() is never used in
this way. Would documenting this potential issue in the header comments and
renaming thread_id() to GetThreadId() address your concern?

> Doing the ScopedAllowWait in the base/ API's impl would be like adding a
> ScopedAllowIO on ReadFileToString IMO. Callers should dictate what they're
okay
> with, the API shouldn't allow it for them.

The difference is ReadFileToString() will most likely be blocking, whereas
thread_id() is currently never being called right as a thread is starting in
non-test code.

Take ReadProcStatusAndGetFieldAsSizeT() as an example. Inside, it calls
ReadFileToString() on /proc/pid/status, but we know reading from /proc does not
hit the disk. So there's a ThreadRestrictions::ScopedAllowIO inside to allow I/O
as a convenience to the callers of ReadProcStatusAndGetFieldAsSizeT(). I feel
this is a closer comparison to the situation we are in than ReadFileToString().

[gab, 2015-07-22 17:26:56]

On 2015/07/22 01:37:51, Lei Zhang wrote:
> This is just my opinion. Feel free to disagree. :)
> 
> On 2015/07/21 19:01:55, gab wrote:
> > I could imagine a caller that does:
> > thread.Start()
> > id_ = thread.thread_id();
> > 
> > Just wanting to cache the |id_| for whatever reason for later, not realizing
> > that this actually results in blocking, at least if you force the caller to
> > specify ScopedAllowWait instead of implicitly specifying it for them, then
> they
> > have to decide whether blocking is okay for them.
> 
> How often is this going to happen though? As is, thread_id() is never used in
> this way. Would documenting this potential issue in the header comments and
> renaming thread_id() to GetThreadId() address your concern?

Okay, your call, a big warning in the header is definitely required IMO if we do
the implicit wait.

> 
> > Doing the ScopedAllowWait in the base/ API's impl would be like adding a
> > ScopedAllowIO on ReadFileToString IMO. Callers should dictate what they're
> okay
> > with, the API shouldn't allow it for them.
> 
> The difference is ReadFileToString() will most likely be blocking, whereas
> thread_id() is currently never being called right as a thread is starting in
> non-test code.
> 
> Take ReadProcStatusAndGetFieldAsSizeT() as an example. Inside, it calls
> ReadFileToString() on /proc/pid/status, but we know reading from /proc does
not
> hit the disk. So there's a ThreadRestrictions::ScopedAllowIO inside to allow
I/O
> as a convenience to the callers of ReadProcStatusAndGetFieldAsSizeT(). I feel
> this is a closer comparison to the situation we are in than
ReadFileToString().

[Takashi Toyoshima, 2015-07-23 06:20:55]

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

[Takashi Toyoshima, 2015-07-23 06:29:06]

+jam@ for caller side mechanical changes on components, gpu, media, sandbox, and
win8.

Lei: thank you for finding a common ground. I applied your suggestion to Patch
Set 17. PTAL.

[jam, 2015-07-23 16:26:50]

lgtm

[jam, 2015-07-23 16:27:32]

On 2015/07/23 16:26:50, jam wrote:
> lgtm

btw for future reference, you don't need owners review to block this. per
https://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Review...


For widespread or refactoring changes that are mechanical or global in nature,
it is acceptable to TBR the OWNERS required for submission, as long as your
changes have had the appropriate level of review or general approval.  (For
example, if touching hundreds of files across the codebase, a thread on the
chromium-dev mailing list may be better than getting individual approval from
dozens of different OWNERS.)

[gab, 2015-07-23 17:16:15]

lgtm w/ two last nits.

Thanks,
Gab

https://codereview.chromium.org/1207823004/diff/470001/base/threading/platfor...
File base/threading/platform_thread_win.cc (right):

https://codereview.chromium.org/1207823004/diff/470001/base/threading/platfor...
base/threading/platform_thread_win.cc:118: CreateThread(nullptr, stack_size,
ThreadFunc, params, flags, nullptr);
At first I thought we should remove the ID parameter from this call altogether,
but then realized this was the system call, mind adding a "::" prefix to this
call (we typically do this to highlight system calls on Windows).

https://codereview.chromium.org/1207823004/diff/470001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1207823004/diff/470001/base/threading/thread....
base/threading/thread.h:177: // WARNING: This function will block if the thread
does not start running yet.
s/does not start running yet/hasn't started yet/

[Lei Zhang, 2015-07-23 18:09:13]

base/ lgtm with gab's comments addressed.

[Takashi Toyoshima, 2015-07-24 03:47:11]

Thank you everyone for a long review.
(Sorry, it takes three weeks...)

jam@ thank you for sharing TBR rule information.

https://codereview.chromium.org/1207823004/diff/470001/base/threading/platfor...
File base/threading/platform_thread_win.cc (right):

https://codereview.chromium.org/1207823004/diff/470001/base/threading/platfor...
base/threading/platform_thread_win.cc:118: CreateThread(nullptr, stack_size,
ThreadFunc, params, flags, nullptr);
Agreed. Win32 API often has a similar name with local method. That's confusing
and '::' prefix sounds a good solution to distinguish it easily.

https://codereview.chromium.org/1207823004/diff/470001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/1207823004/diff/470001/base/threading/thread....
base/threading/thread.h:177: // WARNING: This function will block if the thread
does not start running yet.
On 2015/07/23 17:16:15, gab wrote:
> s/does not start running yet/hasn't started yet/

Done.

[Takashi Toyoshima, 2015-07-24 03:47:25]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2015-07-24 03:47:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, gab@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1207823004/#ps490001
(title: "the last two nits")

[commit-bot: I haz the power, 2015-07-24 03:54:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207823004/490001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1207823004/490001

[commit-bot: I haz the power, 2015-07-24 04:04:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-24 04:04:42]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Takashi Toyoshima, 2015-07-24 06:29:10]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2015-07-24 06:29:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207823004/490001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1207823004/490001

[commit-bot: I haz the power, 2015-07-24 09:25:41]

Committed patchset #18 (id:490001)

[commit-bot: I haz the power, 2015-07-24 09:26:13]

Patchset 18 (id:??) landed as
https://crrev.com/c41261e9491c3a763c472fe35737ec36200d6310
Cr-Commit-Position: refs/heads/master@{#340242}