Last Comment Bug 800031 - Include paint time in tab switch telemetry
: Include paint time in tab switch telemetry
Status: RESOLVED FIXED
[snappy]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla19
Assigned To: Jeff Muizelaar [:jrmuizel]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-10 10:28 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2012-10-24 08:27 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Include paint time in tabswitch telemetry (7.54 KB, patch)
2012-10-10 10:37 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
A working version (7.55 KB, patch)
2012-10-10 11:03 PDT, Jeff Muizelaar [:jrmuizel]
jacob.benoit.1: review-
Details | Diff | Splinter Review
Use newBrowser instead (7.54 KB, patch)
2012-10-10 11:27 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
V3 (8.37 KB, patch)
2012-10-10 11:37 PDT, Jeff Muizelaar [:jrmuizel]
dao+bmo: review+
ehsan: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-10-10 10:28:02 PDT
The current telemetry doesn't include paint time.
Comment 1 Jeff Muizelaar [:jrmuizel] 2012-10-10 10:37:13 PDT
Created attachment 670030 [details] [diff] [review]
Include paint time in tabswitch telemetry

This will look something like this.
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-10-10 11:03:52 PDT
Created attachment 670038 [details] [diff] [review]
A working version
Comment 3 Dão Gottwald [:dao] 2012-10-10 11:11:33 PDT
Comment on attachment 670038 [details] [diff] [review]
A working version

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -831,18 +831,21 @@
>       <method name="updateCurrentBrowser">
>         <parameter name="aForceUpdate"/>
>         <body>
>           <![CDATA[
>             var newBrowser = this.getBrowserAtIndex(this.tabContainer.selectedIndex);
>             if (this.mCurrentBrowser == newBrowser && !aForceUpdate)
>               return;
> 
>-            if (!aForceUpdate)
>+            if (!aForceUpdate) {
>               TelemetryStopwatch.start("FX_TAB_SWITCH_UPDATE_MS");
>+              this.mCurrentBrowser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).
>+                              getInterface(Ci.nsIDOMWindowUtils).beginTabSwitch();
>+            }

Calling beginTabSwitch on the content window that's becoming inactive doesn't make much sense to me. this.mCurrentBrowser is the previously selected browser here, newBrowser is the new one.
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2012-10-10 11:20:38 PDT
Comment on attachment 670038 [details] [diff] [review]
A working version

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

I only reviewed the gfx/ files. Not competent for other directories.

Even for that, I need an explanation about 'frame time recording':

::: gfx/layers/Layers.cpp
@@ +878,5 @@
>      mLastFrameTime = now;
>    }
> +  if (!mTabSwitchStart.IsNull()) {
> +    Telemetry::Accumulate(Telemetry::FX_TAB_SWITCH_TOTAL_MS,
> +                          int32_t((TimeStamp::Now() - mTabSwitchStart).ToMilliseconds()));

No need for this explicit int32_t cast. If a cast were needed, which it isn't AFAICS, that would be to uint32_t.

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +589,5 @@
>        PaintLayer(mTarget, mRoot, aCallback, aCallbackData, nullptr);
>        if (mWidget) {
>          FlashWidgetUpdateArea(mTarget);
>        }
> +      LayerManager::PostPresent();

Apparently PostPresent() is already used for "frame time recording". Explain to me how adding this call isn't going to change the results of frame time recording if it was enabled -- or how this change is wanted.
Comment 5 Jeff Muizelaar [:jrmuizel] 2012-10-10 11:27:13 PDT
Created attachment 670044 [details] [diff] [review]
Use newBrowser instead
Comment 6 Jeff Muizelaar [:jrmuizel] 2012-10-10 11:32:10 PDT
(In reply to Benoit Jacob [:bjacob] from comment #4)
> 
> No need for this explicit int32_t cast. If a cast were needed, which it
> isn't AFAICS, that would be to uint32_t.

Fixed.

> ::: gfx/layers/basic/BasicLayerManager.cpp
> @@ +589,5 @@
> >        PaintLayer(mTarget, mRoot, aCallback, aCallbackData, nullptr);
> >        if (mWidget) {
> >          FlashWidgetUpdateArea(mTarget);
> >        }
> > +      LayerManager::PostPresent();
> 
> Apparently PostPresent() is already used for "frame time recording". Explain
> to me how adding this call isn't going to change the results of frame time
> recording if it was enabled -- or how this change is wanted.

Previously, PostPresent wasn't called by the basic layer manager. This meant that frame time recording didn't work with that layer manager.
Comment 7 Jeff Muizelaar [:jrmuizel] 2012-10-10 11:37:26 PDT
Created attachment 670050 [details] [diff] [review]
V3

Addresses Benoit's comments and a comment to the idl and changes the uuid.
Comment 8 :Ehsan Akhgari 2012-10-10 12:32:17 PDT
Comment on attachment 670050 [details] [diff] [review]
V3

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

r=me on the domwindowutils dumpster.  ;-)
Comment 9 Dão Gottwald [:dao] 2012-10-10 13:20:05 PDT
Comment on attachment 670050 [details] [diff] [review]
V3

>+              newBrowser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).
>+                              getInterface(Ci.nsIDOMWindowUtils).beginTabSwitch();

formatting nit:

              newBrowser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
                        .getInterface(Ci.nsIDOMWindowUtils).beginTabSwitch();

>   "FX_TAB_SWITCH_UPDATE_MS": {
>     "kind": "exponential",
>     "high": "1000",
>     "n_buckets": 20,
>     "description": "Firefox: Time in ms spent updating UI in response to a tab switch"
>   },
>+  "FX_TAB_SWITCH_TOTAL_MS": {
>+    "kind": "exponential",
>+    "high": "1000",
>+    "n_buckets": 20,
>+    "description": "Firefox: Time in ms spent updating UI in response to a tab switch"
>+  },

Can you add to the description that this includes painting time?

Should we get rid of FX_TAB_SWITCH_UPDATE_MS?
Comment 10 Ed Morley [:emorley] 2012-10-12 04:18:31 PDT
https://hg.mozilla.org/mozilla-central/rev/155f7491ad68
Comment 11 (dormant account) 2012-10-15 10:20:24 PDT
*** Bug 790752 has been marked as a duplicate of this bug. ***
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-15 14:55:23 PDT
(In reply to Dão Gottwald [:dao] from comment #9)
> Should we get rid of FX_TAB_SWITCH_UPDATE_MS?

I think it's probably useful to have both numbers.
Comment 13 Jeff Muizelaar [:jrmuizel] 2012-10-18 11:23:40 PDT
Comment on attachment 670050 [details] [diff] [review]
V3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): We would like this so we can compare tab switch performance between 18 and 19.
User impact if declined: Minimal
Testing completed (on m-c, etc.): Has been on mozilla-central for about a wekk.
Risk to taking this patch (and alternatives if risky): There should be no behaviour change. The only things that came up during testing were an exception being caused by beginTabSwitch() this had very obvious side effects and shouldn't be happening at all anymore.
String or UUID changes made by this patch: None
Comment 14 Jeff Muizelaar [:jrmuizel] 2012-10-18 11:48:23 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/868d886d3616

Note You need to log in before you can comment on or make changes to this bug.