Bug 177467 – webkitpy: Notify parent process when a worker is spawned

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

[Jonathan Bedard]

There are a few resources which don't copy well when multiprocessing.  File handles and sockets are the best examples.  We should provide a way for ports to release such resources after creating workers.

[Jonathan Bedard]

Created attachment 321749 [details]
Patch

[Jonathan Bedard]

A few notes to this change after discussing with Dan Bates today (9-25) and exploring a few other techniques for doing this.

1) We could tie this behavior to the Worker.start(...) function of the worker, but function runs on the Worker, not the parent.

2) _Worker.start(...) could own this functionality, but this would mean that _Worker would need to own the release_worker_resources callback, I don't want to pass the callback more than we need to.

Note that we can't use the worker_factory callback because the worker_factory callback is actually run before we fork the process.  If we were going to place the callback here, we would need to lazily initialize whatever resources we were reseting in the workers.

[Jonathan Bedard]

Created attachment 321769 [details]
Patch

[Jonathan Bedard]

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

Discussed this with Dan, he suggested cleaning up resources for each worker individually instead of all at the same time.

[Daniel Bates]

Comment on attachment 321769 [details]
Patch

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

> Tools/Scripts/webkitpy/common/message_pool.py:65
> +    def __init__(self, caller, worker_factory, num_workers, worker_startup_delay_secs=0.0, host=None, release_worker_resources=lambda worker_number: None):

Is it necessary to add a new argument to this constructor? It seems sufficient to make use of the existing |caller|. For our usage, the argument caller is an instance of LayoutTestRunner. And LayoutTestRunner has a reference to the port object. I suggest we add a method called _handle_did_spawn_worker to LayoutTestRunner that takes a worker_number and turns around and calls the port's did_spawn_worker passing the worker number. Then have MessagePool._start_workers() call self._caller.handle('did_spawn_worker', worker_number).

> Tools/Scripts/webkitpy/port/base.py:1551
> +    def release_worker_resources(self, worker_number):

Assuming we make the change I suggested above in MessagePool. It would be consistent to name this method did_spawn_worker.

> Tools/Scripts/webkitpy/port/base.py:1552
> +        # File handle or sockets instantiated in the parent process but managed by workers should be released on the parent process.

Assuming we name this function did_spawn_worker then I would add the following docstring, wrapping as appropriate:

# This is overridden by ports that need to do work in the parent process after a worker subprocess is spawned, such as closing file descriptors that were implicitly cloned to the worker.

[Jonathan Bedard]

Comment on attachment 321769 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/message_pool.py:65
>> +    def __init__(self, caller, worker_factory, num_workers, worker_startup_delay_secs=0.0, host=None, release_worker_resources=lambda worker_number: None):
> 
> Is it necessary to add a new argument to this constructor? It seems sufficient to make use of the existing |caller|. For our usage, the argument caller is an instance of LayoutTestRunner. And LayoutTestRunner has a reference to the port object. I suggest we add a method called _handle_did_spawn_worker to LayoutTestRunner that takes a worker_number and turns around and calls the port's did_spawn_worker passing the worker number. Then have MessagePool._start_workers() call self._caller.handle('did_spawn_worker', worker_number).

No, I did it this way to avoid needing to modify other users.

It turns out that there is only one other user, so I think your handler suggestion is cleaner.

[Jonathan Bedard]

Created attachment 321838 [details]
Patch

[Daniel Bates]

Comment on attachment 321838 [details]
Patch

Thank you for updating the patch, Jonathan.

r=me

[Daniel Bates]

I updated the title of the bug. Please take care to update the ChangeLog before landing. Should we have a radar bug for this Bugzilla bug?

[Jonathan Bedard]

(In reply to Daniel Bates from comment #9)
> I updated the title of the bug. Please take care to update the ChangeLog
> before landing. Should we have a radar bug for this Bugzilla bug?

I'll manually file it (along with a bug on the importer, since it didn't do it's job).

[Jonathan Bedard]

<rdar://problem/34660124>

[Radar WebKit Bug Importer]

<rdar://problem/34660194>

[Jonathan Bedard]

Created attachment 321847 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 321847 [details]
Patch for landing

Clearing flags on attachment: 321847

Committed r222510: <http://trac.webkit.org/changeset/222510>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.