Don't create redundant ThreadData for Worker Threads

Don't create redundant ThreadData for Worker Threads

Before creating tracked_objects::ThreadData for a thread, see if it's a
WorkerThread. If is, try reusing ThreadData instance for previously
finished WorkerThread.

This stops unneccessary |ThreadData| instance to be created, reduces a
small portion of memory usage, especially on long running renderer
processes.

BUG=599820
Committed: https://crrev.com/a55ed003ab5d1a34d0b7b00e2a662aae272d71d8
Cr-Commit-Position: refs/heads/master@{#398369}

[Zhenyu Shan, 2016-04-01 09:29:01]

Description was changed from

==========
Specify worker thread in ThreadData properly

This patch sets |ThreadData::worker_thread_number| properly. Which stops
unneccessary |ThreadData| instance to be created, reduces a small
portion of memory usage, especially on long running renderer processes.

BUG=
==========

to

==========
Specify worker thread in ThreadData properly

This patch sets |ThreadData::worker_thread_number| properly. Which stops
unneccessary |ThreadData| instance to be created, reduces a small
portion of memory usage, especially on long running renderer processes.

BUG=599820
==========

[Zhenyu Shan, 2016-04-01 09:31:35]

zhenyu.shan@intel.com changed reviewers:
+ dcheng@chromium.org, thakis@chromium.org

[Zhenyu Shan, 2016-04-01 09:32:12]

PTAL, thanks!

[dcheng, 2016-04-01 10:52:31]

https://codereview.chromium.org/1845753006/diff/1/base/threading/platform_thr...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/1845753006/diff/1/base/threading/platform_thr...
base/threading/platform_thread.h:147: static void SetName(const std::string&
name, bool is_worker_thread = false);
Plumbing this bit through a function called SetName() feels a bit odd.

https://codereview.chromium.org/1845753006/diff/1/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/1845753006/diff/1/base/tracked_objects.cc#new...
base/tracked_objects.cc:358: bool is_worker_thread) {
Interesting, the original assumption is that worker threads are always unnamed:
https://chromium.googlesource.com/chromium/src/+/620e0190cea3663581dd91b6f6bc...

There seems to be a disconnect between what ThreadData::Get() expects (worker
threads won't set names) vs what happens today. The Windows worker pool, for
instance, doesn't call SetName().

Being able to name a worker pool seems useful, but some other worker pool
implementations, e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...,
also have this problem, so you'll (probably?) have to fix those as well.

https://codereview.chromium.org/1845753006/diff/1/base/tracked_objects.cc#new...
base/tracked_objects.cc:365: if (is_worker_thread) {
Can we just early return at the beginning for worker threads? It looks like
.Get() understands how to lazily create a ThreadData for a worker thread, so we
should be able to just rely on that.

[dcheng, 2016-04-01 18:07:35]

Also, do we have any measurements on how this affects performance? I'm wondering
if the complexity of reusing ThreadData objects here is even worth it.

[Zhenyu Shan, 2016-04-06 08:35:14]

On 2016/04/01 10:52:31, dcheng wrote:
>
https://codereview.chromium.org/1845753006/diff/1/base/threading/platform_thr...
> File base/threading/platform_thread.h (right):
> 
>
https://codereview.chromium.org/1845753006/diff/1/base/threading/platform_thr...
> base/threading/platform_thread.h:147: static void SetName(const std::string&
> name, bool is_worker_thread = false);
> Plumbing this bit through a function called SetName() feels a bit odd.
> 

I totally agree with you. But the root cause here is calling
InitializeThreadContext() inside SetName(), which feels a little bit odd as
well.
Is there a common thread initializing function other than SetName()? Maybe we
should put InitializeThreadContext() there. Or we can call it directly from
ThreadMain()? What do you think about this issue?

> https://codereview.chromium.org/1845753006/diff/1/base/tracked_objects.cc
> File base/tracked_objects.cc (right):
> 
>
https://codereview.chromium.org/1845753006/diff/1/base/tracked_objects.cc#new...
> base/tracked_objects.cc:358: bool is_worker_thread) {
> Interesting, the original assumption is that worker threads are always
unnamed:
>
https://chromium.googlesource.com/chromium/src/+/620e0190cea3663581dd91b6f6bc...
> 
> There seems to be a disconnect between what ThreadData::Get() expects (worker
> threads won't set names) vs what happens today. The Windows worker pool, for
> instance, doesn't call SetName().
> 
> Being able to name a worker pool seems useful, but some other worker pool
> implementations, e.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...,
> also have this problem, so you'll (probably?) have to fix those as well.

You mean reusing ThreadData instance for other worker threads as well?

>
https://codereview.chromium.org/1845753006/diff/1/base/tracked_objects.cc#new...
> base/tracked_objects.cc:365: if (is_worker_thread) {
> Can we just early return at the beginning for worker threads? It looks like
> .Get() understands how to lazily create a ThreadData for a worker thread, so
we
> should be able to just rely on that.

Sounds reasonable. I'm not sure if there are other places assume that ThreadData
initialization is done after calling InitializeThreadContext(). I'll give it a
try.

[Zhenyu Shan, 2016-04-06 08:52:16]

On 2016/04/01 18:07:35, dcheng wrote:
> Also, do we have any measurements on how this affects performance? I'm
wondering
> if the complexity of reusing ThreadData objects here is even worth it.

From memory perspective I think reusing is good. In our test we can save about
300KB at most after loading ten webpages.
And for performance, I can do a measurement if needed. But I think there's no
big deal because it's just several lines of code in thread creation and
termination, plus it avoids creating redundant ThreadData instances which
includes a base::hash_map and std::map, which I assume might be more
time-consuming than simply reusing.

[Zhenyu Shan, 2016-04-19 12:47:05]

On 2016/04/01 18:07:35, dcheng wrote:
> Also, do we have any measurements on how this affects performance? I'm
wondering
> if the complexity of reusing ThreadData objects here is even worth it.

I've tried to measure performance change. Like in
base/threading/thread_perftest.cc we create normal base::Thread, post tasks and
measure their cpu time. I tried to use WorkerPool::PostTask instead, but the
result turns out to be very unstable...

Do you have some hint on doing this?

[jar (doing other things), 2016-04-19 21:10:21]

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

[jar (doing other things), 2016-04-19 21:10:22]

I'm trying to understand the consequences of the original status quo.  Is the
current code (mistakenly) renaming the worker threads, or is it constructing
useless ThreadData instances, rather than recycling them for worker threads?

[dcheng, 2016-04-19 21:21:51]

On 2016/04/19 at 21:10:22, jar wrote:
> I'm trying to understand the consequences of the original status quo.  Is the
current code (mistakenly) renaming the worker threads, or is it constructing
useless ThreadData instances, rather than recycling them for worker threads?

From what I can tell, the original code assumes threads in worker pools have no
name. If the thread has no name, then Get() will reuse a ThreadData from a
worker thread that's gone (or create a new one if there's none available in the
free pool).

Since there are worker pools that name their threads now, this means we're
creating new ThreadData instances for worker threads instead of recycling them.
The problem is there are worker pools outside //base as well which don't use
this convention: my question is how important it is to keep this optimization?

Also, why do are they 'useless'? Is this because worker threads are typically
short-lived?

[jar (doing other things), 2016-04-20 04:34:51]

It has been years since I originally wrote a lot of this... and I think the code
has evolved some... but here are a few comments and concerns.

Thanks in advance for looking deeply!

Note that If the bug is as bad as suggested, then you *should* be able to see
the impact by looking at the about:profiler page.  You should click on "columns"
(top right of window), and then "uncheck" the box for "merge similar threads." 
That will allow you to see the pools broken out.  Any thread that had a task
execute on it, or post from it, should be listed... and that should act as a
tell-tale for needless growth of ThreadData instances.  You could also add
instrumentation to look at the count of distinct ThreadData instances that were
created... and YMMV.

I was historically VERY concerned with this class of mistake, as it was
historically instigated by faults in the interface with the TLS infrastructure. 
After I nailed that issues (which was only burning us on Win XP as I recall), I
carefully checked that these structures were NOT being wildly allocated.  That
said... with enough evolution... anything can happen (regress??).

https://codereview.chromium.org/1845753006/diff/20001/base/threading/platform...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/1845753006/diff/20001/base/threading/platform...
base/threading/platform_thread.h:146: // otherwise.
I suspect the code has changed, because it appears that contrary to this
comment, this call probably has a side-effect of creating the ThreadData
instance (among other things).  This comment should probably be changed... or
perhaps you should look at the history to see when these semantics were changed.

https://codereview.chromium.org/1845753006/diff/20001/base/threading/worker_p...
File base/threading/worker_pool_posix.cc (right):

https://codereview.chromium.org/1845753006/diff/20001/base/threading/worker_p...
base/threading/worker_pool_posix.cc:83: PlatformThread::SetName(name, true);
It is surprising to me that this is the *only* place where we SetName() for a
worker thread (in your CL).  What about Windows? Android? etc.?

Why don't the other platforms need this code?

https://codereview.chromium.org/1845753006/diff/20001/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/1845753006/diff/20001/base/tracked_objects.cc...
base/tracked_objects.cc:362: if (current_thread_data or is_worker_thread)
The test for is_worker_thread should be done between line 358 and 359, and you
can return at that point.

The call on line 359 should probably be renamed for clarity:

Initialize() ---> EnsureTlsInitialization()

as it sets things up (see its definition later in this file) so that the call on
line 361 can proceed.

https://codereview.chromium.org/1845753006/diff/20001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/1845753006/diff/20001/base/tracked_objects.h#...
base/tracked_objects.h:454: // only used by the message loop, which has a well
defined thread name.
This comment appears to state that this should *only* be called by threads with
real names, and probably was never meant to be called by worker threads
(transient members of a specific prefixed worker pool).  Perhaps the semantics
have changed, and that is why you must now neuter the call when it is
(surprisingly) called for initialization of a worker thread.

Given your change, you should at least change the first line of this comment
from saying:
"Initialize ... with a new instance of ThreadData"

To instead say:

"If "bool_is_worker_thread|, do partial initialization of the thread context
(and wait for just-in-time initialization by a potentially recycled context),
otherwise fully initialize ...with a new instance of ThreadData."

As I'm writing this, I'm starting to see that there probably *should* be
different recycling pools based on different proposed prefix strings.  If you
"mix up" the different recycling pools, then the contexts will grow much larger
than need be (because they will includes posts from all possible pools, rather
than just from their specific group of threads in a named pool).

Can you verify that various uses of worker-threads handle this properly?

[jar (doing other things), 2016-04-20 04:34:53]

It has been years since I originally wrote a lot of this... and I think the code
has evolved some... but here are a few comments and concerns.

Thanks in advance for looking deeply!

Note that If the bug is as bad as suggested, then you *should* be able to see
the impact by looking at the about:profiler page.  You should click on "columns"
(top right of window), and then "uncheck" the box for "merge similar threads." 
That will allow you to see the pools broken out.  Any thread that had a task
execute on it, or post from it, should be listed... and that should act as a
tell-tale for needless growth of ThreadData instances.  You could also add
instrumentation to look at the count of distinct ThreadData instances that were
created... and YMMV.

I was historically VERY concerned with this class of mistake, as it was
historically instigated by faults in the interface with the TLS infrastructure. 
After I nailed that issues (which was only burning us on Win XP as I recall), I
carefully checked that these structures were NOT being wildly allocated.  That
said... with enough evolution... anything can happen (regress??).

[Zhenyu Shan, 2016-05-04 23:33:03]

https://codereview.chromium.org/1845753006/diff/20001/base/threading/platform...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/1845753006/diff/20001/base/threading/platform...
base/threading/platform_thread.h:146: // otherwise.
On 2016/04/20 04:34:51, jar (doing other things) wrote:
> I suspect the code has changed, because it appears that contrary to this
> comment, this call probably has a side-effect of creating the ThreadData
> instance (among other things).  This comment should probably be changed... or
> perhaps you should look at the history to see when these semantics were
changed.

Done.

This change is introduced in https://codereview.chromium.org/8558003

https://codereview.chromium.org/1845753006/diff/20001/base/threading/worker_p...
File base/threading/worker_pool_posix.cc (right):

https://codereview.chromium.org/1845753006/diff/20001/base/threading/worker_p...
base/threading/worker_pool_posix.cc:83: PlatformThread::SetName(name, true);
On 2016/04/20 04:34:51, jar (doing other things) wrote:
> It is surprising to me that this is the *only* place where we SetName() for a
> worker thread (in your CL).  What about Windows? Android? etc.?
> 
> Why don't the other platforms need this code?

Android shares posix WorkerThread::ThreadMain code.
Windows uses system managed worker pool implementation, see
worker_pool_win.cc:50 calling QueueUserWorkItem().

https://codereview.chromium.org/1845753006/diff/20001/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/1845753006/diff/20001/base/tracked_objects.cc...
base/tracked_objects.cc:362: if (current_thread_data or is_worker_thread)
On 2016/04/20 04:34:51, jar (doing other things) wrote:
> The test for is_worker_thread should be done between line 358 and 359, and you
> can return at that point.
> 
> The call on line 359 should probably be renamed for clarity:
> 
> Initialize() ---> EnsureTlsInitialization()
> 
> as it sets things up (see its definition later in this file) so that the call
on
> line 361 can proceed.

Done.

https://codereview.chromium.org/1845753006/diff/20001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/1845753006/diff/20001/base/tracked_objects.h#...
base/tracked_objects.h:454: // only used by the message loop, which has a well
defined thread name.
On 2016/04/20 04:34:51, jar (doing other things) wrote:
> This comment appears to state that this should *only* be called by threads
with
> real names, and probably was never meant to be called by worker threads
> (transient members of a specific prefixed worker pool).  Perhaps the semantics
> have changed, and that is why you must now neuter the call when it is
> (surprisingly) called for initialization of a worker thread.
> 
> Given your change, you should at least change the first line of this comment
> from saying:
> "Initialize ... with a new instance of ThreadData"
> 
> To instead say:
> 
> "If "bool_is_worker_thread|, do partial initialization of the thread context
> (and wait for just-in-time initialization by a potentially recycled context),
> otherwise fully initialize ...with a new instance of ThreadData."
> 
> As I'm writing this, I'm starting to see that there probably *should* be
> different recycling pools based on different proposed prefix strings.  If you
> "mix up" the different recycling pools, then the contexts will grow much
larger
> than need be (because they will includes posts from all possible pools, rather
> than just from their specific group of threads in a named pool).
> 
> Can you verify that various uses of worker-threads handle this properly? 

Done.

I'm not very sure if I've understood you correctly. Seems this change only
affects worker threads with prefix "WorkerPool" on posix platforms. There are
other worker pool  based on base::SimpleThread, or base::TaskRunner...Which has
nothing to do with base::WorkerThread.
What do you mean by "mix up" exactly?

[Zhenyu Shan, 2016-05-04 23:38:23]

On 2016/04/20 04:34:53, jar (doing other things) wrote:
> It has been years since I originally wrote a lot of this... and I think the
code
> has evolved some... but here are a few comments and concerns.
> 
> Thanks in advance for looking deeply!
> 
> Note that If the bug is as bad as suggested, then you *should* be able to see
> the impact by looking at the about:profiler page.  You should click on
"columns"
> (top right of window), and then "uncheck" the box for "merge similar threads."

> That will allow you to see the pools broken out.  Any thread that had a task
> execute on it, or post from it, should be listed... and that should act as a
> tell-tale for needless growth of ThreadData instances.  You could also add
> instrumentation to look at the count of distinct ThreadData instances that
were
> created... and YMMV.

I have a rough statistic for ThreadData instance: After refreshing a web page
(http://www.adobe.com/) for 10 times, there are over 400 instances of
ThreadData(not exactly same every time). With this CL this number could be
reduce to ~40-50 (don't have exact number)

[Zhenyu Shan, 2016-05-05 01:00:21]

On 2016/05/04 23:38:23, Zhenyu Shan wrote:
> On 2016/04/20 04:34:53, jar (doing other things) wrote:
> > It has been years since I originally wrote a lot of this... and I think the
> code
> > has evolved some... but here are a few comments and concerns.
> > 
> > Thanks in advance for looking deeply!
> > 
> > Note that If the bug is as bad as suggested, then you *should* be able to
see
> > the impact by looking at the about:profiler page.  You should click on
> "columns"
> > (top right of window), and then "uncheck" the box for "merge similar
threads."
> 
> > That will allow you to see the pools broken out.  Any thread that had a task
> > execute on it, or post from it, should be listed... and that should act as a
> > tell-tale for needless growth of ThreadData instances.  You could also add
> > instrumentation to look at the count of distinct ThreadData instances that
> were
> > created... and YMMV.
> 
> I have a rough statistic for ThreadData instance: After refreshing a web page
> (http://www.adobe.com/) for 10 times, there are over 400 instances of
> ThreadData(not exactly same every time). With this CL this number could be
> reduce to ~40-50 (don't have exact number)

One mistake: This number is the account of ThreadData instances for
base::WorkerThread only, not the total number in renderer process.

[jar (doing other things), 2016-05-08 03:04:09]

LGTM

Thanks for answering my questions.  This seems like a very positive change.

[Zhenyu Shan, 2016-05-09 02:00:56]

The CQ bit was checked by zhenyu.shan@intel.com

[commit-bot: I haz the power, 2016-05-09 02:01:05]

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

[commit-bot: I haz the power, 2016-05-09 02:07:42]

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

[commit-bot: I haz the power, 2016-05-09 02:07:44]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Zhenyu Shan, 2016-05-09 02:18:57]

zhenyu.shan@intel.com changed reviewers:
+ jam@chromium.org

[Zhenyu Shan, 2016-05-09 02:19:39]

Could you file owners for base/ take a look at this? Thanks!

[Zhenyu Shan, 2016-05-11 00:59:11]

zhenyu.shan@intel.com changed reviewers:
+ mcolbert@chromium.org, thestig@chromium.org

[Zhenyu Shan, 2016-05-12 10:07:49]

ping
PTAL, thanks!

[Mark, 2016-05-12 16:52:12]

mcolbert@chromium.org changed reviewers:
- mcolbert@chromium.org

[Lei Zhang, 2016-05-12 17:25:47]

Please try to pick an OWNER rather than many. It could be that we all think
someone else will take care of it, and then nobody does.

You still have red Mac trybots. Get all your bots green first?

[Lei Zhang, 2016-05-12 19:47:40]

I agree plumbing it through SetName() is a bit weird, but I don't have a better
suggestion at the moment.

https://codereview.chromium.org/1845753006/diff/40001/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/1845753006/diff/40001/base/tracked_objects.cc...
base/tracked_objects.cc:359: if (is_worker_thread)
Instead of adding another parameter, can the caller just avoid calling
InitializeThreadContext() instead?

https://codereview.chromium.org/1845753006/diff/40001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/1845753006/diff/40001/base/tracked_objects.h#...
base/tracked_objects.h:452: // and wait for just-in-time initilization using
potentially recycled
typo

[Zhenyu Shan, 2016-05-13 02:45:35]

The CQ bit was checked by zhenyu.shan@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-13 02:45:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845753006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845753006/60001

[commit-bot: I haz the power, 2016-05-13 03:19:31]

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

[commit-bot: I haz the power, 2016-05-13 03:19:32]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Zhenyu Shan, 2016-05-13 03:22:24]

Patchset #4 (id:60001) has been deleted

[Zhenyu Shan, 2016-05-13 03:24:50]

The CQ bit was checked by zhenyu.shan@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-13 03:24:59]

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

[commit-bot: I haz the power, 2016-05-13 04:58:52]

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

[commit-bot: I haz the power, 2016-05-13 04:58:54]

Dry run: This issue passed the CQ dry run.

[Zhenyu Shan, 2016-05-13 06:40:50]

The CQ bit was checked by zhenyu.shan@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-13 06:41:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845753006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845753006/100001

[commit-bot: I haz the power, 2016-05-13 08:41:47]

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

[commit-bot: I haz the power, 2016-05-13 08:41:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Zhenyu Shan, 2016-05-13 09:02:17]

The CQ bit was checked by zhenyu.shan@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-13 09:02:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845753006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845753006/100001

[commit-bot: I haz the power, 2016-05-13 10:17:55]

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

[commit-bot: I haz the power, 2016-05-13 10:17:56]

Dry run: This issue passed the CQ dry run.

[Zhenyu Shan, 2016-05-16 02:05:35]

https://codereview.chromium.org/1845753006/diff/40001/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/1845753006/diff/40001/base/tracked_objects.cc...
base/tracked_objects.cc:359: if (is_worker_thread)
On 2016/05/12 19:47:40, Lei Zhang wrote:
> Instead of adding another parameter, can the caller just avoid calling
> InitializeThreadContext() instead?

Done.

https://codereview.chromium.org/1845753006/diff/40001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/1845753006/diff/40001/base/tracked_objects.h#...
base/tracked_objects.h:452: // and wait for just-in-time initilization using
potentially recycled
On 2016/05/12 19:47:40, Lei Zhang wrote:
> typo

Restored the comment here because we moved is_worker_thread checking before
calling InitializeThreadContext

[Zhenyu Shan, 2016-05-20 06:39:33]

Anyone there? Could you please take a look at the new patch set? Thanks in
advance.

[Zhenyu Shan, 2016-05-27 06:49:58]

On 2016/05/20 06:39:33, Zhenyu Shan wrote:
> Anyone there? Could you please take a look at the new patch set? Thanks in
> advance.

ping again

[Lei Zhang, 2016-05-31 21:43:24]

Idea - instead of adding an is_worker boolean to SetName(), can
ThreadData::InitializeThreadContext() just check
base::WorkerPool::RunsTasksOnCurrentThread() and do nothing if that's the case?

https://codereview.chromium.org/1845753006/diff/100001/base/threading/platfor...
File base/threading/platform_thread_linux.cc (right):

https://codereview.chromium.org/1845753006/diff/100001/base/threading/platfor...
base/threading/platform_thread_linux.cc:70: bool is_worker_thread) {
nit: fix on the previous line, just like all the other platform impls.

[Zhenyu Shan, 2016-06-03 09:20:28]

The CQ bit was checked by zhenyu.shan@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-03 09:20:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845753006/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845753006/120001

[Zhenyu Shan, 2016-06-03 09:22:52]

On 2016/05/31 21:43:24, Lei Zhang (OOO) wrote:
> Idea - instead of adding an is_worker boolean to SetName(), can
> ThreadData::InitializeThreadContext() just check
> base::WorkerPool::RunsTasksOnCurrentThread() and do nothing if that's the
case?

Great idea, which really simplifies things a lot.
Updated the patch, please take a look. Thanks.

> 
>
https://codereview.chromium.org/1845753006/diff/100001/base/threading/platfor...
> File base/threading/platform_thread_linux.cc (right):
> 
>
https://codereview.chromium.org/1845753006/diff/100001/base/threading/platfor...
> base/threading/platform_thread_linux.cc:70: bool is_worker_thread) {
> nit: fix on the previous line, just like all the other platform impls.

[commit-bot: I haz the power, 2016-06-03 11:26:01]

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

[commit-bot: I haz the power, 2016-06-03 11:26:03]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-06-06 22:25:53]

On 2016/06/03 09:22:52, Zhenyu Shan wrote:
> On 2016/05/31 21:43:24, Lei Zhang (OOO) wrote:
> > Idea - instead of adding an is_worker boolean to SetName(), can
> > ThreadData::InitializeThreadContext() just check
> > base::WorkerPool::RunsTasksOnCurrentThread() and do nothing if that's the
> case?
> 
> Great idea, which really simplifies things a lot.
> Updated the patch, please take a look. Thanks.

Can you update the CL description? The CL has changed significantly since patch
set 1.

[Zhenyu Shan, 2016-06-07 13:40:51]

Description was changed from

==========
Specify worker thread in ThreadData properly

This patch sets |ThreadData::worker_thread_number| properly. Which stops
unneccessary |ThreadData| instance to be created, reduces a small
portion of memory usage, especially on long running renderer processes.

BUG=599820
==========

to

==========
Before creating tracked_objects::ThreadData for a thread, see if it's a
WorkerThread. If is, try reusing ThreadData instance for previously finished
WorkerThread.

This stops
unneccessary |ThreadData| instance to be created, reduces a small
portion of memory usage, especially on long running renderer processes.

BUG=599820
==========

[Zhenyu Shan, 2016-06-07 13:41:13]

Description was changed from

==========
Before creating tracked_objects::ThreadData for a thread, see if it's a
WorkerThread. If is, try reusing ThreadData instance for previously finished
WorkerThread.

This stops
unneccessary |ThreadData| instance to be created, reduces a small
portion of memory usage, especially on long running renderer processes.

BUG=599820
==========

to

==========
Before creating tracked_objects::ThreadData for a thread, see if it's a
WorkerThread. If is, try reusing ThreadData instance for previously finished
WorkerThread.

This stops unneccessary |ThreadData| instance to be created, reduces a small
portion of memory usage, especially on long running renderer processes.

BUG=599820
==========

[Zhenyu Shan, 2016-06-07 13:41:42]

On 2016/06/06 22:25:53, Lei Zhang (Slow) wrote:
> On 2016/06/03 09:22:52, Zhenyu Shan wrote:
> > On 2016/05/31 21:43:24, Lei Zhang (OOO) wrote:
> > > Idea - instead of adding an is_worker boolean to SetName(), can
> > > ThreadData::InitializeThreadContext() just check
> > > base::WorkerPool::RunsTasksOnCurrentThread() and do nothing if that's the
> > case?
> > 
> > Great idea, which really simplifies things a lot.
> > Updated the patch, please take a look. Thanks.
> 
> Can you update the CL description? The CL has changed significantly since
patch
> set 1.

Done

[Lei Zhang, 2016-06-07 17:47:13]

Description was changed from

==========
Before creating tracked_objects::ThreadData for a thread, see if it's a
WorkerThread. If is, try reusing ThreadData instance for previously finished
WorkerThread.

This stops unneccessary |ThreadData| instance to be created, reduces a small
portion of memory usage, especially on long running renderer processes.

BUG=599820
==========

to

==========
Don't create redundant ThreadData for Worker Threads

Before creating tracked_objects::ThreadData for a thread, see if it's a
WorkerThread. If is, try reusing ThreadData instance for previously
finished WorkerThread.

This stops unneccessary |ThreadData| instance to be created, reduces a
small portion of memory usage, especially on long running renderer
processes.

BUG=599820
==========

[Lei Zhang, 2016-06-07 17:47:41]

On 2016/06/07 13:41:42, Zhenyu Shan wrote:
> Done

Thanks. I did a bit of light editing on the CL description. lgtm.

[Lei Zhang, 2016-06-07 17:47:47]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-06-07 17:47:47]

The patchset sent to the CQ was uploaded after l-g-t-m from jar@chromium.org
Link to the patchset: https://codereview.chromium.org/1845753006/#ps120001
(title: " ")

[commit-bot: I haz the power, 2016-06-07 17:48:09]

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

[commit-bot: I haz the power, 2016-06-07 21:05:51]

Description was changed from

==========
Don't create redundant ThreadData for Worker Threads

Before creating tracked_objects::ThreadData for a thread, see if it's a
WorkerThread. If is, try reusing ThreadData instance for previously
finished WorkerThread.

This stops unneccessary |ThreadData| instance to be created, reduces a
small portion of memory usage, especially on long running renderer
processes.

BUG=599820
==========

to

==========
Don't create redundant ThreadData for Worker Threads

Before creating tracked_objects::ThreadData for a thread, see if it's a
WorkerThread. If is, try reusing ThreadData instance for previously
finished WorkerThread.

This stops unneccessary |ThreadData| instance to be created, reduces a
small portion of memory usage, especially on long running renderer
processes.

BUG=599820
==========

[commit-bot: I haz the power, 2016-06-07 21:05:52]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-06-07 21:07:26]

Description was changed from

==========
Don't create redundant ThreadData for Worker Threads

Before creating tracked_objects::ThreadData for a thread, see if it's a
WorkerThread. If is, try reusing ThreadData instance for previously
finished WorkerThread.

This stops unneccessary |ThreadData| instance to be created, reduces a
small portion of memory usage, especially on long running renderer
processes.

BUG=599820
==========

to

==========
Don't create redundant ThreadData for Worker Threads

Before creating tracked_objects::ThreadData for a thread, see if it's a
WorkerThread. If is, try reusing ThreadData instance for previously
finished WorkerThread.

This stops unneccessary |ThreadData| instance to be created, reduces a
small portion of memory usage, especially on long running renderer
processes.

BUG=599820

Committed: https://crrev.com/a55ed003ab5d1a34d0b7b00e2a662aae272d71d8
Cr-Commit-Position: refs/heads/master@{#398369}
==========

[commit-bot: I haz the power, 2016-06-07 21:07:28]

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