Bug 37377 - Don't reinvent Executive.cpu_count for every port
Summary: Don't reinvent Executive.cpu_count for every port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-09 18:06 PDT by Adam Barth
Modified: 2010-04-10 07:56 PDT (History)
4 users (show)

See Also:


Attachments
Patch (10.27 KB, patch)
2010-04-09 18:08 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-04-09 18:06:35 PDT
Don't reinvent Executive.cpu_count for every port
Comment 1 Adam Barth 2010-04-09 18:08:33 PDT
Created attachment 53026 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-04-09 18:20:05 PDT
Comment on attachment 53026 [details]
Patch

OK.
Comment 3 Adam Barth 2010-04-09 19:17:53 PDT
Comment on attachment 53026 [details]
Patch

Clearing flags on attachment: 53026

Committed r57399: <http://trac.webkit.org/changeset/57399>
Comment 4 Adam Barth 2010-04-09 19:17:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Dirk Pranke 2010-04-09 20:27:31 PDT
Comment on attachment 53026 [details]
Patch

diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py
> index e5a0108..5185947 100755
> --- a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py
> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py
> @@ -50,6 +50,7 @@ import logging
>  import math
>  import optparse
>  import os
> +import platform
>  import Queue
>  import random
>  import re
> @@ -71,6 +72,8 @@ from test_types import image_diff
>  from test_types import test_type_base
>  from test_types import text_diff
>  
> +from webkitpy.common.system.executive import Executive
> +
>  import port
>  
>  _log = logging.getLogger("webkitpy.layout_tests.run_webkit_tests")
> @@ -1389,6 +1392,7 @@ def main(options, args):
>                          stream=meter)
>  
>      port_obj = port.get(options.platform, options)
> +    executive = Executive()
>  
>      if not options.configuration:
>          options.configuration = port_obj.default_configuration()
> @@ -1422,8 +1426,13 @@ def main(options, args):
>                                ignore_errors=True)
>  
>      if not options.num_dump_render_trees:
> -        # TODO(ojan): Investigate perf/flakiness impact of using numcores + 1.
> -        options.num_dump_render_trees = port_obj.num_cores()
> +        # FIXME: Investigate perf/flakiness impact of using cpu_count + 1.
> +        options.num_dump_render_trees = executive.cpu_count()
> +        # FIXME: new-run-webkit-tests is unstable on Mac running more than
> +        # four threads in parallel.
> +        # See https://bugs.webkit.org/show_bug.cgi?id=36622
> +        if platform.system() == "Dawin" and options.num_dump_render_trees > 4:
> +            options.num_dump_render_trees = 4

The instability isn't generic, it's port-specific, and there should be a port-specific way to limit parallelism. For example, the fact that the base mac 
port is unstable due to DumpRenderTree doesn't mean that we should limit the parallelism that Chromoium gets.

I think a better way to implement this patch would've been to have base.num_cores() call the Executive module, and leave the hook in for any ports that didn't want the default logic.

I went pretty far ensuring that anything platform-specific needed by run-webkit-tests would be wrapped up in the Port interface, so that a Port only needs to change things in one place. The rest of the Python code does not seem to have a similar since of layering (as evidenced by the switch statement in system/executive.py). Please don't undo this layering without replacing it with something equivalent or at least having more of a discussion about this.

>  
>      write = create_logging_writer(options, 'config')
>      write("Running %s DumpRenderTrees in parallel" % options.num_dump_render_trees)
> @@ -1461,9 +1470,9 @@ def main(options, args):
>          # Creating the expecations for each platform/configuration pair does
>          # all the test list parsing and ensures it's correct syntax (e.g. no
>          # dupes).
> -        for platform in port_obj.test_platform_names():
> -            test_runner.parse_expectations(platform, is_debug_mode=True)
> -            test_runner.parse_expectations(platform, is_debug_mode=False)
> +        for platform_name in port_obj.test_platform_names():
> +            test_runner.parse_expectations(platform_name, is_debug_mode=True)
> +            test_runner.parse_expectations(platform_name, is_debug_mode=False)
>          meter.update("")
>          print ("If there are no fail messages, errors or exceptions, then the "
>              "lint succeeded.")

I have no objection to this change, but it doesn't seem to have anything to do with the rest of this patch. Did it sneak in accidentally?
Comment 6 Chris Jerdonek 2010-04-10 06:07:55 PDT
Typo in this patch:

+        if system_name == "Dawin":
+            return int(self.run_command(["sysctl", "-n", "hw.ncpu"]))
Comment 7 Adam Barth 2010-04-10 07:56:46 PDT
> > +        if platform.system() == "Dawin" and options.num_dump_render_trees > 4:
> > +            options.num_dump_render_trees = 4
> 
> The instability isn't generic, it's port-specific, and there should be a
> port-specific way to limit parallelism. For example, the fact that the base mac 
> port is unstable due to DumpRenderTree doesn't mean that we should limit the
> parallelism that Chromoium gets.

Ok.  It seems better to just fix the bug than to create something complicated to work around the issue.

> I think a better way to implement this patch would've been to have
> base.num_cores() call the Executive module, and leave the hook in for any ports
> that didn't want the default logic.

How about a base.default_dump_render_tree_count method?  The number of cores your machine has doesn't vary by port, but the number of DRT instances you want could.

> I went pretty far ensuring that anything platform-specific needed by
> run-webkit-tests would be wrapped up in the Port interface, so that a Port only
> needs to change things in one place. The rest of the Python code does not seem
> to have a similar since of layering (as evidenced by the switch statement in
> system/executive.py). Please don't undo this layering without replacing it with
> something equivalent or at least having more of a discussion about this.

There's really two things going on.  We have a notion of a port (Gtk, Chromium, Qt, etc) and a notion of  a platform (Mac, Windows, Linux).  At the moment, the two are smashed together, which leads to the copy/paste code.

In this particular case, the OS-specific code is just making up for a deficiency in the Python 2.5 library.  In Python 2.6, the multiprocessing package abstracts away the platform specific logic for counting CPUs.

In any case, I'll write a patch that I think will make you happy.  :)

> > @@ -1461,9 +1470,9 @@ def main(options, args):
> >          # Creating the expecations for each platform/configuration pair does
> >          # all the test list parsing and ensures it's correct syntax (e.g. no
> >          # dupes).
> > -        for platform in port_obj.test_platform_names():
> > -            test_runner.parse_expectations(platform, is_debug_mode=True)
> > -            test_runner.parse_expectations(platform, is_debug_mode=False)
> > +        for platform_name in port_obj.test_platform_names():
> > +            test_runner.parse_expectations(platform_name, is_debug_mode=True)
> > +            test_runner.parse_expectations(platform_name, is_debug_mode=False)
> >          meter.update("")
> >          print ("If there are no fail messages, errors or exceptions, then the "
>
> I have no objection to this change, but it doesn't seem to have anything to do
> with the rest of this patch. Did it sneak in accidentally?

The name "platform" conflicts with the import platform statements I added at the top of the file.