Summary: | [CSS Shaders] Remove the cache of validated programs | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexandru Chiculita <achicu> | ||||||||||||
Component: | Layout and Rendering | Assignee: | Max Vujovic <mvujovic> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, dino, esprehn+autocc, michelangelo, mvujovic, simon.fraser, webkit-ews | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 112602 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Alexandru Chiculita
2013-03-20 14:32:54 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. Created attachment 197892 [details]
Patch
Posting a patch for Alex's informal review. Also, Qt EWS probably won't like this patch.
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 on attachment 197892 [details] Patch Attachment 197892 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/132021 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 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 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. Created attachment 198188 [details]
Patch
Unlike the last patch, this patch doesn't try to refactor everything :). It just removes the mesh cache.
(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 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. Created attachment 198201 [details]
Patch
In this patch, I've removed the weak reference to CustomFilterGlobalContext in CustomFilterValidatedProgram.
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. Created attachment 198204 [details]
Patch
In this patch, used compiledProgram.release() like Alex suggested.
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. Created attachment 198338 [details]
Patch for Landing
Thanks for the review! The ChangeLog does look better without those lines :)
Comment on attachment 198338 [details] Patch for Landing Clearing flags on attachment: 198338 Committed r148518: <http://trac.webkit.org/changeset/148518> All reviewed patches have been landed. Closing bug. |