1314305 - Enable e10s mode for the linux64-ccov build

Status: RESOLVED FIXED

(Testing :: Code Coverage, defect)

Description

[Greg Mierzwinski [:sparky]]

All that needs to be done here is to remove the e10s restriction for linux64-ccov in the mochitest test definition and fix any unexpected problems.

Comment 1

[Marco Castelluccio [:marco]]

Where is the restriction set?
E.g. https://dxr.mozilla.org/mozilla-central/rev/20dff607fb88ee69135a280bbb7f32df75a86237/taskcluster/ci/test/tests.yml#415 is only setting e10s to false for linux64-jsdcov/opt.

Comment 2

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

check this out:
https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/transforms/tests.py#501

we set e10s=false

Comment 3

[Greg Mierzwinski [:sparky]]

Beat me too it. :) 

I think we can close this bug now since it's implemented through that transform.

Comment 4

[Greg Mierzwinski [:sparky]]

(In reply to Greg Mierzwinski [:gmierz] from comment #3)
> I think we can close this bug now since it's implemented through that
> transform.

Nvm...

Comment 5

[Marco Castelluccio [:marco]]

Try build, looks mostly green, except a couple of intermittent failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e00b3e11f7ef1e7b3190b2d05ad2c831799f4df2

Comment 6

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

there are 4 failures:
gtest - intermittent
c3 - perma fail (toolkit/content/tests/chrome/test_bug437844.xul | Assertion count 17 is less than expected range 18-22 assertions.) - we could just adjust the expected assertions :)
bc2 - intermittent
dt7 - almost perma fail: devtools/client/framework/test/browser_toolbox_races.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. -


so we need to fix c3 and dt7 before doing this fully, in the past we skip tests which are problematic just like we would for any other intermittent or new platform.

Comment 7

[Marco Castelluccio [:marco]]

> c3 - perma fail (toolkit/content/tests/chrome/test_bug437844.xul | Assertion count 17 is less than expected range 18-22 assertions.) - we could just adjust the expected assertions :)

I filed bug 1359458 for this.

> dt7 - almost perma fail: devtools/client/framework/test/browser_toolbox_races.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. -

I've asked more information about this in bug 1331122.

Comment 10

[Marco Castelluccio [:marco]]

Attachment: Patch

The two blocking bugs have been fixed, the new try build is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6e9cefe95adc3dd281bf8e2a2f897e8f4839e51.

The two failures (GTest and bc2) are intermittent and not related to e10s.

Comment 11

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

the last thing I would like to determine is if there is any difference in coverage between e10s and non-e10s.  Since we are running 99% of the same tests, could we do a run with only e10s and compare it against an existing run for coverage?  This would help determine what differences we have.

We will be disabling the non-e10s tests as soon as we get a chance (everywhere, not just ccov) and we are only shipping e10s for Firefox 57- so it might be better to rip the bandaid off now and just report e10s only coverage!

In your patch there is a removed line:
test['e10s'] = False

if you wanted to test e10s only, that could be changed to:
test['e10s'] = True

right now with this patch applied it defaults to whatever the test has, in this case most likely:
test['e10s'] = Both

Comment 12

[Marco Castelluccio [:marco]]

(In reply to Joel Maher ( :jmaher) from comment #11)
> the last thing I would like to determine is if there is any difference in
> coverage between e10s and non-e10s.  Since we are running 99% of the same
> tests, could we do a run with only e10s and compare it against an existing
> run for coverage?  This would help determine what differences we have.
> 
> We will be disabling the non-e10s tests as soon as we get a chance
> (everywhere, not just ccov) and we are only shipping e10s for Firefox 57- so
> it might be better to rip the bandaid off now and just report e10s only
> coverage!
> 
> In your patch there is a removed line:
> test['e10s'] = False
> 
> if you wanted to test e10s only, that could be changed to:
> test['e10s'] = True
> 
> right now with this patch applied it defaults to whatever the test has, in
> this case most likely:
> test['e10s'] = Both

I'm generating a report from the `both` build now, I will generate another one for `e10s-only` later today.

Comment 13

[Marco Castelluccio [:marco]]

Actually, I should be able to generate it from the same build, I can simply filter out the artifacts coming from non-e10s tests.

Comment 14

[Marco Castelluccio [:marco]]

I've tried setting `test['e10s'] = True`, but it makes some suites fail: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd5842297a8d83d641d30b925baf38ba77ed1493.

I will do:
(In reply to Marco Castelluccio [:marco] from comment #13)
> Actually, I should be able to generate it from the same build, I can simply
> filter out the artifacts coming from non-e10s tests.

by filtering out the suites that have a e10s alternative, so tc-Fxfn-l, tc-Fxfn-r, tc-M, tc-R, tc-W. Does it sound reasonable to you?

Comment 15

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

I think your idea of filtering out artifacts from a full run sound good.

the tests that failed on e10s=true, those don't run in e10s as they are not full browser processes, only headless shells.  So many details!

Comment 16

[Marco Castelluccio [:marco]]

You can find the reports here: https://github.com/marco-c/code-coverage-reports.

Comment 17

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

quite a difference!  I see the accessible/* being different- did you get mochitest-a11y in the results for e10s?  In the world where we will disable non-e10s that will only be for cases where we have duplicated tests, and right now there is not a mochitest-a11y-e10s version.

Comment 18

[Marco Castelluccio [:marco]]

(In reply to Joel Maher ( :jmaher) from comment #17)
> quite a difference!  I see the accessible/* being different- did you get
> mochitest-a11y in the results for e10s?  In the world where we will disable
> non-e10s that will only be for cases where we have duplicated tests, and
> right now there is not a mochitest-a11y-e10s version.

Yeah, I didn't include mochitest-a11y.

Comment 19

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

ok, so this is slightly misleading, maybe the best bet is to compare coverage of:
mochitest-{x}
mochitest-browser-chrome-{x}

vs:
mochitest-e10s-{x}
mochitest-browser-chrome-e10s-{x}


this way we don't have to nitpick on details, we will be able to see the coverage comparisons.

Comment 20

[Marco Castelluccio [:marco]]

I've uploaded all reports to https://github.com/marco-c/code-coverage-reports.

Comment 21

[Marco Castelluccio [:marco]]

There's a large difference between the e10s and non-e10s reports, so there must be something wrong. Maybe bug 1358201 isn't fully fixed.

Comment 22

[Marco Castelluccio [:marco]]

I've generated new reports from a try build that contains the patch from bug 1362478 (https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f65588b80a6aca8b6d356029f7be8e2a859dcef&selectedJob=96949269): https://github.com/marco-c/code-coverage-reports.

At first glance, they look fine to me.

Joel, can you confirm? After your confirmation, I think we can land this patch.

Comment 23

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

after chatting with Kyle, it sounds like this is closer and there are a few remaining known issues.

Comment 24

[Marco Castelluccio [:marco]]

(In reply to Joel Maher ( :jmaher) from comment #23)
> after chatting with Kyle, it sounds like this is closer and there are a few
> remaining known issues.

For posterity: the known issues that I noticed with the reports were fixed by bug 1362478. Kyle is looking at the new reports to see if there are other issues.

Comment 25

[Marco Castelluccio [:marco]]

The patch landed in bug 1365293, Kyle I take you validated the e10s results? Did you find anything out of the ordinary?

Comment 26

[Kyle Lahnakoski [:ekyle]]

I had a preliminary look, and there were large differences, but I worry marco's try run was not ingested properly:  After all, my problems were ingesting properly; one dumb mistake after another. Now that we have a nightly e10s coverage we can iterate...

Here is a small program that diff's coverage: https://github.com/klahnakoski/coco-diff

For revision 37d777d87200, here are the 20 largest differences; comparing e10s coverage to baseline (non-e10s, mochitest); baseline has....

> 25316 additional lines in db/sqlite3/src/sqlite3.c
> 5890 additional lines in dom/indexedDB/ActorsParent.cpp
> 5265 additional lines in docshell/base/nsDocShell.cpp
> 5234 additional lines in obj-firefox/dom/bindings/CSS2PropertiesBinding.cpp
> 5141 additional lines in js/src/jit/IonBuilder.cpp
> 5055 additional lines in dom/base/nsGlobalWindow.cpp
> 5020 additional lines in dom/base/nsDocument.cpp
> 4967 additional lines in js/src/jit/CodeGenerator.cpp
> 4629 additional lines in layout/style/nsCSSParser.cpp
> 3878 additional lines in layout/generic/nsFrame.cpp
> 3810 additional lines in obj-firefox/dom/bindings/WebGLRenderingContextBinding.cpp
> 3635 additional lines in layout/base/PresShell.cpp
> 3584 additional lines in layout/base/nsCSSFrameConstructor.cpp
> 3554 additional lines in layout/generic/nsTextFrame.cpp
> 3534 additional lines in dom/base/nsContentUtils.cpp
> 3502 additional lines in js/src/frontend/BytecodeEmitter.cpp
> 3352 additional lines in js/src/vm/Debugger.cpp
> 3191 additional lines in js/src/frontend/Parser.cpp
> 3188 additional lines in obj-firefox/ipc/ipdl/PContentChild.cpp
> 3187 additional lines in js/src/jit/MIR.h

I believe the biggest problem is the lack of e10s coverage for sqlite3. Is there a thread, or process, that uses it, and the coverage is not enabled?

Comment 27

[Kyle Lahnakoski [:ekyle]]

So that last comparison was wrong: It had zero e10s coverage records.  Running a new one...

Comment 29

[Kyle Lahnakoski [:ekyle]]

A better analysis, and from both sides. Each line shows the number of lines covered, and the line-level difference; lines covered by one, but not the other.  
I believe the conclusion is: "There are differences, but it is probably a result of the program behavior, not a problem in the coverage collection".

The next step is to visualize the difference; it can help us understand the nature of the difference, and confirm it is what we expect.

> non-e10s (824 lines) has additional 824 lines over e10s (0 lines) in uriloader/prefetch/nsOfflineCacheUpdate.cpp
> non-e10s (907 lines) has additional 724 lines over e10s (183 lines) in netwerk/cache/nsDiskCacheDeviceSQL.cpp
> non-e10s (440 lines) has additional 411 lines over e10s (29 lines) in obj-firefox/dom/bindings/PushManagerBinding.cpp
> non-e10s (6044 lines) has additional 367 lines over e10s (5757 lines) in dom/indexedDB/ActorsParent.cpp
> non-e10s (555 lines) has additional 310 lines over e10s (245 lines) in editor/txtsvc/nsTextServicesDocument.cpp
> non-e10s (329 lines) has additional 302 lines over e10s (27 lines) in dom/offline/nsDOMOfflineResourceList.cpp
> non-e10s (292 lines) has additional 292 lines over e10s (0 lines) in netwerk/cache/nsCacheEntryDescriptor.cpp
> non-e10s (2713 lines) has additional 269 lines over e10s (2539 lines) in netwerk/protocol/http/nsHttpChannel.cpp
> non-e10s (643 lines) has additional 265 lines over e10s (379 lines) in netwerk/cache/nsCacheService.cpp
> non-e10s (282 lines) has additional 254 lines over e10s (28 lines) in obj-firefox/dom/bindings/PushSubscriptionBinding.cpp
> non-e10s (25072 lines) has additional 214 lines over e10s (24924 lines) in db/sqlite3/src/sqlite3.c
> non-e10s (184 lines) has additional 184 lines over e10s (0 lines) in dom/push/PushManager.cpp
> non-e10s (232 lines) has additional 163 lines over e10s (69 lines) in obj-firefox/dom/bindings/OfflineResourceListBinding.cpp
> non-e10s (4541 lines) has additional 162 lines over e10s (4581 lines) in dom/base/nsGlobalWindow.cpp
> non-e10s (143 lines) has additional 142 lines over e10s (1 lines) in netwerk/cache2/OldWrappers.cpp
> non-e10s (142 lines) has additional 142 lines over e10s (0 lines) in dom/push/PushSubscription.cpp
> non-e10s (201 lines) has additional 135 lines over e10s (66 lines) in uriloader/prefetch/nsOfflineCacheUpdateService.cpp
> non-e10s (1661 lines) has additional 132 lines over e10s (1822 lines) in dom/events/EventStateManager.cpp
> non-e10s (546 lines) has additional 129 lines over e10s (421 lines) in layout/forms/nsComboboxControlFrame.cpp
> non-e10s (138 lines) has additional 127 lines over e10s (11 lines) in dom/push/PushNotifier.cpp
> ---
> e10s (1993 lines) has additional 1778 lines over non-e10s (215 lines) in obj-firefox/dom/bindings/TestInterfaceJSBinding.cpp
> e10s (1620 lines) has additional 1604 lines over non-e10s (16 lines) in obj-firefox/dom/bindings/TestInterfaceJSMaplikeSetlikeIterableBinding.cpp
> e10s (2677 lines) has additional 1563 lines over non-e10s (1114 lines) in security/nss/lib/freebl/mpi/mp_comba.c
> e10s (2771 lines) has additional 1498 lines over non-e10s (1277 lines) in obj-firefox/ipc/ipdl/PContentChild.cpp
> e10s (1478 lines) has additional 1471 lines over non-e10s (7 lines) in js/src/wasm/AsmJS.cpp
> e10s (2688 lines) has additional 1449 lines over non-e10s (1243 lines) in obj-firefox/ipc/ipdl/PContentParent.cpp
> e10s (1756 lines) has additional 1428 lines over non-e10s (328 lines) in obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp
> e10s (1320 lines) has additional 1318 lines over non-e10s (2 lines) in dom/media/MediaStreamGraph.cpp
> e10s (3023 lines) has additional 1186 lines over non-e10s (1837 lines) in dom/html/HTMLInputElement.cpp
> e10s (2017 lines) has additional 1184 lines over non-e10s (834 lines) in dom/canvas/CanvasRenderingContext2D.cpp
> e10s (1010 lines) has additional 1009 lines over non-e10s (1 lines) in gfx/layers/apz/src/AsyncPanZoomController.cpp
> e10s (927 lines) has additional 927 lines over non-e10s (0 lines) in dom/canvas/ImageBitmapUtils.cpp
> e10s (1415 lines) has additional 877 lines over non-e10s (538 lines) in dom/crypto/WebCryptoTask.cpp
> e10s (1563 lines) has additional 863 lines over non-e10s (700 lines) in obj-firefox/dom/bindings/SubtleCryptoBinding.cpp
> e10s (788 lines) has additional 773 lines over non-e10s (15 lines) in obj-firefox/dom/bindings/WebAuthenticationBinding.cpp
> e10s (1124 lines) has additional 739 lines over non-e10s (385 lines) in netwerk/protocol/http/HttpChannelChild.cpp
> e10s (732 lines) has additional 726 lines over non-e10s (6 lines) in dom/canvas/ImageBitmap.cpp
> e10s (1136 lines) has additional 702 lines over non-e10s (434 lines) in obj-firefox/ipc/ipdl/PBrowserChild.cpp
> e10s (1125 lines) has additional 697 lines over non-e10s (428 lines) in obj-firefox/ipc/ipdl/PBrowserParent.cpp
> e10s (691 lines) has additional 691 lines over non-e10s (0 lines) in dom/canvas/ImageBitmapColorUtils.cpp

Comment 30

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

one other useful piece of information would be a list of tests that were ran in both e10 and non-e10s for the data analyzed.  Quite possibly we are running more tests (individual tests in manifests) in non-e10s and that could explain the difference.

Comment 31

[Kyle Lahnakoski [:ekyle]]

Looking into this now, but it appears the e10s variation did not load into the unittests. This can be because I am looking for the wrong thing, or the ETL pipeline has an error.

Comment 32

[Marco Castelluccio [:marco]]

Kyle, any news here?

Comment 33

[Kyle Lahnakoski [:ekyle]]

This is done, and working.