Don't reinvent Executive.cpu_count for every port
Created attachment 53026 [details] Patch
Comment on attachment 53026 [details] Patch OK.
Comment on attachment 53026 [details] Patch Clearing flags on attachment: 53026 Committed r57399: <http://trac.webkit.org/changeset/57399>
All reviewed patches have been landed. Closing bug.
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?
Typo in this patch: + if system_name == "Dawin": + return int(self.run_command(["sysctl", "-n", "hw.ncpu"]))
> > + 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.