1401617 - Autophone - GeckoView example always uses e10s

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, enhancement)

Description

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/blob/master/tests/s1s2geckoviewtest.py#L42

if self.e10s:
    e10s = 'true' if self.e10s else 'false'
    local_extra_args.extend(['--ez', 'use_multiprocess %s' % e10s])

We need to specify --ez "use_multiprocess false" to turn e10s off since it is on by default. The if self.e10s: check prevents us from turning e10s off altogether. doh.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: bug-1401617-v1.patch

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8910356 [details] [diff] [review]
bug-1401617-v1.patch

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

good find

Comment 3

[Bob Clary [:bc] (inactive)]

Thanks!

https://github.com/mozilla/autophone/commit/ed4d35acbcb08e0fe06704d9363a3d2186fb7db2
will deploy in a few minutes.

Not such a good find. It took non-e10s GeckoView being broken and Autophone not catching it for me to be aware of it.

Comment 4

[Bob Clary [:bc] (inactive)]

snorp: I noticed that tg didn't fail after this patch even though the geckoview_example hadn't been fixed yet when this landed.  I also noticed that the e10s and non-e10s graphs still looked identical. I looked into how Autophone uses adb_android.py to launch Firefox and it seemed that my simple approach here did not work the way I thought it did. I did a debug verbose run last night on autophone-4 and I believe it shows the use_multiprocess argument is not correctly set:

https://treeherder.allizom.org/#/jobs?repo=autoland&revision=39b52c2ec77cae87ba503ea1de0e29c1c78bd769&filter-searchStr=autophone

https://treeherder.allizom.org/logviewer.html#?job_id=125805074&repo=autoland&lineNumber=580

Can you confirm for me that this isn't working and reopen this bug if so?

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: bug-1401617-pass-use_multiprocess-v1.patch

The use_multiprocess must be passed in extras separately from the args.

The review comment has r=self, but if you are around to review it I'll amend it. Otherwise I'll probably go ahead and land this later tonight.

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: bug-1401617-adb_android-launch-app-bool-v1.patch

There is an outstanding bug in adb_android's launch_application which incorrectly detects a boolean extra as an int. This patch also adds info logging to log the command being issued in order to help identify these issues without relying on logging level debug. We'll have to patch the version in m-c as well.

ditto r=.

Comment 7

[Bob Clary [:bc] (inactive)]

I did several test runs:
<https://treeherder.allizom.org/#/jobs?repo=autoland&filter-searchStr=autophone&fromchange=d22408a702ecfbee65c6921d4268500e39c4c30d&tochange=284f9a059a93357a95b78ce79b7381d50b878263>

The first, d22408a702ec, is without any patches.

The second, 05b0fccb403e, is with the final set of patches including the command logging change to adb_android and is run with logging level info and verbose False.

The third, 284f9a059a93, was actually run first. After I discovered the int/bool issue I cancelled the run, fixed the issue and restarted. This run has logging level debug and verbose True.

You can see in

<http://phonedash-dev.allizom.org/#/2017-09-22/2017-09-23/binning=repo-phonetype-phoneid-test_name&rejected=norejected&errorbars=noerrorbars&errorbartype=standarderror&valuetype=median&geckoview-e10s-nytimes=on&geckoview-nytimes=on&throbberstart=on&throbberstop=on&throbbertime=on&first=on&second=on&autoland=on&nexus-5=on&nexus-5-1=on&nexus-5-11=on&nexus-5-7=on&pixel=on&pixel-11=on&pixel-12=on>

that the first run does not distinguish between non-e10s and e10s while the second and third runs do. e10s is appreciably slower than the non-e10s.

One thing that stands out when looking at the run times, the pixel non-e10s runs take twice as long to run. This appears to be the result of the shutdown intent not working which causes us to wait to forcefully stop the process after the shutdown intent fails to do so. esawin, snorp: thoughts?

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8911514 [details] [diff] [review]
bug-1401617-pass-use_multiprocess-v1.patch

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

looks safe

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8911515 [details] [diff] [review]
bug-1401617-adb_android-launch-app-bool-v1.patch

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

oh, great find

Comment 10

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/9289b0cc32614bcce7fb6c99bd82264902b71245
https://github.com/mozilla/autophone/commit/dd74df3fe681595b4b1951876049516e9e3669e3
deploying in a few minutes.

Once this has run a couple of times and we have logs available, I'll file a new bug on the non-e10s shutdown intent.

Comment 11

[Bob Clary [:bc] (inactive)]

igoldan: Heads up that the non-e10s tg tests will show an improvement that is a result of this change in autophone and not due to a code change in fennec.