777967 - Optimize WebGLContext::ValidateBuffers

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Because even though it doesn't show very highly on profiles, it still is typically the #1 WebGL-related symbol on profiles.

Note: this is orthogonal to bug 732660.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Cache the result of WebGLContext::ValidateBuffers

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Just checked on Banabread level 2:
 - without patch, ValidateBuffer is the top WebGLContext symbol at 0.75%. perf annotate shows that that time is primarily spent in CheckedInt arithmetic, and secondarily in if()'s that can't be branch-predicted because of erratic outcome (the if (!vd.enabled) and the if (!mCurrentProgram->IsAttribInUse(i))).
 - with patch, ValidateBuffer is down to 0.07%.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 659917 [details] [diff] [review]
Cache the result of WebGLContext::ValidateBuffers

Review of attachment 659917 [details] [diff] [review]:
-----------------------------------------------------------------

nits:

::: content/canvas/src/WebGLContext.h
@@ +1124,5 @@
>      int32_t mGLMaxVaryingVectors;
>      int32_t mGLMaxFragmentUniformVectors;
>      int32_t mGLMaxVertexUniformVectors;
>  
> +    // Cache the max number of element that can be read from bound VBOs

elements

@@ +1125,5 @@
>      int32_t mGLMaxFragmentUniformVectors;
>      int32_t mGLMaxVertexUniformVectors;
>  
> +    // Cache the max number of element that can be read from bound VBOs
> +    // (result of ValidateBuffers) for better performances.

performance

@@ +1126,5 @@
>      int32_t mGLMaxVertexUniformVectors;
>  
> +    // Cache the max number of element that can be read from bound VBOs
> +    // (result of ValidateBuffers) for better performances.
> +    int32_t mCachedMaxReadFromVBO;

suggested name: mMinInUseBufferLength ????

@@ +1128,5 @@
> +    // Cache the max number of element that can be read from bound VBOs
> +    // (result of ValidateBuffers) for better performances.
> +    int32_t mCachedMaxReadFromVBO;
> +
> +    inline void InvalidateCachedMaxReadFromVBO()

Same

::: content/canvas/src/WebGLContextGL.cpp
@@ +393,5 @@
>  void
>  WebGLContext::BlendFuncSeparate(WebGLenum srcRGB, WebGLenum dstRGB,
>                                  WebGLenum srcAlpha, WebGLenum dstAlpha)
>  {
> +    InvalidateCachedMaxReadFromVBO();

X

@@ +417,5 @@
>                                         GLsizeiptr size,
>                                         const GLvoid *data,
>                                         GLenum usage)
>  {
> +    InvalidateCachedMaxReadFromVBO();

Only do it in BufferData

@@ +443,5 @@
>  NS_IMETHODIMP
>  WebGLContext::BufferData(WebGLenum target, const JS::Value& data, GLenum usage,
>                           JSContext* cx)
>  {
> +    InvalidateCachedMaxReadFromVBO();

No need to do it in the old-bindings functions.

Comment 4

[Nicolas Silva [:nical]]

Attachment: Cache the result of WebGLContext::ValidateBuffers

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 660094 [details] [diff] [review]
Cache the result of WebGLContext::ValidateBuffers

Review of attachment 660094 [details] [diff] [review]:
-----------------------------------------------------------------

Changed my mind --- please change before committing:

::: content/canvas/src/WebGLContext.h
@@ +1126,5 @@
>      int32_t mGLMaxVertexUniformVectors;
>  
> +    // Cache the max number of elements that can be read from bound VBOs
> +    // (result of ValidateBuffers).
> +    int32_t mMinInUseBufferLength;

Actually, I was wrong: this should be AttribArrayLength, not BufferLength. Indeed this has nothing to do with the length of the buffer (in bytes) and everything to do with how many vertices can be drawn i.e. the attrib array length

@@ +1128,5 @@
> +    // Cache the max number of elements that can be read from bound VBOs
> +    // (result of ValidateBuffers).
> +    int32_t mMinInUseBufferLength;
> +
> +    inline void InvalidateCachedMinInUseBufferLength()

Same.

Comment 6

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c05ef941ce03

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c05ef941ce03

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: move the InvalidateCachedMinInUseAttribArrayLength calls a bit

I should have caught that during review: these calls need to be moved down a bit, ideally just before the point where we start modifying any state as a result of the current WebGL function call.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 660824 [details] [diff] [review]
move the InvalidateCachedMinInUseAttribArrayLength calls a bit

Review of attachment 660824 [details] [diff] [review]:
-----------------------------------------------------------------

Indenting nit, but r+ otherwise.

::: content/canvas/src/WebGLContextGL.cpp
@@ +3712,5 @@
>      if (!ValidateObject("linkProgram", program))
>          return;
>  
> +    InvalidateCachedMinInUseAttribArrayLength(); // we do it early in this function
> +      // as some of the validation below changes program state

Bad indenting here.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b8ee83073a77

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b8ee83073a77