Bug 112602 - [CSS Shaders] Implement a StyleCustomFilterProgram cache
Summary: [CSS Shaders] Implement a StyleCustomFilterProgram cache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandru Chiculita
URL:
Keywords:
Depends on:
Blocks: 112844
  Show dependency treegraph
 
Reported: 2013-03-18 11:33 PDT by Alexandru Chiculita
Modified: 2013-03-21 15:20 PDT (History)
15 users (show)

See Also:


Attachments
Patch V1 (47.41 KB, patch)
2013-03-20 15:09 PDT, Alexandru Chiculita
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch V2 (49.05 KB, patch)
2013-03-20 15:33 PDT, Alexandru Chiculita
dino: review+
achicu: commit-queue-
Details | Formatted Diff | Diff
Patch for commit (48.95 KB, patch)
2013-03-21 14:53 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 2013-03-18 11:33:21 PDT
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.
Comment 1 Alexandru Chiculita 2013-03-20 15:09:17 PDT
Created attachment 194130 [details]
Patch V1
Comment 2 Early Warning System Bot 2013-03-20 15:16:12 PDT
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
Comment 3 Early Warning System Bot 2013-03-20 15:25:53 PDT
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
Comment 4 Alexandru Chiculita 2013-03-20 15:33:23 PDT
Created attachment 194135 [details]
Patch V2
Comment 5 Dean Jackson 2013-03-20 16:58:10 PDT
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?
Comment 6 Alexandru Chiculita 2013-03-20 17:07:27 PDT
(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.
Comment 7 Alexandru Chiculita 2013-03-20 17:07:59 PDT
Comment on attachment 194135 [details]
Patch V2

cq- as I need to include the feedback.
Comment 8 Alexandru Chiculita 2013-03-21 14:53:34 PDT
Created attachment 194351 [details]
Patch for commit
Comment 9 WebKit Review Bot 2013-03-21 15:20:09 PDT
Comment on attachment 194351 [details]
Patch for commit

Clearing flags on attachment: 194351

Committed r146529: <http://trac.webkit.org/changeset/146529>
Comment 10 WebKit Review Bot 2013-03-21 15:20:14 PDT
All reviewed patches have been landed.  Closing bug.