Bug 86906 – GeneratorGeneratedImage should cache intermediate images

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Tim Horton]

Some pages include complicated, expensive, large gradients (the only currently implemented GeneratorGeneratedImage). Worse yet, some include them in fixed-position elements, which we currently have to repaint every frame during a scroll.

We should temporarily cache gradients while they're being used to relieve some of this stress on the graphics library.

We have measured significant (>2x!) speedups scrolling some high-profile public sites on the Mac port.

<rdar://problem/11484852>

[Tim Horton]

I have a patch.

[Tim Horton]

Created attachment 142805 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 142805 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142805&action=review

> Source/WebCore/platform/graphics/Gradient.cpp:267
> +    struct {
> +        bool radial;
> +        FloatPoint p0;
> +        FloatPoint p1;
> +        float r0;
> +        float r1;
> +        float aspectRatio;
> +        mutable Vector<ColorStop, 2> stops;
> +        mutable int lastStop;
> +        GradientSpreadMethod spreadMethod;
> +        AffineTransform gradientSpaceTransformation;
> +    } parameters = {
> +        m_radial,
> +        m_p0,
> +        m_p1,
> +        m_r0,
> +        m_r1,
> +        m_aspectRatio,
> +        m_stops,
> +        m_lastStop,
> +        m_spreadMethod,
> +        m_gradientSpaceTransformation
> +    };
> +

Is there a guarantee there there aren't uninitialized bytes of padding here (say, after the first bool)?

I don't think you need the mutables here.

> Source/WebCore/platform/graphics/Gradient.h:101
> +        void setP0(const FloatPoint& p) { m_p0 = p; clearHashCache(); }
> +        void setP1(const FloatPoint& p) { m_p1 = p; clearHashCache(); }

You should compare old and new values before clearing the hash.

[Build Bot]

Comment on attachment 142805 [details]
patch

Attachment 142805 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12733001

[Early Warning System Bot]

Comment on attachment 142805 [details]
patch

Attachment 142805 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12732008

[Early Warning System Bot]

Comment on attachment 142805 [details]
patch

Attachment 142805 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12719980

[WebKit Review Bot]

Comment on attachment 142805 [details]
patch

Attachment 142805 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12724614

[Gyuyoung Kim]

Comment on attachment 142805 [details]
patch

Attachment 142805 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12723819

[Build Bot]

Comment on attachment 142805 [details]
patch

Attachment 142805 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12733141

[Tim Horton]

Created attachment 142957 [details]
patch

[Early Warning System Bot]

Comment on attachment 142957 [details]
patch

Attachment 142957 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12730554

[Early Warning System Bot]

Comment on attachment 142957 [details]
patch

Attachment 142957 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12728867

[WebKit Review Bot]

Comment on attachment 142957 [details]
patch

Attachment 142957 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12733533

[Tim Horton]

Created attachment 142959 [details]
patch

[Dean Jackson]

Comment on attachment 142959 [details]
patch

Seems like smfr's comments were addressed. I have one non-important naming question - why clearHashCache() yet m_cacheHash?

[Tim Horton]

(In reply to comment #15)
> (From update of attachment 142959 [details])
> Seems like smfr's comments were addressed. I have one non-important naming question - why clearHashCache() yet m_cacheHash?

Thanks, Dean!

Fixed your naming question, landed in http://trac.webkit.org/changeset/117858

[Darin Adler]

Comment on attachment 142959 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142959&action=review

> Source/WebCore/ChangeLog:22
> +        (Generator):

The change log has a lot of bogus lines like this in it. Please edit those out by hand if the list is not good. Better, cultivate the having of adding per-function comments.

> Source/WebCore/platform/graphics/Generator.h:42
> +    virtual unsigned hash() = 0;

Should this be const?

> Source/WebCore/platform/graphics/GeneratorGeneratedImage.cpp:67
> +    if (!m_cachedImageBuffer
> +        || m_cachedGeneratorHash != generatorHash
> +        || m_cachedAdjustedSize != adjustedSize

I think this would read better if these checks were all on one line.

> Source/WebCore/platform/graphics/GeneratorGeneratedImage.cpp:69
> +        // Create a BitmapImage and call drawPattern on it.

This comment that you moved is not good. Since it just says what the code does and adds nothing you should remove it.

> Source/WebCore/platform/graphics/GeneratorGeneratedImage.cpp:75
> +        GraphicsContext* graphicsContext = m_cachedImageBuffer->context();

I don’t think this local variable adds clarity. I’d just do it in one line.

> Source/WebCore/platform/graphics/GeneratorGeneratedImage.cpp:76
> +        graphicsContext->fillRect(FloatRect(FloatPoint(), adjustedSize), *m_generator.get());

Should not need the get() here, since the * operator should be implemented.

> Source/WebCore/platform/graphics/GeneratorGeneratedImage.h:68
> +    class GeneratedImageCacheTimer : public TimerBase {

It would be much better if you factored the timer I wrote for the other cache so we could share, instead of copying and pasting.

> Source/WebCore/platform/graphics/GeneratorGeneratedImage.h:72
> +        GeneratedImageCacheTimer(GeneratorGeneratedImage * parent)
> +        : m_shouldRestartWhenTimerFires(false)
> +        , m_parent(parent) { }

This isn’t our usual format. That would be:

    GeneratedImageCacheTimer(GeneratorGeneratedImage* parent)
        : m_shouldRestartWhenTimerFires(false)
        , m_parent(parent)
    {
    }

> Source/WebCore/platform/graphics/GeneratorGeneratedImage.h:82
> +        };
> +    private:

Extra unneeded semicolon here. Should add a blank line before "private".

> Source/WebCore/platform/graphics/GeneratorGeneratedImage.h:83
> +        virtual void fired() OVERRIDE

I don’t think it’s beneficial to have this function definition here in the header.

> Source/WebCore/platform/graphics/GeneratorGeneratedImage.h:95
> +        };

Extra semicolon here.

> Source/WebCore/platform/graphics/GeneratorGeneratedImage.h:103
> +    GeneratedImageCacheTimer m_cacheTimer;

A timer is a pretty big object. This makes generated image objects considerably bigger. Is there some way we can share a global timer instead of having a timer on each generated image?

> Source/WebCore/platform/graphics/Gradient.cpp:264
> +    memset(&parameters, 0, sizeof(parameters));

You need a “why” comment here. It’s critical to do this memset, but not obvious why to most readers.

> Source/WebCore/platform/graphics/Gradient.cpp:276
> +    unsigned parametersHash = StringHasher::hashMemory(&parameters, sizeof(parameters));

StringHasher has some requirements for the size of the memory passed in. Do we have a guarantee that this structure will meet those requirements?

> Source/WebCore/platform/graphics/Gradient.cpp:278
> +    ColorStop * stopData = m_stops.data();

This local variable doesn’t add value. I suggest omitting it. Also the * has an extra space before it.

> Source/WebCore/platform/graphics/Gradient.cpp:283
> +    std::pair<unsigned, unsigned> hashes(parametersHash, stopHash);
> +    
> +    m_cachedHash = DefaultHash<std::pair<unsigned, unsigned> >::Hash::hash(hashes);

A better way to hash two 32-bit values together is to turn them into a 64-bit integer and hash that. We could make a helper function to make that cleaner or just write this:

    m_cachedHash = intHash((static_cast<uint64_t>(parametersHash) << 32) | stopHash);

Another way to do it would be to make StringHasher more flexible so we can hash two batches of memory and get the overall hash.

> Source/WebCore/platform/graphics/Gradient.h:176
> +        void clearHashCache() { m_cachedHash = 0; }

I’d call this function invalidateHash().

> Source/WebCore/platform/graphics/GraphicsContext.cpp:794
> +    AffineTransform localTransform = getCTM();
> +    AffineTransform bufferTransform = buffer->context()->getCTM();
> +
> +    if (localTransform.xScale() != bufferTransform.xScale() || localTransform.yScale() != bufferTransform.yScale())
> +        return false;
> +
> +    if (isAcceleratedContext() != buffer->context()->isAcceleratedContext())
> +        return false;
> +
> +    return true;

I’d like to see a helper function named scalesMatch. With that you could eliminate CTM locals and write it like this:

    GraphicsContext* bufferContext = buffer->context();
    return scalesMatch(getCTM(), bufferContext->getCTM())) && isAcceleratedContext() == bufferContext->isAcceleratedContext();

I think that would be clearer.

[Dean Jackson]

Thanks Darin. I have slapped my own wrists.

[Tim Horton]

(In reply to comment #17)
> (From update of attachment 142959 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142959&action=review

Darin: sorry I haven't gotten to this quicker; these are all excellent points! I will post a followup patch, I just have to finish factoring out the timer. Might get split into a few patches.

[Tim Horton]

Timer refactor is http://trac.webkit.org/changeset/118942

[Tim Horton]

Everything else except switching to a single timer is in http://trac.webkit.org/changeset/119307