Enable e10s mode for the linux64-ccov build

RESOLVED FIXED

Status

Testing
Code Coverage
RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: gmierz, Assigned: marco)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Blocks: 1301170
(Assignee)

Comment 1

6 months ago
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.
Flags: needinfo?(jmaher)
Flags: needinfo?(gmierz2)
check this out:
https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/transforms/tests.py#501

we set e10s=false
Flags: needinfo?(jmaher)
(Reporter)

Comment 3

6 months ago
Beat me too it. :) 

I think we can close this bug now since it's implemented through that transform.
Flags: needinfo?(gmierz2)
(Reporter)

Comment 4

6 months ago
(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...
(Assignee)

Updated

6 months ago
Depends on: 1358201
(Assignee)

Comment 5

6 months ago
Try build, looks mostly green, except a couple of intermittent failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e00b3e11f7ef1e7b3190b2d05ad2c831799f4df2
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.
(Assignee)

Updated

6 months ago
Depends on: 1359458
(Assignee)

Comment 7

6 months ago
> 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.
(Assignee)

Updated

6 months ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Enable e10s mode for mochitest plain on the linux64-ccov build. → Enable e10s mode for the linux64-ccov build
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1314298
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1314304
(Assignee)

Updated

6 months ago
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
(Assignee)

Updated

6 months ago
Depends on: 1331122
(Assignee)

Comment 10

6 months ago
Created attachment 8862338 [details] [diff] [review]
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.
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
(Assignee)

Comment 12

6 months ago
(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.
(Assignee)

Comment 13

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

Comment 14

6 months ago
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?
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!
(Assignee)

Comment 16

6 months ago
You can find the reports here: https://github.com/marco-c/code-coverage-reports.
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.
(Assignee)

Comment 18

6 months ago
(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.
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.
(Assignee)

Comment 20

6 months ago
I've uploaded all reports to https://github.com/marco-c/code-coverage-reports.
Depends on: 1362052
(Assignee)

Comment 21

6 months ago
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.
(Assignee)

Updated

6 months ago
Blocks: 1362052
No longer depends on: 1362052
(Assignee)

Updated

6 months ago
Depends on: 1362478
(Assignee)

Comment 22

6 months ago
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.
Flags: needinfo?(jmaher)
after chatting with Kyle, it sounds like this is closer and there are a few remaining known issues.
Flags: needinfo?(jmaher)
(Assignee)

Comment 24

5 months ago
(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.
Flags: needinfo?(klahnakoski)
Blocks: 510232
Flags: needinfo?(klahnakoski)
Depends on: 1365293
(Assignee)

Comment 25

5 months ago
The patch landed in bug 1365293, Kyle I take you validated the e10s results? Did you find anything out of the ordinary?
Flags: needinfo?(klahnakoski)
Comment hidden (obsolete)
So that last comparison was wrong: It had zero e10s coverage records.  Running a new one...
(Assignee)

Updated

5 months ago
Duplicate of this bug: 510232
(Assignee)

Updated

5 months ago
No longer blocks: 510232
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
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.
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.
(Assignee)

Comment 32

4 months ago
Kyle, any news here?
This is done, and working.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.