Talos performance tests do not set dom.max_chrome_script_run_time to zero

RESOLVED FIXED

Status

defect
P1
normal
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: igor, Assigned: anodelman)

Tracking

({fixed1.9.1})

Dependency tree / graph
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Reporter

Description

11 years ago
As far as I can see talos performace tests do not set dom.max_chrome_script_run_time to 0 in the profile to run tests, only dom.max_script_run_time is zeroed. (Please mark this bug is invalid if it is not so!) 

This means that if a chrome script runs for more than 20 seconds (the default value for dom.max_chrome_script_run_time), the system may show the slow script warning dialog which will result in the test reported as busted. 

This could the reason why the current patch from the bug 453157 (1.9.1 blocker) could not pass the testing. With the patch the timing accounting becomes more precise and the 20 seconds limit could expire on a busy box.
Flags: blocking1.9.1?
Assignee

Comment 1

11 years ago
We've specifically not set dom.max_chrome_script_run_time as it has caught a regression in the past having to do with slow script warnings that were being raised inappropriately.

All talos machines are dedicated boxes so there is no 'busy' box issue, all a talos box does is run the given test.  Is there any reason that we shouldn't consider a slow script warning being raised where one was not seen before as something other than a regression?
Reporter

Comment 2

11 years ago
(In reply to comment #1)
> All talos machines are dedicated boxes so there is no 'busy' box issue, all a
> talos box does is run the given test.

The problem is that at least with TraceMonkey tinderboxes the total timing for gfx tests varies from 30-40 seconds to over 3 minutes with Windows tests. If a cycle collector would kick in during the tests, I suspect it may well push the timing for one of the pages from the test beyond 20 seconds. At least this what happens when we run the tests locally.

So if setting max_chrome_script_run_time to 0 is not an option, at least bumping it to, say 100 (100 seconds) should be considered. Otherwise I suspect the patch for bug  453157 would never land. It makes the accounting for long-running scripts much more reliable and the current default limit 20 is not enough.

Note also that the patch itself already contains the changes to multiply the limits by 10 for debug builds that do the mochitests. The need for that was discovered during the previous attempts to land the patch.
Assignee

Comment 3

11 years ago
Okay, this sounds more reasonable now.  I'm always wary of making changes to the test harness to ensure tests passing - in case we are just avoiding investigating a regression.
Assignee

Updated

11 years ago
Assignee: nobody → anodelman
Priority: -- → P2
Attachment #354901 - Flags: review?(aki) → review+
Comment on attachment 354901 [details] [diff] [review]
[Checked in]add requested preference to talos configuration

Looks good.

Updated

11 years ago
Flags: blocking1.9.1? → blocking1.9.1+

Updated

11 years ago
Priority: P2 → P1
Assignee

Comment 6

11 years ago
Comment on attachment 354901 [details] [diff] [review]
[Checked in]add requested preference to talos configuration

/cvsroot/mozilla/testing/performance/talos/sample.config,v  <--  sample.config
new revision: 1.23; previous revision: 1.22
done
Attachment #354901 - Attachment description: add requested preference to talos configuration → [Checked in]add requested preference to talos configuration
Attachment #354901 - Flags: checked‑in+ checked‑in+
Reporter

Comment 7

11 years ago
Is this fixed so, for example, the talos boxes for http://tinderbox.mozilla.org/showbuilds.cgi?tree=TraceMonkey uses the profile with dom.max_chrome_script_run_time set to 0?
Reporter

Comment 8

11 years ago
Any input on the previous comment?
IANATalosGuru, but the change you requested has been checked in, and Talos machines pull that file from the CVS tip at the start of every run, so I think you are good to go.
Yes, the checked in change affects all automated talos runs regardless of branch.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

10 years ago
Component: Release Engineering: Talos → Release Engineering
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.