Bug 83513

*** Bug 83511 has been marked as a duplicate of this bug. ***

Created attachment 136535 [details] Patch

Ken, please have a look.

Comment on attachment 136535 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136535&action=review Looks OK in general but r- for one API simplification. One question as well below. Also, was this tested with the test case I vaguely recall from the Chromium bug report to demonstrate the performance improvement? > Source/WebCore/html/canvas/WebGLProgram.h:64 > + void invalidateCachedInfo() { m_infoValid = false; } It would be better to make this private and call it from the implementation of increaseLinkCount(). This would make the API simpler and reduce the possibility of errors. > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3135 > + if (program->getLinkStatus()) Why make this change? We know that the program can't link successfully, and avoiding this call will avoid a bunch of caching work that we want to defer as long as possible. > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3143 > + program->invalidateCachedInfo(); Calling invalidateCachedInfo() as a side-effect of increaseLinkCount() would allow this line to be deleted.

(In reply to comment #4) > (From update of attachment 136535 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136535&action=review > > Looks OK in general but r- for one API simplification. One question as well below. Also, was this tested with the test case I vaguely recall from the Chromium bug report to demonstrate the performance improvement? > > > Source/WebCore/html/canvas/WebGLProgram.h:64 > > + void invalidateCachedInfo() { m_infoValid = false; } > > It would be better to make this private and call it from the implementation of increaseLinkCount(). This would make the API simpler and reduce the possibility of errors. Done. > > > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3135 > > + if (program->getLinkStatus()) > > Why make this change? We know that the program can't link successfully, and avoiding this call will avoid a bunch of caching work that we want to defer as long as possible. Basically if after the previous successful link, if the info is never cached, then simply set the link_status to false won't work. Because the next caching will overwrite the link_status to true (which is totally wrong). In order to avoid that, we call getLinkStatus() here to force an info cache, then the linkStatus = false will stay. > > > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3143 > > + program->invalidateCachedInfo(); > > Calling invalidateCachedInfo() as a side-effect of increaseLinkCount() would allow this line to be deleted. Done.

Created attachment 136697 [details] Patch

Created attachment 136702 [details] Patch

Revised. Please have another look.

(In reply to comment #5) > Basically if after the previous successful link, if the info is never cached, then simply set the link_status to false won't work. Because the next caching will overwrite the link_status to true (which is totally wrong). In order to avoid that, we call getLinkStatus() here to force an info cache, then the linkStatus = false will stay. This sounds fragile. Can you think of some way to make the state inside this class more robust? I think there should be some way to forcibly invalidate the cache and say that it is valid in its cleared state. If that were possible, then that could be done in this code path. I'll r+ this as is if you want, but would appreciate it if you'd give this some more thought.

(In reply to comment #9) > (In reply to comment #5) > > Basically if after the previous successful link, if the info is never cached, then simply set the link_status to false won't work. Because the next caching will overwrite the link_status to true (which is totally wrong). In order to avoid that, we call getLinkStatus() here to force an info cache, then the linkStatus = false will stay. > > This sounds fragile. Can you think of some way to make the state inside this class more robust? I think there should be some way to forcibly invalidate the cache and say that it is valid in its cleared state. If that were possible, then that could be done in this code path. > > I'll r+ this as is if you want, but would appreciate it if you'd give this some more thought. I just realized the basic logic here is setLinkStatus() should just be like other getters, need to force a cache update if needed before setting the status.

Just fyi, I just tested this with the sample in the original issue. Compiling 12 complex shaders went from 152ms to 4ms.

Created attachment 136734 [details] Patch

(In reply to comment #11) > Just fyi, I just tested this with the sample in the original issue. > > Compiling 12 complex shaders went from 152ms to 4ms. Thanks Gregg. My build is still building :) Ken, can you have another look?

Comment on attachment 136734 [details] Patch Looks good to me as long as the performance gains are still there. r=me

Committed r113915: <http://trac.webkit.org/changeset/113915>