Bug 112844 – [CSS Shaders] Remove the cache of validated programs

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

[Alexandru Chiculita]

Once 112602 is fixed we could remove the redundant cache of the validated programs. We could also remove the ValidatedCustomFilterOperation as we can reference the CustomFilterValidatedProgram directly from the CustomFilterProgram.

[Max Vujovic]

(In reply to comment #0)
> Once 112602 is fixed we could remove the redundant cache of the validated programs. We could also remove the ValidatedCustomFilterOperation as we can reference the CustomFilterValidatedProgram directly from the CustomFilterProgram.

Looking forward to this! It should simplify things.

[Max Vujovic]

Created attachment 197892 [details]
Patch

Posting a patch for Alex's informal review. Also, Qt EWS probably won't like this patch.

[WebKit Commit Bot]

Attachment 197892 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.cpp', u'Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.h', u'Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.cpp', u'Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.h', u'Source/WebCore/platform/graphics/filters/CustomFilterProgram.cpp', u'Source/WebCore/platform/graphics/filters/CustomFilterProgram.h', u'Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp', u'Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.h', u'Source/WebCore/platform/graphics/filters/FECustomFilter.cpp', u'Source/WebCore/platform/graphics/filters/FECustomFilter.h', u'Source/WebCore/rendering/FilterEffectRenderer.cpp', u'Source/WebCore/rendering/RenderLayer.cpp']" exit_code: 1
Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.h:100:  The parameter name "program" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.h:112:  The parameter name "mixSettings" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 197892 [details]
Patch

Attachment 197892 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/132021

[Early Warning System Bot]

Comment on attachment 197892 [details]
Patch

Attachment 197892 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-queues.appspot.com/results/19168

[Alexandru Chiculita]

Comment on attachment 197892 [details]
Patch

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

Looks good! Thanks for working on this!

> Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.cpp:77
> +    if (!m_program) {

This change seems to require a test file.

> Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.h:-78
> -    CustomFilterValidatedProgramsMap m_programs;

Remove the typedef from this file.

> Source/WebCore/platform/graphics/filters/CustomFilterProgram.h:36
> +#include "CustomFilterValidatedProgram.h"

Remove the CustomFilterValidatedProgram from this file.

> Source/WebCore/platform/graphics/filters/CustomFilterProgram.h:66
> +    void setValidatedProgram(PassRefPtr<CustomFilterValidatedProgram> validatedProgram) { m_validatedProgram = validatedProgram; };

Move this to the .cpp file to avoid brining CustomFilterValidatedProgram in the header.

> Source/WebCore/platform/graphics/filters/CustomFilterProgram.h:88
>      CustomFilterProgramMixSettings m_mixSettings;

Eventually we should move this out of CustomFilterProgramInfo.h.

> Source/WebCore/platform/graphics/filters/FECustomFilter.h:-56
> -        unsigned meshRows, unsigned meshColumns, CustomFilterMeshType);

This looks like it could have been a different patch.

> Source/WebCore/rendering/RenderLayer.cpp:6334
> +                validatedProgram = CustomFilterValidatedProgram::create(program.get(), globalContext->mixShaderValidator(), globalContext->webglShaderValidator());

Let's extract the validation out of CustomFilterValidatedProgram into a separate class like CustomFilterValidator where we could store the mix/webgl shader validators and have all the blending code.

CustomFilterValidatedProgram should only contain the "storing" logic, it is just the result of the CustomFilterValidator. I think we could consolidate that with CustomFilterProgram (which is the 'raw' program information).

[Max Vujovic]

Comment on attachment 197892 [details]
Patch

Thanks for the review, Alex!

I'll split this patch up into smaller, incremental refactors:
Bug 114640
Bug 114637
bug 114639
Bug 102529 (already existed)

And a bug to add a test:
Bug 114636

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

>> Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.cpp:77
>> +    if (!m_program) {
> 
> This change seems to require a test file.

Yes, I won't do this change here. I've filed bug 114636 to cover it.

>> Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.h:-78
>> -    CustomFilterValidatedProgramsMap m_programs;
> 
> Remove the typedef from this file.

Thanks!

>> Source/WebCore/platform/graphics/filters/CustomFilterProgram.h:36
>> +#include "CustomFilterValidatedProgram.h"
> 
> Remove the CustomFilterValidatedProgram from this file.

Ok.

>> Source/WebCore/platform/graphics/filters/CustomFilterProgram.h:66
>> +    void setValidatedProgram(PassRefPtr<CustomFilterValidatedProgram> validatedProgram) { m_validatedProgram = validatedProgram; };
> 
> Move this to the .cpp file to avoid brining CustomFilterValidatedProgram in the header.

Ok.

>> Source/WebCore/platform/graphics/filters/CustomFilterProgram.h:88
>>      CustomFilterProgramMixSettings m_mixSettings;
> 
> Eventually we should move this out of CustomFilterProgramInfo.h.

Yes. I filed a bug 114637.

>> Source/WebCore/platform/graphics/filters/FECustomFilter.h:-56
>> -        unsigned meshRows, unsigned meshColumns, CustomFilterMeshType);
> 
> This looks like it could have been a different patch.

Yes, turns out we have bug 102529 for this.

>> Source/WebCore/rendering/RenderLayer.cpp:6334
>> +                validatedProgram = CustomFilterValidatedProgram::create(program.get(), globalContext->mixShaderValidator(), globalContext->webglShaderValidator());
> 
> Let's extract the validation out of CustomFilterValidatedProgram into a separate class like CustomFilterValidator where we could store the mix/webgl shader validators and have all the blending code.
> 
> CustomFilterValidatedProgram should only contain the "storing" logic, it is just the result of the CustomFilterValidator. I think we could consolidate that with CustomFilterProgram (which is the 'raw' program information).

I agree. Filed bug 114639.

[Max Vujovic]

Created attachment 198188 [details]
Patch

Unlike the last patch, this patch doesn't try to refactor everything :). It just removes the mesh cache.

[Max Vujovic]

(In reply to comment #8)
> Created an attachment (id=198188) [details]
> Patch
> 
> Unlike the last patch, this patch doesn't try to refactor everything :). It just removes the mesh cache.

*the validated program cache I mean. (I'm thinking about too many caches..)

[Alexandru Chiculita]

Comment on attachment 198188 [details]
Patch

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

Looks better!

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.h:-109
> -    void detachFromGlobalContext() { m_globalContext = 0; }

m_globalContext is now a weakReference to the Global Context that could be deleted before this object. Not sure how that could happen, but it would be better to remove the need for this reference.

[Max Vujovic]

Created attachment 198201 [details]
Patch

In this patch, I've removed the weak reference to CustomFilterGlobalContext in CustomFilterValidatedProgram.

[Alexandru Chiculita]

Comment on attachment 198201 [details]
Patch

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

Looks good, just one small nit.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:168
>          m_customFilterRenderer->setCompiledProgram(m_validatedProgram->compiledProgram());

You could use compiledProgram.release() in here.

[Max Vujovic]

Created attachment 198204 [details]
Patch

In this patch, used compiledProgram.release() like Alex suggested.

[Dean Jackson]

Comment on attachment 198204 [details]
Patch

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

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

My favourite comment - remove this line :)

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

And here.

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

And here.

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

And here.

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

And here.

[Max Vujovic]

Created attachment 198338 [details]
Patch for Landing

Thanks for the review! The ChangeLog does look better without those lines :)

[WebKit Commit Bot]

Comment on attachment 198338 [details]
Patch for Landing

Clearing flags on attachment: 198338

Committed r148518: <http://trac.webkit.org/changeset/148518>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.