Add tracking for data allocation calls in WebGL

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: svargas, Assigned: svargas)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → svargas
(Assignee)

Comment 2

2 years ago
Rebased patch to be independent of other changes (the webgl.max -> webgl.perf.max rename).
Attachment #8871510 - Attachment is obsolete: true
Attachment #8871510 - Flags: review?(jgilbert)
Attachment #8871940 - Flags: review?(jgilbert)
(Assignee)

Comment 3

2 years ago
Attachment #8871940 - Attachment is obsolete: true
Attachment #8871940 - Flags: review?(jgilbert)
Comment on attachment 8871958 [details] [diff] [review]
0001-Bug-1367919-Add-tracking-for-data-allocation-calls-i.patch

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

::: dom/canvas/WebGLBuffer.cpp
@@ +153,4 @@
>          }
>      } else {
>          gl->fBufferData(target, size, uploadData, usage);
> +        mContext->OnDataAllocCall();

Why not put this below, outside the if/else, so we only need to write it once?

::: dom/canvas/WebGLContext.cpp
@@ -932,4 @@
>          mBackbufferNeedsClear = true;
>  
>          gl->ResetSyncCallCount("Existing WebGLContext resized.");
> -

Looks like you accidentally removed a line here.

@@ +1596,5 @@
> +
> +void
> +WebGLContext::OnDataAllocCall() const
> +{
> +   mDataAllocGLCallCount++;

Move this to the header for inlining.

@@ +1602,5 @@
> +
> +uint64_t
> +WebGLContext::GetNumGLDataAllocCalls() const
> +{
> +   return mDataAllocGLCallCount;

Move this to the header for inlining.

@@ +1610,5 @@
> +WebGLContext::OnEndOfFrame() const
> +{
> +   if (gfxPrefs::WebGLSpewFrameAllocs()) {
> +      GeneratePerfWarning("[webgl.perf.spew-frame-allocs] %" PRIu64 " Data allocations this frame.\n",
> +                     mDataAllocGLCallCount);

Align this with the previous line's `(`.

@@ +1646,3 @@
>  
>      mShouldPresent = false;
>      gl->ResetSyncCallCount("WebGLContext PresentScreenBuffer");

ResetSyncCallCount should go in OnEndOfFrame.
Attachment #8871958 - Flags: review-
Flags: needinfo?(svargas)
Priority: -- → P1
Whiteboard: gfx-noted
(Assignee)

Comment 5

2 years ago
Attachment #8871958 - Attachment is obsolete: true
Flags: needinfo?(svargas)
Attachment #8873157 - Flags: review?(jgilbert)
Comment on attachment 8873157 [details] [diff] [review]
0001-Bug-1367919-Add-tracking-for-data-allocation-calls-i.patch

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

::: dom/canvas/WebGLBuffer.cpp
@@ +142,4 @@
>  
>          gl::GLContext::LocalErrorScope errorScope(*gl);
>          gl->fBufferData(target, size, uploadData, usage);
> +

Revert this newline.

::: dom/canvas/WebGLContext.cpp
@@ +931,4 @@
>          mResetLayer = true;
>          mBackbufferNeedsClear = true;
>  
> +        gl->ResetSyncCallCount("Existing WebGLContext resized.");

Move this to the proper patch. I think it's from the other bug.

@@ +1595,5 @@
> +void
> +WebGLContext::OnEndOfFrame() const
> +{
> +   if (gfxPrefs::WebGLSpewFrameAllocs()) {
> +      GeneratePerfWarning("[webgl.perf.spew-frame-allocs] %" PRIu64 " Data allocations this frame.\n",

s/Data/data/

No ending newlines for Generate*Warning().

@@ +1599,5 @@
> +      GeneratePerfWarning("[webgl.perf.spew-frame-allocs] %" PRIu64 " Data allocations this frame.\n",
> +                          mDataAllocGLCallCount);
> +   }
> +   mDataAllocGLCallCount = 0;
> +   gl->ResetSyncCallCount("WebGLContext OnEndOfFrame");

Move this to the proper patch. I think it's from the other bug.
Attachment #8873157 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 7

2 years ago
Attachment #8873157 - Attachment is obsolete: true
Attachment #8878727 - Flags: review?(jgilbert)
Attachment #8878727 - Flags: review?(jgilbert) → review+
Blocks: 1367613
Please rebase on inbound.
No longer blocks: 1367613
Flags: needinfo?(svargas)
Blocks: 1367613
(Assignee)

Comment 10

2 years ago
Applies cleanly to mozilla-inbound
Attachment #8878727 - Attachment is obsolete: true
Flags: needinfo?(svargas)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 12

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db8a4903fd27
Add tracking for data allocation calls in WebGL. r=jgilbert
Keywords: checkin-needed

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/db8a4903fd27
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.