1204429 - Counting number of vertices and triangles of a snapshot

Status: RESOLVED FIXED

(DevTools Graveyard :: Canvas Debugger, defect)

Description

[Daosheng Mu[:daoshengmu]]

Currently, CanvasDebugger has displayed number of draw-calls and function calls. In order to assist developers to find performance issue, we should add vertices and triangles count to provide them more information.

Comment 1

[Daosheng Mu[:daoshengmu]]

Attachment: Part 1: Collect triangle and vertex counts information. r=vporof.

Part 1:
 - Counting triangle and vertex number while calling WebGL drawArrays and drawElements functions.


Next steps:

Part 2: 
 - Add tests.
Part 3:
 - Need to display it at the snapshot list?

Comment 2

[Daosheng Mu[:daoshengmu]]

Attachment: Part 2: Add tests for counting number of vertices and triangles in WebGL draw functions. r=vporof.

Part 2: 
 - Add draw line, point, and triangle tests for WebGL drawArray and drawElements functions.

Next step:
 - Need to display it at the snapshot list?

Comment 3

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8667787 [details] [diff] [review]
Part 2: Add tests for counting number of vertices and triangles in WebGL draw functions. r=vporof.

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

Victor, do you know how to upload test files to EXAMPLE_URL?

::: browser/devtools/canvasdebugger/test/head.js
@@ +39,5 @@
>  const SIMPLE_CANVAS_DEEP_STACK_URL = EXAMPLE_URL + "doc_simple-canvas-deep-stack.html";
>  const WEBGL_ENUM_URL = EXAMPLE_URL + "doc_webgl-enum.html";
>  const WEBGL_BINDINGS_URL = EXAMPLE_URL + "doc_webgl-bindings.html";
> +const WEBGL_DRAW_ARRAYS = "file:///Volumes/Firefox/gecko-dev/browser/devtools/canvasdebugger/test/" + "doc_webgl-drawArrays.html";
> +const WEBGL_DRAW_ELEMENTS = "file:///Volumes/Firefox/gecko-dev/browser/devtools/canvasdebugger/test/" + "doc_webgl-drawElements.html";

In fact, I think I should put my test files to EXAMPLE_URL, "http://example.com/browser/browser/devtools/canvasdebugger/test/", but I don't know how to put my test files to this url link.

Comment 4

[Victor Porof [:vporof][:vp]]

Comment on attachment 8664763 [details] [diff] [review]
Part 1: Collect triangle and vertex counts information. r=vporof.

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

Needs tests.

::: toolkit/devtools/server/actors/canvas.js
@@ +133,2 @@
>     */
> +  initialize: function(conn, { canvas, calls, screenshot, triCount, vertexCount, pointCount, lineCount }) {

Nit: put triCount, vertexCount etc. into an object.

@@ +505,5 @@
> +
> +      let { name, args } = functionCall.details;
> +
> +      // Calculate tris and vertices
> +      if (CanvasFront.DRAW_FUNCS[0] === name) {  // drawArrays

This is very fragile. Suppose DRAW_FUNCS is changed by an unknown party, this would completely break. Should just test the names directly.

Comment 5

[Victor Porof [:vporof][:vp]]

Comment on attachment 8664763 [details] [diff] [review]
Part 1: Collect triangle and vertex counts information. r=vporof.

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

Oh, I see there's a test patch attached to this bug as well. Will review that separately.

In the meantime, let's think of a mechanism that allows primitive counts to be recorded in realtime. Such information would really fit nicely in a graph in the performance tool, and would correlate well with the other graphs there. Be it sharing some code, or relying on a single implementation, could you look into that a bit?

Comment 6

[Victor Porof [:vporof][:vp]]

(In reply to Daosheng Mu[:daoshengmu] from comment #3)
> Comment on attachment 8667787 [details] [diff] [review]
> Part 2: Add tests for counting number of vertices and triangles in WebGL
> draw functions. r=vporof.
> 
> Review of attachment 8667787 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Victor, do you know how to upload test files to EXAMPLE_URL?
> 
> ::: browser/devtools/canvasdebugger/test/head.js
> @@ +39,5 @@
> >  const SIMPLE_CANVAS_DEEP_STACK_URL = EXAMPLE_URL + "doc_simple-canvas-deep-stack.html";
> >  const WEBGL_ENUM_URL = EXAMPLE_URL + "doc_webgl-enum.html";
> >  const WEBGL_BINDINGS_URL = EXAMPLE_URL + "doc_webgl-bindings.html";
> > +const WEBGL_DRAW_ARRAYS = "file:///Volumes/Firefox/gecko-dev/browser/devtools/canvasdebugger/test/" + "doc_webgl-drawArrays.html";
> > +const WEBGL_DRAW_ELEMENTS = "file:///Volumes/Firefox/gecko-dev/browser/devtools/canvasdebugger/test/" + "doc_webgl-drawElements.html";
> 
> In fact, I think I should put my test files to EXAMPLE_URL,
> "http://example.com/browser/browser/devtools/canvasdebugger/test/", but I
> don't know how to put my test files to this url link.

You don't upload these files anywhere. There is a local server running when you start these tests. Just create a new file in the same dir and add it to browser.ini

Comment 7

[Victor Porof [:vporof][:vp]]

Comment on attachment 8667787 [details] [diff] [review]
Part 2: Add tests for counting number of vertices and triangles in WebGL draw functions. r=vporof.

Postponing review until above comments are addressed.

Comment 8

[Daosheng Mu[:daoshengmu]]

Attachment: 0001-Bug-1204429-Part-1-Collect-triangle-and-vertex-count.patch

Part1:
  - Collect triangle and vertex counts information. r=vporof.

V2:
  - Adjusting the mechanism of counting primitive. I move them to devtools/server/primitive.js. It will collect primitive counts at every rAF. Therefore, it can support snapshot and timeline mode simultaneously. If we would like to use it in Performance tool, we need to use CallWatcherActor to monitor functioncalls and execute Primitive.handleDrawPrimitive to collect primitive counts.

Comment 9

[Victor Porof [:vporof][:vp]]

Comment on attachment 8674077 [details] [diff] [review]
0001-Bug-1204429-Part-1-Collect-triangle-and-vertex-count.patch

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

Nice implementation with the separate actor. I'm not sure why it's also monitoring framerate though. Add a test and we can land this.

::: devtools/server/primitive.js
@@ +17,5 @@
> +};
> +
> +/**
> + * A utility for monitoring WebGL draw primitives. Takes a `tabActor`
> + * and monitors primitive over time.

nit: s/primitive/primitive draws/

@@ +22,5 @@
> + */
> +const WebGLDrawArrays = "drawArrays";
> +const WebGLDrawElements = "drawElements";
> +
> +var Primitive = exports.Primitive = Class({

Nit: I'd rename this to WebGLDrawWatcher or something similar.

@@ +37,5 @@
> +     this.stopRecording();
> +  },
> +
> +  /**
> +   * Starts monitoring primitive, storing the primitive count per tick.

typo: primitives.

@@ +44,5 @@
> +    if (this._recording) {
> +      return;
> +    }
> +    this._recording = true;
> +    this._ticks = [];

Why are we also storing ticks here? We have a separate framerate actor for that.

@@ +98,5 @@
> +
> +  /**
> +   * Gets the refresh driver ticks recorded so far.
> +   */
> +  getPendingTicks: function(beginAt = 0, endAt = Number.MAX_SAFE_INTEGER) {

No need for this method, right? Why measure framerate too?

Comment 10

[Daosheng Mu[:daoshengmu]]

(In reply to Victor Porof [:vporof][:vp] from comment #9)
> Comment on attachment 8674077 [details] [diff] [review]
> 0001-Bug-1204429-Part-1-Collect-triangle-and-vertex-count.patch
> 
> Review of attachment 8674077 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice implementation with the separate actor. I'm not sure why it's also
> monitoring framerate though. Add a test and we can land this.
> 
> ::: devtools/server/primitive.js
> @@ +17,5 @@
> > +};
> > +
> > +/**
> > + * A utility for monitoring WebGL draw primitives. Takes a `tabActor`
> > + * and monitors primitive over time.
> 
> nit: s/primitive/primitive draws/
> 
Do you mean moving it to server/primitive/draws ?

> @@ +22,5 @@
> > + */
> > +const WebGLDrawArrays = "drawArrays";
> > +const WebGLDrawElements = "drawElements";
> > +
> > +var Primitive = exports.Primitive = Class({
> 
> Nit: I'd rename this to WebGLDrawWatcher or something similar.
> 
> @@ +37,5 @@
> > +     this.stopRecording();
> > +  },
> > +
> > +  /**
> > +   * Starts monitoring primitive, storing the primitive count per tick.
> 
> typo: primitives.
> 
> @@ +44,5 @@
> > +    if (this._recording) {
> > +      return;
> > +    }
> > +    this._recording = true;
> > +    this._ticks = [];
> 
> Why are we also storing ticks here? We have a separate framerate actor for
> that.
> 
> @@ +98,5 @@
> > +
> > +  /**
> > +   * Gets the refresh driver ticks recorded so far.
> > +   */
> > +  getPendingTicks: function(beginAt = 0, endAt = Number.MAX_SAFE_INTEGER) {
> 
> No need for this method, right? Why measure framerate too?

getPendingTicks and _ticks that are used for displaying information in a graph in performance tool. IIUC, Performance tool is a timeline-based tool. _ticks helps us collect the drawcall data per tick.

Comment 11

[Victor Porof [:vporof][:vp]]

(In reply to Daosheng Mu[:daoshengmu] from comment #10)
> (In reply to Victor Porof [:vporof][:vp] from comment #9)
> > Comment on attachment 8674077 [details] [diff] [review]
> > 0001-Bug-1204429-Part-1-Collect-triangle-and-vertex-count.patch
> > 
> > Review of attachment 8674077 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Nice implementation with the separate actor. I'm not sure why it's also
> > monitoring framerate though. Add a test and we can land this.
> > 
> > ::: devtools/server/primitive.js
> > @@ +17,5 @@
> > > +};
> > > +
> > > +/**
> > > + * A utility for monitoring WebGL draw primitives. Takes a `tabActor`
> > > + * and monitors primitive over time.
> > 
> > nit: s/primitive/primitive draws/
> > 
> Do you mean moving it to server/primitive/draws ?

Rename "primitive" to "primitive draws"

> 
> > @@ +22,5 @@
> > > + */
> > > +const WebGLDrawArrays = "drawArrays";
> > > +const WebGLDrawElements = "drawElements";
> > > +
> > > +var Primitive = exports.Primitive = Class({
> > 
> > Nit: I'd rename this to WebGLDrawWatcher or something similar.
> > 
> > @@ +37,5 @@
> > > +     this.stopRecording();
> > > +  },
> > > +
> > > +  /**
> > > +   * Starts monitoring primitive, storing the primitive count per tick.
> > 
> > typo: primitives.
> > 
> > @@ +44,5 @@
> > > +    if (this._recording) {
> > > +      return;
> > > +    }
> > > +    this._recording = true;
> > > +    this._ticks = [];
> > 
> > Why are we also storing ticks here? We have a separate framerate actor for
> > that.
> > 
> > @@ +98,5 @@
> > > +
> > > +  /**
> > > +   * Gets the refresh driver ticks recorded so far.
> > > +   */
> > > +  getPendingTicks: function(beginAt = 0, endAt = Number.MAX_SAFE_INTEGER) {
> > 
> > No need for this method, right? Why measure framerate too?
> 
> getPendingTicks and _ticks that are used for displaying information in a
> graph in performance tool. IIUC, Performance tool is a timeline-based tool.
> _ticks helps us collect the drawcall data per tick.

Comment 12

[Victor Porof [:vporof][:vp]]

(In reply to Daosheng Mu[:daoshengmu] from comment #10)
> > 
> > No need for this method, right? Why measure framerate too?
> 
> getPendingTicks and _ticks that are used for displaying information in a
> graph in performance tool. IIUC, Performance tool is a timeline-based tool.
> _ticks helps us collect the drawcall data per tick.

But this is a different actor. Why have this functionality duplicated in another actor?

Comment 13

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8674077 [details] [diff] [review]
0001-Bug-1204429-Part-1-Collect-triangle-and-vertex-count.patch

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

I am ok to remove it if you think it is not appropriate.

::: devtools/server/primitive.js
@@ +125,5 @@
> +        lines: this._lines
> +      }
> +    }
> +
> +    this._ticks.push(tick);

Actually, _ticks in this actor stores primitive info not framerate.

Comment 14

[Daosheng Mu[:daoshengmu]]

Attachment: Part 1: Collect primitive counts information (V3)

Victor, please help me review it. Thanks a lot.

V3:

Refer to Comment 9 to correct typos.

Comment 15

[Daosheng Mu[:daoshengmu]]

Attachment: Part 1: Collect primitive counts information (V4)

Victor, please help me review it. Thanks a lot.

V4:
Refer to Comment 9 to correct typos. Same as to V3 but adjust patch lines.

Comment 16

[Daosheng Mu[:daoshengmu]]

Attachment: Part 2: Add tests for counting number of primitives in WebGL draw functions (V2)

V2:

Making some adjustments because part 1 is changed.

Comment 17

[Daosheng Mu[:daoshengmu]]

Attachment: Part 2: Add tests for counting number of primitives in WebGL draw functions. (V3)

V3:

Making some adjustments because part 1 is changed.

Comment 18

[Victor Porof [:vporof][:vp]]

Comment on attachment 8686508 [details] [diff] [review]
Part 1: Collect primitive counts information (V4)

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

::: devtools/server/actors/canvas.js
@@ +380,5 @@
>        return;
>      }
>      if (CanvasFront.DRAW_CALLS.has(name) && this._animationStarted) {
>        this._handleDrawCall(functionCall);
> +      this._drawWatcher.handleDrawPrimitive(functionCall);

Looks like the drawWatcher isn't really watching anything. It's more of a helper. Rename the class and this member to WebGLPrimitiveCounter.

::: devtools/server/primitive.js
@@ +32,5 @@
> +     this.stopRecording();
> +  },
> +
> +  /**
> +   * Starts monitoring primitive draws, storing the primitives count per tick.

This isn't monitoring anything by itself.

@@ +34,5 @@
> +
> +  /**
> +   * Starts monitoring primitive draws, storing the primitives count per tick.
> +   */
> +  startRecording: function() {

This is incorrectly named, it doesn't actually start recording anything. Should rename it to `resetCounts` or something.

@@ +35,5 @@
> +  /**
> +   * Starts monitoring primitive draws, storing the primitives count per tick.
> +   */
> +  startRecording: function() {
> +    if (this._recording) {

Is _recording really necessary?

@@ +47,5 @@
> +    this._startTime = this.tabActor.docShell.now();
> +  },
> +
> +  /**
> +   * Stops monitoring primitive draws, returning the recorded values.

Bad comment. This helper doesn't monitor anything by itself.

@@ +49,5 @@
> +
> +  /**
> +   * Stops monitoring primitive draws, returning the recorded values.
> +   */
> +  stopRecording: function(beginAt = 0, endAt = Number.MAX_SAFE_INTEGER) {

Should rename this to getCounts.

@@ +62,5 @@
> +
> +    this._tris = 0;
> +    this._vertices = 0;
> +    this._points = 0;
> +    this._lines = 0;

This method does more than it should. Using `resetCounts` is enough.

@@ +63,5 @@
> +    this._tris = 0;
> +    this._vertices = 0;
> +    this._points = 0;
> +    this._lines = 0;
> +    

trailing whitespace.

@@ +70,5 @@
> +
> +  /**
> +   * Stops monitoring primitive draws, without returning the recorded values.
> +   */
> +  cancelRecording: function() {

No need for this method.

@@ +72,5 @@
> +   * Stops monitoring primitive draws, without returning the recorded values.
> +   */
> +  cancelRecording: function() {
> +    this._recording = false;
> +    this._ticks = null;

Remove _ticks

@@ +79,5 @@
> +
> +  /**
> +   * Returns whether this instance is currently recording.
> +   */
> +  isRecording: function() {

Is this method really necessary?

Comment 19

[Victor Porof [:vporof][:vp]]

Comment on attachment 8686604 [details] [diff] [review]
Part 2: Add tests for counting number of primitives in WebGL draw functions. (V3)

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

::: devtools/client/canvasdebugger/test/browser_profiling-webgl.js
@@ +74,5 @@
> +  
> +  yield removeTab(target.tab);
> +  
> +  finish();
> +}

Please remove trailing whitespace.

Comment 20

[Daosheng Mu[:daoshengmu]]

Attachment: Part 1: Collect primitive counts information (V5)

Victor, thanks for your comments.

V5 - Refer to Comment 18, removing redundant functions and correct function naming.

Comment 21

[Daosheng Mu[:daoshengmu]]

Attachment: Part 2: Add tests for counting number of primitives in WebGL draw functions. (V4)

V4 - Refer to Comment 19, removing trailing whitespace.

Comment 22

[Daosheng Mu[:daoshengmu]]

Attachment: Part 2: Add tests for counting number of primitives in WebGL draw functions. (V5)

V5 - Update the patch by removing trailing whitespace and take r+ from Comment 19.

Comment 23

[Daosheng Mu[:daoshengmu]]

Try link. Looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e2e00ee0b96

Please help land Attachment #8690415 [details] [diff] and Attachment #8698348 [details] [diff] to m-c.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/548a5523e43c
https://hg.mozilla.org/integration/fx-team/rev/83705c77787d

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/548a5523e43c
https://hg.mozilla.org/mozilla-central/rev/83705c77787d