Bug 73847 - NRWT creates too many workers on RAM-limited machines
Summary: NRWT creates too many workers on RAM-limited machines
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 77419
  Show dependency treegraph
 
Reported: 2011-12-05 11:10 PST by Adam Roben (:aroben)
Modified: 2022-03-01 03:08 PST (History)
8 users (show)

See Also:


Attachments
Patch (8.78 KB, patch)
2011-12-05 15:04 PST, Eric Seidel (no email)
dpranke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2011-12-05 11:10:31 PST
NRWT creates the same number of workers regardless of how much RAM is installed. On RAM-limited machines (e.g., 12 cores, 3GB RAM), so many workers are created that the machine starts paging like crazy and all tests end up timing out.

It would be nice if NRWT were to take RAM into account when deciding how many workers to create.
Comment 1 Eric Seidel (no email) 2011-12-05 11:17:44 PST
http://support.hyperic.com/display/SIGAR/Home is a way to do this.  But I don't think we can auto-install that, due to the binary files it includes.  I suspect we'll have to roll our own if we want this functionality.  Juse like we do for Executive.cpu_count()
Comment 2 Eric Seidel (no email) 2011-12-05 11:18:23 PST
How much ram should NRWT expect a DRT instance to need?  500mb?
Comment 3 Tony Chang 2011-12-05 11:23:16 PST
(In reply to comment #2)
> How much ram should NRWT expect a DRT instance to need?  500mb?

500mb sounds plenty conservative.  Make sure to leave ram for the system and NRWT.
Comment 4 Dirk Pranke 2011-12-05 12:20:51 PST
It hadn't occurred to me before that we might have a lot of many-core but low-RAM machines. Makes sense to change this.
Comment 5 Eric Seidel (no email) 2011-12-05 15:04:27 PST
Created attachment 117944 [details]
Patch
Comment 6 Dirk Pranke 2011-12-05 15:22:52 PST
Comment on attachment 117944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117944&action=review

> Tools/Scripts/run-webkit-tests:79
> +    return !isQt();

Are we sure that it was a RAM issue and not something else?

Also, in the future I'd probably change the defaults in port/mac.py et al. rather than duplicating the logic here in Perl.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:177
> +        return cpu_count

I'm not sure zero should be a legal value here, so maybe you should return at least 1? If it is zero (essentially saying we need at least 300MB to run), there should probably at least be a check in run_webkit_tests: _set_up_derived_options() to handle zero cleanly and bail out.
Comment 7 Eric Seidel (no email) 2011-12-05 15:37:54 PST
(In reply to comment #6)
> (From update of attachment 117944 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117944&action=review
> 
> > Tools/Scripts/run-webkit-tests:79
> > +    return !isQt();
> 
> Are we sure that it was a RAM issue and not something else?

I don't know.  But it doesn't hurt to try.

> Also, in the future I'd probably change the defaults in port/mac.py et al. rather than duplicating the logic here in Perl.

This is intentional.  run-webkit-tests is meant to match ORWT, to be a shim.  Folks who run new-run-webkit-test directly always get parallel testing, regardless of the state of the bots.

> > Tools/Scripts/webkitpy/layout_tests/port/base.py:177
> > +        return cpu_count
> 
> I'm not sure zero should be a legal value here, so maybe you should return at least 1? If it is zero (essentially saying we need at least 300MB to run), there should probably at least be a check in run_webkit_tests: _set_up_derived_options() to handle zero cleanly and bail out.

This is no different from setting --child-count=0 directly.  I think that's out of scope of this change.
Comment 8 Dirk Pranke 2011-12-05 15:45:06 PST
Comment on attachment 117944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117944&action=review

>>> Tools/Scripts/webkitpy/layout_tests/port/base.py:177
>>> +        return cpu_count
>> 
>> I'm not sure zero should be a legal value here, so maybe you should return at least 1? If it is zero (essentially saying we need at least 300MB to run), there should probably at least be a check in run_webkit_tests: _set_up_derived_options() to handle zero cleanly and bail out.
> 
> This is no different from setting --child-count=0 directly.  I think that's out of scope of this change.

I disagree, in the sense that there was no way default_child_processes() could return 0 before, and now it can, potentially breaking things down the road. That said, it'll probably never come up in practice. Up to you ...
Comment 9 Eric Seidel (no email) 2011-12-05 15:48:59 PST
Committed r102056: <http://trac.webkit.org/changeset/102056>