[Webkit-unassigned] [Bug 68353] [Qt] Apply ParalellJobs for ImageBuffer::platformTransformColorSpace method

[bugzilla-daemon at webkit.org]

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


Andreas Kling <kling at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #107994|review?                     |review-
               Flag|                            |




--- Comment #5 from Andreas Kling <kling at webkit.org>  2011-09-20 06:44:26 PST ---
(From update of attachment 107994)
View in context: https://bugs.webkit.org/attachment.cgi?id=107994&action=review

> Source/WebCore/ChangeLog:14
> +        No new tests / no behavioral changes.

So how much faster is it?

> Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:169
> +static const int minimalAreaOfImage = 128 * 128; // Empirical data limit for parallel jobs

Empirical data limit?
Where does this value come from?
The name 'minimalAreaOfImage' doesn't sufficiently explain what the constant does.

> Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:171
> +struct ThreadParam {

We try not to abbreviate unnecessarily in WebKit. ThreadParameters?

> Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:172
> +    int yStart, yEnd;

One declaration per line.

> Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:179
> +static void worker(ThreadParam* tp)

'tp' is a bad variable name. parameters?

> Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:189
> +        for (int x = 0; x < tp->width; ++x) {
> +            QRgb& pixel = scanLine[x];
> +            pixel = qRgba((*tp->lookUpTable)[qRed(pixel)],
> +                          (*tp->lookUpTable)[qGreen(pixel)],
> +                          (*tp->lookUpTable)[qBlue(pixel)],
> +                          qAlpha(pixel));
> +        }

First off, we should share this code with the non-parallel path.
Second, this is a terrible algorithm, performance wise. I'm sure you can make it a *lot* faster by manipulating the color byte fields directly instead of going through qRgba and  qRed/qGreen/qBlue/qAlpha.

> Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:208
> +    int areaOfImage = m_size.width() * m_size.height();
> +    int optimalThreadCount = areaOfImage / minimalAreaOfImage;

Granted, I haven't studied the math behind this, but I would think that the optimal thread counts relates more to the number of scanlines, not so much to the X*Y area.

> Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:210
> +        ParallelJobs<ThreadParam> parallelJobs(&worker, 2);

Why hard-code 2 threads if we know the optimal thread count?

> Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:211
> +        int threadCount = parallelJobs.numberOfJobs();

You don't need this variable.

> Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:213
> +        int dy = m_size.height() / threadCount;

'dy' is a bad variable name. scanLinesPerThread?

> Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:215
> +            ThreadParam& tp = parallelJobs.parameter(i);

'tp' is a bad variable name. parameters?

> Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:218
> +            tp.yEnd = (i == threadCount-1) ? m_size.height() : yStart;

Coding style, threadCount-1 should be threadCount - 1

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.