Benoit Jacob [:bjacob] (mostly away) Reporter
	Description • 12 years ago

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.

	Benoit Jacob [:bjacob] (mostly away) Reporter
	Comment 2 • 12 years ago

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

	Benoit Jacob [:bjacob] (mostly away) Reporter
	Comment 3 • 12 years ago

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.

	Benoit Jacob [:bjacob] (mostly away) Reporter
	Comment 5 • 12 years ago

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.

	Nicolas Silva [:nical] Assignee
	Comment 6 • 12 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 7 • 12 years ago

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

	Benoit Jacob [:bjacob] (mostly away) Reporter
	Comment 8 • 12 years ago

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.

	Kelsey Gilbert [:jgilbert]
	Comment 9 • 12 years ago

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.

	Benoit Jacob [:bjacob] (mostly away) Reporter
	Comment 10 • 12 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 12 years ago

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