Bug 112602 – [CSS Shaders] Implement a StyleCustomFilterProgram cache

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexandru Chiculita]

This is the current state:
- Each time a new custom filter is computed, StyleResolver will create a new StyleCustomFilterProgram. It will later on reference the CachedShaders required to load the shaders.
- CustomFilterOperation references the StyleCustomFilterProgram.
- The RenderLayerFilterInfo will attach itself as a client of the Cached Shaders. That happens through the instance of the StyleCustomFilterProgram.
- Next time the filter is changed, the StyleResolver will create a new StyleCustomFilterProgram.
- The RenderLayerFilterInfo will not attach again as the CustomFilterOperation is equal, as it only compares the cached shaders to be equal. However, the old StyleCustomFilterProgram will be dereferenced, meaning that the only client is now gone.
- RenderLayer will try to draw, but the shaders are sometimes unloaded from memory, meaning that it will just try to draw empty shaders.

This patch will add a HashMap for the StyleCustomFilterPrograms using the KURL of the vertex/fragment shaders as a key. This way we avoid recreating the StyleCustomFilterPrograms all the times. The destructor of the StyleCustomFilterProgram will be responsible to remote itself from the owning HashMap, meaning that we only store StyleCustomFilterPrograms as long as they are used by the elements. The cache will be owned by the StyleResolver.

Also, by having unique StyleCustomFilterPrograms, we can compare StyleCustomFilterPrograms by pointer instead.

[Alexandru Chiculita]

Created attachment 194130 [details]
Patch V1

[Early Warning System Bot]

Comment on attachment 194130 [details]
Patch V1

Attachment 194130 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17202544

[Early Warning System Bot]

Comment on attachment 194130 [details]
Patch V1

Attachment 194130 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17202550

[Alexandru Chiculita]

Created attachment 194135 [details]
Patch V2

[Dean Jackson]

Comment on attachment 194135 [details]
Patch V2

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

> Source/WebCore/ChangeLog:27
> +        using the pointer value and not the values. However, that will always invalidate the "fitler" property because

typo fitler

> Source/WebCore/ChangeLog:49
> +        (WebCore):

Remove (there are a few of these)

> Source/WebCore/ChangeLog:52
> +        if no program is found. loadPendingShaders is responsible with adding the program in the cache if it is actually going to be used.

s/with/for/

> Source/WebCore/css/StyleResolver.cpp:3941
> +    if (value->isWebKitCSSShaderValue())
> +        return static_cast<WebKitCSSShaderValue*>(value);

We should probably add a toWebKitCSSShaderValue() method, the way other classes have done.

> Source/WebCore/css/StyleResolver.cpp:3996
> +                    WebKitCSSShaderValue* shaderValue = static_cast<StylePendingShader*>(program->vertexShader())->cssShaderValue();

And here for toStylePendingShader, although that could be in a separate patch (where we fix all occurrences)

> Source/WebCore/platform/graphics/filters/CustomFilterProgram.h:81
> +    // CustomFilterPrograms are unique combinations of shaders and can be 
> +    // compared using just the pointer value instead.
> +    bool operator==(const CustomFilterProgram&) const;
> +    bool operator!=(const CustomFilterProgram&) const;

I suggest mentioning here that you're trying to catch people comparing directly (the way toWhatever function comments mention catching people doing unnecessary casting).

> Source/WebCore/rendering/style/StyleCustomFilterProgram.cpp:41
> +namespace WebCore {
> +
> +
> +StyleCustomFilterProgram::~StyleCustomFilterProgram()

Nit: extra space

> Source/WebCore/rendering/style/StyleCustomFilterProgramCache.cpp:76
> +    ASSERT(m_cache.find(key) != m_cache.end());

Is this necessary?

[Alexandru Chiculita]

(In reply to comment #5)
> (From update of attachment 194135 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194135&action=review
> 
Thanks, that was quick!

> > Source/WebCore/ChangeLog:27
> > +        using the pointer value and not the values. However, that will always invalidate the "fitler" property because
> 
> typo fitler
I always do that. Thanks. :)

> 
> > Source/WebCore/ChangeLog:49
> > +        (WebCore):
> 
> Remove (there are a few of these)
> 
That's generated automatically by the script. Should I always remove it? Should I log a bug for the script?

> > Source/WebCore/ChangeLog:52
> > +        if no program is found. loadPendingShaders is responsible with adding the program in the cache if it is actually going to be used.
> 
> s/with/for/
> 
Ok.

> > Source/WebCore/css/StyleResolver.cpp:3941
> > +    if (value->isWebKitCSSShaderValue())
> > +        return static_cast<WebKitCSSShaderValue*>(value);
> 
> We should probably add a toWebKitCSSShaderValue() method, the way other classes have done.
> 
> > Source/WebCore/css/StyleResolver.cpp:3996
> > +                    WebKitCSSShaderValue* shaderValue = static_cast<StylePendingShader*>(program->vertexShader())->cssShaderValue();
> 
> And here for toStylePendingShader, although that could be in a separate patch (where we fix all occurrences)
> 
> > Source/WebCore/platform/graphics/filters/CustomFilterProgram.h:81
> > +    // CustomFilterPrograms are unique combinations of shaders and can be 
> > +    // compared using just the pointer value instead.
> > +    bool operator==(const CustomFilterProgram&) const;
> > +    bool operator!=(const CustomFilterProgram&) const;
> 
> I suggest mentioning here that you're trying to catch people comparing directly (the way toWhatever function comments mention catching people doing unnecessary casting).
> 
Ok.

> > Source/WebCore/rendering/style/StyleCustomFilterProgram.cpp:41
> > +namespace WebCore {
> > +
> > +
> > +StyleCustomFilterProgram::~StyleCustomFilterProgram()
> 
> Nit: extra space
> 
> > Source/WebCore/rendering/style/StyleCustomFilterProgramCache.cpp:76
> > +    ASSERT(m_cache.find(key) != m_cache.end());
> 
> Is this necessary?
Not really. I was using it for debugging and figured it might be good to have. It should assert that the hash of the CustomFitlerProgramInfo works correctly. I will remove it.

[Alexandru Chiculita]

Comment on attachment 194135 [details]
Patch V2

cq- as I need to include the feedback.

[Alexandru Chiculita]

Created attachment 194351 [details]
Patch for commit

[WebKit Review Bot]

Comment on attachment 194351 [details]
Patch for commit

Clearing flags on attachment: 194351

Committed r146529: <http://trac.webkit.org/changeset/146529>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.