Optimize WebGLContext::ValidateBuffers

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: bjacob, Assigned: nical)

Tracking

(Blocks 1 bug)

unspecified
mozilla18
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [games:p2])

Attachments

(2 attachments, 1 obsolete attachment)

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

Updated

7 years ago
Assignee: nobody → nsilva
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 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.
Whiteboard: [games:p2]
Assignee

Comment 4

7 years ago
Attachment #659917 - Attachment is obsolete: true
Attachment #660094 - Flags: review?(bjacob)
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.
Attachment #660094 - Flags: review?(bjacob) → review+
https://hg.mozilla.org/mozilla-central/rev/c05ef941ce03
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
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.
Attachment #660824 - Flags: review?(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.
Attachment #660824 - Flags: review?(jgilbert) → review+
You need to log in before you can comment on or make changes to this bug.