Bug 83513 – WebGLRenderingContext should defer caching program info

• Component: WebGL
• Status: RESOLVED FIXED

[Gregg Tavares]

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

[Zhenyao Mo]

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

[Zhenyao Mo]

Created attachment 136535 [details]
Patch

[Zhenyao Mo]

Ken, please have a look.

[Kenneth Russell]

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.

[Zhenyao Mo]

(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.

[Zhenyao Mo]

Created attachment 136697 [details]
Patch

[Zhenyao Mo]

Created attachment 136702 [details]
Patch

[Zhenyao Mo]

Revised.  Please have another look.

[Kenneth Russell]

(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.

[Zhenyao Mo]

(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.

[Gregg Tavares]

Just fyi, I just tested this with the sample in the original issue.

Compiling 12 complex shaders went from 152ms to 4ms.

[Zhenyao Mo]

Created attachment 136734 [details]
Patch

[Zhenyao Mo]

(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?

[Kenneth Russell]

Comment on attachment 136734 [details]
Patch

Looks good to me as long as the performance gains are still there. r=me

[Zhenyao Mo]

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