While profiling EFL's MiniBrowser under gDEBugger, I've noticed quite some state change function calls were redundant. This bug intends to add more state variables inside GraphicsContext3DState to avoid redundant state changes and thus reduce amount of OpenGL calls.
Created attachment 190292 [details] Patch Proposed patch (depends on bug 110817)
Created attachment 190726 [details] Patch Added state variables for gl{Enable,Disable}, simplified state variables for glPixelStorei
Created attachment 190741 [details] Patch Fixed a minor typo
Comment on attachment 190741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190741&action=review > Source/WebCore/platform/graphics/GraphicsContext3D.h:1114 > + GC3Denum sourceFactor; The addition of these state variables seems fine, but please provide some indication that some of these fields are not set or used by all ports. People coming in later may find it confusing that they aren't maintained everywhere.
(In reply to comment #4) > (From update of attachment 190741 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190741&action=review > > > Source/WebCore/platform/graphics/GraphicsContext3D.h:1114 > > + GC3Denum sourceFactor; > > The addition of these state variables seems fine, but please provide some indication that some of these fields are not set or used by all ports. People coming in later may find it confusing that they aren't maintained everywhere. Thanks for the feedback. I'll update the patch with this info as well as last minute additions I have here.
Created attachment 190836 [details] Patch Updated patch w/ comments
Comment on attachment 190836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190836&action=review Looks generally OK, but there's one bug, and one comment update needed. > Source/WebCore/platform/graphics/GraphicsContext3D.h:1115 > + // might not be handled correctly on non-optimized GL drivers. This still isn't clear. Could you please say something like: The cached values below are used by some, but not all, ports to reduce redundant state transitions. Do not rely on their being up-to-date in common code. > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:939 > + if (m_state.stencilFail == fail && m_state.stencilZFail && zfail && m_state.stencilZPass == zpass) m_state.stencilZFail == zfail
Created attachment 190855 [details] Patch Updated patch w/ comment clarification and fix reviewed by Kenneth
Comment on attachment 190855 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190855&action=review Sorry I didn't notice these before, but code inspection revealed a couple more bugs. I hope you've tested this change thoroughly and that the performance gains from these changes warrant the risk of caching these values. > Source/WebCore/platform/graphics/GraphicsContext3D.h:1091 > + , sourceFactor(GraphicsContext3D::ZERO) This value is wrong. The initial value is GraphicsContext3D::ONE. > Source/WebCore/platform/graphics/GraphicsContext3D.h:1097 > + , scissorHeight(0) The scissor width and height default to the initial width and height of the drawable. I hope this cache will be correct enough. > Source/WebCore/platform/graphics/GraphicsContext3D.h:1107 > + , viewportHeight(0) Similar issue with the viewport width and height. > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:204 > + pixelStorei(GL_PACK_ALIGNMENT, 4); Sorry for not noticing this before -- but isn't this incorrect? You haven't changed the call to ::glPixelStorei below in the same method, so after calling this, the cached value in the pixelStoreIntMap will be incorrect. Since you're querying the GL for the value here anyway, I think you should just leave this alone.
(In reply to comment #9) > (From update of attachment 190855 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190855&action=review > > Sorry I didn't notice these before, but code inspection revealed a couple more bugs. I hope you've tested this change thoroughly and that the performance gains from these changes warrant the risk of caching these values. Yes, using a CSS 3D Transform continuous animation profiled using gDEBugger I could reduce redundant state changes from ~16% of the overall GL calls to virtually 0%. > > > Source/WebCore/platform/graphics/GraphicsContext3D.h:1091 > > + , sourceFactor(GraphicsContext3D::ZERO) > > This value is wrong. The initial value is GraphicsContext3D::ONE. You're right :) Fixing on the next patch update. > > > Source/WebCore/platform/graphics/GraphicsContext3D.h:1097 > > + , scissorHeight(0) > > The scissor width and height default to the initial width and height of the drawable. I hope this cache will be correct enough. > > > Source/WebCore/platform/graphics/GraphicsContext3D.h:1107 > > + , viewportHeight(0) > > Similar issue with the viewport width and height. For scissor and viewport values, as well as the *Map ones, do you agree on having some sort of init() function that could fill the initial values when the GC3DState object is created? Otherwise I may not have access to the drawable on initialization list. > > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:204 > > + pixelStorei(GL_PACK_ALIGNMENT, 4); > > Sorry for not noticing this before -- but isn't this incorrect? You haven't changed the call to ::glPixelStorei below in the same method, so after calling this, the cached value in the pixelStoreIntMap will be incorrect. Since you're querying the GL for the value here anyway, I think you should just leave this alone. Yes, like you said, calling ::glPixelStorei() directly w/o the state check might lead to state variable inconsistency. I shall change the other ::glPixelStorei() call to also use the internal function.
Comment on attachment 190855 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190855&action=review > Source/WebCore/platform/graphics/GraphicsContext3D.h:1098 > + , stencilFunc(GraphicsContext3D::ALWAYS) Seems like an overkill to query all the *Map ones, viewport, scissor etc. Why not accept the first cache miss and save subsequent calls ??
(In reply to comment #11) > (From update of attachment 190855 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190855&action=review > > > Source/WebCore/platform/graphics/GraphicsContext3D.h:1098 > > + , stencilFunc(GraphicsContext3D::ALWAYS) > > Seems like an overkill to query all the *Map ones, viewport, scissor etc. Why not accept the first cache miss and save subsequent calls ?? Agreed, I shall stick with 0 values for viewport/scissor and empty *Maps (filled in runtime) then.
Created attachment 190946 [details] Patch Updated patch w/ second round of fixes reviewed by Kenneth
(In reply to comment #10) > Yes, using a CSS 3D Transform continuous animation profiled using gDEBugger I could reduce redundant state changes from ~16% of the overall GL calls to virtually 0%. James Robinson has convinced me that this can be a useful optimization on bad drivers. Since each cached variable adds code complexity though, wouldn't it be better to first discover which variables actually affect performance? On many drivers (like the fairly high quality and open Intel ones), redundant state changes have very minor affects on performance. I think doing this blindly means that we will never be able to understand or remove this code, assuming we head toward a bright future of high quality drivers. I don't want to block this patch, but I think it should be done with an eye toward measurable performance improvements and not superstition. In the case of Chromium, caching state variables is cheap because it is done via code generation and intermediate layers. In this case we have to thread the cache through WebCore code which imposes an engineering overhead on everyone. I believe those kind of costs should not have zero weight when considering a change.
Created attachment 191205 [details] gDEBugger profile data (.csv) before patch
Created attachment 191207 [details] gDEBugger profile data (.csv) after patch
(In reply to comment #14) > (In reply to comment #10) > > > Yes, using a CSS 3D Transform continuous animation profiled using gDEBugger I could reduce redundant state changes from ~16% of the overall GL calls to virtually 0%. > > James Robinson has convinced me that this can be a useful optimization on bad drivers. Since each cached variable adds code complexity though, wouldn't it be better to first discover which variables actually affect performance? > > On many drivers (like the fairly high quality and open Intel ones), redundant state changes have very minor affects on performance. I think doing this blindly means that we will never be able to understand or remove this code, assuming we head toward a bright future of high quality drivers. > > I don't want to block this patch, but I think it should be done with an eye toward measurable performance improvements and not superstition. In the case of Chromium, caching state variables is cheap because it is done via code generation and intermediate layers. In this case we have to thread the cache through WebCore code which imposes an engineering overhead on everyone. I believe those kind of costs should not have zero weight when considering a change. I've added two .zip files, each containing gDEBugger profiles before (attachment 191205 [details]) and after (attachment 191207 [details]) patch applied. These results were obtained from a running instance of WebKit EFL port (MiniBrowser) on Kaizoumark [1]. From the zipped files, you can notice the overall redundant state change-related calls drop from 21.85% to 0.01%. As for the Kaizoumark results below, we can also notice a difference (transforms tests raised from complexity 10 to 11): *before*: Transforms scale: 9 [got 26fps instead of 30fps at complexity 10] Transforms scale (with opacity): 9 [got 26fps instead of 30fps at complexity 10] *after*: Transforms: scale: 10 [got 21fps instead of 30fps at complexity 11] Transforms: scale (with opacity): 10 [got 20fps instead of 30fps at complexity 11] I've run these using a standard Desktop environment (Ubuntu Linux 12.10, x86-64, Intel HD 2000 integrated graphics card w/ Mesa OpenGL drivers). Unfortunately I do not have resources to benchmark this data on other platforms (specially on those with non-optimized drivers). If we are worried about adding code complexity, we could think about having this code safeguarded by a feature flag (i.e. USE(CACHED_OPENGL_CALLS)). What do you think? Please let me know if you need more data. Links: [1] http://www.kaizou.org/code/kaizoumark
(In reply to comment #17) > *before*: > Transforms scale: 9 [got 26fps instead of 30fps at complexity 10] > Transforms scale (with opacity): 9 [got 26fps instead of 30fps at complexity 10] > > *after*: > Transforms: scale: 10 [got 21fps instead of 30fps at complexity 11] > Transforms: scale (with opacity): 10 [got 20fps instead of 30fps at complexity 11] At least for me, gDebugger output is not convincing, because it doesn't necessarily indicate a performance improvement. The benchmark results look really promising though! Do you know if there is one particular piece of state which dominates the others for these performance improvements? My comment was simply about caching only those values that actually make a difference in performance. > I've run these using a standard Desktop environment (Ubuntu Linux 12.10, x86-64, Intel HD 2000 integrated graphics card w/ Mesa OpenGL drivers). Unfortunately I do not have resources to benchmark this data on other platforms (specially on those with non-optimized drivers). If we are worried about adding code complexity, we could think about having this code safeguarded by a feature flag (i.e. USE(CACHED_OPENGL_CALLS)). What do you think? Please let me know if you need more data. No, I'm not sure that's a good idea. The problem with code complexity is not what code you are running, but what code you are maintaining. One way you might make this code simpler to maintain, is to move the code that sets the state of the GraphicsContext3DState into methods on the state itself. I also hav a concern with the mode of GraphicsContext3D that works with the "current context." In that mode your cache may be invalid, so perhaps you shouldn't keep it for that mode or keep it only during certain periods of execution.
(In reply to comment #18) > (In reply to comment #17) > > > *before*: > > Transforms scale: 9 [got 26fps instead of 30fps at complexity 10] > > Transforms scale (with opacity): 9 [got 26fps instead of 30fps at complexity 10] > > > > *after*: > > Transforms: scale: 10 [got 21fps instead of 30fps at complexity 11] > > Transforms: scale (with opacity): 10 [got 20fps instead of 30fps at complexity 11] > > At least for me, gDebugger output is not convincing, because it doesn't necessarily indicate a performance improvement. The benchmark results look really promising though! Do you know if there is one particular piece of state which dominates the others for these performance improvements? My comment was simply about caching only those values that actually make a difference in performance. So far I've focused implementing the cached values for those state functions which indicated high percentage of redundant state changes, but w/o per-call analyzis (however I've seen most impact in redundant state changes comes from caching glBlender* calls). We can do this, though it requires some time for profiling each function individually. > > > I've run these using a standard Desktop environment (Ubuntu Linux 12.10, x86-64, Intel HD 2000 integrated graphics card w/ Mesa OpenGL drivers). Unfortunately I do not have resources to benchmark this data on other platforms (specially on those with non-optimized drivers). If we are worried about adding code complexity, we could think about having this code safeguarded by a feature flag (i.e. USE(CACHED_OPENGL_CALLS)). What do you think? Please let me know if you need more data. > > No, I'm not sure that's a good idea. The problem with code complexity is not what code you are running, but what code you are maintaining. One way you might make this code simpler to maintain, is to move the code that sets the state of the GraphicsContext3DState into methods on the state itself. Yes, that's doable, though it would make the GraphicsContext3D header file even bigger, so I'd say we move this out to a separate header? > > I also hav a concern with the mode of GraphicsContext3D that works with the "current context." In that mode your cache may be invalid, so perhaps you shouldn't keep it for that mode or keep it only during certain periods of execution. I guess you mean the cache may be invalid as the context state can be changed elsewhere, indeed, so maybe we may have a property on the GraphicsContext3D stating we're certain the GL states from the corresponding context are solely handled there.
Created attachment 191887 [details] Benchmark test Benchmark test used to profile GL calls (FPS data from WEBKIT_SHOW_FPS).
Created attachment 191894 [details] Patch Disabled caching by default on all ports (except on EFL, which reportely decreases no. of GL calls on offscreen context), moved code complexity to is*Redundant() functions
@Martin: I've profiled per-function caching using the benchmark test (attachment 191887 [details]), however I couldn't notice considerable performance impact using my Desktop as target device. Unfortunately I do not have resources to profile/test on less empowered devices (i.e. embedded/mobile devices), so if anyone is interested in doing so, please feel free (if using EFL, you can run the benchmark test with WEBKIT_SHOW_FPS=1 and --full-screen=true on MiniBrowser).
Comment on attachment 191894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191894&action=review I'd like to defer to mrobinson for the r+. A couple of concerns about the current patch. > Source/WebCore/platform/graphics/GraphicsContext3D.h:1095 > + GraphicsContext3DState(bool isCacheable = false) isCacheable sounds ambiguous. Please use a name like "isCached" or "usesCache". Also, for the constructor, to make call sites easier to read, please define an enum like CacheState with values like UseCache and DoNotUseCache. > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:303 > + if (m_state.activeTexture == texture) Please guard this check like the others.
Comment on attachment 191894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191894&action=review > Source/WebCore/platform/graphics/GraphicsContext3D.h:1128 > + bool isBlendFuncRedundant(GC3Denum sourceFactor, GC3Denum destinationFactor) Having isFoobar functions with a side effect is strange :) How about updateBlendFunc etc.
(In reply to comment #23) > (From update of attachment 191894 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191894&action=review > > I'd like to defer to mrobinson for the r+. A couple of concerns about the current patch. > > > Source/WebCore/platform/graphics/GraphicsContext3D.h:1095 > > + GraphicsContext3DState(bool isCacheable = false) > > isCacheable sounds ambiguous. Please use a name like "isCached" or "usesCache". > > Also, for the constructor, to make call sites easier to read, please define an enum like CacheState with values like UseCache and DoNotUseCache. Good idea, I'll add this new enum: enum CacheType { DoNotUseCache = 0, UseCache }; I'm going to move the cached/non-cached check internally. There's also the fact that if we won't use cache, the cache variables would never be used. I'm thinking about putting them inside a struct, and have this initialized only if we have 'cacheType == UseCache'. > > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:303 > > + if (m_state.activeTexture == texture) > > Please guard this check like the others. Ack.
(In reply to comment #24) > (From update of attachment 191894 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191894&action=review > > > Source/WebCore/platform/graphics/GraphicsContext3D.h:1128 > > + bool isBlendFuncRedundant(GC3Denum sourceFactor, GC3Denum destinationFactor) > > Having isFoobar functions with a side effect is strange :) > How about updateBlendFunc etc. Good idea as well, I thought about update<name>Cache (i.e. updateBlendFuncCache), though I'm thinking about the return value, would it be confusing on the GC3D side to return early if return is 'true'?
Comment on attachment 191894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191894&action=review >>> Source/WebCore/platform/graphics/GraphicsContext3D.h:1128 >>> + bool isBlendFuncRedundant(GC3Denum sourceFactor, GC3Denum destinationFactor) >> >> Having isFoobar functions with a side effect is strange :) >> How about updateBlendFunc etc. > > Good idea as well, I thought about update<name>Cache (i.e. updateBlendFuncCache), though I'm thinking about the return value, would it be confusing on the GC3D side to return early if return is 'true'? It should return early if return value is false. I think if (!m_state.updateScissor(..) return; is ok.
Comment on attachment 191894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191894&action=review > Source/WebCore/platform/graphics/GraphicsContext3D.h:1129 > + { Agree with Noam. Should we move GraphicsContext3DState into it's own header ??
(In reply to comment #28) > (From update of attachment 191894 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191894&action=review > > > Source/WebCore/platform/graphics/GraphicsContext3D.h:1129 > > + { > > Agree with Noam. > > Should we move GraphicsContext3DState into it's own header ?? I'd say so, yes, it's getting bigger as I add the extra functionalities.
Created attachment 192276 [details] Patch Various fixed reviewed by Kenneth and Noam, moved GC3DState to a separate header, avoid allocation of state cache variables if not going to cache, disable cache by default on all ports.
Note: I had to use raw values for GL enums on GraphicsContext3DState.h to avoid any dependency on GraphicsContext3D.h, however that would be solved if we move the GL enums from GraphicsContext3D.h to GraphicsTypes3D.h.
Comment on attachment 192276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192276&action=review > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:169 > + activeTexture(GL_TEXTURE0); This looks wrong. Assume we have a current bound location other than GL_TEXTURE0 i.e GL_TEXTURE1, m_state.activeTexture is GL_TEXTURE1. After this call it is set to GL_TEXTURE0. We try to reset the original bound location below (line 173). With this change we would be trying to reset to GL_TEXTURE0 rather than GL_TEXTURE1.
Comment on attachment 192276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192276&action=review >> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:169 >> + activeTexture(GL_TEXTURE0); > > This looks wrong. Assume we have a current bound location other than GL_TEXTURE0 i.e GL_TEXTURE1, m_state.activeTexture is GL_TEXTURE1. After this call it is set to GL_TEXTURE0. We try to reset the original bound location below (line 173). With this change we would be trying to reset to GL_TEXTURE0 rather than GL_TEXTURE1. You are right, I hoped I handled the issue with activeTexture set as both 'current bound location' and cached value by storing it in two different locations (m_state.activeTexture for current implementation, and m_state.cacheData.activeTexture for cached value), though I might not have considered the corner case you mentioned. I'll remove the cached activeTexture implementation and revert it as defined on the first patch.
Created attachment 192452 [details] Patch Fixed corner-case mentioned by Kalyan.
Created attachment 193070 [details] Patch Moved implementation to new file GraphicsContext3DState.cpp, so we can include GC3D header and thus avoid using raw OpenGL enum values.
Since your message to webkit-efl mentioned GPU-Proxy. We are investigating a gpu-proxy for WebKit-EFL.
(In reply to comment #36) > Since your message to webkit-efl mentioned GPU-Proxy. We are investigating a gpu-proxy for WebKit-EFL. If possible, could you reply to the mail with some details on this??
Comment on attachment 193070 [details] Patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.