|
|
DescriptionReuse ThreadData instances associated with terminated named threads.
With this CL, whenever a thread is terminated, its ThreadData is added
to a retired list. When a thread which doesn't have a ThreadData yet
needs one, we first check if a ThreadData from the retired list could be
reused. A ThreadData can be reused if it was previously associated with
a thread that had the same name as the current thread, ignoring trailing
digits. If no ThreadData can be reused, a new one is allocated.
This change is important because TaskScheduler tears down its named
threads when they aren't used for a while and recreates them when they
are needed. We don't want this behavior to increase the number of
ThreadData instances in the process indefinitely over time.
BUG=645196
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/f607a846cc997adf49d621388a3fd3ce9b6956e1
Cr-Commit-Position: refs/heads/master@{#436745}
Patch Set 1 #Patch Set 2 : self-review #Patch Set 3 : remove changes to about:profiler #Patch Set 4 : fix build error #Patch Set 5 : fix clang build and test errors #Patch Set 6 : fix test errors #Patch Set 7 : fix build error #Patch Set 8 : fix build errors #Patch Set 9 : fix test error #
Total comments: 4
Patch Set 10 : CR Ilya Sherman #38 #Patch Set 11 : remove dcheck #
Total comments: 8
Patch Set 12 : CR danakj #55 (nits) #
Total comments: 26
Patch Set 13 : CR thestig #65 #Patch Set 14 : fix build error #Patch Set 15 : rebase #Messages
Total messages: 100 (69 generated)
Description was changed from ========== Reuse ThreadData instances of retired named threads. BUG= ========== to ========== Reuse ThreadData instances of retired named threads. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Reuse ThreadData instances of retired named threads. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Reuse ThreadData instances associated with retired named thread. With this CL, whenever a thread is teared down, its ThreadData is added to a retired list. When a thread which doesn't have a ThreadData yet needs one, we first check if a ThreadData from the retired list could be reused. A ThreadData can be reused if it was previously associated with a thread that had the same name as the current thread, ignoring trailing digits. If no ThreadData can be reused, a new one is allocated. This change is important because TaskScheduler tears down its named threads when they aren't used for a while and recreates them when they are needed. We don't want this behavior to increase the number of ThreadData instances in the process indefinitely over time. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Reuse ThreadData instances associated with retired named thread. With this CL, whenever a thread is teared down, its ThreadData is added to a retired list. When a thread which doesn't have a ThreadData yet needs one, we first check if a ThreadData from the retired list could be reused. A ThreadData can be reused if it was previously associated with a thread that had the same name as the current thread, ignoring trailing digits. If no ThreadData can be reused, a new one is allocated. This change is important because TaskScheduler tears down its named threads when they aren't used for a while and recreates them when they are needed. We don't want this behavior to increase the number of ThreadData instances in the process indefinitely over time. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Reuse ThreadData instances associated with retired named thread. With this CL, whenever a thread is terminated, its ThreadData is added to a retired list. When a thread which doesn't have a ThreadData yet needs one, we first check if a ThreadData from the retired list could be reused. A ThreadData can be reused if it was previously associated with a thread that had the same name as the current thread, ignoring trailing digits. If no ThreadData can be reused, a new one is allocated. This change is important because TaskScheduler tears down its named threads when they aren't used for a while and recreates them when they are needed. We don't want this behavior to increase the number of ThreadData instances in the process indefinitely over time. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
fdoray@chromium.org changed reviewers: + thakis@chromium.org
PTAL
Description was changed from ========== Reuse ThreadData instances associated with retired named thread. With this CL, whenever a thread is terminated, its ThreadData is added to a retired list. When a thread which doesn't have a ThreadData yet needs one, we first check if a ThreadData from the retired list could be reused. A ThreadData can be reused if it was previously associated with a thread that had the same name as the current thread, ignoring trailing digits. If no ThreadData can be reused, a new one is allocated. This change is important because TaskScheduler tears down its named threads when they aren't used for a while and recreates them when they are needed. We don't want this behavior to increase the number of ThreadData instances in the process indefinitely over time. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Reuse ThreadData instances associated with terminated named thread. With this CL, whenever a thread is terminated, its ThreadData is added to a retired list. When a thread which doesn't have a ThreadData yet needs one, we first check if a ThreadData from the retired list could be reused. A ThreadData can be reused if it was previously associated with a thread that had the same name as the current thread, ignoring trailing digits. If no ThreadData can be reused, a new one is allocated. This change is important because TaskScheduler tears down its named threads when they aren't used for a while and recreates them when they are needed. We don't want this behavior to increase the number of ThreadData instances in the process indefinitely over time. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Reuse ThreadData instances associated with terminated named thread. With this CL, whenever a thread is terminated, its ThreadData is added to a retired list. When a thread which doesn't have a ThreadData yet needs one, we first check if a ThreadData from the retired list could be reused. A ThreadData can be reused if it was previously associated with a thread that had the same name as the current thread, ignoring trailing digits. If no ThreadData can be reused, a new one is allocated. This change is important because TaskScheduler tears down its named threads when they aren't used for a while and recreates them when they are needed. We don't want this behavior to increase the number of ThreadData instances in the process indefinitely over time. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Reuse ThreadData instances associated with terminated named threads. With this CL, whenever a thread is terminated, its ThreadData is added to a retired list. When a thread which doesn't have a ThreadData yet needs one, we first check if a ThreadData from the retired list could be reused. A ThreadData can be reused if it was previously associated with a thread that had the same name as the current thread, ignoring trailing digits. If no ThreadData can be reused, a new one is allocated. This change is important because TaskScheduler tears down its named threads when they aren't used for a while and recreates them when they are needed. We don't want this behavior to increase the number of ThreadData instances in the process indefinitely over time. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
this doesn't build
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org, isherman@chromium.org
+isherman since this is related to the task profiler
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thakis@: Now the CL passes the CQ dry run. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % nits. Thanks! https://codereview.chromium.org/2488073002/diff/160001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2488073002/diff/160001/base/tracked_objects.c... base/tracked_objects.cc:803: // Assuming that there aren't more than a few tens of retired ThreadData Is it worth DCHECKing the list size? https://codereview.chromium.org/2488073002/diff/160001/base/tracked_objects.h File base/tracked_objects.h (right): https://codereview.chromium.org/2488073002/diff/160001/base/tracked_objects.h... base/tracked_objects.h:64: // Location instance, as well as a pointer to the ThreadData bound to thread on nit: s/to thread/to the thread
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2488073002/diff/160001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2488073002/diff/160001/base/tracked_objects.c... base/tracked_objects.cc:803: // Assuming that there aren't more than a few tens of retired ThreadData On 2016/11/12 01:21:16, Ilya Sherman wrote: > Is it worth DCHECKing the list size? Done. DCHECKed that it stays under 30. https://codereview.chromium.org/2488073002/diff/160001/base/tracked_objects.h File base/tracked_objects.h (right): https://codereview.chromium.org/2488073002/diff/160001/base/tracked_objects.h... base/tracked_objects.h:64: // Location instance, as well as a pointer to the ThreadData bound to thread on On 2016/11/12 01:21:16, Ilya Sherman wrote: > nit: s/to thread/to the thread Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
asvitkine@chromium.org changed reviewers: - asvitkine@chromium.org
Nico: PTAL
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fdoray@chromium.org changed reviewers: + danakj@chromium.org
+danakj@: PTAL (Nico OOO)
> BUG= Surely there's a bug tracking this work? If not please file one.
Description was changed from ========== Reuse ThreadData instances associated with terminated named threads. With this CL, whenever a thread is terminated, its ThreadData is added to a retired list. When a thread which doesn't have a ThreadData yet needs one, we first check if a ThreadData from the retired list could be reused. A ThreadData can be reused if it was previously associated with a thread that had the same name as the current thread, ignoring trailing digits. If no ThreadData can be reused, a new one is allocated. This change is important because TaskScheduler tears down its named threads when they aren't used for a while and recreates them when they are needed. We don't want this behavior to increase the number of ThreadData instances in the process indefinitely over time. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Reuse ThreadData instances associated with terminated named threads. With this CL, whenever a thread is terminated, its ThreadData is added to a retired list. When a thread which doesn't have a ThreadData yet needs one, we first check if a ThreadData from the retired list could be reused. A ThreadData can be reused if it was previously associated with a thread that had the same name as the current thread, ignoring trailing digits. If no ThreadData can be reused, a new one is allocated. This change is important because TaskScheduler tears down its named threads when they aren't used for a while and recreates them when they are needed. We don't want this behavior to increase the number of ThreadData instances in the process indefinitely over time. BUG=645196 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2016/11/16 21:41:43, danakj wrote: > > BUG= > > Surely there's a bug tracking this work? If not please file one. Done
First quick pass. If Nico is more familiar it might be faster to have him review this I'm very out of familiar territory here. https://codereview.chromium.org/2488073002/diff/200001/base/tracked_objects.h File base/tracked_objects.h (right): https://codereview.chromium.org/2488073002/diff/200001/base/tracked_objects.h... base/tracked_objects.h:98: // destroyed (dies). Since a ThreadData is bound to a most one thread at a time, at most https://codereview.chromium.org/2488073002/diff/200001/base/tracked_objects.h... base/tracked_objects.h:108: // Births map is index by Location and the DeathData map is indexed by Births*. indexed https://codereview.chromium.org/2488073002/diff/200001/base/tracked_objects.h... base/tracked_objects.h:681: ThreadData* next_retired_; nit: next_retired_thread_data_ https://codereview.chromium.org/2488073002/diff/200001/base/tracked_objects.h... base/tracked_objects.h:684: // replaced with "*". Are the number of digits always the same?
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Nico: PTAL https://codereview.chromium.org/2488073002/diff/200001/base/tracked_objects.h File base/tracked_objects.h (right): https://codereview.chromium.org/2488073002/diff/200001/base/tracked_objects.h... base/tracked_objects.h:98: // destroyed (dies). Since a ThreadData is bound to a most one thread at a time, On 2016/11/17 02:26:36, danakj wrote: > at most Done. https://codereview.chromium.org/2488073002/diff/200001/base/tracked_objects.h... base/tracked_objects.h:108: // Births map is index by Location and the DeathData map is indexed by Births*. On 2016/11/17 02:26:36, danakj wrote: > indexed Done. https://codereview.chromium.org/2488073002/diff/200001/base/tracked_objects.h... base/tracked_objects.h:681: ThreadData* next_retired_; On 2016/11/17 02:26:36, danakj wrote: > nit: next_retired_thread_data_ Done. https://codereview.chromium.org/2488073002/diff/200001/base/tracked_objects.h... base/tracked_objects.h:684: // replaced with "*". On 2016/11/17 02:26:36, danakj wrote: > Are the number of digits always the same? Clarified comment. All trailing digits are replaced with a single "*". Thread1 -> Thread* Thread100 -> Thread*
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fdoray@chromium.org changed reviewers: + mark@chromium.org
mark@ or danakj@: I have a hard time getting this reviewed by Nico. Could you take a look?
On Wed, Nov 23, 2016 at 5:07 AM, <fdoray@chromium.org> wrote: > mark@ or danakj@: I have a hard time getting this reviewed by Nico. Could > you > take a look? > Ive got a few days of CLs in my inbox I'm struggling to get thru so hopefully mark can review. > > https://codereview.chromium.org/2488073002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
looking onw
Sorry about the delay. I wasn't familiar with this code either and I didn't have 90 uninterrupted minutes until now (was out sick much of last week and had to catch up). I have one real comment below, I marked it with (XXX). https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.c... base/tracked_objects.cc:327: PushToHeadOfList(); // Which sets real incarnation_count_for_pool_. maybe DCHECK(sanitized_thread_name.empty() || !isdigit(sanitized_thread_name.back()))? https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.c... base/tracked_objects.cc:380: GetRetiredOrCreateThreadData("WorkerThread-*"); How much will things explode if someone one day decides to create a named thread called "WorkerThread"? (Can't think of anything better than a string-y ID type for threads either though) https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.c... base/tracked_objects.cc:406: DCHECK(!next_retired_thread_data_); Why is this dcheck true? https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.c... base/tracked_objects.cc:799: ThreadData** previous_thread_data_next_retired_thread_data_pointer = this name is both long too long for a local and confusing (not clear to me what "previous", "next" and "pointer" refer to). I'd just call this "pcursor" or "pcurrent" or something like that, and s/current_thread_data/cursor/ (or current, or...), and with all the characters you save you can consider putting in a link to something like http://grisha.org/blog/2013/04/02/linus-on-understanding-pointers/ if someone hasn't seen this -- but I think it's pretty well-known) https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.c... base/tracked_objects.cc:803: // Assuming that there aren't more than a few tens of retired ThreadData I'm a bit nervous about having to do potentially unbounded work every time a thread is created. After some looking, I couldn't find any uma metrics that'd alert us if this function ended up taking a lot of time. What's the plan for making sure that this really is "few tens" in practice going forward, and not just now? (XXX) https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.c... base/tracked_objects.cc:809: // Note: Test processes may have more than a few tens or retired ThreadData s/or/of/ https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.h File base/tracked_objects.h (right): https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.h... base/tracked_objects.h:88: // The maxium amount of memory used in the above data structures is the product typo maxium Also, the changes to this paragraph are pretty funny :-D ("fortunately we don't use memory proportional to the product because…" -> "we use memory proportional to the product, but…" – but from what I understand you're just rewording the lhs and this doesn't actually change?) https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.h... base/tracked_objects.h:97: // as accumulate the run-time and queue-time durations for the instance as it is "as well as to accumulate"? (lhs is the same as rhs, but I found the sentence hard to parse. then again, not a native speaker) https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.h... base/tracked_objects.h:126: // list_lock_. Thanks for the comprehensive comment update! https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.h... base/tracked_objects.h:128: // When new ThreadData instances is added to the global list, it is pre-pended, (s/is added/are added/. yes, not your code.) https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.h... base/tracked_objects.h:235: std::string thread_name; This should probably now be called sanitized_name or at least grow a comment explaining that it's not actually the thread_name any more? https://cs.chromium.org/chromium/src/chrome/browser/task_profiler/task_profil... wasn't changed – is that code fine with receiving sanitized names now? Probably a progression? (maybe something checks if the names are identical and if so assumes a task was born and died on the same thread – that wouldn't work anymore. didn't check if that happens though) https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects_u... File base/tracked_objects_unittest.cc (right): https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects_u... base/tracked_objects_unittest.cc:114: int GetNumThreadData() { doesn't this have to take the list lock? https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects_u... base/tracked_objects_unittest.cc:1205: "456Dummy%d"}; include a thread that has only digits in the name ("%d")
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.c... base/tracked_objects.cc:327: PushToHeadOfList(); // Which sets real incarnation_count_for_pool_. On 2016/11/24 01:42:04, Nico wrote: > maybe DCHECK(sanitized_thread_name.empty() || > !isdigit(sanitized_thread_name.back()))? Done. https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.c... base/tracked_objects.cc:380: GetRetiredOrCreateThreadData("WorkerThread-*"); On 2016/11/24 01:42:04, Nico wrote: > How much will things explode if someone one day decides to create a named thread > called "WorkerThread"? > > (Can't think of anything better than a string-y ID type for threads either > though) I added a DCHECK in InitializeThreadContext() to prevent that. https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.c... base/tracked_objects.cc:406: DCHECK(!next_retired_thread_data_); On 2016/11/24 01:42:04, Nico wrote: > Why is this dcheck true? See comment on member declaration. I also added a comment here. https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.c... base/tracked_objects.cc:799: ThreadData** previous_thread_data_next_retired_thread_data_pointer = On 2016/11/24 01:42:04, Nico wrote: > this name is both long too long for a local and confusing (not clear to me what > "previous", "next" and "pointer" refer to). I'd just call this "pcursor" or > "pcurrent" or something like that, and s/current_thread_data/cursor/ (or > current, or...), and with all the characters you save you can consider putting > in a link to something like > http://grisha.org/blog/2013/04/02/linus-on-understanding-pointers/ if someone > hasn't seen this -- but I think it's pretty well-known) Done. https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.c... base/tracked_objects.cc:803: // Assuming that there aren't more than a few tens of retired ThreadData On 2016/11/24 01:42:04, Nico wrote: > I'm a bit nervous about having to do potentially unbounded work every time a > thread is created. After some looking, I couldn't find any uma metrics that'd > alert us if this function ended up taking a lot of time. What's the plan for > making sure that this really is "few tens" in practice going forward, and not > just now? > > (XXX) I added a UMA histogram to keep track of the time spent in this function. I will register to be notified if it ever changes. I also thought about keeping track of the number of traversed ThreadData instances, but I decided that time would be more actionable (100 ms spent in this function is clearly bad while it's hard to say if traversing 100 ThreadData instances is bad). https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.c... base/tracked_objects.cc:809: // Note: Test processes may have more than a few tens or retired ThreadData On 2016/11/24 01:42:04, Nico wrote: > s/or/of/ Done. https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.h File base/tracked_objects.h (right): https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.h... base/tracked_objects.h:88: // The maxium amount of memory used in the above data structures is the product On 2016/11/24 01:42:04, Nico wrote: > typo maxium > > Also, the changes to this paragraph are pretty funny :-D ("fortunately we don't > use memory proportional to the product because…" -> "we use memory proportional > to the product, but…" – but from what I understand you're just rewording the lhs > and this doesn't actually change?) I'm saying the same thing as before, with fewer words (except that I changed "threads" to "ThreadData instances" to highlight the fact that repetitively creating/destroying a thread with the same name doesn't have an impact on the memory used by tracked_objects). https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.h... base/tracked_objects.h:97: // as accumulate the run-time and queue-time durations for the instance as it is On 2016/11/24 01:42:04, Nico wrote: > "as well as to accumulate"? (lhs is the same as rhs, but I found the sentence > hard to parse. then again, not a native speaker) Done. https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.h... base/tracked_objects.h:126: // list_lock_. On 2016/11/24 01:42:04, Nico wrote: > Thanks for the comprehensive comment update! Acknowledged. https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.h... base/tracked_objects.h:128: // When new ThreadData instances is added to the global list, it is pre-pended, On 2016/11/24 01:42:04, Nico wrote: > (s/is added/are added/. yes, not your code.) Done. https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects.h... base/tracked_objects.h:235: std::string thread_name; On 2016/11/24 01:42:04, Nico wrote: > This should probably now be called sanitized_name or at least grow a comment > explaining that it's not actually the thread_name any more? > > https://cs.chromium.org/chromium/src/chrome/browser/task_profiler/task_profil... > wasn't changed – is that code fine with receiving sanitized names now? Probably > a progression? (maybe something checks if the names are identical and if so > assumes a task was born and died on the same thread – that wouldn't work > anymore. didn't check if that happens though) Done. The receiver of the json generated by task_profiler_data_serializer.cc replaces trailing digits with *. If names without trailing digits are provided, this will be a no-op. I plan to clean the javascript code in a separate CL. Code that replaces trailing digits with *: https://cs.chromium.org/chromium/src/chrome/browser/resources/profiler/profil... / This behavior is controlled by an hidden check box that is checked by default (i.e. behavior is always enabled): https://cs.chromium.org/chromium/src/chrome/browser/resources/profiler/profil... I plan to clean the Javascript/HTML code in a separate CL. https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects_u... File base/tracked_objects_unittest.cc (right): https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects_u... base/tracked_objects_unittest.cc:114: int GetNumThreadData() { On 2016/11/24 01:42:04, Nico wrote: > doesn't this have to take the list lock? No. ThreadData::first() takes a lock. The list can then be traversed without a lock because ThreadData instances are never destroyed and |next_| is immutable. Added a comment above the declaration of |next_|. It would be hard to make |next_| const because setting its value and updating the global |all_thread_data_list_head_| must be done atomically. https://codereview.chromium.org/2488073002/diff/220001/base/tracked_objects_u... base/tracked_objects_unittest.cc:1205: "456Dummy%d"}; On 2016/11/24 01:42:04, Nico wrote: > include a thread that has only digits in the name ("%d") Done.
Nico: PTAnL
lgtm, thanks for the update. I'm still worried about how expensive the lock/loop will be in practice, but now we can just look at the data to see if this fear was justified. (If you haven't seen it, the bug and doc linked from https://codereview.chromium.org/1011683002/ are somewhat related and interesting.) A tools/histograms owner should probably look at the histograms.xml change (my lgtm accidentally covers this via tools/OWNERS, but I didn't look at that and I'm not qualified to do so.)
histograms lgtm
The CQ bit was checked by fdoray@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
fdoray@chromium.org changed reviewers: + palmer@chromium.org - danakj@chromium.org, mark@chromium.org
palmer@: Please review changes in content/common/child_process_messages.h
fdoray@chromium.org changed reviewers: + wfh@chromium.org
wfh@: Please review changes in content/common/child_process_messages.h (No reply from palmer@)
content/common/child_process_messages.h lgtm
The CQ bit was checked by wfh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, isherman@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2488073002/#ps280001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 280001, "attempt_start_ts": 1481055966221640, "parent_rev": "ed20ce23d0580ee2722bf28ed80a6d79b63e419c", "commit_rev": "a9a65b5d8a5fa17febea5362c1c544329fbd3163"}
Message was sent while issue was closed.
Description was changed from ========== Reuse ThreadData instances associated with terminated named threads. With this CL, whenever a thread is terminated, its ThreadData is added to a retired list. When a thread which doesn't have a ThreadData yet needs one, we first check if a ThreadData from the retired list could be reused. A ThreadData can be reused if it was previously associated with a thread that had the same name as the current thread, ignoring trailing digits. If no ThreadData can be reused, a new one is allocated. This change is important because TaskScheduler tears down its named threads when they aren't used for a while and recreates them when they are needed. We don't want this behavior to increase the number of ThreadData instances in the process indefinitely over time. BUG=645196 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Reuse ThreadData instances associated with terminated named threads. With this CL, whenever a thread is terminated, its ThreadData is added to a retired list. When a thread which doesn't have a ThreadData yet needs one, we first check if a ThreadData from the retired list could be reused. A ThreadData can be reused if it was previously associated with a thread that had the same name as the current thread, ignoring trailing digits. If no ThreadData can be reused, a new one is allocated. This change is important because TaskScheduler tears down its named threads when they aren't used for a while and recreates them when they are needed. We don't want this behavior to increase the number of ThreadData instances in the process indefinitely over time. BUG=645196 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Reuse ThreadData instances associated with terminated named threads. With this CL, whenever a thread is terminated, its ThreadData is added to a retired list. When a thread which doesn't have a ThreadData yet needs one, we first check if a ThreadData from the retired list could be reused. A ThreadData can be reused if it was previously associated with a thread that had the same name as the current thread, ignoring trailing digits. If no ThreadData can be reused, a new one is allocated. This change is important because TaskScheduler tears down its named threads when they aren't used for a while and recreates them when they are needed. We don't want this behavior to increase the number of ThreadData instances in the process indefinitely over time. BUG=645196 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Reuse ThreadData instances associated with terminated named threads. With this CL, whenever a thread is terminated, its ThreadData is added to a retired list. When a thread which doesn't have a ThreadData yet needs one, we first check if a ThreadData from the retired list could be reused. A ThreadData can be reused if it was previously associated with a thread that had the same name as the current thread, ignoring trailing digits. If no ThreadData can be reused, a new one is allocated. This change is important because TaskScheduler tears down its named threads when they aren't used for a while and recreates them when they are needed. We don't want this behavior to increase the number of ThreadData instances in the process indefinitely over time. BUG=645196 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/f607a846cc997adf49d621388a3fd3ce9b6956e1 Cr-Commit-Position: refs/heads/master@{#436745} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/f607a846cc997adf49d621388a3fd3ce9b6956e1 Cr-Commit-Position: refs/heads/master@{#436745}
Message was sent while issue was closed.
siggi@chromium.org changed reviewers: + siggi@chromium.org
Message was sent while issue was closed.
I'm worried that the ThreadData naming scheme here will break the profiler. As we discussed, the profiler JS already folds "like threads" by default, where like threads are Foo/1, Foo/2 and so on. Can I ask you to go back to individual thread naming, where threads are named by their "kind" and an ordinal number, separated by slash? |