[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

--- Comment #4 from Dean Johnson <dean_johnson at apple.com> ---
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

r=me after the comments, although I can't speak for the non-Python changes.

>> 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"?

I think this naming conventions is fine and accurate.

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:87
> +            plimit = int(subprocess.check_output(["launchctl", "limit", "maxproc"]).strip().split()[1])

I think it'd be easier to do:

limit = int(subprocess.check_outpu(["ulimit", "-u"]))

>> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:88
>> +            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"?

current_num_process -> current_num_processes

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:90
> +            num_sim = (plimit - current_num_process) / self.NUM_PROCESSES_PER_SIMULATOR

Nit: If we want to strictly do integer division we should probably use // instead of /. (This doesn't apply to Python2, but explicit is better than implicit :))

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:91
> +        except:

Which exception are you trying to catch? subprocess.CalledProcessError? We should be explicit in what we're catching so we don't run into edge cases of excepting something else unexpectedly.

>> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:93
>> +            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?

This will cause issues if we don't raise the process limit cap. A quick sampling of our bots shows that around 330 processes are running on them, which means we will hit the default limit (709) spawning any more than 3 simulators.

I think if we fail on shell'ing out to the appropriate commands we should simply default to 1 simulator, but I don't see a case where we should fail to run the commands if we use ps and ulimit.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:158
> +        test_inputs = self._get_test_inputs(test_names, self._options.repeat_each, self._options.iterations)

Do these options default to integer values, or strings?

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:159
> +        num_workers = self._runner.get_num_workers(test_inputs, int(self._options.child_processes))

If the above options DO default to integers, we should probably follow the same convention for child_processes.

>> 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?

I think num_workers is fine. I'd assume num_workers is the number of workers to use.

> Tools/Scripts/webkitpy/port/ios.py:203
> +            device_udid = self.testing_device(i).udid

Why is testing_device a function? Shouldn't it be `testing_devices` as an array?

> Tools/Scripts/webkitpy/port/ios.py:212
> +            Simulator.wait_until_device_is_booted(self.testing_device(i).udid)

Ditto on testing_device comment.

> Tools/Scripts/webkitpy/port/ios.py:233
> +            except:

Can we explicitly catch an exception here instead of catching all cases?

> Tools/Scripts/webkitpy/port/ios.py:255
> +            testing_device = self.testing_device(i)

Ditto to testing_device comment above.

> Tools/Scripts/webkitpy/port/ios.py:256
> +            _log.debug('Verifying Simulator' + str(i) + ' with UDID {0}.'.format(testing_device.udid))

This is more idiomatic:
_log.debug('Verifying Simulator{0} with UDID {1}.'.format(i, testing_device.udid))

> Tools/Scripts/webkitpy/port/ios.py:335
> +    def testing_device(self, number):

Ah, I see why you are using it as a function now. Still, maybe we should create all the testing_devices and put them in an array to access?

-- 
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/20151119/64eed68c/attachment.html>