787916 - ASan: mochitest-1 is extremely slow on try but not locally

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Christian Holler (:decoder)]

I recently pushed the AddressSanitizer patch to try again, with mochitests enabled for linux64 (debug+opt and opt-only). Even on the opt-only run, the mochitest-1 testsuite was terminated by buildbot after running over 90 minutes:

https://tbpl.mozilla.org/?tree=Try&rev=4463b3439e5f

The log shows that all tests in mochitest-1 ran extremely slow. I downloaded the build and ran mochitest-1 locally and did not get a large penalty (it completed within 50 minutes).

Already the very first test that runs on try is slow vs. local run:

try: 34 INFO TEST-END | /tests/Harness_sanity/test_SpecialPowersExtension.html | finished in 18412ms

locally: 34 INFO TEST-END | /tests/Harness_sanity/test_SpecialPowersExtension.html | finished in 230ms

That's a slowdown factor of over 80 and I haven't been able to reproduce this at all locally. Strangely enough, the mochitests 3/4/5 completed within reasonable time on the opt-build.

Comment 1

[Christian Holler (:decoder)]

Successfully reproduced this in this push again (only running mt-1/5):

https://tbpl.mozilla.org/?tree=Try&rev=c60bf9b51ce3

Comment 2

[Christian Holler (:decoder)]

I was able to reproduce the issue locally, and the reason for the slowdown is the machine swapping. It wasn't clear however what is causing the massive memory usage in the first place.

After some investigation, I found out that the Mochitest table generated from the test files is extremely large in mochitest-1, mainly due to dom/ and content/ tests. Ripping the table code out of mochitest seems to solve the problem, doing a try run now. If that turns out to be the solution then we should make a pref to disable the table code, which will also speed up non-ASan tests (especially on mobile as jmaher is pointing out).

Comment 3

[Nick Thomas [:nthomas] (UTC+12)]

The large table issue is ringing a bell in my memory, so there may already be a pref for this in the harness.

Comment 4

[Christian Holler (:decoder)]

Attachment: Disable Mochitest test table when --close-when-done option is specified.

This is a first proposal, feel free to r- if this is the wrong way to solve it.

What I did is: In runtests.py, I propagated the --close-when-done option to server.js through the -e param to xpcshell, like it is done for some other options. In server.js, that options is then used to decide if the table should be sent.

Additionally, I made a change in SimpleTest/TestRunner.js to only update the test table row if it exists, and just skip that step if there is no such row/table.

I'm sending this to m-c try now to make sure it doesn't break anything else.

Comment 5

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

Comment on attachment 658677 [details] [diff] [review]
Disable Mochitest test table when --close-when-done option is specified.

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

this looks great, just a few nits to cleanup.

::: testing/mochitest/runtests.py
@@ +389,5 @@
>  
>      args = ["-g", self._xrePath,
>              "-v", "170",
>              "-f", "./" + "httpd.js",
> +            "-e", """const _PROFILE_PATH = '%(profile)s';const _SERVER_PORT = '%(port)s'; const _SERVER_ADDR = '%(server)s'; 

nit: trailing whitespace

@@ +390,5 @@
>      args = ["-g", self._xrePath,
>              "-v", "170",
>              "-f", "./" + "httpd.js",
> +            "-e", """const _PROFILE_PATH = '%(profile)s';const _SERVER_PORT = '%(port)s'; const _SERVER_ADDR = '%(server)s'; 
> +                     const _TEST_PREFIX = %(testPrefix)s; const _AUTO_UI = %(autoUI)s;""" %

I am not a fan of AUTO_UI, maybe we could call it DISPLAY_RESULTS or something

@@ +391,5 @@
>              "-v", "170",
>              "-f", "./" + "httpd.js",
> +            "-e", """const _PROFILE_PATH = '%(profile)s';const _SERVER_PORT = '%(port)s'; const _SERVER_ADDR = '%(server)s'; 
> +                     const _TEST_PREFIX = %(testPrefix)s; const _AUTO_UI = %(autoUI)s;""" %
> +                   {"profile" : self._profileDir.replace('\\', '\\\\'), "port" : self.httpPort, "server" : self.webServer, 

nit: trailing whitespace

::: testing/mochitest/server.js
@@ +148,5 @@
>      throw "please define _SERVER_PORT (as a port number) before running server.js";
>    }
>  
> +  if (typeof(_AUTO_UI) != "undefined") {
> +    autoUI = _AUTO_UI;

can we ensure this binds properly to true/false?

@@ +633,5 @@
>            DIV({class: "toggle"},
>              A({href: "#", id: "toggleNonTests"}, "Show Non-Tests"),
>              BR()
>            ),
> +    	  

nit: trailing whitespace

@@ +635,5 @@
>              BR()
>            ),
> +    	  
> +          (
> +           autoUI ? "" : 

nit: trailing whitespace

Comment 6

[Christian Holler (:decoder)]

Attachment: Updated patch with nits fixed

Updated patch with fixes as suggested. Green on try as expected, landing now :)

Comment 7

[Christian Holler (:decoder)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13afdf7150cf

Comment 8

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/13afdf7150cf