Bug 83513 - WebGLRenderingContext should defer caching program info
: WebGLRenderingContext should defer caching program info
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebGL
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Zhenyao Mo
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-09 15:00 PDT by Gregg Tavares
Modified: 2012-04-11 15:12 PDT (History)
2 users (show)

See Also:


Attachments
Patch (7.48 KB, patch)
2012-04-10 13:59 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
Patch (7.76 KB, patch)
2012-04-11 10:45 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
Patch (7.76 KB, patch)
2012-04-11 10:53 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
Patch (7.51 KB, patch)
2012-04-11 13:16 PDT, Zhenyao Mo
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gregg Tavares 2012-04-09 15:00:42 PDT
Currently WebGLRenderingContext::linkProgram immediately queries the program link status and caches locations

This makes it block while the program is being linked.

Instead, move the caching code to a function, WebGLRenderingContext::cacheProgramInfoIfNotCached

And then call cacheProgramInfoIfNotCached in these functions 

   WebGLRenderingContext::getProgramParameter
   WebGLRenderingContext::getActiveAttrib
   WebGLRenderingContext::getActiveUniform
   WebGLRenderingContext::getUniformLocation
   WebGLRenderingContext::getAttribLocation

And invalidate the cache in WebGLRenderingContext::linkProgram
Comment 1 Zhenyao Mo 2012-04-09 15:15:17 PDT
*** Bug 83511 has been marked as a duplicate of this bug. ***
Comment 2 Zhenyao Mo 2012-04-10 13:59:41 PDT
Created attachment 136535 [details]
Patch
Comment 3 Zhenyao Mo 2012-04-10 14:00:07 PDT
Ken, please have a look.
Comment 4 Kenneth Russell 2012-04-10 18:20:14 PDT
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.
Comment 5 Zhenyao Mo 2012-04-11 10:24:49 PDT
(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.
Comment 6 Zhenyao Mo 2012-04-11 10:45:18 PDT
Created attachment 136697 [details]
Patch
Comment 7 Zhenyao Mo 2012-04-11 10:53:16 PDT
Created attachment 136702 [details]
Patch
Comment 8 Zhenyao Mo 2012-04-11 10:55:57 PDT
Revised.  Please have another look.
Comment 9 Kenneth Russell 2012-04-11 11:34:05 PDT
(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.
Comment 10 Zhenyao Mo 2012-04-11 12:41:05 PDT
(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.
Comment 11 Gregg Tavares 2012-04-11 13:00:56 PDT
Just fyi, I just tested this with the sample in the original issue.

Compiling 12 complex shaders went from 152ms to 4ms.
Comment 12 Zhenyao Mo 2012-04-11 13:16:05 PDT
Created attachment 136734 [details]
Patch
Comment 13 Zhenyao Mo 2012-04-11 13:16:57 PDT
(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 14 Kenneth Russell 2012-04-11 14:56:20 PDT
Comment on attachment 136734 [details]
Patch

Looks good to me as long as the performance gains are still there. r=me
Comment 15 Zhenyao Mo 2012-04-11 15:12:52 PDT
Committed r113915: <http://trac.webkit.org/changeset/113915>