Summary: | Enable ParallelJobs by default | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Balazs Kelemen <kbalazs> | ||||||||||||
Component: | Web Template Framework | Assignee: | Balazs Kelemen <kbalazs> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | darin, dglazkov, krit, loki, simon.fraser, thorton, webkit.review.bot, zherczeg, zimmermann | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 68540, 70409, 72885, 72941 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Balazs Kelemen
2011-10-13 09:37:13 PDT
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.
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 |