Bug 180555 – webkitpy: Reimplement simulator code

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

There are a number of problems with the existing simulator code:
    - Use of regex instead of json output
    - No access to a simulator's device type after it's been booted
    - Starting/stopping Simulator.app is managed in the port object
    - Simulators are re-built each time simctl is run
    - If multiple simulators are already booted, we will only use one
    - Device types are only considered when booting simulators

This bug tracks a refactor of our simulator management code.

[Jonathan Bedard]

Created attachment 328754 [details]
Patch

[EWS Watchlist]

Comment on attachment 328754 [details]
Patch

Attachment 328754 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5537692

New failing tests:
webrtc/video-replace-muted-track.html

[EWS Watchlist]

Created attachment 328778 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Jonathan Bedard]

Created attachment 328977 [details]
Patch

[Jonathan Bedard]

Created attachment 328997 [details]
Patch

[Jonathan Bedard]

Created attachment 329249 [details]
Patch

[Dean Johnson]

Comment on attachment 329249 [details]
Patch

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

Some comments, let's work on those and then circle back. :)

> Tools/Scripts/webkitpy/xcode/device_type.py:39
> +            software_version=version)

I don't really understand what's going on here from reading through. Can you provide an example input at the top, and comments?

> Tools/Scripts/webkitpy/xcode/device_type.py:41
> +    def __init__(self, hardware_family=None, hardware_type=None, software_version=None, software_variant=None):

Can you please provide example values for hardware_family, hardware_type, software_version and software_variant? It's difficult to review this code without knowing the values that are expected to be coming through.

> Tools/Scripts/webkitpy/xcode/device_type.py:43
> +            hardware_family = 'iPhone'

I'm not sure this is a great abstraction to make -- if we want to default to a specific device type, we should make those the default values.

I'd recommend writing a validate_configuration function that takes hardware_family, hardware_type, software_version and software_variant and raises various exceptions if the provided configuration is invalid.

> Tools/Scripts/webkitpy/xcode/device_type.py:52
> +            self.software_variant = software_variant

This could be written better by pre-defining self.software_variant above, then writing the below else block as `if self.hardware_family is not None:`

> Tools/Scripts/webkitpy/xcode/device_type.py:63
> +            assert self.software_variant is not None

IMO this check should happen after checking self.hardware_type is not None. You should also check `if self.software_version is not None`

> Tools/Scripts/webkitpy/xcode/device_type.py:77
> +        return result

Instead of using += to build up a string, can you use formatters? It will make it more clear and easier to read.

> Tools/Scripts/webkitpy/xcode/device_type.py:79
> +    # This technique of matching treats 'None' a wild-card.

Nice comment :)

> Tools/Scripts/webkitpy/xcode/device_type.py:92
> +    def contained_in(self, other):

Following the pattern of using python meta classes for equality and strings, you may consider writing this using __contains__ which Python will use if you call `other in DeviceType`.

> Tools/Scripts/webkitpy/xcode/device_type.py:105
> +class DeviceRequest(object):

It's unclear to me what a DeviceRequest is... can you come up with a better name?

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:1
> +# Copyright (C) 2017 Apple Inc. All rights reserved.

Why is 'new' in the name of the file?

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:43
> +

Nit: Extra line.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:55
> +    MEMORY_ESTIMATE_PER_SIMULATOR_INSTANCE = 2 * 1024 * 1024 * 1024  # ~2 gigs a simulator.

Nit: This may read better `2 * (1024 ** 3)  # 2GB`

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:69
> +            simctl_list_output = json.loads(host.executive.run_command([SimulatedDeviceManager.xcrun, 'simctl', 'list', '--json']))

A better name for this variable might be `simulators`.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:74
> +        SimulatedDeviceManager._device_identifier_to_name = {}

This resets the class's _device_identifier_to_name mapping to be empty... is that necessary? Generally, I think we should avoid writing code that changes a class's state in static methods.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:77
> +

The above 3 lines could be written more succinctly as the following:

(Python2.7+)
SimulatedDeviceManager._device_identifier_to_name = {device['identifier']: device['name'] for device in simctl_list_output['devicetypes']}

(Python2.6+, iirc)
SimulatedDeviceManager._device_identifier_to_name = dict((device['identifier'], device['name']) for device in simctl_list_output['devicetypes'])

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:82
> +            SimulatedDeviceManager.AVAILABLE_RUNTIMES.append(SimulatedDeviceManager.Runtime(runtime_dict))

You can write this more succinctly as follows:
available_runtimes = [runtime for runtime in simctl_list_output['runtimes'] if runtime['availability'] == ' (available)']
SimulatedDeviceManager.AVAILABLE_RUNTIMES.extend(available_runtimes)

Is there a reason we don't instantiate SimulatedDeviceManager.AVAILABLE_RUNTIMES here similarly to what we do for SimulatedDeviceManager._device_identifier_to_name?

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:88
> +                    continue

Why is this necessary? Isn't this done in the above code? If not, how does simctl_list_output['device'][runtime.name] differ?

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:100
> +                    device_type_string = SimulatedDeviceManager._device_identifier_to_name[plistlib.readPlist(host.filesystem.open_binary_file_for_reading(device_plist))['deviceType']]

Consider writing helper functions for some of the calls here so your implementation reads more cleanly. Example:

def plist_for_simulator_device(host, udid):
    plist_path = host.filesystem.expanduser(host.filesystem.join(SimulatedDeviceManager.simulator_device_path, udid, 'device.plist'))
    if not host.filesystem.isfile(plist_path):
        # raise Exception or return None

    return plistlib.readPlist(host.filesystem.open_binary_file_for_reading(plist_path))

# In the above code
device_specific_plist = plist_for_simulator_device(host, device_dict['udid'])

# use try/except or check if `if device_specific_plist is None`
device_type = device_specific_plist['deviceType']
device_name = SimulatedDeviceManager._device_identifier_to_name[device_type]  # You could in-line the above call, if wanted.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:101
> +

Nit: Move the newline above the device_type_string declaration.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:109
> +                device.platform_device._state = SimulatedDevice.NAME_FOR_STATE.index(device_dict['state'].upper())

What is this state used for?

[Jonathan Bedard]

Comment on attachment 329249 [details]
Patch

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

I will update this patch addressing Dean's comments.

I've also responded to a few directly.

>> Tools/Scripts/webkitpy/xcode/device_type.py:43
>> +            hardware_family = 'iPhone'
> 
> I'm not sure this is a great abstraction to make -- if we want to default to a specific device type, we should make those the default values.
> 
> I'd recommend writing a validate_configuration function that takes hardware_family, hardware_type, software_version and software_variant and raises various exceptions if the provided configuration is invalid.

I'm not sure what we get from validation.

If you construct an 'invalid' DeviceType, the only effect will be that you won't be able to find a simulated device which matches your specified DeviceType.  I don't see the benefit of a validation table, and then we would have to keep track of one.  This table-less matching allows us to rely on the output of simctl to define what a valid device is.  simctl should always be the authority on this, if webkitpy knows about a DeviceType that simctl doesn't, webkitpy cannot use this DeviceType anyways.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:1
>> +# Copyright (C) 2017 Apple Inc. All rights reserved.
> 
> Why is 'new' in the name of the file?

This file will replace simulated_device.py.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:88
>> +                    continue
> 
> Why is this necessary? Isn't this done in the above code? If not, how does simctl_list_output['device'][runtime.name] differ?

There is a distinction between runtimes and devices.  simctl makes this distinction, so we will do.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:109
>> +                device.platform_device._state = SimulatedDevice.NAME_FOR_STATE.index(device_dict['state'].upper())
> 
> What is this state used for?

We are most interested in BOOTED/SHUTDOWN.  Normally, we would grab this state from the device's .plist file.  However, simctl is giving us this information, so we update devices, eliminating the need to check these files for a bit.

[Jonathan Bedard]

Created attachment 329386 [details]
Patch

[Dean Johnson]

Comment on attachment 329386 [details]
Patch

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

Some more comments, let's continue the review in-person in a little while.

> Tools/Scripts/webkitpy/xcode/device_type.py:32
> +        split_str = str.split(' ')

'''
Converts a string like 'iPhone 6s' or 'Apple TV 4K' into a DeviceType object.

Example input + output:
'iPhone 6 Plus' -> DeviceType(hardware_family='iPhone', hardware_type='6 Plus', software_version=None)
'''

I'd recommend a docstring like the above to fully explain what is happening here.

Also, don't use `str` as a variable name as its Python's default built-in string type.

> Tools/Scripts/webkitpy/xcode/device_type.py:75
> +    # software_variant groups together hardware families which share apps, like iPad and iPhone. iOS, tvOS and watchOS are examples.

Put this into a docstring please.

> Tools/Scripts/webkitpy/xcode/device_type.py:132
> +        self.use_booted = use_booted  # Will match a booted simulator.

Nit: use_booted -> use_booted_simulator

> Tools/Scripts/webkitpy/xcode/device_type.py:133
> +        self.use_existing = use_existing  # Will use an existing simulator instead of creating a new one.

Nit use_existing -> use_existing_simulator

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:54
> +    MEMORY_ESTIMATE_PER_SIMULATOR_INSTANCE = 2 * (3 ** 1024)  # 2GB a simulator.

>>> 3 ** 1024
373391848741020043532959754184866588225409776783734007750636931722079040617265251229993688938803977220468765065431475158108727054592160858581351336982809187314191748594262580938807019951956404285571818041046681288797402925517668012340617298396574731619152386723046235125934896058590588284654793540505936202376547807442730582144527058988756251452817793413352141920744623027518729185432862375737063985485319476416926263819972887006907013899256524297198527698749274196276811060702333710356481L

That's a lot of RAM. ;)

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:64
> +    def _runtimes_from_json(runtimes_json):

Consider renaming this from _runtimes_from_json -> _create_runtimes(runtimes)

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:76
> +    def _available_device_with_runtime_from_json(host, runtime, device_json):

JSON is a string format, consider renaming device_json to device_info.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:142
> +                return name

You may consider writing this like so:
     def _find_available_name(name_base):
         created_index = 0
         while True:
             name = '{} {}'.format(name_base, created_index)
             created_index += 1
             for device in SimulatedDeviceManager.INITIALIZED_DEVICES:
                 if device is None:
                     continue
                 if device.platform_device.name == name:
                     break
             else:
                 return name

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:145
> +    def _disambiguate_device_type(type):

Please rename type to _type.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:158
> +                full_type.software_version = runtime.version

Consider putting this code into its own function so you can early exit if you get a full match.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:177
> +    def runtime_from_type(type):

Please rename this function to get_runtime_for_device_type, and type -> device_type.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:190
> +        return None

Ditto.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:203
> +                return device

Can you turn L196-L203 into a function called 'find_existing_device_for_request'?

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:221
> +        SimulatedDeviceManager.available_devices(host)

I recommend changing how you've organized available devices:

1. Move SimulatedDeviceManager.available_devices(host) code that initializes AVAILABLE_DEVICES into a new function `SimulatedDeviceManager.populate_available_devices` (if you can come up with a better name, go for it).
2. SimulatedDeviceManager.available_devices(host) should essentially read as follows:

@staticmethod
def available_devices(host):
    if SimulatedDeviceManager.AVAILABLE_DEVICES == []:
        SimulatedDeviceManager.populate_available_devices()

    return SimulatedDeviceManager.AVAILABLE_DEVICES

3. Now, everywhere possible call SimulatedDeviceManager.available_devices() instead of SimulatedDeviceManager.AVAILABLE_DEVICES. This ensures we are using the external API as we expect our consumers to do.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:255
> +        return None

Can we add logging for which match we make? (exact, fuzzy, incomplete)

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:258
> +    def _wait_till_device_in_state(device, state, deadline):

Nit: till -> until

[Jonathan Bedard]

Created attachment 329417 [details]
Patch

[Jonathan Bedard]

Created attachment 329683 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/36131381>

[Jonathan Bedard]

Comment on attachment 329683 [details]
Patch

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

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:242
> +        SimulatedDeviceManager.available_devices(host)

Use populate_available_devices()

[Dean Johnson]

Comment on attachment 329683 [details]
Patch

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

Much better, almost there! Let's see one more iteration and I think this will be ready to land.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:242
>> +        SimulatedDeviceManager.available_devices(host)
> 
> Use populate_available_devices()

Please add a comment about why we need to re-sync AVAILABLE_DEVICES.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:251
> +        if device.platform_device.state() != SimulatedDevice.DeviceState.BOOTING and device.platform_device.state() != SimulatedDevice.DeviceState.BOOTED:

Please move this check to its own function since we perform it in more than one place. Otherwise, it's likely someone working with the code in the future will forget to update one call site.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:302
> +        if not isinstance(requests, list):

Make sure to handle tuple types here, too.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:304
> +        requests = [request if isinstance(request, DeviceRequest) else DeviceRequest(request) for request in requests]

Speaking in person, let's remove this line. We will only handle DeviceRequests.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:308
> +        for device in SimulatedDeviceManager.AVAILABLE_DEVICES:

for device in SimulatedDeviceManager.available_devices(host):

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:318
> +            for request in list(requests):

Use copy.deepcopy instead of `list`.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:336
> +            SimulatedDeviceManager.INITIALIZED_DEVICES.append(device)

It seems the above 4 lines would be better served in a function called SimulatedDeviceManager.initialize_device(device).

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:370
> +        return min(maximum_simulator_count_on_this_system, best_child_process_count_for_hardware)

I think it'd be more clear to write this as follows:

system_process_count_limit = int(host.executive.run_command(['/usr/bin/ulimit', '-u']).strip())
current_process_count = len(host.executive.run_command(['/bin/ps', 'aux']).strip().split('\n'))
_log.debug('Process limit: {}, current #processes: {}'.format(system_process_count_limit, current_process_count))

max_supported_simulators_for_hardware = min(host.executive.cpu_count() / 2, host.platform.total_bytes_memory() // SimulatedDeviceManager.MEMORY_ESTIMATE_PER_SIMULATOR_INSTANCE)
max_supported_simulators_locally = (system_process_count_limit - current_process_count) // SimulatedDeviceManager.PROCESS_COUNT_ESTIMATE_PER_SIMULATOR_INSTANCE

if (max_supported_simulators_for_hardware < max_supported_simulators_locally):
    _log.warn('This machine could support {} simulators, but is only configured for {}.'.format(best_child_process_count_for_hardware, maximum_simulator_count_on_this_system))
    _log.warn('Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.')

if max_supported_simulators_locally == 0:
    max_supported_simulators_locally = 1

return min(max_supported_simulators_for_hardware, max_supported_simulators_locally)

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:459
> +    def shutdown(self, timeout=10.0):

Can we prefix this function with an '_'?

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:466
> +            SimulatedDeviceManager.INITIALIZED_DEVICES.remove(self)

This isn't necessary given self.teardown().

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:473
> +    def delete(self, timeout=10.0):

Can we prefix this function with an '_'?

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:482
> +    def teardown(self, timeout=10.0):

Can we prefix this function with an '_'?

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:548
> +            return None

Please return False here stead.

> Tools/Scripts/webkitpy/xcode/new_simulated_device_unittest.py:542
> +    def device_by_criteria(function):

This should probably go on SimulatedDeviceManager as a public function.

[Jonathan Bedard]

Comment on attachment 329683 [details]
Patch

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

Dean and I discussed trying to use available_devices(...) over AVAILABLE_DEVICES more frequently in this patch.  The new patch does it's best, but many of the functions using AVAILABLE_DEVICES are called by available_devices(...).

>>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:242
>>> +        SimulatedDeviceManager.available_devices(host)
>> 
>> Use populate_available_devices()
> 
> Please add a comment about why we need to re-sync AVAILABLE_DEVICES.

Addressed in new patch.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:251
>> +        if device.platform_device.state() != SimulatedDevice.DeviceState.BOOTING and device.platform_device.state() != SimulatedDevice.DeviceState.BOOTED:
> 
> Please move this check to its own function since we perform it in more than one place. Otherwise, it's likely someone working with the code in the future will forget to update one call site.

Addressed in new patch.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:302
>> +        if not isinstance(requests, list):
> 
> Make sure to handle tuple types here, too.

Addressed in new patch.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:304
>> +        requests = [request if isinstance(request, DeviceRequest) else DeviceRequest(request) for request in requests]
> 
> Speaking in person, let's remove this line. We will only handle DeviceRequests.

Addressed in new patch.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:308
>> +        for device in SimulatedDeviceManager.AVAILABLE_DEVICES:
> 
> for device in SimulatedDeviceManager.available_devices(host):

Addressed in new patch.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:318
>> +            for request in list(requests):
> 
> Use copy.deepcopy instead of `list`.

Addressed in new patch.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:336
>> +            SimulatedDeviceManager.INITIALIZED_DEVICES.append(device)
> 
> It seems the above 4 lines would be better served in a function called SimulatedDeviceManager.initialize_device(device).

I think that would be a bit confusing...after all, devices can be initialized other places in this function.

I'm going to use '_boot_device' instead.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:370
>> +        return min(maximum_simulator_count_on_this_system, best_child_process_count_for_hardware)
> 
> I think it'd be more clear to write this as follows:
> 
> system_process_count_limit = int(host.executive.run_command(['/usr/bin/ulimit', '-u']).strip())
> current_process_count = len(host.executive.run_command(['/bin/ps', 'aux']).strip().split('\n'))
> _log.debug('Process limit: {}, current #processes: {}'.format(system_process_count_limit, current_process_count))
> 
> max_supported_simulators_for_hardware = min(host.executive.cpu_count() / 2, host.platform.total_bytes_memory() // SimulatedDeviceManager.MEMORY_ESTIMATE_PER_SIMULATOR_INSTANCE)
> max_supported_simulators_locally = (system_process_count_limit - current_process_count) // SimulatedDeviceManager.PROCESS_COUNT_ESTIMATE_PER_SIMULATOR_INSTANCE
> 
> if (max_supported_simulators_for_hardware < max_supported_simulators_locally):
>     _log.warn('This machine could support {} simulators, but is only configured for {}.'.format(best_child_process_count_for_hardware, maximum_simulator_count_on_this_system))
>     _log.warn('Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.')
> 
> if max_supported_simulators_locally == 0:
>     max_supported_simulators_locally = 1
> 
> return min(max_supported_simulators_for_hardware, max_supported_simulators_locally)

Addressed in new patch.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:459
>> +    def shutdown(self, timeout=10.0):
> 
> Can we prefix this function with an '_'?

Addressed in new patch.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:466
>> +            SimulatedDeviceManager.INITIALIZED_DEVICES.remove(self)
> 
> This isn't necessary given self.teardown().

Addressed in new patch.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:473
>> +    def delete(self, timeout=10.0):
> 
> Can we prefix this function with an '_'?

Addressed in new patch.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:482
>> +    def teardown(self, timeout=10.0):
> 
> Can we prefix this function with an '_'?

Addressed in new patch.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device_unittest.py:542
>> +    def device_by_criteria(function):
> 
> This should probably go on SimulatedDeviceManager as a public function.

Moved in the new patch.

[Jonathan Bedard]

Created attachment 329962 [details]
Patch

[Dean Johnson]

Comment on attachment 329962 [details]
Patch

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

LGTM with the requested changes. Unofficial r=me.

> Tools/Scripts/webkitpy/xcode/device_type.py:44
> +    def from_string(cls, device_string, version=None):

Docstrings are supposed to go under the function definition.

> Tools/Scripts/webkitpy/xcode/device_type.py:94
> +    """

Ditto.

> Tools/Scripts/webkitpy/xcode/device_type.py:95
> +    # software_variant

This comment is probably unnecessary now.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:188
> +                return runtime

These two if statements and loops are the exact same... is this necessary? If so, please add a comment.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:240
> +

Nit: Probably don't need a new line here.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:254
> +

Nit: Probably don't need a newline here.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:269
> +        if not SimulatedDeviceManager._is_device_booted_or_booting(device):

It seems `_is_booted_or_booting` should be on Device rather than SimulatedDeviceManager.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:291
> +        for request in list(requests):

Mentioned in the previous review. Use copy.deepcopy instead of `list`.

[Jonathan Bedard]

Comment on attachment 329962 [details]
Patch

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

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:188
>> +                return runtime
> 
> These two if statements and loops are the exact same... is this necessary? If so, please add a comment.

The second loop should be checking for a partial match.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:291
>> +        for request in list(requests):
> 
> Mentioned in the previous review. Use copy.deepcopy instead of `list`.

I did some more thorough testing this morning before uploading a final patch.

copy.deepcopy is not correct here (or in the other location, actually) because of copy.deepcopy works and how the DeviceRequest object works.  copy.deepcopy will copy not only the list by value, but each element in the list by value.  What we want here is the list copied by value but the contents of the list copied by reference.  Basically, here are the two differences:

copy.deepcopy()
original list: [object1 with value x, object2 with value y]
copied list: [object3 with value x, object4 with value y]

list()
original list: [object1 with value x, object2 with value y]
copied list: [object1 with value x, object2 with value y]

In order to get copy.deepcopy to work, we would need to be able to compare requests by value instead of by reference.  The trouble is, we don't want to be comparing DeviceRequests by value because it is quite likely that there will be multiple requests with the same value in the request list.  Further complicating things, such a comparison function couldn't use the comparison function for DeviceType, because that comparison function allows for partial matching, which is not what we want in this case.  Because of both of these, it makes more sense to continue to compare DeviceRequests by reference instead of by value.

Given both your and my confusion here, I'm going to do this instead:

# DeviceRequests are compared by reference
requests_copy = [request for request in requests]
for request in requests_copy:
    ....

[Jonathan Bedard]

Created attachment 330042 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #20)
> Created attachment 330042 [details]
> Patch

Note that this patch has no risk to CI because nothing uses the refactored code yet.  I've also tested this patch locally with Part 2 and confirmed that some of the features that are difficult to construct unit tests for (things like creating simulators from scratch, tearing down simulators owned by the script even when exits occur unexpectedly, handling simulators in the process of shutting down) are all behaving as expected.

[Alexey Proskuryakov]

Comment on attachment 330042 [details]
Patch

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

One thing that I think needs to be changed in the unusual handling of None value in comparisons. Other comments are nitpicks.

> Tools/ChangeLog:9
> +        The new Simulator interface defined in this patch is designed to

It doesn't seem like there is a new Simulator interface defined in this patch.

> Tools/ChangeLog:11
> +        attributes, the the manager will return a list of devices fulfilling the

then*

> Tools/ChangeLog:14
> +                - Existing simulators currently shutdown

shut down*

> Tools/ChangeLog:32
> +        of a device can usually be implied from it's hardware_family.

its*

> Tools/ChangeLog:33
> +        (DeviceType.verify): Verify that the software_variant matches the hardware_family

I think that checkConsistency would be a better name.

> Tools/ChangeLog:87
> +        (SimulatorManager.teardown): Shutdown any simulators managed by this class.

Shut down*

Also, the name for the function would be tearDown.

> Tools/ChangeLog:92
> +        the device. Note that this function will only read the plist every second, and will

I'd say "will cache the result for a second". Seems potentially dangerous, but I don't have a better suggestion.

> Tools/ChangeLog:95
> +        (SimulatedDevice._shutdown): Shut down this device and remove it from the list of

Ditto.

> Tools/ChangeLog:99
> +        (SimulatedDevice._teardown): Shutdown and delete this device, if it is managed by

Ditto.

> Tools/ChangeLog:109
> +        (SimulatedDeviceTest.tearDown):

!

> Tools/Scripts/webkitpy/port/device.py:105
> +        if other is None:
> +            return False

Ugh, so DeviceTypes have None a wildcard, but Devices work the opposite way? This seems wrong.

> Tools/Scripts/webkitpy/xcode/device_type.py:32
> +        Converts a string like 'iPhone 6s' or 'Apple TV 4K' into a DeviceType object.

It would be useful to more strictly document the supported variants, before we get into a situation similar to the one with ports (is mac a port? are mac-wk2 or mac-sierra-wk2 ports?)

> Tools/Scripts/webkitpy/xcode/device_type.py:37
> +        'iPhone 6 Plus' -> DeviceType(hardware_family='iPhone', hardware_type='6 Plus', software_version=None)
> +        'iPhone' -> DeviceType(hardware_family='iPhone', hardware_type=None, software_version=None)
> +        'Apple TV 4K' -> DeviceType(hardware_family='TV', hardware_type='4K', software_version=None)

Do we ever get a software_version from parsing?

> Tools/Scripts/webkitpy/xcode/device_type.py:90
> +        :param hardware_type: 6s, Series 2 - 42mm, 4k are all examples

Please verify that all examples have regression tests.

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:43
> +    def __init__(self, device_type, use_booted_simulator=True, use_existing_simulator=True, allow_incomplete_match=False, collapse_requests=False):

Is this collapsing, coalescing, merging or joining?

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:375
> +            system_process_count_limit = int(host.executive.run_command(['/usr/bin/ulimit', '-u']).strip())
> +            current_process_count = len(host.executive.run_command(['/bin/ps', 'aux']).strip().split('\n'))
> +            _log.debug('Process limit: {}, current #processes: {}'.format(system_process_count_limit, current_process_count))

Can/should this be done by code at a lower abstraction level?

> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:513
> +        # FIXME: This is a workaround for <rdar://problem/30273973>, Racey failure of simctl install.

Do we still need this?

[Jonathan Bedard]

Comment on attachment 330042 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/device.py:105
>> +            return False
> 
> Ugh, so DeviceTypes have None a wildcard, but Devices work the opposite way? This seems wrong.

This is a really good point.

I'll remove this code and work with Python's list idioms.

>> Tools/Scripts/webkitpy/xcode/device_type.py:32
>> +        Converts a string like 'iPhone 6s' or 'Apple TV 4K' into a DeviceType object.
> 
> It would be useful to more strictly document the supported variants, before we get into a situation similar to the one with ports (is mac a port? are mac-wk2 or mac-sierra-wk2 ports?)

I will update the comment:

These strings should be of the form '<hardware_family> <hardware_type>', where <hardware_family> is mandatory and <hardware_family> is optional.

>> Tools/Scripts/webkitpy/xcode/device_type.py:90
>> +        :param hardware_type: 6s, Series 2 - 42mm, 4k are all examples
> 
> Please verify that all examples have regression tests.

Needed to add test cases for the comments on line 35 and 36.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:43
>> +    def __init__(self, device_type, use_booted_simulator=True, use_existing_simulator=True, allow_incomplete_match=False, collapse_requests=False):
> 
> Is this collapsing, coalescing, merging or joining?

coalescing or merging is probably the better term.

I'll use merging.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:375
>> +            _log.debug('Process limit: {}, current #processes: {}'.format(system_process_count_limit, current_process_count))
> 
> Can/should this be done by code at a lower abstraction level?

This could be moved to the platform info object (much like the CPU count is)

Previously, this was owned by the IOSSimulatorPort.  I would advocate keeping this code here until we demonstrate that we need to use it elsewhere.

>> Tools/Scripts/webkitpy/xcode/new_simulated_device.py:513
>> +        # FIXME: This is a workaround for <rdar://problem/30273973>, Racey failure of simctl install.
> 
> Do we still need this?

Probably not.

I'll remove the code.  I suppose if we start to see install failures, we can add it back in.

[Jonathan Bedard]

Created attachment 330076 [details]
Part 1

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #24)
> Created attachment 330076 [details]
> Patch

To address Alexey's R-, there are now 2 places in the code where instead of checking if something is 'in' the list, we iterate through the list to screen out any 'None' values.  That occurs on line 160 and 512.

[WebKit Commit Bot]

Comment on attachment 330076 [details]
Part 1

Clearing flags on attachment: 330076

Committed r226263: <https://trac.webkit.org/changeset/226263>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 330257 [details]
Patch

[Jonathan Bedard]

Comment on attachment 330257 [details]
Patch

Part 2 should not land until we have people watching the bots again.

[Aakash Jain]

Comment on attachment 330257 [details]
Patch

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

Please make sure to manually test this well.

> Tools/Scripts/webkitpy/port/ios_simulator.py:53
> +        processes_to_use = 0

rename processes_to_use to more meaningful name which indicate it is number of simulators.

> Tools/Scripts/webkitpy/port/ios_simulator.py:54
> +        if not self.get_option('dedicated_simulators', False):

Shouldn't this condition be reverse?

> Tools/Scripts/webkitpy/port/ios_simulator.py:56
> +            def filter(device):

Can we rename fllter() to booted_ios_devices_filter() ?

> Tools/Scripts/webkitpy/port/ios_simulator.py:59
> +                return device.platform_device.device_type in DeviceType(software_variant='iOS', software_version=self.ios_version())

"DeviceType(software_variant='iOS', software_version=self.ios_version()" is used at two places. You can consider making it a class variable.

e.g.: IOS_DEVICE_TYPE = DeviceType(software_variant='iOS', software_version=self.ios_version()

> Tools/Scripts/webkitpy/port/ios_simulator.py:60
> +            processes_to_use = len(SimulatedDeviceManager.device_by_filter(filter, host=self.host))

So new simulators are not booted if dedicated_simulators is used? i.e.: if machine supports 12 simulators (12 child_processes) but only 2 simulators are currently booted, will we boot 10 extra simulators?

> Tools/Scripts/webkitpy/port/ios_simulator.py:62
> +        processes_to_use = processes_to_use if processes_to_use else self.get_option('child_processes', self.default_child_processes())

Please simplify this to something like:

if not processes_to_use:
    processes_to_use = self.get_option('child_processes', self.default_child_processes())

You can also consider to initialize this variable with this value and you wouldn't need the if check here.

> Tools/Scripts/webkitpy/port/ios_simulator.py:64
> +            _log.warn('The specified number of child processes does not match the number of Simulators.  Setting child_processes to {}.'.format(processes_to_use))

Warning message is misleading. Please correct as discussed. Nit: two spaces after .

> Tools/Scripts/webkitpy/port/ios_simulator.py:99
> +        _log.debug('setup_test_run for {}'.format(device_type))

This debug statement can be improved. e.g.: "creating devices for {}".format(device_type)

> Tools/Scripts/webkitpy/port/ios_simulator.py:102
>          for i in xrange(self.child_processes()):

This for loop is probably not required, you are making multiple copies of same requests. you can do that using something like:
requests = [request] * self.child_processes()

> Tools/Scripts/webkitpy/port/ios_simulator.py:136
> +        target_type = DeviceType(software_variant='iOS', software_version=self.ios_version())

you can consider renaming target_type to something more readable like target_device_type

> Tools/Scripts/webkitpy/port/ios_simulator.py:138
> +            if device.platform_device.device_type in (target_type):

are () required around target_type?

> Tools/Scripts/webkitpy/port/ios_simulator.py:140
> +        _log.error('Cannot find a Simulated device matching "{}"'.format(str(target_type)))

Please see if this message can be made more actionable.

[Jonathan Bedard]

Comment on attachment 330257 [details]
Patch

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

I've ran a number of local tests, I want to outline some behavior on my machine (max 4 simulators):

No sims booted, --child-processes=None: 4 simulators used
1 sims booted, --child-processes=None: 1 simulator used
2 sims booted, --child-processes=None: 2 simulators used
No sims booted, --child-processes=3: 3 simulators used
No sims booted, --child-processes=6: 4 simulators used
2 sims booted, --child-processes=1: 1 simulator used

>> Tools/Scripts/webkitpy/port/ios_simulator.py:54
>> +        if not self.get_option('dedicated_simulators', False):
> 
> Shouldn't this condition be reverse?

If we are not using dedicated_simulators, then we first going to check booted simulators.

This condition is correct.

>> Tools/Scripts/webkitpy/port/ios_simulator.py:59
>> +                return device.platform_device.device_type in DeviceType(software_variant='iOS', software_version=self.ios_version())
> 
> "DeviceType(software_variant='iOS', software_version=self.ios_version()" is used at two places. You can consider making it a class variable.
> 
> e.g.: IOS_DEVICE_TYPE = DeviceType(software_variant='iOS', software_version=self.ios_version()

I think this would make the code more confusing since ios_version() is dynamic and depends on the provided host.

>> Tools/Scripts/webkitpy/port/ios_simulator.py:60
>> +            processes_to_use = len(SimulatedDeviceManager.device_by_filter(filter, host=self.host))
> 
> So new simulators are not booted if dedicated_simulators is used? i.e.: if machine supports 12 simulators (12 child_processes) but only 2 simulators are currently booted, will we boot 10 extra simulators?

It's the other way around.

If dedicated_simulators is used, existing simulators are never used.  Otherwise, we use only the booted simulators unless there are 0 booted simulators, in which case we must boot our own.

[Jonathan Bedard]

Created attachment 330758 [details]
Patch

[Aakash Jain]

Comment on attachment 330758 [details]
Patch

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

> Tools/Scripts/webkitpy/port/ios_simulator.py:63
> +            number_of_simulators_to_use = self.get_option('child_processes', self.default_child_processes())

This logic to determine number_of_simulators_to_use seems unnecessarily complicated. You can consider initializing number_of_simulators_to_use with self.get_option('child_processes', self.default_child_processes())

> Tools/Scripts/webkitpy/port/ios_simulator.py:66
> +            _log.warn('The specified number of child processes is greater than the number of Simulators to be used. Setting child_processes to {}.'.format(number_of_simulators_to_use))

This doesn't happen while using dedicated simulator. We will incorrectly print this statement, while still using higher number of simulators.

Also this print statement should be accompanied with the corresponding assignment.

[Jonathan Bedard]

Comment on attachment 330758 [details]
Patch

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

I've responded to Aakash's comments.  It also seems that my effort to reduce the amount of code has come at the cost of clarity.  I would like to propose the following for the constructor:

def __init__(self, host, port_name, **kwargs):
    super(IOSSimulatorPort, self).__init__(host, port_name, **kwargs)

    optional_device_class = self.get_option('device_class')
    self._device_class = optional_device_class if optional_device_class else self.DEFAULT_DEVICE_CLASS
    _log.debug('IOSSimulatorPort _device_class is %s', self._device_class)

    def booted_ios_devices_filter(device):
        if not device.platform_device.is_booted_or_booting():
            return False
        return device.platform_device.device_type in DeviceType(software_variant='iOS', software_version=self.ios_version())

    number_of_booted_sims = len(SimulatedDeviceManager.device_by_filter(booted_ios_devices_filter, host=self.host))
    if not self.get_option('dedicated_simulators', False) and number_of_booted_sims:
        number_of_simulators_to_use = min(number_of_booted_sims, self.get_option('child_processes', self.default_child_processes()))

        if self.get_option('child_processes') is not None and self.get_option('child_processes') > number_of_simulators_to_use:
            _log.warn('The specified number of child processes is greater than the number of Simulators to be used. Setting child_processes to {}.'.format(number_of_simulators_to_use))
        self.set_option('child_processes', number_of_simulators_to_use)

>> Tools/Scripts/webkitpy/port/ios_simulator.py:63
>> +            number_of_simulators_to_use = self.get_option('child_processes', self.default_child_processes())
> 
> This logic to determine number_of_simulators_to_use seems unnecessarily complicated. You can consider initializing number_of_simulators_to_use with self.get_option('child_processes', self.default_child_processes())

Initializing number_of_simulators_to_use with self.get_option('child_processes', self.default_child_processes()) breaks the logic.

If len(SimulatedDeviceManager.device_by_filter(booted_ios_devices_filter, host=self.host)) is 0, we are implicitly using dedicated simulators.

>> Tools/Scripts/webkitpy/port/ios_simulator.py:66
>> +            _log.warn('The specified number of child processes is greater than the number of Simulators to be used. Setting child_processes to {}.'.format(number_of_simulators_to_use))
> 
> This doesn't happen while using dedicated simulator. We will incorrectly print this statement, while still using higher number of simulators.
> 
> Also this print statement should be accompanied with the corresponding assignment.

This statement is meant to be a warning that the specified child_processes is not be honored (hence the 'self.get_option('child_processes') is not None' bit).  We can't tie this directly to the corresponding assignment since the corresponding assignment is unconditional.  You are correct that this doesn't happen while using dedicated simulators, however, this statement won't print when using a higher number of simulators because number_of_simulators_to_use will be equal to self.get_option('child_processes').

[Jonathan Bedard]

There is another option to handle the booted-simulator vs requested child processes mis-match. (which Aakash and I briefly discussed previously)

We could, if more processes than booted-simulators are requested, boot additional simulators to fill-out the missing ones.  This would be simpler in the IOSSimulatorPort code, but may result in some confusing behavior (particularly if someone had simulators booted and didn't realize it).

[Jonathan Bedard]

Created attachment 330840 [details]
Patch

[Jonathan Bedard]

Here is a brief description of the the new behavior:
    Sims booted, no --child-processes: Use all booted simulators
    No sims booted, no --child-processes: Boot the maximum number of simulators supported
    No sims booted, --child-processes defined: Boot 1 sim per child process, even if this is greater than the maximum number of simulators
    Sims booted, --child-processes defined: Ensure that there is 1 sim per child process, booting new ones if there are not enough existing simulators.

[Aakash Jain]

Comment on attachment 330840 [details]
Patch

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

Please make sure to keep an eye on the bots after this land.

> Tools/Scripts/webkitpy/port/ios_simulator.py:81
> +        return num_booted_sims

As discussed this code can be reorganized to be more readable/clear.

[Jonathan Bedard]

Created attachment 330857 [details]
Part 2

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #40)
> Created attachment 330857 [details]
> Patch

This patch will be landed Jan 10 in the morning, I would like to be able to watch the bots.

[WebKit Commit Bot]

Comment on attachment 330857 [details]
Part 2

Clearing flags on attachment: 330857

Committed r226715: <https://trac.webkit.org/changeset/226715>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 330909 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #45)
> Created attachment 330909 [details]
> Patch

Note that this patch is just moving existing code around and deleting some now dead code.

new_simulated_device.py is replacing simulated_device.py and the old simulator parser is being removed.

[EWS Watchlist]

Comment on attachment 330909 [details]
Patch

Attachment 330909 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6021817

New failing tests:
webgl/1.0.2/conformance/uniforms/uniform-default-values.html

[EWS Watchlist]

Created attachment 330920 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Jonathan Bedard]

Created attachment 330921 [details]
Follow-up fix

[Jonathan Bedard]

Comment on attachment 330921 [details]
Follow-up fix

Landing manually to expedite the process.

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #50)
> Comment on attachment 330921 [details]
> Patch for landing
> 
> Landing manually to expedite the process.

Landed <https://trac.webkit.org/changeset/226723/webkit>.

[Jonathan Bedard]

Created attachment 331116 [details]
Part 3

[Jonathan Bedard]

Comment on attachment 331116 [details]
Part 3

Landed in <https://trac.webkit.org/changeset/226812/webkit>.

[Jonathan Bedard]

Created attachment 331204 [details]
Part 4

[WebKit Commit Bot]

Comment on attachment 331204 [details]
Part 4

Clearing flags on attachment: 331204

Committed r226918: <https://trac.webkit.org/changeset/226918>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.