Bug 86483 – TimingFunction and TransformOperation refcounting is not thread safe

• Component: WebKit BlackBerry
• Status: RESOLVED INVALID

Before dereferencing LayerAnimation from the main WebKit thread the compositing thread needs to dereference first. This requires dispatching from the main WebKit thread onto the compositing thread which is an expensive operation. Making LayerAnimation thread safe would make this unnecessary and improve performance.

Created attachment 141984 [details]
Patch

[Antonio Gomes]

Comment on attachment 141984 [details]
Patch

It looks good to me. Arvid?

[Alexey Proskuryakov]

It is surprising that making refcounting alone thread safe is sufficient to make using the objects from multiple threads safe.

[Alexey Proskuryakov]

Comment on attachment 141984 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141984&action=review

> Source/WebCore/platform/graphics/blackberry/LayerAnimation.h:63
> +        if (m_name.isEmpty())
> +            return String("");
> +        return String(m_name);

What's the purpose of this change? All empty strings share the same StringImpl anyway. Constructing a copy of a string doesn't do anything useful for thread safety either.

Please use ChangeLog to explain changes for each function.

[Arvid Nilsson]

(In reply to comment #3)
> It is surprising that making refcounting alone thread safe is sufficient to make using the objects from multiple threads safe.

Hi Alexey! Thanks for having a look!

I knew from the start this was going to be a tough sell because switching to ThreadSafeRefCounted brings a small performance penalty compared to regular old RefCounted, and no other WebKit port uses these classes the way the BlackBerry port does.

We're not in this to make the classes thread safe, instead what we do is treat these as immutable as soon as they enter our accelerated compositing backend. No TransformOperation or TimingFunction data is changed after they're handed off to the BlackBerry accelerated animation implementation (which is pretty much a copy/paste job of the WebKit animation implementation, only running on another thread).

However, even if we don't touch any data of these objects, the refcount will still be mutated as we pass these objects around and from one thread to the other.

We put this patch up so it can be the subject of early discussion, then perhaps we can discuss if other ports are interested in moving in the direction of making the WebKit animation implementation usable from a secondary thread instead of just the WebKit thread. The Chromium CCLayer thread seems to be moving in this direction, but I'm not sure if they're developing an animation framework from scratch instead of reusing the interpolation implementation in WebKit.

For Apple's WebKit port, you have Core Animation, so you already have an animation framework and I guess there's no point for you to make the WebKit animation implementation usable from a secondary thread. Instead you copy over all Keyframe and Timing function information to Core Animation data structure, et voilà, no threading problem.

[Simon Fraser (smfr)]

Comment on attachment 141984 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141984&action=review

> Source/WebCore/platform/animation/TimingFunction.h:32
> -class TimingFunction : public RefCounted<TimingFunction> {
> +class TimingFunction : public ThreadSafeRefCounted<TimingFunction> {

Why force all platforms to take a perf hit when only BlackBerry needs the thread safety?

> Source/WebCore/platform/graphics/transforms/TransformOperation.h:37
> -class TransformOperation : public RefCounted<TransformOperation> {
> +class TransformOperation : public ThreadSafeRefCounted<TransformOperation> {

Ditto.

[Alexey Proskuryakov]

> However, even if we don't touch any data of these objects, the refcount will still be mutated as we pass these objects around and from one thread to the other.

I think that you need to generously sprinkle ASSERT(isMainThread()) checks around this code to document and verify this requirement.

Not commenting on whether the performance hit of making these classes ThreadSafeRefCounted.

Created attachment 143825 [details]
Patch

[Arvid Nilsson]

Ping, for CSS filters we got away with something like this:

 #include <wtf/RefCounted.h>
 #include <wtf/text/WTFString.h>
 
+#if PLATFORM(BLACKBERRY)
+#include <wtf/ThreadSafeRefCounted.h>
+#endif
+
 // Annoyingly, wingdi.h #defines this.
 #ifdef PASSTHROUGH
 #undef PASSTHROUGH
@@ -44,7 +48,11 @@ namespace WebCore {
 
 // CSS Filters
 
+#if PLATFORM(BLACKBERRY)
+class FilterOperation : public ThreadSafeRefCounted<FilterOperation> {
+#else
 class FilterOperation : public RefCounted<FilterOperation> {
+#endif
 public:

Maybe Ed can do the same in this instance?

https://bugs.webkit.org/show_bug.cgi?id=92685

[Arvid Nilsson]

Move this to BlackBerry component, with the proposed solution it doesn't impact any other port.

[Arvid Nilsson]

PR #153047

[Alexey Proskuryakov]

[] prefixes are not OK on patches that touch cross-platform files.

I find it inappropriate for core classes to have different threading characteristics in different ports.

[Arvid Nilsson]

(In reply to comment #12)
> [] prefixes are not OK on patches that touch cross-platform files.
> 
> I find it inappropriate for core classes to have different threading characteristics in different ports.

Thanks Alexey, I didn't know that. But I hope the new component is more appropriate.

I think we should upstream the non-controversial bits in a separate, [BlackBerry], patch to avoid merge conflicts in our port-specific files.

[Arvid Nilsson]

I'm gonna remove the noise that is the BlackBerry-specific parts of the patch.

The reason the BlackBerry port uses TimingFunction and TransformOperation on multiple threads is that these classes (and subclasses) are immutable - once constructed, their data don't change.

An immutable class is typically thread safe.

The only mutable property of these classes is the refcount, which we wish to change to be thread safe (but only for the BlackBerry port).

[Arvid Nilsson]

Created attachment 158259 [details]
Patch

[Arvid Nilsson]

Bug #93946 was created for the BlackBerry-specific bits