[BlackBerry] LayerAnimation is not immutable, which makes dereferencing an expensive...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 14 Aug 2012 12:39:17 +0000 (12:39 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 14 Aug 2012 12:39:17 +0000 (12:39 +0000)
https://bugs.webkit.org/show_bug.cgi?id=93946

Patch by Ed Baker <edbaker@rim.com> on 2012-08-14
Reviewed by Antonio Gomes.

Make LayerAnimation immutable so it can be dereferenced from the main
WebKit thread without having to dispatch to the compositing thread,
which is an expensive operation.

TransformOperation and TimingFunction need to be made thread safe as
they are referenced in LayerAnimation, but that effort is being tracked
by a separate bug, #86483.

Reviewed internally by Arvid Nilsson.

No change in behavior, no new tests.

* platform/graphics/blackberry/LayerAnimation.h:
(WebCore::LayerAnimation::name):
(LayerAnimation):
(WebCore::LayerAnimation::LayerAnimation):
(WebCore::LayerAnimation::setName):
* platform/graphics/blackberry/LayerCompositingThread.cpp:
(WebCore):
* platform/graphics/blackberry/LayerCompositingThread.h:
(LayerCompositingThread):
* platform/graphics/blackberry/LayerWebKitThread.cpp:
(WebCore::LayerWebKitThread::~LayerWebKitThread):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@125552 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/blackberry/LayerAnimation.h
Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp
Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h
Source/WebCore/platform/graphics/blackberry/LayerWebKitThread.cpp

index b5d791a..47fbaa5 100644 (file)
@@ -1,3 +1,34 @@
+2012-08-14  Ed Baker  <edbaker@rim.com>
+
+        [BlackBerry] LayerAnimation is not immutable, which makes dereferencing an expensive operation
+        https://bugs.webkit.org/show_bug.cgi?id=93946
+
+        Reviewed by Antonio Gomes.
+
+        Make LayerAnimation immutable so it can be dereferenced from the main
+        WebKit thread without having to dispatch to the compositing thread,
+        which is an expensive operation.
+
+        TransformOperation and TimingFunction need to be made thread safe as
+        they are referenced in LayerAnimation, but that effort is being tracked
+        by a separate bug, #86483.
+
+        Reviewed internally by Arvid Nilsson.
+
+        No change in behavior, no new tests.
+
+        * platform/graphics/blackberry/LayerAnimation.h:
+        (WebCore::LayerAnimation::name):
+        (LayerAnimation):
+        (WebCore::LayerAnimation::LayerAnimation):
+        (WebCore::LayerAnimation::setName):
+        * platform/graphics/blackberry/LayerCompositingThread.cpp:
+        (WebCore):
+        * platform/graphics/blackberry/LayerCompositingThread.h:
+        (LayerCompositingThread):
+        * platform/graphics/blackberry/LayerWebKitThread.cpp:
+        (WebCore::LayerWebKitThread::~LayerWebKitThread):
+
 2012-08-14  Arvid Nilsson  <anilsson@rim.com>
 
         [BlackBerry] GraphicsLayerBlackBerry::willBeDestroyed() must call superclass implementation
index c486fe5..2c61b21 100644 (file)
@@ -34,17 +34,8 @@ class Animation;
 class LayerCompositingThread;
 class TransformationMatrix;
 
-// This class uses non-threadsafe refcounting in the WebCore::Animation and
-// WebCore::String members, so using threadsafe refcounting here would be a big
-// cover-up. Instead, you must be careful to use ref/deref this class only on
-// the WebKit thread, or when the WebKit and compositing threads are in sync.
-class LayerAnimation : public RefCounted<LayerAnimation> {
+class LayerAnimation : public ThreadSafeRefCounted<LayerAnimation> {
 public:
-    // WebKit thread only
-    // Because the refcounting done in constructor and destructor is not thread safe,
-    // the LayerAnimation must be created or destroyed on the WebKit thread, or when
-    // the WebKit and compositing threads are in sync.
-    // Also, the name property is using a String which has non-threadsafe refcounting.
     // The setStartTime method is not threadsafe and must only be called on a newly
     // created LayerAnimation before sending it off to the compositing thread.
     static PassRefPtr<LayerAnimation> create(const KeyframeValueList& values, const IntSize& boxSize, const Animation* animation, const String& name, double timeOffset)
@@ -65,7 +56,13 @@ public:
     {
     }
 
-    const String& name() const { return m_name; }
+    String name() const
+    {
+        if (m_name.isEmpty())
+            return String("");
+        return String(m_name);
+    }
+
     void setStartTime(double time) { m_startTime = time; }
 
     // These functions are thread safe (immutable state).
@@ -77,14 +74,10 @@ public:
     double timeOffset() const { return m_timeOffset; }
     double startTime() const { return m_startTime; }
     size_t valueCount() const { return m_values.size(); }
-
-    // NOTE: Don't try to ref a TimingFunction, that's not a threadsafe operation.
     const TimingFunction* timingFunction() const { return m_timingFunction.get(); }
     double duration() const { return m_duration; }
     int iterationCount() const { return m_iterationCount; }
     Animation::AnimationDirection direction() const { return m_direction; }
-
-    // NOTE: Don't try to clone() an AnimationValue, that's not a threadsafe operation since it mutates refcounts.
     const AnimationValue* valueAt(size_t i) const { return m_values.at(i); }
     bool finished() const { return m_finished; }
 
@@ -97,7 +90,6 @@ private:
         : m_id(reinterpret_cast<int>(animation))
         , m_values(values)
         , m_boxSize(boxSize)
-        , m_name(name)
         , m_timeOffset(timeOffset)
         , m_startTime(0)
         , m_timingFunction(0)
@@ -110,14 +102,14 @@ private:
             m_timingFunction = animation->timingFunction();
 
         validateTransformLists();
+        setName(name);
     }
 
     LayerAnimation(const LayerAnimation& other)
-        :  RefCounted<LayerAnimation>()
+        :  ThreadSafeRefCounted<LayerAnimation>()
         , m_id(other.m_id)
         , m_values(other.m_values)
         , m_boxSize(other.m_boxSize)
-        , m_name(other.m_name)
         , m_timeOffset(other.m_timeOffset)
         , m_startTime(other.m_startTime)
         , m_transformFunctionListValid(other.m_transformFunctionListValid)
@@ -127,17 +119,26 @@ private:
         , m_direction(other.m_direction)
         , m_finished(false)
     {
+        setName(other.name());
     }
 
     void validateTransformLists();
 
+    void setName(const String& name)
+    {
+        unsigned length = name.length();
+        m_name.resize(length);
+        if (length)
+            memcpy(m_name.data(), name.characters(), sizeof(UChar) * length);
+    }
+
     int m_id;
 
     // NOTE: Don't expose the KeyframeValueList directly, since its copy
     // constructor mutates refcounts and thus is not thread safe.
     KeyframeValueList m_values;
     IntSize m_boxSize;
-    String m_name;
+    Vector<UChar> m_name; // Must not use String member when deriving from ThreadSafeRefCounted
     double m_timeOffset;
     double m_startTime;
     bool m_transformFunctionListValid;
index cef2e5d..2c716fa 100644 (file)
@@ -357,21 +357,6 @@ void LayerCompositingThread::setMediaPlayer(MediaPlayer* mediaPlayer)
 }
 #endif
 
-void LayerCompositingThread::clearAnimations()
-{
-    // Animations don't use thread safe refcounting, and must only be
-    // touched when the two threads are in sync.
-    if (!isCompositingThread()) {
-        dispatchSyncCompositingMessage(BlackBerry::Platform::createMethodCallMessage(
-            &LayerCompositingThread::clearAnimations,
-            this));
-        return;
-    }
-
-    m_runningAnimations.clear();
-    m_suspendedAnimations.clear();
-}
-
 void LayerCompositingThread::removeSublayer(LayerCompositingThread* sublayer)
 {
     ASSERT(isCompositingThread());
index b7223b1..7a29993 100644 (file)
@@ -131,7 +131,6 @@ public:
 #if ENABLE(VIDEO)
     void setMediaPlayer(MediaPlayer*);
 #endif
-    void clearAnimations();
 
     // Not thread safe
 
index e9d3dff..888b04c 100644 (file)
@@ -76,8 +76,6 @@ LayerWebKitThread::LayerWebKitThread(LayerType type, GraphicsLayerBlackBerry* ow
 
 LayerWebKitThread::~LayerWebKitThread()
 {
-    m_layerCompositingThread->clearAnimations();
-
     if (m_frontBufferLock)
         pthread_mutex_destroy(m_frontBufferLock);