Bug 86906

Summary: GeneratorGeneratedImage should cache intermediate images
Product: WebKit Reporter: Tim Horton <thorton>
Component: Layout and RenderingAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, dglazkov, dino, eoconnor, gustavo, philn, simon.fraser, webkit.review.bot, xan.lopez
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
buildbot: commit-queue-
patch
webkit-ews: commit-queue-
patch dino: review+

Description Tim Horton 2012-05-18 15:12:04 PDT
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>
Comment 1 Tim Horton 2012-05-18 15:12:10 PDT
I have a patch.
Comment 2 Tim Horton 2012-05-18 15:48:32 PDT
Created attachment 142805 [details]
patch
Comment 3 Simon Fraser (smfr) 2012-05-18 16:17:15 PDT
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.
Comment 4 Build Bot 2012-05-18 16:18:21 PDT
Comment on attachment 142805 [details]
patch

Attachment 142805 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12733001
Comment 5 Early Warning System Bot 2012-05-18 16:21:21 PDT
Comment on attachment 142805 [details]
patch

Attachment 142805 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12732008
Comment 6 Early Warning System Bot 2012-05-18 16:21:30 PDT
Comment on attachment 142805 [details]
patch

Attachment 142805 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12719980
Comment 7 WebKit Review Bot 2012-05-18 16:43:38 PDT
Comment on attachment 142805 [details]
patch

Attachment 142805 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12724614
Comment 8 Gyuyoung Kim 2012-05-18 17:55:34 PDT
Comment on attachment 142805 [details]
patch

Attachment 142805 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12723819
Comment 9 Build Bot 2012-05-19 04:52:02 PDT
Comment on attachment 142805 [details]
patch

Attachment 142805 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12733141
Comment 10 Tim Horton 2012-05-21 00:06:47 PDT
Created attachment 142957 [details]
patch
Comment 11 Early Warning System Bot 2012-05-21 00:37:13 PDT
Comment on attachment 142957 [details]
patch

Attachment 142957 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12730554
Comment 12 Early Warning System Bot 2012-05-21 00:41:00 PDT
Comment on attachment 142957 [details]
patch

Attachment 142957 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12728867
Comment 13 WebKit Review Bot 2012-05-21 00:44:24 PDT
Comment on attachment 142957 [details]
patch

Attachment 142957 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12733533
Comment 14 Tim Horton 2012-05-21 00:47:52 PDT
Created attachment 142959 [details]
patch
Comment 15 Dean Jackson 2012-05-21 16:59:19 PDT
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?
Comment 16 Tim Horton 2012-05-21 17:28:32 PDT
(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
Comment 17 Darin Adler 2012-05-22 09:34:38 PDT
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.
Comment 18 Dean Jackson 2012-05-22 11:31:51 PDT
Thanks Darin. I have slapped my own wrists.
Comment 19 Tim Horton 2012-05-24 11:38:36 PDT
(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.
Comment 20 Tim Horton 2012-05-30 11:31:59 PDT
Timer refactor is http://trac.webkit.org/changeset/118942
Comment 21 Tim Horton 2012-06-01 18:41:28 PDT
Everything else except switching to a single timer is in http://trac.webkit.org/changeset/119307