<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:ap@webkit.org" title="Alexey Proskuryakov <ap@webkit.org>"> <span class="fn">Alexey Proskuryakov</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Use multiple iOS simulator instead of multiple apps on single simulator for running layout-tests"
href="https://bugs.webkit.org/show_bug.cgi?id=151243">bug 151243</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #265510 Flags</td>
<td>review?, commit-queue?
</td>
<td>review-, commit-queue-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Use multiple iOS simulator instead of multiple apps on single simulator for running layout-tests"
href="https://bugs.webkit.org/show_bug.cgi?id=151243#c3">Comment # 3</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Use multiple iOS simulator instead of multiple apps on single simulator for running layout-tests"
href="https://bugs.webkit.org/show_bug.cgi?id=151243">bug 151243</a>
from <span class="vcard"><a class="email" href="mailto:ap@webkit.org" title="Alexey Proskuryakov <ap@webkit.org>"> <span class="fn">Alexey Proskuryakov</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=265510&action=diff" name="attach_265510" title="Updated patch">attachment 265510</a> <a href="attachment.cgi?id=265510&action=edit" title="Updated patch">[details]</a></span>
Updated patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=265510&action=review">https://bugs.webkit.org/attachment.cgi?id=265510&action=review</a>
I have a number of comments, but not very deep ones. This needs to be reviewed by Dan Bates I think.
Great work overall!
<span class="quote">> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:81
> + self.MAX_NUM_OF_SIMULATOR = 5</span >
What does this number mean, and how did you come up with it?
<span class="quote">> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:82
> + self.NUM_PROCESSES_PER_SIMULATOR = 100</span >
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"?
<span class="quote">> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:84
> + def get_num_workers(self, test_inputs, num_workers):</span >
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.
<span class="quote">> 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'))</span >
Same comments about abbreviations. Maybe "system_process_count_limit" or "maxproc"? "current_process_count"?
<span class="quote">> 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</span >
When do we get here? Does it need to be a recoverable error, or can the script just die?
<span class="quote">> 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)</span >
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.
<span class="quote">> Tools/Scripts/webkitpy/port/base.py:885
> + def reset_preferences(self, num_workers=1):</span >
As elsewhere, it would help to elaborate on "num" - is this requested number, maximum number, or something else?
<span class="quote">> Tools/Scripts/webkitpy/port/ios.py:81
> + SIMULATOR_APP_PATH = "/Applications/Xcode.app/Contents/Developer/Applications/Simulator.app"</span >
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.
<span class="quote">> Tools/Scripts/webkitpy/port/ios.py:208
> + time.sleep(1)</span >
Should this only be done when host OS is 10.11 or older?
<span class="quote">> Tools/Scripts/webkitpy/port/ios.py:410
> + subprocess.call([self.LSREGISTER_PATH, "-kill"])</span >
This a big hammer! Is there any way to not rebuild everything?
<span class="quote">> Tools/Scripts/webkitpy/port/ios.py:420
> + #_log.error("Data path for simulator " + str(i) + " is: "+ data_path)</span >
Let's not check in commented out code.
<span class="quote">> 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"])</span >
Same comment about Xcode path.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>