Bug 83513 - WebGLRenderingContext should defer caching program info
: WebGLRenderingContext should defer caching program info
Status: RESOLVED FIXED
: WebKit
WebGL
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-04-09 15:00 PST by
Modified: 2012-04-11 15:12 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-09 15:00:42 PST
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 From 2012-04-09 15:15:17 PST -------
*** Bug 83511 has been marked as a duplicate of this bug. ***
------- Comment #2 From 2012-04-10 13:59:41 PST -------
Created an attachment (id=136535) [details]
Patch
------- Comment #3 From 2012-04-10 14:00:07 PST -------
Ken, please have a look.
------- Comment #4 From 2012-04-10 18:20:14 PST -------
(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.

> 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 From 2012-04-11 10:24:49 PST -------
(In reply to comment #4)
> (From update of attachment 136535 [details] [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 From 2012-04-11 10:45:18 PST -------
Created an attachment (id=136697) [details]
Patch
------- Comment #7 From 2012-04-11 10:53:16 PST -------
Created an attachment (id=136702) [details]
Patch
------- Comment #8 From 2012-04-11 10:55:57 PST -------
Revised.  Please have another look.
------- Comment #9 From 2012-04-11 11:34:05 PST -------
(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 From 2012-04-11 12:41:05 PST -------
(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 From 2012-04-11 13:00:56 PST -------
Just fyi, I just tested this with the sample in the original issue.

Compiling 12 complex shaders went from 152ms to 4ms.
------- Comment #12 From 2012-04-11 13:16:05 PST -------
Created an attachment (id=136734) [details]
Patch
------- Comment #13 From 2012-04-11 13:16:57 PST -------
(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 From 2012-04-11 14:56:20 PST -------
(From update of attachment 136734 [details])
Looks good to me as long as the performance gains are still there. r=me
------- Comment #15 From 2012-04-11 15:12:52 PST -------
Committed r113915: <http://trac.webkit.org/changeset/113915>