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

Issue 1204333002: Revert of Lazily constructed bespoke cancellable timer task. (Closed)

Created:
5 years, 6 months ago by alex clarke (OOO till 29th)
Modified:
5 years, 5 months ago
CC:
blink-reviews, oilpan-reviews
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Revert of Lazily constructed bespoke cancellable timer task. (patchset #3 id:100001 of https://codereview.chromium.org/1185643002/) Reason for revert: Looks like this is breaking: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fnotifications%2Fserviceworker-notificationclick-openwindow-crash.html Original issue's description: > Lazily constructed bespoke cancellable timer task. This is > a bit cheaper that using the CancellableTaskFactory and > goes part way towards fixing the performance regression in > dromaeo.jslibmodifyprototype. > > For perf results with this patch, see: > http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-06-23_10-45-33 and > http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-06-23_10-42-47 > > BUG=498229, 463143 > > Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197752 > > Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197814 TBR=jochen@chromium.org,sigbjornf@opera.com NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=498229, 463143

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -37 lines) Patch
M Source/platform/Timer.h View 5 chunks +13 lines, -27 lines 0 comments Download
M Source/platform/Timer.cpp View 4 chunks +4 lines, -10 lines 0 comments Download

Messages

Total messages: 5 (2 generated)
alex clarke (OOO till 29th)
Created Revert of Lazily constructed bespoke cancellable timer task.
5 years, 6 months ago (2015-06-25 16:16:16 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1204333002/1
5 years, 6 months ago (2015-06-25 16:16:51 UTC) #2
commit-bot: I haz the power
5 years, 6 months ago (2015-06-25 16:17:39 UTC) #4
Failed to apply patch for Source/platform/Timer.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/platform/Timer.h
  Hunk #3 FAILED at 87.
  Hunk #4 succeeded at 153 (offset 1 line).
  Hunk #5 succeeded at 187 (offset 1 line).
  1 out of 5 hunks FAILED -- saving rejects to file Source/platform/Timer.h.rej

Patch:       Source/platform/Timer.h
Index: Source/platform/Timer.h
diff --git a/Source/platform/Timer.h b/Source/platform/Timer.h
index
d5981e772583ea4c65736980b893026b94725301..60fec18a6b0356eff0326002f44bd70d8b777ba1
100644
--- a/Source/platform/Timer.h
+++ b/Source/platform/Timer.h
@@ -28,6 +28,7 @@
 
 #include "platform/PlatformExport.h"
 #include "platform/heap/Handle.h"
+#include "platform/scheduler/CancellableTaskFactory.h"
 #include "public/platform/WebTraceLocation.h"
 #include "wtf/AddressSanitizer.h"
 #include "wtf/Noncopyable.h"
@@ -71,6 +72,11 @@
 
     void didChangeAlignmentInterval(double now);
 
+#if defined(ADDRESS_SANITIZER)
+protected:
+    CancellableTaskFactory& cancellableTaskFactory() { return
m_cancellableTaskFactory; }
+#endif
+
 private:
     virtual void fired() = 0;
 
@@ -81,37 +87,13 @@
 
     void setNextFireTime(double now, double delay);
 
-    void runInternal();
-
-    class CancellableTimerTask final : public WebThread::Task {
-        WTF_MAKE_NONCOPYABLE(CancellableTimerTask);
-    public:
-        explicit CancellableTimerTask(TimerBase* timer) : m_timer(timer) { }
-
-        ~CancellableTimerTask() override { }
-
-        void run() override
-        {
-            if (m_timer) {
-                m_timer->m_cancellableTimerTask = nullptr;
-                m_timer->runInternal();
-            }
-        }
-
-        void cancel()
-        {
-            m_timer = nullptr;
-        }
-
-    private:
-        TimerBase* m_timer; // NOT OWNED
-    };
+    void run();
 
     double m_nextFireTime; // 0 if inactive
     double m_unalignedNextFireTime; // m_nextFireTime not considering alignment
interval
     double m_repeatInterval; // 0 if not repeating
     WebTraceLocation m_location;
-    CancellableTimerTask* m_cancellableTimerTask; // NOT OWNED
+    CancellableTaskFactory m_cancellableTaskFactory;
     WebScheduler* m_webScheduler; // Not owned.
 
 #if ENABLE(ASSERT)
@@ -146,6 +128,10 @@
     Timer(TimerFiredClass* o, TimerFiredFunction f)
         : m_object(o), m_function(f)
     {
+#if ENABLE(LAZY_SWEEPING) && defined(ADDRESS_SANITIZER)
+        if (IsGarbageCollectedType<TimerFiredClass>::value)
+            cancellableTaskFactory().setUnpoisonBeforeUpdate();
+#endif
     }
 
 protected:
@@ -176,7 +162,7 @@
 inline bool TimerBase::isActive() const
 {
     ASSERT(m_thread == currentThread());
-    return m_cancellableTimerTask;
+    return m_cancellableTaskFactory.isPending();
 }
 
 } // namespace blink

Powered by Google App Engine
This is Rietveld 408576698