[webkit-reviews] review denied: [Bug 68353] [Qt] Apply ParalellJobs for ImageBuffer::platformTransformColorSpace method : [Attachment 107994] [Qt] Apply ParalellJobs for ImageBuffer::platformTransformColorSpace method

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Andras Piroska
<pandras at inf.u-szeged.hu>'s request for review:
Bug 68353: [Qt] Apply ParalellJobs for ImageBuffer::platformTransformColorSpace
method
https://bugs.webkit.org/show_bug.cgi?id=68353

Attachment 107994: [Qt] Apply ParalellJobs for
ImageBuffer::platformTransformColorSpace method
https://bugs.webkit.org/attachment.cgi?id=107994&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
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