Run a Telemetry experiment to vet the unaccelerated compositor process in Nightly 56

RESOLVED FIXED in Firefox 56

Status

()

Core
Graphics
RESOLVED FIXED
5 months ago
2 months ago

People

(Reporter: ashughes, Assigned: ashughes)

Tracking

(Depends on: 1 bug)

56 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56+ fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

5 months ago
This is bug tracks running a Telemetry Experiment to A/B test the unaccelerated compositor process as implemented in bug 1356091.
(Assignee)

Comment 1

5 months ago
Assigning to myself, code incoming.
Depends on: 1356091
QA Contact: anthony.s.hughes
Whiteboard: [gfx-noted]
(Assignee)

Comment 2

5 months ago
Created attachment 8870611 [details] [diff] [review]
bug1367246_v1.patch

Initial patch to implement Telemetry Experiment.

Note, this experiment has three cohorts:
1) control: users with unaccelerated compositor process enabled
2) disabled: users with unaccelerated compositor process disabled
3) disqualified: users with hardware acceleration

The reason for the 'disqualified' cohort is so that accelerated users are not mistakenly factored in to the 'control' cohort data. All three conditions have been tested successfully on a local machine.

David, please take a quick look and let me know if you see any issues.

Felipe, please review this when you get a chance as we'd like this experiment to conclude before June 12th. I still need to get the XPI signed and approval from Release Management, pending your review.
Attachment #8870611 - Flags: review?(felipc)
Attachment #8870611 - Flags: feedback?(dvander)
(Assignee)

Updated

5 months ago
Assignee: nobody → anthony.s.hughes
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 months ago
Created attachment 8870929 [details]
Screenshot

I left the Nightly running overnight which had the experiment active and came back to this screen. It looks like Nightly had decided to disable my TelEx overnight and is prompting me to re-enable it. I've never seen this before. Is it expected?
(Assignee)

Comment 4

5 months ago
(In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #3)
> Created attachment 8870929 [details]
> Screenshot
> 
> I left the Nightly running overnight which had the experiment active and
> came back to this screen. It looks like Nightly had decided to disable my
> TelEx overnight and is prompting me to re-enable it. I've never seen this
> before. Is it expected?

Sorry, forgot to flag the need-infos.
Flags: needinfo?(felipc)
Flags: needinfo?(dothayer)
I never saw this doorhanger before. Andy, do you know if this is from the work to disable legacy addons?
Flags: needinfo?(felipc) → needinfo?(amckay)

Comment 6

5 months ago
I believe thats the doorhanger that occurs for sideloading and add-on. Looking at the add-on it is not multi process compatible, according to the install.rdf. We shouldn't be shipping any non-e10s add-ons.

/cc'ing aswan to check what I said.
Flags: needinfo?(amckay) → needinfo?(aswan)

Comment 7

5 months ago
The doorhanger in the "Screenshot" attachment is indeed the one used for sideloading.  I don't know much about telemetry experiments but I thought that we were trying to deprecate them in favor of shield?
As for multiprocessCompatible, we don't pay attention to it for addons of types other than "extension" (not intentionally anyway...)
Flags: needinfo?(aswan)
Ah this notice used to be an about: page, I didn't know it was changed to a doorhanger. Hm I'm guessing this might have been triggered by the fact that the xpi is not signed yet, but I'm not sure. If after signing the same problem continues to happen, we should investigate because it would be a recent bug.
Flags: needinfo?(dothayer)
Comment on attachment 8870611 [details] [diff] [review]
bug1367246_v1.patch

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

::: experiments/unaccelerated-compositor-process-nightly55/code/bootstrap.js
@@ +86,5 @@
> +    if (compositor == "basic") {
> +      initializeExperiment();
> +    } else {
> +      Experiments.instance().setExperimentBranch(kSELF_ID, "disqualified").then(null, Cu.reportError);
> +      initializeExperiment();

since you're now already setting the branch here, you don't need to call initializeExperiment() here or in the similar case below.

r+ with this changed.

::: experiments/unaccelerated-compositor-process-nightly55/manifest.json
@@ +12,5 @@
> +    "appName"          : ["Firefox"],
> +    "channel"          : ["nightly"],
> +    "minVersion"       : "55.0a1",
> +    "maxVersion"       : "55.0a1",
> +    "os"               : ["WINNT"]

we should use a sample here to not go to 100% of users
Attachment #8870611 - Flags: review?(felipc) → review+

Comment 10

5 months ago
(In reply to :Felipe Gomes (needinfo me!) from comment #8)
> Ah this notice used to be an about: page, I didn't know it was changed to a
> doorhanger. Hm I'm guessing this might have been triggered by the fact that
> the xpi is not signed yet, but I'm not sure. If after signing the same
> problem continues to happen, we should investigate because it would be a
> recent bug.

The doorhanger is unrelated to signing, it is shown for any sideloaded extension.
To be clear, your test uses the experiment manifest to have Experiments.jsm set up the experiment right?

Kris, could this (telemetry experiments showing the sideload doorhanger) be an (unintended?) side effect of bug 1358846?
Flags: needinfo?(kmaglione+bmo)
(In reply to Andrew Swan [:aswan] from comment #10)
> (In reply to :Felipe Gomes (needinfo me!) from comment #8)
> > Ah this notice used to be an about: page, I didn't know it was changed to a
> > doorhanger. Hm I'm guessing this might have been triggered by the fact that
> > the xpi is not signed yet, but I'm not sure. If after signing the same
> > problem continues to happen, we should investigate because it would be a
> > recent bug.
> 
> The doorhanger is unrelated to signing, it is shown for any sideloaded
> extension.
> To be clear, your test uses the experiment manifest to have Experiments.jsm
> set up the experiment right?

Correct
(In reply to Andrew Swan [:aswan] from comment #10)
> Kris, could this (telemetry experiments showing the sideload doorhanger) be
> an (unintended?) side effect of bug 1358846?

That or bug 1356826 seem like likely culprits, yes.

I was under the impression that we were no longer deploying new telemetry experiments in favor of Shield studies, though.
Flags: needinfo?(kmaglione+bmo)
Ah no, telemetry experiments are still being used, and there's a few coming up soon.
OK, then I'll look into this.
(Assignee)

Comment 15

5 months ago
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #14)
> OK, then I'll look into this.

Do you want me to file a bug?
(In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #15)
> (In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment
> #14)
> > OK, then I'll look into this.
> 
> Do you want me to file a bug?

Yes, that would help. Thanks.
(Assignee)

Updated

5 months ago
Depends on: 1367823
(Assignee)

Comment 17

5 months ago
Be advised that we're not passed a point where we can run this experiment on Nightly 55 due to bug 1367823. With any luck we might be able to do this in Beta, assuming the lack of an experiment in Nightly doesn't block this feature from riding the trains.

Updated

5 months ago
Depends on: 1371363
(Assignee)

Comment 18

4 months ago
Updating this to reflect that I plan to run this experiment in 56 now, not 55.
Summary: Run a Telemetry experiment to vet the unaccelerated compositor process in Nightly 55 → Run a Telemetry experiment to vet the unaccelerated compositor process in Nightly 56
Version: unspecified → 56 Branch
(Assignee)

Comment 19

4 months ago
Created attachment 8883662 [details] [diff] [review]
unaccelerated-compositor-process-nightly56.patch

Hi Felipe,

Here is an updated patch. I've tested it locally and everything works as expected. I made the changes you previously requested but given the amount of time that's past can you please review it?

Thanks
Attachment #8883662 - Flags: review?(felipc)
Attachment #8883662 - Flags: review?(felipc) → review+
(Assignee)

Comment 20

4 months ago
Created attachment 8884072 [details] [diff] [review]
unaccelerated-compositor-process-nightly56.patch

Felipe, thanks for the r+. I'm uploading a new patch with the following cosmetic changes:
- Revised timestamps and duration to start on July 10, end on July 20
- Removed previously commented out code
- Made new XPI with these changes

I trust this doesn't need re-review.
Attachment #8870611 - Attachment is obsolete: true
Attachment #8883662 - Attachment is obsolete: true
Attachment #8870611 - Flags: feedback?(dvander)
Attachment #8884072 - Flags: review+
(Assignee)

Comment 21

4 months ago
Created attachment 8884075 [details]
experiment.xpi (unsigned)

Jason, can you please sign this XPI?
Attachment #8884075 - Flags: feedback?(jthomas)
Created attachment 8884087 [details]
experiment.xpi signed

Please see attached.
(Assignee)

Comment 23

4 months ago
Thanks Jason.

Felipe, can you please push this to staging so I can test it before requesting RelMan approval?
Flags: needinfo?(felipc)

Updated

4 months ago
Attachment #8884075 - Flags: feedback?(jthomas)
Done!
https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/61df6e6c03cf4232a869699c1775ae6f51931c2e
Flags: needinfo?(felipc)
(Assignee)

Comment 25

3 months ago
(In reply to :Felipe Gomes (needinfo me!) from comment #24)
> Done!
> https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/
> 61df6e6c03cf4232a869699c1775ae6f51931c2e

Hi Felipe, I'm not seeing any active experiments when I point to https://telemetry-experiment-dev.allizom.org/firefox-manifest.json
Flags: needinfo?(felipc)
I cloned the repo in a different folder (to ensure it wasn't a problem of untracked files that weren't pushed), and built it locally without problems. I wonder if it's some infra issue
Flags: needinfo?(felipc)
(Assignee)

Comment 27

3 months ago
(In reply to :Felipe Gomes (needinfo me!) from comment #26)
> I cloned the repo in a different folder (to ensure it wasn't a problem of
> untracked files that weren't pushed), and built it locally without problems.
> I wonder if it's some infra issue

I concur, locally it works but pointing to the staging server does not. I'm returned with no active experiments on staging. I don't see anything on the mailing lists that would indicate an infra issue. Who can I contact to look in to this?
(Assignee)

Comment 28

3 months ago
:Usul, do you know what's happening here? I'm confused as to why this would work locally but not on staging.
Flags: needinfo?(ludovic)
(In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #28)
> :Usul, do you know what's happening here? I'm confused as to why this would
> work locally but not on staging.

If staging doesn't have video drivers then you'd see a difference (wondering why you asked me though :))
Flags: needinfo?(ludovic)
(In reply to Ludovic Hirlimann [:Usul] from comment #29)
> (In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #28)
> > :Usul, do you know what's happening here? I'm confused as to why this would
> > work locally but not on staging.
> 
> If staging doesn't have video drivers then you'd see a difference (wondering
> why you asked me though :))

Reading more , I have no clue and no access to these systems ask someone from :jason's team
(Assignee)

Comment 31

3 months ago
(In reply to Ludovic Hirlimann [:Usul] from comment #30)
> Reading more , I have no clue and no access to these systems ask someone from :jason's team

Sorry, it was suggested I reach out to you.

Jason, do you have any idea why I'm seeing no active experiments on staging? This experiment should be active there but I don't see it when I point to https://telemetry-experiment-dev.allizom.org/firefox-manifest.json.
Flags: needinfo?(jthomas)
Sorry I don't think I am going to be much help here. The service telemetry-experiment-dev.allizom.org is currently not managed by my team (Data Operations) and is in IT's control (resides on IT's infrastructure, specifically the generic cluster). Data Operations *should* take control over this service and move it out of IT's control.

IT Webops might be the right person to reach out. The Mozilla MOC should be able to assist with contacting the correct people. 

https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=38553804 has some documentation on this serivce.
Flags: needinfo?(jthomas)
Component: Graphics → WebOps: Other
Product: Core → Infrastructure & Operations
Version: 56 Branch → unspecified

Updated

3 months ago
Whiteboard: [gfx-noted] → [kanban:https://webops.kanbanize.com/ctrl_board/2/5061] [gfx-noted]
Taking a look at this now.
There was a hung hg pull that was clogging up things.  I've cleared it out and this is working now (though you may have a local cache).  If hg pull hangs are a known thing we should probably run this with a timeout command so it doesn't hang forever.
(Assignee)

Comment 35

3 months ago
Thanks Eric, this is now working and I've confirmed my experiment is operating as expected on staging. Will now send an email to get approval to push to production.

Updated

3 months ago
Component: WebOps: Other → Graphics
Product: Infrastructure & Operations → Core
Whiteboard: [kanban:https://webops.kanbanize.com/ctrl_board/2/5061] [gfx-noted] → [gfx-noted]
Version: unspecified → 56 Branch
status-firefox56: --- → affected
tracking-firefox56: --- → +
(Assignee)

Comment 36

3 months ago
Hi Felipe,

I just got sign-off from Release Management. The dates in the manifest will need to be updated before pushing to production. Given the setbacks I'd like this to start today (July 17) and run through July 27.

Thanks in advance.
Flags: needinfo?(felipc)
Alright, I updated the start/end dates in the manifest, and added the release_tag for it. Once it appears correctly on staging, all that's left is to file a bug for it to be deployed to production:

https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/a8a91936dac30a53c64aa027a07a8b85c8e0e7c2
https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/6022f3f24315a62063e09a1b6b9e37a2d0a03c1a
Flags: needinfo?(felipc)
(Assignee)

Updated

3 months ago
Blocks: 1381599
(Assignee)

Comment 38

3 months ago
(In reply to :Felipe Gomes (needinfo me!) from comment #37)
> All that's left is to file a bug for it to be deployed to production

Thanks Felipe!
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
(Assignee)

Comment 39

3 months ago
Experiment is now live. Thanks again everyone for your help!
status-firefox56: affected → fixed
Target Milestone: --- → mozilla56
(Assignee)

Comment 40

3 months ago
Flagging some folks to spread this around before it becomes a larger issue...

From bug 1371363:
The problem I was facing in bug 1367246 comment 3 related to this bug has just been shown to me in the wild on one of my officemates' computers. He updated his Nightly and saw the prompt to enable the experiment. I don't think there's anything I can do now but this may impact the number of users getting my experiment and the prompt may be "scary" to some users.
Flags: needinfo?(milan)
Flags: needinfo?(lhenry)
Flags: needinfo?(dvander)

Comment 41

3 months ago
Anthony, can you get the build details for the Firefox version that exhibited this problem?
Flags: needinfo?(anthony.s.hughes)
(Assignee)

Comment 42

3 months ago
(In reply to Andrew Swan [:aswan] from comment #41)
> Anthony, can you get the build details for the Firefox version that
> exhibited this problem?

Adding Roland who should be able to get you the details from his system.
Flags: needinfo?(anthony.s.hughes) → needinfo?(rtanglao)
I'll track this issue in bug 1371363. I wouldn't want users on release to see this warning.
Flags: needinfo?(lhenry)
Created attachment 8888080 [details]
19july2017-windows-10-nightly-about-support.json

as requested in comment 42 by :ashughes
Flags: needinfo?(rtanglao)
(Assignee)

Comment 45

3 months ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #43)
> I'll track this issue in bug 1371363. I wouldn't want users on release to
> see this warning.

As a note, I originally saw this warning when I was developing this experiment back in Nightly 55 so I assume it'd affect 55 Beta users had I experimented there instead.
Flags: needinfo?(milan)
Flags: needinfo?(dvander)
(Assignee)

Updated

2 months ago
Blocks: 1394903
You need to log in before you can comment on or make changes to this bug.