make debug mode ~20% faster on linux

make debug mode ~20% faster on linux

I was doing some benchmarking on tests on linux, and noticed that 10-20% of time was
in ThreadCheckerImpl::CalledOnValidThread. I looked into it and found that we're doing
a syscall to get the thread ID, which is not needed on any platform where pthread_t
is already an integer type.  Replacing that syscall with pthread_self() makes this particular
test (End2EndTest.BasicFakeSoftwareVideo) go from 488ms to 374ms, which is a 20% speedup.
While other tests might not use ThreadChecker quite as much, it should still be a pretty
good win across a large set of tests.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272725

[jam, 2014-05-20 04:33:22]

cool; although I'm not really a good reviewer as I'm not that familiar with
posix. adding piman

[piman, 2014-05-20 05:09:22]

https://codereview.chromium.org/292873002/diff/20001/base/threading/platform_...
File base/threading/platform_thread_posix.cc (right):

https://codereview.chromium.org/292873002/diff/20001/base/threading/platform_...
base/threading/platform_thread_posix.cc:152: return pthread_self();
On my system, PlatformThreadId (a pid_t currently) is an int, whereas pthread_t
is an unsigned long int.

Can we change the definition of PlatformThreadId into a pthread_t on OS_LINUX?

https://codereview.chromium.org/292873002/diff/20001/content/common/set_proce...
File content/common/set_process_title.cc (left):

https://codereview.chromium.org/292873002/diff/20001/content/common/set_proce...
content/common/set_process_title.cc:45:
DCHECK_EQ(base::PlatformThread::CurrentId(), getpid());
nit: maybe DCHECK_EQ(gettid(), getpid()); ?

I think the goal of the DCHECK is to verify we're on the main thread (but before
any MessageLoop is created or anything), otherwise things would be racy.

[hubbe, 2014-05-21 00:22:06]

PTAL

So I tried changing PlatformThreadId to a pthraed_t and ran into a bunch of
problems.
Ultimately I decided that we probaly want one "thread id" that is fast, and
another one
for logging, so I implemented another thread id type instead. The speedups are
the same, but the risk is lower. (Of course, so is the reward, as this has no
impact on release builds.)

[piman, 2014-05-21 00:55:24]

LGTM fwiw, I'm not owner. There's probably a ton of places that would benefit
from using CurrentRef instead of CurrentId (or even ThreadChecker), including
possibly in release. Are you planning on converting those?

https://codereview.chromium.org/292873002/diff/60001/base/synchronization/loc...
File base/synchronization/lock.cc (right):

https://codereview.chromium.org/292873002/diff/60001/base/synchronization/loc...
base/synchronization/lock.cc:24: DCHECK(owning_thread_ref_ ==
PlatformThread::CurrentRef());
nit: DCHECK_EQ

https://codereview.chromium.org/292873002/diff/60001/base/synchronization/loc...
base/synchronization/lock.cc:28: DCHECK(owning_thread_ref_ ==
PlatformThread::CurrentRef());
nit: DCHECK_EQ

[hubbe, 2014-05-21 04:27:35]

https://codereview.chromium.org/292873002/diff/60001/base/synchronization/loc...
File base/synchronization/lock.cc (right):

https://codereview.chromium.org/292873002/diff/60001/base/synchronization/loc...
base/synchronization/lock.cc:24: DCHECK(owning_thread_ref_ ==
PlatformThread::CurrentRef());
On 2014/05/21 00:55:24, piman wrote:
> nit: DCHECK_EQ

DCHECK_EQ requires the values to be printable, ThreadRefs are not.

https://codereview.chromium.org/292873002/diff/60001/base/synchronization/loc...
base/synchronization/lock.cc:28: DCHECK(owning_thread_ref_ ==
PlatformThread::CurrentRef());
On 2014/05/21 00:55:24, piman wrote:
> nit: DCHECK_EQ

See above.

[hubbe, 2014-05-21 04:36:30]

+mark

[Mark Mentovai, 2014-05-23 17:55:16]

The CL description says “make debug mode ~20% faster on linux” but this change
helps more than just Linux, right?

https://codereview.chromium.org/292873002/diff/60001/base/threading/platform_...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/292873002/diff/60001/base/threading/platform_...
base/threading/platform_thread.h:34: // Meant to be as fast as possible.
Your comment should say something about the uniqueness guarantees: for example,
will the same thread always have the same PlatformThreadRef? Can two
PlatformThreadRefs identify the same thread? Can a PlatformThreadRef that once
referred to a now-dead thread subsequently refer to a different thread? It’s OK
for some of these to be unspecified, but you need to say more than what you’ve
said in this comment.

[hubbe, 2014-05-23 19:31:14]

From the look of things, this will only make a difference for linux derivatives
(linux, chromeos, android), for all other platforms the
PlatformThread::CurrentId() is already using pthread_self(). The one exception
is MacOS, but it looks like it's digging the TID out of the pthread internals
instead of making a syscall, which should be just as fast. (Too bad we can't
count on pthread_getthreadid_np existing on linux, that would make all of this
much simpler.)

https://codereview.chromium.org/292873002/diff/60001/base/threading/platform_...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/292873002/diff/60001/base/threading/platform_...
base/threading/platform_thread.h:34: // Meant to be as fast as possible.
On 2014/05/23 17:55:16, Mark Mentovai wrote:
> Your comment should say something about the uniqueness guarantees: for
example,
> will the same thread always have the same PlatformThreadRef? Can two
> PlatformThreadRefs identify the same thread? Can a PlatformThreadRef that once
> referred to a now-dead thread subsequently refer to a different thread? It’s
OK
> for some of these to be unspecified, but you need to say more than what you’ve
> said in this comment.

Better?

[Mark Mentovai, 2014-05-23 20:28:48]

LGTM

[hubbe, 2014-05-23 21:26:03]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-05-23 21:26:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/292873002/80001

[commit-bot: I haz the power, 2014-05-24 02:22:28]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-24 12:53:35]

Change committed as 272725