Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2197753002:
Move OomKillsMonitor from a single-threaded SequencedWorkerPool to a non-joinable thread. (Closed)
|
Created:
4 years, 4 months ago by gab Modified:
4 years, 4 months ago Reviewers:
Luis Héctor Chávez CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@a0_b1_nonjoinable_thread Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove OomKillsMonitor from a single-threaded SequencedWorkerPool to a non-joinable thread.
The semantics are equivalent after this CL:
- SequencedWorkerPool::CONTINUE_ON_SHUTDOWN => non-joinable thread + leaky instance.
- Actually, it's better than before because with lack of a Leaky lazy instance,
there was a join despite the CONTINUE_ON_SHUTDOWN request [1].
- Ramping worker thread down on destruction through OomKillsMonitorHandle.
[1] https://cs.chromium.org/chromium/src/base/threading/sequenced_worker_pool.cc?rcl=1469791693&l=601
BUG=622400
Committed: https://crrev.com/620b836c478741d8eec1ef5255f909e6d7e9b649
Cr-Commit-Position: refs/heads/master@{#411899}
Patch Set 1 #
Total comments: 14
Patch Set 2 : review:lhchavez #13 #
Total comments: 4
Patch Set 3 : update Dependent Patchset bit #Patch Set 4 : use SimpleThread instead of Thread #
Total comments: 2
Patch Set 5 : nit #Patch Set 6 : rebase dependent #
Depends on Patchset: Messages
Total messages: 60 (50 generated)
The CQ bit was checked by gab@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...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from
==========
Move OomKillsMonitor from a single-threaded SequencedWorkerPool to a
non-joinable thread.
BUG=622400
==========
to
==========
Move OomKillsMonitor from a single-threaded SequencedWorkerPool to a
non-joinable thread.
The semantics are equivalent after this CL:
- SequencedWorkerPool::CONTINUE_ON_SHUTDOWN => non-joinable thread + leaky
instance.
- Actually, it's better than before because with lack of a Leaky lazy
instance,
there was a join despite the CONTINUE_ON_SHUTDOWN request [1].
- Ramping worker thread down on destruction through OomKillsMonitorHandle.
[1]
https://cs.chromium.org/chromium/src/base/threading/sequenced_worker_pool.cc?...
BUG=622400
==========
gab@chromium.org changed reviewers: + lhchavez@chromium.org
Luis PTAL, the goal is to get rid of usage of SequencedWorkerPool for single-threaded use cases that won't map nicely to the imminent TaskScheduler migration. PS: CQ dry run is having problems with multiple stacked CLs but for the sake of this review, assume base::Thread::Options::joinable exists with the semantics in https://codereview.chromium.org/2135413003/. Thanks!
Thanks for improving this! https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... File components/arc/metrics/arc_metrics_service.cc (right): https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... components/arc/metrics/arc_metrics_service.cc:34: oom_kills_monitor_handle_ = OomKillsMonitor::StartMonitoring(); Can this be initialized in the initializer list? https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... File components/arc/metrics/oom_kills_monitor.cc (right): https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... components/arc/metrics/oom_kills_monitor.cc:142: OomKillsMonitor* outer) : outer_(outer) {} DCHECK(outer_); https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... File components/arc/metrics/oom_kills_monitor.h (right): https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... components/arc/metrics/oom_kills_monitor.h:30: OomKillsMonitorHandle() = default; Can this be removed? https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... components/arc/metrics/oom_kills_monitor.h:34: OomKillsMonitorHandle(OomKillsMonitor* outer); explicit https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... components/arc/metrics/oom_kills_monitor.h:39: OomKillsMonitor* outer_ = nullptr; Can this be OomKilssMonitor* const outer_;? https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... components/arc/metrics/oom_kills_monitor.h:40: } Can this be DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... components/arc/metrics/oom_kills_monitor.h:47: static void OomKillsMonitorHandle StartMonitoring(); static OomKillsMonitorHandle StartMonitoring(); ?
The CQ bit was checked by gab@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...
Thanks, done, PTAnL https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... File components/arc/metrics/arc_metrics_service.cc (right): https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... components/arc/metrics/arc_metrics_service.cc:34: oom_kills_monitor_handle_ = OomKillsMonitor::StartMonitoring(); On 2016/07/29 21:48:45, Luis Héctor Chávez wrote: > Can this be initialized in the initializer list? Done. https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... File components/arc/metrics/oom_kills_monitor.cc (right): https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... components/arc/metrics/oom_kills_monitor.cc:142: OomKillsMonitor* outer) : outer_(outer) {} On 2016/07/29 21:48:45, Luis Héctor Chávez wrote: > DCHECK(outer_); Done. https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... File components/arc/metrics/oom_kills_monitor.h (right): https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... components/arc/metrics/oom_kills_monitor.h:30: OomKillsMonitorHandle() = default; On 2016/07/29 21:48:45, Luis Héctor Chávez wrote: > Can this be removed? Now that it's created in initializer list, yes :-) https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... components/arc/metrics/oom_kills_monitor.h:34: OomKillsMonitorHandle(OomKillsMonitor* outer); On 2016/07/29 21:48:45, Luis Héctor Chávez wrote: > explicit Done. https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... components/arc/metrics/oom_kills_monitor.h:39: OomKillsMonitor* outer_ = nullptr; On 2016/07/29 21:48:45, Luis Héctor Chávez wrote: > Can this be OomKilssMonitor* const outer_;? Done. https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... components/arc/metrics/oom_kills_monitor.h:40: } On 2016/07/29 21:48:45, Luis Héctor Chávez wrote: > Can this be DISALLOW_COPY_AND_ASSIGN? Now that it's in initializer list, yes :-) -- and it makes sense because we wouldn't want destructor invoked on copy! https://codereview.chromium.org/2197753002/diff/20001/components/arc/metrics/... components/arc/metrics/oom_kills_monitor.h:47: static void OomKillsMonitorHandle StartMonitoring(); On 2016/07/29 21:48:45, Luis Héctor Chávez wrote: > static OomKillsMonitorHandle StartMonitoring(); ? Oops yes, not sure how that compiled..?!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
just two nits. Will l-g-t-m once trybots are green. https://codereview.chromium.org/2197753002/diff/40001/components/arc/metrics/... File components/arc/metrics/oom_kills_monitor.cc (right): https://codereview.chromium.org/2197753002/diff/40001/components/arc/metrics/... components/arc/metrics/oom_kills_monitor.cc:160: } return OomKillsMonitorHandle(instance); ? https://codereview.chromium.org/2197753002/diff/40001/components/arc/metrics/... File components/arc/metrics/oom_kills_monitor.h (right): https://codereview.chromium.org/2197753002/diff/40001/components/arc/metrics/... components/arc/metrics/oom_kills_monitor.h:36: OomKillsMonitor* const outer_; Now that I think of it, doesn't this also need a friend declaration since |is_shutting_down_| is private? If the compiler doesn't like this code as-is, rather than adding a friend declaration, can you just pass in the reference to |is_shutting_down_|?
The CQ bit was checked by gab@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by gab@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 gab@chromium.org to run a CQ dry run
Patchset #4 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Luis PTAnL, actually used a CrOS build this time in light of my inability to code without a compiler ;-). Made me realize (since overriding Run() without arguments doesn't work for base::Thread) that it makes more sense to use base::SimpleThread here per not needing a MessageLoop. Thanks! Gab PS: How can I test this? Just make sure metrics are still reported after it lands? https://codereview.chromium.org/2197753002/diff/40001/components/arc/metrics/... File components/arc/metrics/oom_kills_monitor.cc (right): https://codereview.chromium.org/2197753002/diff/40001/components/arc/metrics/... components/arc/metrics/oom_kills_monitor.cc:160: } On 2016/08/01 15:48:14, Luis Héctor Chávez (OOO 8-7) wrote: > return OomKillsMonitorHandle(instance); ? Yea I was trying to do this on my Windows machine... clearly a failure looking at the bots. Sprung up my CrOS build on Linux now :-) https://codereview.chromium.org/2197753002/diff/40001/components/arc/metrics/... File components/arc/metrics/oom_kills_monitor.h (right): https://codereview.chromium.org/2197753002/diff/40001/components/arc/metrics/... components/arc/metrics/oom_kills_monitor.h:36: OomKillsMonitor* const outer_; On 2016/08/01 15:48:14, Luis Héctor Chávez (OOO 8-7) wrote: > Now that I think of it, doesn't this also need a friend declaration since > |is_shutting_down_| is private? If the compiler doesn't like this code as-is, > rather than adding a friend declaration, can you just pass in the reference to > |is_shutting_down_|? Inner classes have access to their Outer class' private state (as they do in Java, the main difference with Java is that in C++ you need a pointer to your Outer instead of being able to use the fields directly -- because in C++ this wouldn't make sense per lack of automatic refcounting).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits. Unfortunately this doesn't have unit tests, so the only way to test this is to flash an Android-compatible Chrome OS device with this and see if the metrics keep flowing. https://codereview.chromium.org/2197753002/diff/100001/components/arc/metrics... File components/arc/metrics/oom_kills_monitor.cc (right): https://codereview.chromium.org/2197753002/diff/100001/components/arc/metrics... components/arc/metrics/oom_kills_monitor.cc:151: non_joinable_worker_thread_ = base::WrapUnique(new base::DelegateSimpleThread( nit: base::MakeUnique<base::DelegateSimpleThread>(...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Thanks done, FYI waiting on reland of https://codereview.chromium.org/2204333003 to land this if you're wondering :-). https://codereview.chromium.org/2197753002/diff/100001/components/arc/metrics... File components/arc/metrics/oom_kills_monitor.cc (right): https://codereview.chromium.org/2197753002/diff/100001/components/arc/metrics... components/arc/metrics/oom_kills_monitor.cc:151: non_joinable_worker_thread_ = base::WrapUnique(new base::DelegateSimpleThread( On 2016/08/08 14:29:33, Luis Héctor Chávez wrote: > nit: base::MakeUnique<base::DelegateSimpleThread>(...) Nice, didn't know about MakeUnique, thanks!
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 gab@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 gab@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...
Patchset #6 (id:130001) has been deleted
The CQ bit was checked by gab@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 gab@chromium.org to run a CQ dry run
Patchset #6 (id:150001) has been deleted
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.
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2197753002/#ps170001 (title: "rebase dependent")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Move OomKillsMonitor from a single-threaded SequencedWorkerPool to a
non-joinable thread.
The semantics are equivalent after this CL:
- SequencedWorkerPool::CONTINUE_ON_SHUTDOWN => non-joinable thread + leaky
instance.
- Actually, it's better than before because with lack of a Leaky lazy
instance,
there was a join despite the CONTINUE_ON_SHUTDOWN request [1].
- Ramping worker thread down on destruction through OomKillsMonitorHandle.
[1]
https://cs.chromium.org/chromium/src/base/threading/sequenced_worker_pool.cc?...
BUG=622400
==========
to
==========
Move OomKillsMonitor from a single-threaded SequencedWorkerPool to a
non-joinable thread.
The semantics are equivalent after this CL:
- SequencedWorkerPool::CONTINUE_ON_SHUTDOWN => non-joinable thread + leaky
instance.
- Actually, it's better than before because with lack of a Leaky lazy
instance,
there was a join despite the CONTINUE_ON_SHUTDOWN request [1].
- Ramping worker thread down on destruction through OomKillsMonitorHandle.
[1]
https://cs.chromium.org/chromium/src/base/threading/sequenced_worker_pool.cc?...
BUG=622400
==========
Message was sent while issue was closed.
Committed patchset #6 (id:170001)
Message was sent while issue was closed.
Description was changed from
==========
Move OomKillsMonitor from a single-threaded SequencedWorkerPool to a
non-joinable thread.
The semantics are equivalent after this CL:
- SequencedWorkerPool::CONTINUE_ON_SHUTDOWN => non-joinable thread + leaky
instance.
- Actually, it's better than before because with lack of a Leaky lazy
instance,
there was a join despite the CONTINUE_ON_SHUTDOWN request [1].
- Ramping worker thread down on destruction through OomKillsMonitorHandle.
[1]
https://cs.chromium.org/chromium/src/base/threading/sequenced_worker_pool.cc?...
BUG=622400
==========
to
==========
Move OomKillsMonitor from a single-threaded SequencedWorkerPool to a
non-joinable thread.
The semantics are equivalent after this CL:
- SequencedWorkerPool::CONTINUE_ON_SHUTDOWN => non-joinable thread + leaky
instance.
- Actually, it's better than before because with lack of a Leaky lazy
instance,
there was a join despite the CONTINUE_ON_SHUTDOWN request [1].
- Ramping worker thread down on destruction through OomKillsMonitorHandle.
[1]
https://cs.chromium.org/chromium/src/base/threading/sequenced_worker_pool.cc?...
BUG=622400
Committed: https://crrev.com/620b836c478741d8eec1ef5255f909e6d7e9b649
Cr-Commit-Position: refs/heads/master@{#411899}
==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/620b836c478741d8eec1ef5255f909e6d7e9b649 Cr-Commit-Position: refs/heads/master@{#411899} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
