Closed Bug 1317732 Opened 3 years ago Closed 3 years ago

test_value_storage.html runs for too long on Android Debug

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: gbrown, Assigned: gbrown)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

On Android 4.3 Debug, test_value_storage.html runs in mochitest-25, which runs for about 2 hours now -- much higher than our target job run time, and on the cusp of exceeding the 2 hour limit for the job. The job sometimes exceeds 2 hours, failing as in bug 1317602.

We would normally increase the number of Android Debug mochitest chunks to reduce the run time of each chunk, but in this case, the chunks are very unbalanced: mochitest-25 runs for 2 hours, but all other chunks run in under 90 minutes, and some run in under 60 minutes.

test_value_storage.html is consistently the slowest test in mochitest-25, often taking about 30 minutes. (It was timing out recently and the test timeout increased in bug 1244978.) Could the run time of this test be reduced significantly? Could it be skipped on Android Debug?
For comparison, test_value_storage approximate run times on other platforms:

Android opt - 10 minutes
Linux64 opt - 2 minutes
Linux64 debug - 3 minutes
$ ./mach file-info bugzilla-component layout/style/test/test_value_storage.html 
Core :: CSS Parsing and Computation
  layout/style/test/test_value_storage.html
Component: Layout → CSS Parsing and Computation
Do you have any idea what the performance issues are on the hardware?  Is the test CPU-bound, I/O-bound (due to test output), or are we swapping due to low memory?
I don't know. I can look into it a bit. I'm open to suggestions for experiments/measurements.

Your mention of test output reminded me of bug 1045205 and the long history of this test...but I don't know if that's relevant today.
See Also: → 1041075, 1045205, 1073761, 1238840
Blocks: 1317884
There are many, many assertions in this test, resulting in lots of structured log activity. If the body of StructuredLog.testStatus() is commented out, as in https://treeherder.mozilla.org/#/jobs?repo=try&revision=387ac483a0070f139357c3ad222d7c4ea0448841, the run time of Android Debug test_value_storage is reduced from 30 minutes to under 10 minutes. So it looks like any performance improvement from bug 1045205 would go a long way here.
Depends on: 1045205
See Also: 1045205
Blocks: 1318224
See Also: → 1318393
Assignee: nobody → gbrown
This drastically reduces the logging during test_value_storage.html, by guarding almost all is()/isnot() calls with conditionals. One isnot() is left unguarded, at the top of test_value() to provide a gross measure of progress and ensure regular log writes (avoid no-output timeout).

There is a significant reduction in run time:

  linux64 debug:  3 minutes -> 1 minute
  android opt:   10 minutes -> 4 minutes
  android debug: 30 minutes -> 10 minutes


I would prefer to see this resolved with drastic improvements to logging efficiency, but I don't think that is a realistic expectation.

As an alternate "solution", we could disable this test on Android Debug.
Attachment #8812037 - Flags: review?(dbaron)
So when actually debugging test failures (which are very common in this test for people adding new CSS properties or values), it's often very useful to have the failures in the context of where they are in the test.  Similar for debugging against try server, which people sometimes have to do.

So I'd prefer that the change here only be done for Android, and not for all platforms.

Likewise, I'd prefer that you not have to edit the substance of the test and make it less readable.  (It's already hard enough -- but that's another issue!)

So, given those two things, I'd prefer that you wrap the existing is/isnot functions with new is/isnot functions that are used in this file.  This has three advantages:
 * you don't have to edit the body of the test at all
 * the wrapping can easily conditional (e.g., make it Android-only for now)
 * the conditions under which we do the wrapping can be easily changed

(You'll probably need a dummy test to call the real "ok" function in the function that does the wrapping so that you don't get a no-tests-run failure, though.)

Does that sound reasonable?
Flags: needinfo?(gbrown)
That's a great idea! 

This works fine for me locally. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d38272403e218edb9ffa6217185fd14a655f8ac8 is in progress for good measure.

Logging on Android is reduced to one TEST-PASS for each call to test_value(); logging is unchanged on all other platforms.
Attachment #8812037 - Attachment is obsolete: true
Attachment #8812037 - Flags: review?(dbaron)
Flags: needinfo?(gbrown)
Attachment #8812235 - Flags: review?(dbaron)
Comment on attachment 8812235 [details] [diff] [review]
reduce logging in test_value_storage.html, on Android only

>+// On Android, avoid most 'TEST-PASS' logging, to improve performance

Maybe explicitly say "by overriding the is and isnot functions" here?


r=dbaron
Attachment #8812235 - Flags: review?(dbaron) → review+
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07ed7e5fa5ff
Reduce logging in test_value_storage.html on Android only; r=dbaron
Duplicate of this bug: 1317602
Duplicate of this bug: 1317884
Duplicate of this bug: 1318224
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
https://hg.mozilla.org/releases/mozilla-aurora/rev/aca4526abbb9
Whiteboard: [checkin-needed-aurora][checkin-needed-beta] → [checkin-needed-beta]
https://hg.mozilla.org/releases/mozilla-beta/rev/9f6932a2f1c7
Flags: in-testsuite-
Whiteboard: [checkin-needed-beta]
https://hg.mozilla.org/mozilla-central/rev/07ed7e5fa5ff
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.