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.)
Created attachment 110861 [details] Patch
Comment on attachment 110861 [details] Patch Attachment 110861 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10068039
Created attachment 110997 [details] patch v2 Speculatively fixed the chromium build.
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
Was this discussed on the webkit-dev mailing list?
(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 on attachment 110997 [details] patch v2 According to discussion on the list we should go further and remove the feature flag.
Created attachment 112658 [details] Patch
Comment on attachment 112658 [details] Patch Attachment 112658 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10231164
Created attachment 112853 [details] Patch
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.
Created attachment 112859 [details] Patch
Fixed the "fallback terminology" in the new patch. No it is "Fallback to single thread mode.".
EWS's green, got positive feedback on the list. Somebody to review? :-)
> 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 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.
> > 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.
Committed r100913: <http://trac.webkit.org/changeset/100913>
Committed r100988: <http://trac.webkit.org/changeset/100988>
Comment on attachment 112859 [details] Patch Finally landed in http://trac.webkit.org/changeset/102132