Bug 70032 - Enable ParallelJobs by default
Summary: Enable ParallelJobs by default
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords:
Depends on: 68540 70409 72885 72941
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-13 09:37 PDT by Balazs Kelemen
Modified: 2011-12-06 11:39 PST (History)
9 users (show)

See Also:


Attachments
Patch (1.97 KB, patch)
2011-10-13 09:45 PDT, Balazs Kelemen
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch v2 (2.51 KB, patch)
2011-10-14 05:20 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (21.60 KB, patch)
2011-10-27 03:15 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (27.24 KB, patch)
2011-10-28 06:21 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (27.29 KB, patch)
2011-10-28 07:25 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2011-10-13 09:37:13 PDT
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.)
Comment 1 Balazs Kelemen 2011-10-13 09:45:30 PDT
Created attachment 110861 [details]
Patch
Comment 2 WebKit Review Bot 2011-10-13 10:27:05 PDT
Comment on attachment 110861 [details]
Patch

Attachment 110861 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10068039
Comment 3 Balazs Kelemen 2011-10-14 05:20:25 PDT
Created attachment 110997 [details]
patch v2

Speculatively fixed the chromium build.
Comment 4 WebKit Review Bot 2011-10-14 06:20:36 PDT
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
Comment 5 Dirk Schulze 2011-10-18 06:28:52 PDT
Was this discussed on the webkit-dev mailing list?
Comment 6 Balazs Kelemen 2011-10-18 07:28:21 PDT
(In reply to comment #5)
> Was this discussed on the webkit-dev mailing list?

Good point. I have just written a mail about this ;)
Comment 7 Balazs Kelemen 2011-10-19 02:52:12 PDT
Comment on attachment 110997 [details]
patch v2

According to discussion on the list we should go further and remove the feature flag.
Comment 8 Balazs Kelemen 2011-10-19 04:15:48 PDT
Comment on attachment 110997 [details]
patch v2

According to discussion on the list we should go further and remove the feature flag.
Comment 9 Balazs Kelemen 2011-10-27 03:15:33 PDT
Created attachment 112658 [details]
Patch
Comment 10 WebKit Review Bot 2011-10-27 08:57:25 PDT
Comment on attachment 112658 [details]
Patch

Attachment 112658 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10231164
Comment 11 Balazs Kelemen 2011-10-28 06:21:39 PDT
Created attachment 112853 [details]
Patch
Comment 12 Gabor Loki 2011-10-28 06:58:04 PDT
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.
Comment 13 Balazs Kelemen 2011-10-28 07:25:18 PDT
Created attachment 112859 [details]
Patch
Comment 14 Balazs Kelemen 2011-10-28 07:28:03 PDT
Fixed the "fallback terminology" in the new patch. No it is "Fallback to single thread mode.".
Comment 15 Balazs Kelemen 2011-10-31 07:35:38 PDT
EWS's green, got positive feedback on the list. Somebody to review? :-)
Comment 16 Zoltan Herczeg 2011-11-08 03:07:00 PST
> 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.
Comment 17 Zoltan Herczeg 2011-11-21 03:58:04 PST
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.
Comment 18 Balazs Kelemen 2011-11-21 05:43:34 PST
> > 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.
Comment 19 Balazs Kelemen 2011-11-21 07:37:19 PST
Committed r100913: <http://trac.webkit.org/changeset/100913>
Comment 20 Balazs Kelemen 2011-11-22 03:47:57 PST
Committed r100988: <http://trac.webkit.org/changeset/100988>
Comment 21 Balazs Kelemen 2011-12-06 11:39:13 PST
Comment on attachment 112859 [details]
Patch

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