Bug 70032 – Enable ParallelJobs by default

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Balazs Kelemen]

According to measurements ParallelJobs is a significant speedup for SVG - it's only user currently - with all backends (libdispatch, openmp, generic) so I think nothing should stop us using it. My measurements was made with the svg tests of the Methanol benchmark suite (https://gitorious.org/methanol). I measured a 60% speedup on a 6 core MacPro5,1 (libdispatch backend), and a 40% win on my 4 core i5 machine with the generic and also with the openmp backend (these were almost identical). (The generic backend needs the patch in #68450 to achieve this.)

[Balazs Kelemen]

Created attachment 110861 [details]
Patch

[WebKit Review Bot]

Comment on attachment 110861 [details]
Patch

Attachment 110861 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10068039

[Balazs Kelemen]

Created attachment 110997 [details]
patch v2

Speculatively fixed the chromium build.

[WebKit Review Bot]

Comment on attachment 110997 [details]
patch v2

Attachment 110997 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10056545

New failing tests:
svg/dynamic-updates/SVGFEConvolveMatrixElement-svgdom-preserveAlpha-prop.html
svg/dynamic-updates/SVGFEConvolveMatrixElement-dom-preserveAlpha-attr.html

[Dirk Schulze]

Was this discussed on the webkit-dev mailing list?

[Balazs Kelemen]

(In reply to comment #5)
> Was this discussed on the webkit-dev mailing list?

Good point. I have just written a mail about this ;)

[Balazs Kelemen]

Comment on attachment 110997 [details]
patch v2

According to discussion on the list we should go further and remove the feature flag.

[Balazs Kelemen]

Comment on attachment 110997 [details]
patch v2

According to discussion on the list we should go further and remove the feature flag.

[Balazs Kelemen]

Created attachment 112658 [details]
Patch

[WebKit Review Bot]

Comment on attachment 112658 [details]
Patch

Attachment 112658 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10231164

[Balazs Kelemen]

Created attachment 112853 [details]
Patch

[Gabor Loki]

Comment on attachment 112853 [details]
Patch

I have just several comments.

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

> Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp:459
>              // Fallback to the default setInteriorPixels call.

Well, with the patch this comment will be obsolete. This will be the default behaviour. ;)

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:216
> +        // Fallback to sequential mode.

The rest of the algorithm is sequential as well (on each thread). I would say instead the rest of or the whole the work has to be done on the main thread (or something similar).

> Source/WebCore/platform/graphics/filters/FELighting.cpp:277
> +        // Fallback to sequential mode.

Ditto.

> Source/WebCore/platform/graphics/filters/FETurbulence.cpp:408
> +    // Fallback to sequential mode if there is no room for a new thread or the paint area is too small.

Ditto, but the second part of the comment is essential.

[Balazs Kelemen]

Created attachment 112859 [details]
Patch

[Balazs Kelemen]

Fixed the "fallback terminology" in the new patch. No it is "Fallback to single thread mode.".

[Balazs Kelemen]

EWS's green, got positive feedback on the list. Somebody to review? :-)

[Zoltan Herczeg]

> EWS's green, got positive feedback on the list. Somebody to review? :-)

It seems there are no objections. I wait a few more days and I do the review if noone else say anything.

[Zoltan Herczeg]

Comment on attachment 112859 [details]
Patch

There was no objection. r=me with the following minor comments:

(Please check that the patch still apply and works)

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

> Source/JavaScriptCore/ChangeLog:9
> +        great speedup for SVG on multicore.

great -> considerable

> Source/WebCore/ChangeLog:11
> +        great speedup for SVG on multicore.

ditto

> Source/WebCore/ChangeLog:15
> +        by qualifying ParallelJobs with the WTF namespace (otherwise
> +        MSVC believes it belongs to WebCore which is likely a compiler bug).

Are you sure? How do other WTF classes work?

> Source/JavaScriptCore/wtf/ParallelJobsGeneric.cpp:87
> +    // The work for the main thread

Comment should be terminated by a point.

> Source/JavaScriptCore/wtf/ParallelJobsGeneric.h:50
> +    void execute(unsigned char* parameters);

unsigned char* ? The thread function expects void*, please change this to void*.

> Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp:459
> +            // Fallback to single thread mode.

threaded

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:216
> +        // Fallback to single thread mode.

ditto.

> Source/WebCore/platform/graphics/filters/FELighting.cpp:277
> +        // Fallback to single thread mode.

ditto

> Source/WebCore/platform/graphics/filters/FETurbulence.h:74
>      static const int s_minimalRectDimension = (100 * 100); // Empirical data limit for parallel jobs

Comment should be terminated by a point.

[Balazs Kelemen]

> > Source/WebCore/ChangeLog:15
> > +        by qualifying ParallelJobs with the WTF namespace (otherwise
> > +        MSVC believes it belongs to WebCore which is likely a compiler bug).
> 
> Are you sure? How do other WTF classes work?

I guess it's a compiler bug that is only triggered if the namespace of the template and the namespace of it's argument differs. I could not imagine any other reason, there is nothing missing from the Windows buil part, normally it should work the same way as with the other compilers.

[Balazs Kelemen]

Committed r100913: <http://trac.webkit.org/changeset/100913>

[Balazs Kelemen]

Committed r100988: <http://trac.webkit.org/changeset/100988>

[Balazs Kelemen]

Comment on attachment 112859 [details]
Patch

Finally landed in http://trac.webkit.org/changeset/102132