[Webkit-unassigned] [Bug 151243] Use multiple iOS simulator instead of multiple apps on single simulator for running layout-tests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=151243

Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #265510|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

--- Comment #3 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 265510
  --> https://bugs.webkit.org/attachment.cgi?id=265510
Updated patch

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

I have a number of comments, but not very deep ones. This needs to be reviewed by Dan Bates I think.

Great work overall!

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:81
> +        self.MAX_NUM_OF_SIMULATOR = 5

What does this number mean, and how did you come up with it?

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:82
> +        self.NUM_PROCESSES_PER_SIMULATOR = 100

Existing code uses "num" a lot, but I think that we should do better. First, it's an abbreviation, and second, it's not very clear.

Maybe something along the lines of "PROCESS_COUNT_ESTIMATE_PER_SIMULATOR_INSTANCE"?

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:84
> +    def get_num_workers(self, test_inputs, num_workers):

This function does two things - a platform specific "maximum_number_of_simulators" computation, and computing a minimum between that and how many workers the test run needs. It seems better to keep these separate.

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:88
> +            plimit = int(subprocess.check_output(["launchctl", "limit", "maxproc"]).strip().split()[1])
> +            current_num_process = len(subprocess.check_output(["ps", "aux"]).strip().split('\n'))

Same comments about abbreviations. Maybe "system_process_count_limit" or "maxproc"? "current_process_count"?

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:93
> +        except:
> +            self._printer.write_update('Error in calculating number of processes')
> +            num_sim = self.MAX_NUM_OF_SIMULATOR

When do we get here? Does it need to be a recoverable error, or can the script just die?

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:95
> +        num_workers = min(num_workers, len(all_shards), num_sim, self.MAX_NUM_OF_SIMULATOR)

We should print an explanation when the effective simulator count is lower than what we'd like it to be based on the number of CPU cores. The user needs to know that they can change system configuration to make tests run much faster.

> Tools/Scripts/webkitpy/port/base.py:885
> +    def reset_preferences(self, num_workers=1):

As elsewhere, it would help to elaborate on "num" - is this requested number, maximum number, or something else?

> Tools/Scripts/webkitpy/port/ios.py:81
> +    SIMULATOR_APP_PATH = "/Applications/Xcode.app/Contents/Developer/Applications/Simulator.app"

We should support having a non-default Xcode installation path. Not sure if we can just use xcrun, or respect some variable passed to higher level scripts.

> Tools/Scripts/webkitpy/port/ios.py:208
> +            time.sleep(1)

Should this only be done when host OS is 10.11 or older?

> Tools/Scripts/webkitpy/port/ios.py:410
> +        subprocess.call([self.LSREGISTER_PATH, "-kill"])

This a big hammer! Is there any way to not rebuild everything?

> Tools/Scripts/webkitpy/port/ios.py:420
> +            #_log.error("Data path for simulator " + str(i) + " is: "+ data_path)

Let's not check in commented out code.

> Tools/Scripts/webkitpy/port/ios.py:463
> +        subprocess.call(["install_name_tool", "-add_rpath", "/Applications/Xcode.app/Contents/Developer/Library/PrivateFrameworks/", destination + "/Contents/MacOS/Simulator"])

Same comment about Xcode path.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151114/68fbdf54/attachment.html>