Add GeckoView page load performance telemetry

RESOLVED FIXED in Firefox 65

Status

defect
P1
normal
RESOLVED FIXED
9 months ago
3 months ago

People

(Reporter: esawin, Assigned: esawin)

Tracking

(Blocks 1 bug)

51 Branch
mozilla65
All
Android
Dependency tree / graph

Firefox Tracking Flags

(geckoview64 fixed, firefox63 wontfix, firefox64 wontfix, firefox65 fixed)

Details

Attachments

(3 attachments, 8 obsolete attachments)

2.67 KB, text/plain
liuche
: review+
Details
15.25 KB, patch
esawin
: review+
Details | Diff | Splinter Review
26.49 KB, patch
esawin
: review+
Details | Diff | Splinter Review
See bug 1497997: we want to add (and control) the actual and perceived page load performance probes for GeckoView for our performance dashboard.
Add two probes: GV_LOAD_PROGRESS_COMPLETE_MS for perceived and GV_PAGE_LOAD_MS for actual page load time.

snorp: Do you think these probes would be adequate to represent the overall page load times?

chutten: Are the probe configs sane and do I need to add someone else to the mail alerts?
Attachment #9017585 - Flags: review?(snorp)
Attachment #9017585 - Flags: review?(chutten)
Attachment #9017585 - Flags: review?(snorp) → review+
Comment on attachment 9017585 [details] [diff] [review]
0001-Bug-1499418-1.0-Add-GeckoView-page-load-performance-.patch

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

Probe definitions and TelemetryStopwatch uses look sane. You'll need a Data Collection Review for the added collections: https://wiki.mozilla.org/Firefox/Data_Collection
Attachment #9017585 - Flags: review?(chutten) → review+
What do you think about this two probes: one times the total Gecko startup duration on the parent and content processes and the other times the parent module initialization.

This setup should be mostly safe with the preloading mechanics and if not, we should be able to easily filter them out as outliers.

The Java probes would need Telemetry.java and dependencies to be moved under geckoview/ or we could move the probes into native code instead.
Attachment #9020461 - Flags: review?(snorp)
Attachment #9020461 - Flags: review?(nchen)
Comment on attachment 9020461 [details] [diff] [review]
0002-Bug-1499418-2.0-Add-GeckoView-startup-performance-te.patch

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

::: mobile/android/chrome/geckoview/geckoview.js
@@ +78,5 @@
>  
>        this._modules.clear();
>      });
> +
> +    MODULES_INIT_PROBE.finish();

Have you measured how long this typically takes? We don't really do any time-consuming stuff during the init stage, so I'm not sure if this is worth tracking.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java
@@ +8,5 @@
>  import org.mozilla.gecko.annotation.RobocopTarget;
>  import org.mozilla.gecko.annotation.WrapForJNI;
>  import org.mozilla.gecko.mozglue.GeckoLoader;
>  import org.mozilla.gecko.process.GeckoProcessManager;
> +import org.mozilla.gecko.Telemetry;

Should be at the top (above `RobocopTarget`)

@@ +142,5 @@
>      private GeckoProfile mProfile;
>      private String[] mArgs;
>      private Bundle mExtras;
>      private int mFlags;
> +    private static Telemetry.Timer sInitTimer;

Keep with other static fields

@@ +159,5 @@
>                                        final int prefsFd, final int prefMapFd,
>                                        final int ipcFd,
>                                        final int crashFd,
>                                        final int crashAnnotationFd) {
> +        sInitTimer = new Telemetry.UptimeTimer("GV_STARTUP_RUNTIME_MS");

Move to after the `mInitialized` check

::: toolkit/components/telemetry/Histograms.json
@@ +14224,5 @@
> +  },
> +  "GV_STARTUP_RUNTIME_MS": {
> +    "record_in_processes": ["main", "content"],
> +    "alert_emails": [
> +      "esawin@mozilla.com"

Should be a GV team-wide email address

@@ +14236,5 @@
> +  },
> +  "GV_STARTUP_MODULES_MS": {
> +    "record_in_processes": ["main"],
> +    "alert_emails": [
> +      "esawin@mozilla.com"

Same

::: widget/android/nsAppShell.cpp
@@ +433,5 @@
>          mozilla::GeckoProcessManager::Init();
>          mozilla::GeckoScreenOrientation::Init();
>          mozilla::GeckoSystemStateListener::Init();
>          mozilla::PrefsHelper::Init();
> +        mozilla::widget::Telemetry::Init();

`mozilla::widget::Telemetry` is not part of GeckoView ("Telemetry.h" is under "widget/android/fennec")
Attachment #9020461 - Flags: review?(nchen) → feedback+
Attachment #9020461 - Flags: review?(snorp) → review+
Addressed comments.
Attachment #9017585 - Attachment is obsolete: true
Attachment #9022215 - Flags: review+
Addressed comments.
Attachment #9020461 - Attachment is obsolete: true
Attachment #9022216 - Flags: review?(nchen)
Move the telemetry classes into the geckoview directory.
I've renamed Telemetry.java to TelemetryUtils.java and added a Telemetry.java wrapper for Fennec which handles the MMA stuff.
Attachment #9022224 - Flags: review?(nchen)
Comment on attachment 9022215 [details] [diff] [review]
0001-Bug-1499418-1.1-Add-GeckoView-page-load-performance-.patch

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

Just leaving two short drive-by comments.

::: mobile/android/modules/geckoview/GeckoViewTelemetry.jsm
@@ +5,5 @@
> +
> +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetters(this, {
> +  TelemetryStopwatch: "resource://gre/modules/TelemetryStopwatch.jsm",

Note bug 1482091, which is converting `TelemetryStopwatch` to C++ & webidl.

@@ +11,5 @@
> +
> +var EXPORTED_SYMBOLS = ["HistogramStopwatch"];
> +
> +// A helper for histogram timer probes.
> +class HistogramStopwatch {

This looks like a good utility that could live in the Telemetry component directly.
Could you file a follow-up bug for us?
Posted file request.md
Attachment #9022677 - Flags: review?(liuche)
Attachment #9022216 - Flags: review?(nchen) → review+
Comment on attachment 9022224 [details] [diff] [review]
0003-Bug-1499418-3.0-Move-and-split-Fennec-s-telemetry-cl.patch

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/TelemetryUtils.java
@@ +24,5 @@
>   *   avoid timing a user activity when their phone is in their pocket!
>   *
>   * The majority of methods in this class are defined in terms of real time.
>   */
>  @RobocopTarget

Probably don't need this anymore?

@@ +196,5 @@
>          if (method == null) {
>              throw new IllegalArgumentException("Expected non-null method - use Method.NONE?");
>          }
>  
> +        if (true) {

This should still be `!AppConstants.RELEASE_OR_BETA`
Attachment #9022224 - Flags: review?(nchen) → review+
Addressed comment.
Attachment #9022215 - Attachment is obsolete: true
Attachment #9023723 - Flags: review+
Addressed comments.
Attachment #9022216 - Attachment is obsolete: true
Attachment #9023724 - Flags: review+
Addressed comments.
Attachment #9022224 - Attachment is obsolete: true
Attachment #9023725 - Flags: review+
64=wontfix because this issue doesn't block Focus 8.0 from using GV 64.
Comment on attachment 9022677 [details]
request.md

One nit: in addition to the GeckoView mailing list, can you also include an individual's email? That field is a list so you can have multiple items. Having an individual makes it easier for people to know a specific person to ping.


1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, in Histograms.jsm

2) Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, Histograms are controlled by user data controls


3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

esawin

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Type 1, technical data on GeckoView loading and initialization

5) Is the data collection request for default-on or default-off?

Included in library, data collection is up to app

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No

7) Is the data collection covered by the existing Firefox privacy notice?
Yes

8) Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate)**

No, collections of basic performance of Gecko
Attachment #9022677 - Flags: review?(liuche) → review+
Addressed comment and merged patch 1 and 2.
Attachment #9023723 - Attachment is obsolete: true
Attachment #9023724 - Attachment is obsolete: true
Attachment #9024767 - Flags: review+
Addressed comments and fixed one Telemetry.stopUISession() call.
Attachment #9023725 - Attachment is obsolete: true
Attachment #9024769 - Flags: review+
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bc653b02e2f
[1.3] Add GeckoView page load and startup performance telemetry probes. r=snorp,chutten,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/24e87b02707b
[3.2] Refactor and move Fennec's telemetry classes to geckoview/. r=jchen
https://hg.mozilla.org/mozilla-central/rev/8bc653b02e2f
https://hg.mozilla.org/mozilla-central/rev/24e87b02707b
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
64=affected because Eugen recommends we uplift these new telemetry probes to GV 64 Beta for the Focus 8.0 testing.
Comment on attachment 9024767 [details] [diff] [review]
0001-Bug-1499418-1.3-Add-GeckoView-page-load-and-startup-.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: No user impact, but we would miss page load and startup performance telemetry data.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Adding new telemetry probes does not increase risk, but the refactoring of the telemetry classes may pose a low risk for Fennec's telemetry reporting.

String changes made/needed: None
Attachment #9024767 - Flags: approval-mozilla-beta?
Comment on attachment 9024769 [details] [diff] [review]
0002-Bug-1499418-3.2-Refactor-and-move-Fennec-s-telemetry.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: 

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String changes made/needed:
Attachment #9024769 - Flags: approval-mozilla-beta?
(In reply to Eugen Sawin [:esawin] from comment #21)
> Is this code covered by automated tests?: No
> 
> Has the fix been verified in Nightly?: No
> 
> Needs manual test from QE?: No
> 
> If yes, steps to reproduce: 
> 
> List of other uplifts needed: None
> 
> Risk to taking this patch: Low
> 
> Why is the change risky/not risky? (and alternatives if risky): Adding new
> telemetry probes does not increase risk, but the refactoring of the
> telemetry classes may pose a low risk for Fennec's telemetry reporting.
> 
The above doesn't sound like a reasonable combination to me.  Is this something we could land on a GV relbranch to avoid affecting fennec?  Or are there steps we can take to validate fennec telemetry with this change?
Flags: needinfo?(esawin)
Sebastian, is there a way to validate Fennec telemetry (especially the MMA stuff) on Nightly?
I had to refactor some Fennec telemetry code (patch 2) to make it reusable for GeckoView and would like to see that uplifted without regressing Fennec.
Flags: needinfo?(esawin) → needinfo?(s.kaspari)
(In reply to Julien Cristau [:jcristau] from comment #23)
> The above doesn't sound like a reasonable combination to me.  Is this
> something we could land on a GV relbranch to avoid affecting fennec?  Or are
> there steps we can take to validate fennec telemetry with this change?

Landing that on a GV release branch would work, we don't need this changes in Fennec.
Flags: needinfo?(jcristau)
Comment on attachment 9024767 [details] [diff] [review]
0001-Bug-1499418-1.3-Add-GeckoView-page-load-and-startup-.patch

Gecko 64 will be merged to mozilla-release next week. We can create the GV64 relbranch and uplift it there at that time with an approval-mozilla-geckoview64 request (that request flag will also be created next Monday).
Flags: needinfo?(jcristau)
Attachment #9024767 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #9024769 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
We don't have tracking-geckoview64 and status-geckoview64 flags yet, but will want to set this bug to + and affected, respectively, once they've been created next week.
The status-geckoview64 flag has been created. Setting status-geckoview64=affected because Eugen said he would like to uplift these telemetry probes.
Comment on attachment 9024767 [details] [diff] [review]
0001-Bug-1499418-1.3-Add-GeckoView-page-load-and-startup-.patch

[GeckoView Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for consideration: The new telemetry probes are required to track GeckoView page load and startup performance.

User impact if declined: No user impact, engineering impact would be the lack of page load performance telemetry.

Fix Landed on Version: 65

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): For GeckoView, this only adds some new telemetry probes.

String or UUID changes made by this patch: None
Attachment #9024767 - Flags: approval-mozilla-geckoview64?
Comment on attachment 9024769 [details] [diff] [review]
0002-Bug-1499418-3.2-Refactor-and-move-Fennec-s-telemetry.patch

[GeckoView Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for consideration: 

User impact if declined: 

Fix Landed on Version: 

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String or UUID changes made by this patch:
Attachment #9024769 - Flags: approval-mozilla-geckoview64?
Comment on attachment 9024767 [details] [diff] [review]
0001-Bug-1499418-1.3-Add-GeckoView-page-load-and-startup-.patch

Per discussion with cpeterson, approved for GV64.
Attachment #9024767 - Flags: approval-mozilla-geckoview64? → approval-mozilla-geckoview64+
Attachment #9024769 - Flags: approval-mozilla-geckoview64? → approval-mozilla-geckoview64+
Flags: needinfo?(s.kaspari)
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 65 → mozilla65
Blocks: 1531179
See Also: → 1546149
You need to log in before you can comment on or make changes to this bug.