Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2319053004:
[Reland] Make canceling Timers fast. (Closed)
|
Created:
4 years, 3 months ago by alex clarke (OOO till 29th) Modified:
4 years, 3 months ago CC:
altimin, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, dglazkov+blink, esprehn, scheduler-bugs_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake canceling Timers fast.
base::Closure recently got an IsCancelled method. Taking advantage of
that the scheduler can short circuit a bunch of logic for cancelled
tasks and avoid running them and the rest of the task selection
machinery.
On the new TimerPerfTest benchmark this makes running 10000 cancelled
tasks aprox 50x - 60x faster (measured on Android and Linux).
Note this patch reverts many of the changes made in
https://codereview.chromium.org/2258713004 in favor of
WeakPtr based cancellation as favored by the base owners.
NOTE it's possible this might break
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN
I was not able to reproduce the ASAN failure locally however
I suspect it's due to the Timer getting swept away which
should be fixed by the change in the latest patchset.
BUG=605718, 638542, 645876
Committed: https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1
Committed: https://crrev.com/77f183aaf924ea7742df562455db8579607224e0
Cr-Original-Commit-Position: refs/heads/master@{#417621}
Cr-Commit-Position: refs/heads/master@{#417931}
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : Add missing file #
Total comments: 4
Patch Set 4 : Remove mojo dep from the RunAllPerfTests.cpp #Patch Set 5 : Address Sami's comments #Patch Set 6 : Getting printf to work with size_t cross platform is too hard, use C++ io streams instead #
Total comments: 16
Patch Set 7 : Responding to feedback #Patch Set 8 : Fix android linking problem #Patch Set 9 : Move TimerPerfTest into web/ as requested #Patch Set 10 : Revert change to third_party/WebKit/Source/platform/BUILD.gn #Patch Set 11 : Speculative fix, don't call m_weakPtrFactory.revokeAll if canFire is false. #Patch Set 12 : Rebased #Messages
Total messages: 78 (58 generated)
The CQ bit was checked by alexclarke@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...
Description was changed from ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circut a bunch of logic for cancelled tasks and avoid runnig them and the rest of the or the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). BUG=605718, 638542 ========== to ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circut a bunch of logic for cancelled tasks and avoid runnig them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). BUG=605718, 638542 ==========
Description was changed from ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circut a bunch of logic for cancelled tasks and avoid runnig them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). BUG=605718, 638542 ========== to ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). BUG=605718, 638542 ==========
alexclarke@chromium.org changed reviewers: + haraken@chromium.org, skyostil@chromium.org
PTAL
Description was changed from ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). BUG=605718, 638542 ========== to ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation favored by the base owners. BUG=605718, 638542 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by alexclarke@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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexclarke@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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 alexclarke@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...
lgtm https://codereview.chromium.org/2319053004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2319053004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:218: // Schedule a later call to MoveReadyDelayedTasksToDelayedWorkQueue.. nit: extra '.'? https://codereview.chromium.org/2319053004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2319053004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:280: if (queue->GetQuiescenceMonitored()) Might want to move this below the early-out.
All comments addressed https://codereview.chromium.org/2319053004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2319053004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:218: // Schedule a later call to MoveReadyDelayedTasksToDelayedWorkQueue.. On 2016/09/08 16:35:02, Sami wrote: > nit: extra '.'? Done. https://codereview.chromium.org/2319053004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2319053004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:280: if (queue->GetQuiescenceMonitored()) On 2016/09/08 16:35:02, Sami wrote: > Might want to move this below the early-out. Done.
The CQ bit was checked by alexclarke@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alexclarke@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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/Timer.cpp (right): https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/Timer.cpp:127: m_weakPtrFactory.revokeAll(); Maybe do we want to move this to above line 124? However, I'm not sure if we really need to call revokeAll() here. Given that it's guaranteed that only one timer is scheduled in the task queue (can we add DCHECK about this fact?), there should be no active weak pointers when runInternal() is fired. https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/TimerPerfTest.cpp (right): https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/TimerPerfTest.cpp:38: std::vector<std::unique_ptr<Timer<TimerPerfTest>>> timers(numIterations); Use Vector. https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/TimerPerfTest.cpp:40: timers[i] = base::MakeUnique<Timer<TimerPerfTest>>(this, &TimerPerfTest::nopTask); Use wrapUnique. https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/TimerPerfTest.cpp:56: std::cout << "TimerBase::startOneShot cost (us/call) "; Use LOG. https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:338: // Enqueue all delayed tasks that should be running now. Mention how cancelled tasks are handled. https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/work_queue.cc (right): https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/work_queue.cc:23: }*/ Can we remove this code? https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/RunAllPerfTests.cpp (right): https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/RunAllPerfTests.cpp:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Hmm, I'm not sure if it's worth introducing RunAllPerfTests. Can we just use webkit_unittests for TimerPerfTest?
PTAL https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/Timer.cpp (right): https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/Timer.cpp:127: m_weakPtrFactory.revokeAll(); On 2016/09/09 02:34:07, haraken wrote: > > Maybe do we want to move this to above line 124? > > However, I'm not sure if we really need to call revokeAll() here. Given that > it's guaranteed that only one timer is scheduled in the task queue (can we add > DCHECK about this fact?), there should be no active weak pointers when > runInternal() is fired. We do need to call it here to match the previous behavior of TimerBase::isActive https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/TimerPerfTest.cpp (right): https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/TimerPerfTest.cpp:38: std::vector<std::unique_ptr<Timer<TimerPerfTest>>> timers(numIterations); On 2016/09/09 02:34:07, haraken wrote: > > Use Vector. Done. https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/TimerPerfTest.cpp:40: timers[i] = base::MakeUnique<Timer<TimerPerfTest>>(this, &TimerPerfTest::nopTask); On 2016/09/09 02:34:07, haraken wrote: > > Use wrapUnique. Used reset in the end. https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/TimerPerfTest.cpp:56: std::cout << "TimerBase::startOneShot cost (us/call) "; On 2016/09/09 02:34:07, haraken wrote: > > Use LOG. Done. https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:338: // Enqueue all delayed tasks that should be running now. On 2016/09/09 02:34:07, haraken wrote: > > Mention how cancelled tasks are handled. Done. https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/work_queue.cc (right): https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/work_queue.cc:23: }*/ On 2016/09/09 02:34:07, haraken wrote: > > Can we remove this code? Ah I forgot about that. I've restored the original code. https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/RunAllPerfTests.cpp (right): https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/RunAllPerfTests.cpp:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/09/09 02:34:07, haraken wrote: > > Hmm, I'm not sure if it's worth introducing RunAllPerfTests. Can we just use > webkit_unittests for TimerPerfTest? I didn't think we were allowed to use things from web/ in platform, which suggests TimerPerfTest needs to move if we want to use webkit_unit_tests. So where should be move TimerPerfTest to? :)
The CQ bit was checked by alexclarke@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by alexclarke@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/2319053004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/RunAllPerfTests.cpp (right): https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/RunAllPerfTests.cpp:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/09/09 09:40:59, alex clarke wrote: > On 2016/09/09 02:34:07, haraken wrote: > > > > Hmm, I'm not sure if it's worth introducing RunAllPerfTests. Can we just use > > webkit_unittests for TimerPerfTest? > > I didn't think we were allowed to use things from web/ in platform, which > suggests TimerPerfTest needs to move if we want to use webkit_unit_tests. So > where should be move TimerPerfTest to? :) Sorry, where is TimerPerfTest using web/?
As discussed offline, let's move TimerPerfTest to web/tests/. With that fix, LGTM.
The CQ bit was checked by alexclarke@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...
alexclarke@chromium.org changed reviewers: + jochen@chromium.org
+jochen@ can you please review the change to third_party/WebKit/public/platform/WebTaskRunner.h
Description was changed from ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation favored by the base owners. BUG=605718, 638542 ========== to ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. BUG=605718, 638542 ==========
lgtm
The CQ bit was unchecked by alexclarke@chromium.org
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2319053004/#ps160001 (title: "Move TimerPerfTest into web/ as requested")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, jochen@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2319053004/#ps180001 (title: "Revert change to third_party/WebKit/Source/platform/BUILD.gn")
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 ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. BUG=605718, 638542 ========== to ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. BUG=605718, 638542 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. BUG=605718, 638542 ========== to ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. BUG=605718, 638542 Committed: https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1 Cr-Commit-Position: refs/heads/master@{#417621} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1 Cr-Commit-Position: refs/heads/master@{#417621}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/2326313003/ by fs@opera.com. The reason for reverting is: Wreaks havoc on the ASAN bots: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN STDERR: ================================================================= STDERR: ==4==ERROR: AddressSanitizer: use-after-poison on address 0x7ed9a5616190 at pc 0x00000768b0da bp 0x7fff4bbca630 sp 0x7fff4bbca628 STDERR: READ of size 8 at 0x7ed9a5616190 thread T0 (content_shell) STDERR: #0 0x768b0d9 in operator-> third_party/WebKit/Source/wtf/RefPtr.h:68:50 STDERR: #1 0x768b0d9 in revokeAll third_party/WebKit/Source/wtf/WeakPtr.h:146:0 STDERR: #2 0x768b344 in ?? third_party/WebKit/Source/platform/Timer.cpp:124:22 STDERR: #3 0x47a4461 in Run base/callback.h:56:12 STDERR: #4 0x47a4461 in RunTask base/debug/task_annotator.cc:54:0 STDERR: #5 0x79e2381 in ProcessTaskFromWorkQueue third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:316:19 ... STDERR: AddressSanitizer can not describe address in more detail (wild memory access suspected). STDERR: SUMMARY: AddressSanitizer: use-after-poison Appears to be accessing a "user-poison" area, so maybe a timer in something that was swept? (Wild guess.).
Message was sent while issue was closed.
Description was changed from ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. BUG=605718, 638542 Committed: https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1 Cr-Commit-Position: refs/heads/master@{#417621} ========== to ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. BUG=605718, 638542 Committed: https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1 Cr-Commit-Position: refs/heads/master@{#417621} ==========
The CQ bit was checked by alexclarke@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...
Description was changed from ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. BUG=605718, 638542 Committed: https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1 Cr-Commit-Position: refs/heads/master@{#417621} ========== to ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. BUG=605718, 638542, 645876 Committed: https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1 Cr-Commit-Position: refs/heads/master@{#417621} ==========
The CQ bit was checked by alexclarke@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/2319053004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/Timer.cpp (right): https://codereview.chromium.org/2319053004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/Timer.cpp:127: m_weakPtrFactory.revokeAll(); On 2016/09/09 09:40:58, alex clarke wrote: > On 2016/09/09 02:34:07, haraken wrote: > > > > Maybe do we want to move this to above line 124? > > > > However, I'm not sure if we really need to call revokeAll() here. Given that > > it's guaranteed that only one timer is scheduled in the task queue (can we add > > DCHECK about this fact?), there should be no active weak pointers when > > runInternal() is fired. > > We do need to call it here to match the previous behavior of TimerBase::isActive It looks moving this above caused the bot failures. Lets try with it below. Considering it's about to get swept away maybe the result of TimerBase::isActive doesn't matter?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm to retry with the revokeAll() change.
Description was changed from ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. BUG=605718, 638542, 645876 Committed: https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1 Cr-Commit-Position: refs/heads/master@{#417621} ========== to ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. NOTE it's possible this might break https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN I was not able to reproduce the ASAN failure locally however I suspect it's due to the Timer getting swept away which should be fixed by the chage in the latest patchset. BUG=605718, 638542, 645876 Committed: https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1 Cr-Commit-Position: refs/heads/master@{#417621} ==========
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2319053004/#ps220001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. NOTE it's possible this might break https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN I was not able to reproduce the ASAN failure locally however I suspect it's due to the Timer getting swept away which should be fixed by the chage in the latest patchset. BUG=605718, 638542, 645876 Committed: https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1 Cr-Commit-Position: refs/heads/master@{#417621} ========== to ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. NOTE it's possible this might break https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN I was not able to reproduce the ASAN failure locally however I suspect it's due to the Timer getting swept away which should be fixed by the change in the latest patchset. BUG=605718, 638542, 645876 Committed: https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1 Cr-Commit-Position: refs/heads/master@{#417621} ==========
Message was sent while issue was closed.
Description was changed from ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. NOTE it's possible this might break https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN I was not able to reproduce the ASAN failure locally however I suspect it's due to the Timer getting swept away which should be fixed by the change in the latest patchset. BUG=605718, 638542, 645876 Committed: https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1 Cr-Commit-Position: refs/heads/master@{#417621} ========== to ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. NOTE it's possible this might break https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN I was not able to reproduce the ASAN failure locally however I suspect it's due to the Timer getting swept away which should be fixed by the change in the latest patchset. BUG=605718, 638542, 645876 Committed: https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1 Cr-Commit-Position: refs/heads/master@{#417621} ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. NOTE it's possible this might break https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN I was not able to reproduce the ASAN failure locally however I suspect it's due to the Timer getting swept away which should be fixed by the change in the latest patchset. BUG=605718, 638542, 645876 Committed: https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1 Cr-Commit-Position: refs/heads/master@{#417621} ========== to ========== Make canceling Timers fast. base::Closure recently got an IsCancelled method. Taking advantage of that the scheduler can short circuit a bunch of logic for cancelled tasks and avoid running them and the rest of the task selection machinery. On the new TimerPerfTest benchmark this makes running 10000 cancelled tasks aprox 50x - 60x faster (measured on Android and Linux). Note this patch reverts many of the changes made in https://codereview.chromium.org/2258713004 in favor of WeakPtr based cancellation as favored by the base owners. NOTE it's possible this might break https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN I was not able to reproduce the ASAN failure locally however I suspect it's due to the Timer getting swept away which should be fixed by the change in the latest patchset. BUG=605718, 638542, 645876 Committed: https://crrev.com/e4e5868c5f32b015bf0d07a6eeace892d6a789a1 Committed: https://crrev.com/77f183aaf924ea7742df562455db8579607224e0 Cr-Original-Commit-Position: refs/heads/master@{#417621} Cr-Commit-Position: refs/heads/master@{#417931} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/77f183aaf924ea7742df562455db8579607224e0 Cr-Commit-Position: refs/heads/master@{#417931} |
