Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2398753003:
Use FileDescriptorWatcher in AlarmTimer. (Closed)
|
Created:
4 years, 2 months ago by fdoray Modified:
4 years, 2 months ago Reviewers:
Chirantan Ekbote CC:
chromium-reviews, chirantan+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse FileDescriptorWatcher in AlarmTimer.
FileDescriptorWatcher is a new API that replaces
MessageLoopForIO::WatchFileDescriptor.
This CL also gets rid of the base::Thread created in the anonymous
namespace of alarm_timer_chromeos.cc. The FileDescriptorWatcher
API is supported by every TaskScheduler thread. Very soon, most
tasks in Chrome will run in TaskScheduler. Therefore, there is no
reason to create a base::Thread to allow AlarmTimer to be used from
threads that don't support the FileDescriptorWatcher API.
BUG=645114
Committed: https://crrev.com/8e17d7e9031b7e2e58473fe66a399943051357f0
Cr-Commit-Position: refs/heads/master@{#423617}
Patch Set 1 #Patch Set 2 : fix build error #
Total comments: 2
Messages
Total messages: 21 (13 generated)
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: 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 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.
Description was changed from ========== Use FileDescriptorWatcher in AlarmTimer. FileDescriptorWatcher is a new API that replaces MessageLoopForIO::WatchFileDescriptor. Note that with this CL, an AlarmTimer instance can only be used from the sequence on which it was instantiated. Also, Start() and Stop() must be called from a thread that supports FileDescriptorWatcher. BUG=645114 ========== to ========== Use FileDescriptorWatcher in AlarmTimer. FileDescriptorWatcher is a new API that replaces MessageLoopForIO::WatchFileDescriptor. This CL also gets rid of the base::Thread created in the anonymous namespace of alarm_timer_chromeos.cc. The FileDescriptorWatcher API is supported by every TaskScheduler thread. Very soon, most tasks in Chrome will run in TaskScheduler. Therefore, there is no reason to create a base::Thread to allow AlarmTimer to be used from threads that don't support the FileDescriptorWatcher API. BUG=645114 ==========
fdoray@chromium.org changed reviewers: + chirantan@chromium.org
PTAL
lgtm with one question. Thanks for doing this! https://codereview.chromium.org/2398753003/diff/20001/components/timers/alarm... File components/timers/alarm_timer_chromeos.cc (right): https://codereview.chromium.org/2398753003/diff/20001/components/timers/alarm... components/timers/alarm_timer_chromeos.cc:25: alarm_fd_(timerfd_create(CLOCK_REALTIME_ALARM, 0)), Does this mean it's now ok to make blocking system calls from the UI thread? If so, that's a bit surprising to me.
https://codereview.chromium.org/2398753003/diff/20001/components/timers/alarm... File components/timers/alarm_timer_chromeos.cc (right): https://codereview.chromium.org/2398753003/diff/20001/components/timers/alarm... components/timers/alarm_timer_chromeos.cc:25: alarm_fd_(timerfd_create(CLOCK_REALTIME_ALARM, 0)), On 2016/10/06 18:41:51, Chirantan Ekbote wrote: > Does this mean it's now ok to make blocking system calls from the UI thread? If > so, that's a bit surprising to me. No it's not ok. After the migration to TaskScheduler, the UI thread will be the only thread that doesn't support the FileDescriptorWatcher API (and thus it won't be possible to use AlarmTimer from it). Note that AlarmTimer is currently only used from the IO thread https://cs.chromium.org/chromium/src/components/gcm_driver/gcm_driver_desktop...
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...
On 2016/10/06 18:57:34, fdoray wrote: > https://codereview.chromium.org/2398753003/diff/20001/components/timers/alarm... > File components/timers/alarm_timer_chromeos.cc (right): > > https://codereview.chromium.org/2398753003/diff/20001/components/timers/alarm... > components/timers/alarm_timer_chromeos.cc:25: > alarm_fd_(timerfd_create(CLOCK_REALTIME_ALARM, 0)), > On 2016/10/06 18:41:51, Chirantan Ekbote wrote: > > Does this mean it's now ok to make blocking system calls from the UI thread? > If > > so, that's a bit surprising to me. > > No it's not ok. After the migration to TaskScheduler, the UI thread will be the > only thread that doesn't support the FileDescriptorWatcher API (and thus it > won't be possible to use AlarmTimer from it). > > Note that AlarmTimer is currently only used from the IO thread > https://cs.chromium.org/chromium/src/components/gcm_driver/gcm_driver_desktop... It's also used by a couple of chrome os daemons: shill and powerd. I know powerd uses a MessageLoopForIO but I'm not sure about shill.
Message was sent while issue was closed.
Description was changed from ========== Use FileDescriptorWatcher in AlarmTimer. FileDescriptorWatcher is a new API that replaces MessageLoopForIO::WatchFileDescriptor. This CL also gets rid of the base::Thread created in the anonymous namespace of alarm_timer_chromeos.cc. The FileDescriptorWatcher API is supported by every TaskScheduler thread. Very soon, most tasks in Chrome will run in TaskScheduler. Therefore, there is no reason to create a base::Thread to allow AlarmTimer to be used from threads that don't support the FileDescriptorWatcher API. BUG=645114 ========== to ========== Use FileDescriptorWatcher in AlarmTimer. FileDescriptorWatcher is a new API that replaces MessageLoopForIO::WatchFileDescriptor. This CL also gets rid of the base::Thread created in the anonymous namespace of alarm_timer_chromeos.cc. The FileDescriptorWatcher API is supported by every TaskScheduler thread. Very soon, most tasks in Chrome will run in TaskScheduler. Therefore, there is no reason to create a base::Thread to allow AlarmTimer to be used from threads that don't support the FileDescriptorWatcher API. BUG=645114 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Use FileDescriptorWatcher in AlarmTimer. FileDescriptorWatcher is a new API that replaces MessageLoopForIO::WatchFileDescriptor. This CL also gets rid of the base::Thread created in the anonymous namespace of alarm_timer_chromeos.cc. The FileDescriptorWatcher API is supported by every TaskScheduler thread. Very soon, most tasks in Chrome will run in TaskScheduler. Therefore, there is no reason to create a base::Thread to allow AlarmTimer to be used from threads that don't support the FileDescriptorWatcher API. BUG=645114 ========== to ========== Use FileDescriptorWatcher in AlarmTimer. FileDescriptorWatcher is a new API that replaces MessageLoopForIO::WatchFileDescriptor. This CL also gets rid of the base::Thread created in the anonymous namespace of alarm_timer_chromeos.cc. The FileDescriptorWatcher API is supported by every TaskScheduler thread. Very soon, most tasks in Chrome will run in TaskScheduler. Therefore, there is no reason to create a base::Thread to allow AlarmTimer to be used from threads that don't support the FileDescriptorWatcher API. BUG=645114 Committed: https://crrev.com/8e17d7e9031b7e2e58473fe66a399943051357f0 Cr-Commit-Position: refs/heads/master@{#423617} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8e17d7e9031b7e2e58473fe66a399943051357f0 Cr-Commit-Position: refs/heads/master@{#423617}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2394333002/ by nasko@chromium.org. The reason for reverting is: This CL is causing a memory leak in the unit tests: https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... failures: AlarmTimerTest.RetainNonRepeatIsRunning AlarmTimerTest.MessageLoopShutdown AlarmTimerTest.NonRepeatIsRunning AlarmTimerTest.RetainRepeatIsRunning Sample report: Direct leak of 152 byte(s) in 1 object(s) allocated from: #0 0xab8ebb in operator new(unsigned long) (/b/swarm_slave/w/irlnnUGq/out/Release/components_unittests+0xab8ebb) #1 0x9eba8fd in MakeUnique<base::FileDescriptorWatcher::Controller::Watcher, base::WeakPtr<base::FileDescriptorWatcher::Controller>, base::MessageLoopForIO::Mode &, int &> base/memory/ptr_util.h:56:29 #2 0x9eba8fd in base::FileDescriptorWatcher::Controller::Controller(base::MessageLoopForIO::Mode, int, base::Callback<void (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) base/files/file_descriptor_watcher_posix.cc:159 #3 0x9ebb293 in base::FileDescriptorWatcher::WatchReadable(int, base::Callback<void (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) base/files/file_descriptor_watcher_posix.cc:198:25 #4 0xe0016a3 in timers::AlarmTimer::Reset() components/timers/alarm_timer_chromeos.cc:103:25 #5 0x515b488 in timers::AlarmTimerTest_RetainRepeatIsRunning_Test::TestBody() components/timers/alarm_timer_unittest.cc:310:9. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
