Optimize WebGLContext::ValidateBuffers

RESOLVED FIXED in mozilla18

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: bjacob, Assigned: nical)

Tracking

(Blocks: 1 bug)

unspecified
mozilla18
Points:
---
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

5 years ago
Assignee: nobody → nsilva
(Assignee)

Comment 1

5 years ago
Created attachment 659917 [details] [diff] [review]
Cache the result of WebGLContext::ValidateBuffers
(Reporter)

Comment 2

5 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%.
(Reporter)

Comment 3

5 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.
(Reporter)

Updated

5 years ago
Whiteboard: [games:p2]
(Assignee)

Comment 4

5 years ago
Created attachment 660094 [details] [diff] [review]
Cache the result of WebGLContext::ValidateBuffers
Attachment #659917 - Attachment is obsolete: true
Attachment #660094 - Flags: review?(bjacob)
(Reporter)

Comment 5

5 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.
Attachment #660094 - Flags: review?(bjacob) → review+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c05ef941ce03
https://hg.mozilla.org/mozilla-central/rev/c05ef941ce03
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Reporter)

Comment 8

5 years ago
Created attachment 660824 [details] [diff] [review]
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.
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+
http://hg.mozilla.org/integration/mozilla-inbound/rev/b8ee83073a77
https://hg.mozilla.org/mozilla-central/rev/b8ee83073a77
Flags: in-testsuite-

Updated

4 years ago
Blocks: 710398
You need to log in before you can comment on or make changes to this bug.