Bug 112844 - [CSS Shaders] Remove the cache of validated programs
Summary: [CSS Shaders] Remove the cache of validated programs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Max Vujovic
URL:
Keywords:
Depends on: 112602
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-20 14:32 PDT by Alexandru Chiculita
Modified: 2013-04-16 10:11 PDT (History)
7 users (show)

See Also:


Attachments
Patch (29.30 KB, patch)
2013-04-12 15:08 PDT, Max Vujovic
mvujovic: commit-queue-
Details | Formatted Diff | Diff
Patch (11.71 KB, patch)
2013-04-15 14:47 PDT, Max Vujovic
mvujovic: commit-queue-
Details | Formatted Diff | Diff
Patch (18.81 KB, patch)
2013-04-15 16:42 PDT, Max Vujovic
mvujovic: commit-queue-
Details | Formatted Diff | Diff
Patch (18.89 KB, patch)
2013-04-15 16:57 PDT, Max Vujovic
dino: review+
mvujovic: commit-queue-
Details | Formatted Diff | Diff
Patch for Landing (18.80 KB, patch)
2013-04-16 09:42 PDT, Max Vujovic
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-20 14:32:54 PDT
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.
Comment 1 Max Vujovic 2013-03-20 14:38:44 PDT
(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.
Comment 2 Max Vujovic 2013-04-12 15:08:43 PDT
Created attachment 197892 [details]
Patch

Posting a patch for Alex's informal review. Also, Qt EWS probably won't like this patch.
Comment 3 WebKit Commit Bot 2013-04-12 15:10:54 PDT
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.
Comment 4 Early Warning System Bot 2013-04-12 15:36:23 PDT
Comment on attachment 197892 [details]
Patch

Attachment 197892 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/132021
Comment 5 Early Warning System Bot 2013-04-12 15:42:41 PDT
Comment on attachment 197892 [details]
Patch

Attachment 197892 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-queues.appspot.com/results/19168
Comment 6 Alexandru Chiculita 2013-04-12 16:25:14 PDT
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).
Comment 7 Max Vujovic 2013-04-15 14:36:14 PDT
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.
Comment 8 Max Vujovic 2013-04-15 14:47:29 PDT
Created attachment 198188 [details]
Patch

Unlike the last patch, this patch doesn't try to refactor everything :). It just removes the mesh cache.
Comment 9 Max Vujovic 2013-04-15 14:48:12 PDT
(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..)
Comment 10 Alexandru Chiculita 2013-04-15 15:00:52 PDT
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.
Comment 11 Max Vujovic 2013-04-15 16:42:18 PDT
Created attachment 198201 [details]
Patch

In this patch, I've removed the weak reference to CustomFilterGlobalContext in CustomFilterValidatedProgram.
Comment 12 Alexandru Chiculita 2013-04-15 16:53:46 PDT
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.
Comment 13 Max Vujovic 2013-04-15 16:57:01 PDT
Created attachment 198204 [details]
Patch

In this patch, used compiledProgram.release() like Alex suggested.
Comment 14 Dean Jackson 2013-04-15 17:15:43 PDT
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.
Comment 15 Max Vujovic 2013-04-16 09:42:40 PDT
Created attachment 198338 [details]
Patch for Landing

Thanks for the review! The ChangeLog does look better without those lines :)
Comment 16 WebKit Commit Bot 2013-04-16 10:11:39 PDT
Comment on attachment 198338 [details]
Patch for Landing

Clearing flags on attachment: 198338

Committed r148518: <http://trac.webkit.org/changeset/148518>
Comment 17 WebKit Commit Bot 2013-04-16 10:11:42 PDT
All reviewed patches have been landed.  Closing bug.