Bug 151243 – Use multiple iOS simulator instead of multiple apps on single simulator for running layout-tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Aakash Jain]

Currently, we run multiple instances of WebKitTestRunner.app in a single simulator device. This is not great, as there are many things on iOS that only work for foreground app. We should be starting multiple simulators.

[Aakash Jain]

Created attachment 265465 [details]
Proposed patch

Not completely ready, I am still working on couple of improvements. However I would like to get it reviewed meanwhile.

[Aakash Jain]

Created attachment 265510 [details]
Updated patch

[Alexey Proskuryakov]

Comment on attachment 265510 [details]
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.

[Dean Johnson]

Comment on attachment 265510 [details]
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?

[Aakash Jain]

Created attachment 266605 [details]
Updated patch

Updated patch with the review comments incorporated and misc improvements.

[Aakash Jain]

Created attachment 266808 [details]
Updated patch

[Alexey Proskuryakov]

Comment on attachment 266808 [details]
Updated patch

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

> Tools/Scripts/webkitpy/port/ios.py:78
> +    num_workers = None

Do we need this despite default_child_processes being memoized?

> Tools/Scripts/webkitpy/port/ios.py:135
> +        num_cpu_cores = self._executive.cpu_count() / 2

For consistency, this should have "count" in the name. From the way it's used, I suggest a name like "best_child_process_count_for_cpu".

> Tools/Scripts/webkitpy/port/ios.py:139
> +        maximum_simulator_count_on_this_system = (system_process_count_limit - current_process_count) // self.PROCESS_COUNT_ESTIMATE_PER_SIMULATOR_INSTANCE

Please add a FIXME about checking the amount of RAM.

> Tools/Scripts/webkitpy/port/ios.py:142
> +            _log.info('Please increase the process and file limit to launch more Simulators')

Where do we check the file limit?

Also, let's at least log the same info that Mac does:

                _log.warning("This machine could support %s child processes, but only has enough memory for %s." % (default_count, supportable_instances))

> Tools/Scripts/webkitpy/port/ios.py:144
> +            # FIXME: add url for webpage explaining how to increase these limits

WebKit style is to start FIXME with an upper case letter, and end with a period (i.e. to have a complete sentence).

> Tools/Scripts/webkitpy/port/ios.py:226
> +            if (int(self.host.platform._platform_module.mac_ver()[0].strip().replace("10.", "")) < 12):

I suggest comparing to "10.10" and "10.11" instead - that way it will be easier to find and delete in the future.

[Aakash Jain]

Created attachment 266839 [details]
Updated patch with review comments incorporated.

[Alexey Proskuryakov]

Comment on attachment 266839 [details]
Updated patch with review comments incorporated.

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

> Tools/Scripts/webkitpy/port/ios.py:132
> +    def num_child_processes(self):

Please rename to just "child_processes" or "child_processes_option", as it's the same thing as the one in options.

> Tools/Scripts/webkitpy/port/ios.py:143
> +        # FIXME: We should also take into account the Add logic to take into account the available RAM.

Please fix the grammar, looks like two variants of the comment collided.

> Tools/Scripts/webkitpy/port/ios.py:148
> +            _log.warn("This machine could support %s child processes, but only has enough process limit for %s."
> +                % (best_child_process_count_for_cpu, maximum_simulator_count_on_this_system))
> +            _log.info('Run "launchctl limit" to check these limits')

Is there a reason to use different log levels here? These look like parts of the same message.

> Tools/Scripts/webkitpy/port/ios.py:280
> +            # FIXME: This creates the devices sequentially, this can be optimized.

I think that this can be more specific: "This creates the devices sequentially, doing this in parallel can improve performance".

> Tools/Scripts/webkitpy/port/ios.py:284
> +            # FIXME: this is very slow, especially for mulitple simulators, we probably do not need

Please start the sentence with an upper case letter ("This").

[Aakash Jain]

Created attachment 266916 [details]
Updated patch with review comments incorporated.

[WebKit Commit Bot]

Comment on attachment 266916 [details]
Updated patch with review comments incorporated.

Rejecting attachment 266916 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 266916, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Tools/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/533630

[WebKit Commit Bot]

Comment on attachment 266916 [details]
Updated patch with review comments incorporated.

Clearing flags on attachment: 266916

Committed r193765: <http://trac.webkit.org/changeset/193765>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.