Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(350)

Issue 2488073002: Reuse ThreadData instances associated with terminated named threads. (Closed)

Created:
4 years, 1 month ago by fdoray
Modified:
4 years ago
CC:
chromium-reviews, arv+watch_chromium.org, asvitkine+watch_chromium.org, gab, robliao
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+308 lines, -233 lines) Patch
M base/tracked_objects.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 10 chunks +88 lines, -93 lines 0 comments Download
M base/tracked_objects.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 16 chunks +85 lines, -66 lines 0 comments Download
M base/tracked_objects_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 26 chunks +87 lines, -27 lines 0 comments Download
M chrome/browser/task_profiler/task_profiler_data_serializer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/task_profiler/task_profiler_data_serializer_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +5 lines, -5 lines 0 comments Download
M components/metrics/profiler/profiler_metrics_provider.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +2 lines, -20 lines 0 comments Download
M components/metrics/profiler/profiler_metrics_provider_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 9 chunks +21 lines, -12 lines 0 comments Download
M components/metrics/profiler/tracking_synchronizer_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +6 lines, -6 lines 0 comments Download
M content/common/child_process_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +2 lines, -2 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 100 (69 generated)
fdoray
PTAL
4 years, 1 month ago (2016-11-10 15:38:33 UTC) #5
Nico
this doesn't build
4 years, 1 month ago (2016-11-10 18:51:41 UTC) #16
Alexei Svitkine (slow)
+isherman since this is related to the task profiler
4 years, 1 month ago (2016-11-11 01:43:52 UTC) #22
fdoray
thakis@: Now the CL passes the CQ dry run. PTAL.
4 years, 1 month ago (2016-11-11 16:34:02 UTC) #35
Ilya Sherman
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.cc#newcode803 base/tracked_objects.cc:803: // Assuming that there aren't ...
4 years, 1 month ago (2016-11-12 01:21:16 UTC) #38
fdoray
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.cc#newcode803 base/tracked_objects.cc:803: // Assuming that there aren't more than a few ...
4 years, 1 month ago (2016-11-14 14:32:07 UTC) #41
fdoray
Nico: PTAL
4 years, 1 month ago (2016-11-15 16:33:20 UTC) #45
fdoray
+danakj@: PTAL (Nico OOO)
4 years, 1 month ago (2016-11-16 19:03:58 UTC) #51
danakj
> BUG= Surely there's a bug tracking this work? If not please file one.
4 years, 1 month ago (2016-11-16 21:41:43 UTC) #52
fdoray
On 2016/11/16 21:41:43, danakj wrote: > > BUG= > > Surely there's a bug tracking ...
4 years, 1 month ago (2016-11-16 21:56:02 UTC) #54
danakj
First quick pass. If Nico is more familiar it might be faster to have him ...
4 years, 1 month ago (2016-11-17 02:26:36 UTC) #55
fdoray
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#newcode98 base/tracked_objects.h:98: // destroyed (dies). Since a ThreadData is ...
4 years, 1 month ago (2016-11-17 13:20:19 UTC) #58
fdoray
mark@ or danakj@: I have a hard time getting this reviewed by Nico. Could you ...
4 years ago (2016-11-23 13:07:03 UTC) #62
danakj
On Wed, Nov 23, 2016 at 5:07 AM, <fdoray@chromium.org> wrote: > mark@ or danakj@: I ...
4 years ago (2016-11-23 19:19:28 UTC) #63
Nico
looking onw
4 years ago (2016-11-24 00:45:39 UTC) #64
Nico
Sorry about the delay. I wasn't familiar with this code either and I didn't have ...
4 years ago (2016-11-24 01:42:05 UTC) #65
fdoray
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.cc#newcode327 base/tracked_objects.cc:327: PushToHeadOfList(); // Which sets real incarnation_count_for_pool_. On 2016/11/24 01:42:04, ...
4 years ago (2016-11-29 16:10:33 UTC) #74
fdoray
Nico: PTAnL
4 years ago (2016-11-29 16:10:57 UTC) #75
Nico
lgtm, thanks for the update. I'm still worried about how expensive the lock/loop will be ...
4 years ago (2016-12-02 03:19:30 UTC) #76
Ilya Sherman
histograms lgtm
4 years ago (2016-12-02 03:23:11 UTC) #77
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2488073002/260001
4 years ago (2016-12-02 12:42:40 UTC) #79
commit-bot: I haz the power
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_presubmit/builds/317345)
4 years ago (2016-12-02 12:49:56 UTC) #81
fdoray
palmer@: Please review changes in content/common/child_process_messages.h
4 years ago (2016-12-02 13:23:08 UTC) #83
fdoray
wfh@: Please review changes in content/common/child_process_messages.h (No reply from palmer@)
4 years ago (2016-12-06 13:53:16 UTC) #85
Will Harris
content/common/child_process_messages.h lgtm
4 years ago (2016-12-06 19:09:55 UTC) #86
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2488073002/260001
4 years ago (2016-12-06 19:10:46 UTC) #88
commit-bot: I haz the power
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/builds/119124) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, ...
4 years ago (2016-12-06 19:14:02 UTC) #90
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2488073002/280001
4 years ago (2016-12-06 20:26:45 UTC) #93
commit-bot: I haz the power
Committed patchset #15 (id:280001)
4 years ago (2016-12-06 21:45:20 UTC) #96
commit-bot: I haz the power
Patchset 15 (id:??) landed as https://crrev.com/f607a846cc997adf49d621388a3fd3ce9b6956e1 Cr-Commit-Position: refs/heads/master@{#436745}
4 years ago (2016-12-06 21:47:49 UTC) #98
Sigurður Ásgeirsson
4 years ago (2016-12-08 13:53:08 UTC) #100
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?

Powered by Google App Engine
This is Rietveld 408576698