1154323 - Instrument browser.js for profiling

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

It would be nice to have profiling markers when we start and stop loading pages (according to nsIWebProgressListener) and maybe also mark when browser.js init starts/stops

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Add profiler markers when we start/stop loading a page on Android

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8592271 [details] [diff] [review]
Add profiler markers when we start/stop loading a page on Android

Drive by

>     // Filter optimization: Only really send NETWORK state changes to Java listener
>     if (aStateFlags & Ci.nsIWebProgressListener.STATE_IS_NETWORK) {

I wonder if we should wrap this in a Nightly check using AppContstants

>+      if ((aStateFlags & Ci.nsIWebProgressListener.STATE_START) &&
>+          aWebProgress.isTopLevel) {

No wrap needed

>+        Profiler.AddMarker("Load start: " + aRequest.QueryInterface(Components.interfaces.nsIChannel).originalURI.spec);

Components.interfaces.nsIChannel -> Ci.nsIChannel

>+      } else if ((aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) &&
>+                 !aWebProgress.isLoadingDocument) {

No wrap needed

>+        Profiler.AddMarker("Load stop: " + aRequest.QueryInterface(Components.interfaces.nsIChannel).originalURI.spec);

Components.interfaces.nsIChannel -> Ci.nsIChannel

Comment 3

[:Margaret Leibovic]

Comment on attachment 8592271 [details] [diff] [review]
Add profiler markers when we start/stop loading a page on Android

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

I agree with mfinkle's comments. I agree it would probably be good to put this behind a Nightly only check.

f+ for now and I'll re-review with comments addressed.

::: mobile/android/chrome/content/browser.js
@@ +4406,5 @@
>  
>      // Filter optimization: Only really send NETWORK state changes to Java listener
>      if (aStateFlags & Ci.nsIWebProgressListener.STATE_IS_NETWORK) {
> +      if ((aStateFlags & Ci.nsIWebProgressListener.STATE_START) &&
> +          aWebProgress.isTopLevel) {

I don't think we need this isTopLevel check, since we bail early if this isn't the top level window.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Add profiler markers when we start/stop loading a page on Android

Comment 5

[:Margaret Leibovic]

Comment on attachment 8595367 [details] [diff] [review]
Add profiler markers when we start/stop loading a page on Android

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

::: mobile/android/chrome/content/browser.js
@@ +4411,5 @@
> +      if (AppConstants.NIGHTLY_BUILD && (aStateFlags & Ci.nsIWebProgressListener.STATE_START)) {
> +        Profiler.AddMarker("Load start: " + aRequest.QueryInterface(Ci.nsIChannel).originalURI.spec);
> +      } else if (AppConstants.NIGHTLY_BUILD && (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) && !aWebProgress.isLoadingDocument) {
> +        Profiler.AddMarker("Load stop: " + aRequest.QueryInterface(Ci.nsIChannel).originalURI.spec);
> +      }

Nit: To make this a bit easier to reader, you could just put this flags checking logic all inside a single |if (AppConstants.NIGHTLY_BUILD)| block (instead of checking that in 2 different if statements).

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be5b399dc1a0

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/be5b399dc1a0