Change 451099 had a related patch set uploaded (by Brion VIBBER; owner: Brion VIBBER):
[mediawiki/core@master] ParallelMaintenance, QueueingForkController

https://gerrit.wikimedia.org/r/451099

Adding to techcom-rfc board to make sure we don't forget to discuss this, if cross-cutting issues are forseen.

The ability to communicate results back from the children is an important thing that has not been possible before, e.g. rebuildLocalisationCache.php doesn't print the number of languages rebuilt when using --threads.

TechCom is hosting a IRC meeting next week on 5 September 2pm PST(21:00 UTC, 23:00 CET) in
#wikimedia-office

Note via Timo -- currently we run maint scripts in production through hhvm, which sets processor affinity on all child processes. :P

Should work fine still, but won't actually parallelize until we move to zend php7. \o/

Results from TechCom's public meeting regarding this RFC today:

• minutes: https://tools.wmflabs.org/meetbot/wikimedia-office/2018/wikimedia-office.2018-09-05-21.00.html
• log: https://tools.wmflabs.org/meetbot/wikimedia-office/2018/wikimedia-office.2018-09-05-21.00.log.html

I'm retooling the proposal based on feedback. Key things:

• switching from loop to dispatch to avoid confusion about inner loop body vs outer
• removing exposure of the drain() method, instead you can set up runner contexts and run additional stuff after done.
• switching from interfaces with fixed method names to callbacks, allowing multiple work types
• using regular execute() method, with a helper fork() method (bikeshed that name!) that creates the controller, a dispatch callback, and the work/result callbacks

Considering also first-class support for multiple event types within a single go to avoid having to manually add it.

using regular execute() method, with a helper fork() method (bikeshed that name!) that creates the controller, a dispatch callback, and the work/result callbacks

Oh! I should call *that* $this->dispatch().

Updated summary with the reworked API for ParallelMaintenance.

(Still have to see if the exec mode can be got working, do a little debugging, and add a hook for closing extensions.)

Feedback question -- is the method name mapping from 'somethingWorker' to 'something' too clever? Should it just let you pass any method name to $controller->queue()?

Problem statement is clear. Proposal has been formed. No objections from TechCom and no stakeholders were identified beyond CPT and the various developers that have already participated so far, including during the IRC meeting.

Moving to our Inbox for next week. I will propose then that TechCom put this on Last Call with intent to approve 3 weeks from now.

Moving to last call per decision in today's TechCom meeting, looking to approve in 3 weeks.

In T201970#4569113, @brion wrote:

Feedback question -- is the method name mapping from 'somethingWorker' to 'something' too clever? Should it just let you pass any method name to $controller->queue()?

My preference would be latter. PHP doesn't have refactoring friendly way of referencing methods (unless you count [ Foo::class, 'methodname' ]) but being able to search the exact method name to see if it is used is a desired property. Apart from that, the interface seems good.

Another thought I had was whether this should be an interface (or a trait) instead of a class, in case subclassing is already used for something else -- but that seems quite rare.

+1. Looks like we'll have a 1-1 relation between ForkStreamController and ParallelMaintenance objects, as such we could presumably use the method name directly as event name without them being separate strings or concepts. That way the correlation is more obvious, e.g. $controller->queue( 'repeatWorker', 42 ); using the task description's example.

I do like the idea of enforcing the naming convention, though. In particular because it makes it easier to recognise in code that this method is "special" in that it doesn't share state with the parent and is meant to be called only by the Controller. Using a strictly-enforced naming convention is one way to do that. A few other ways that might work:

1. Require the user to bind the method explicitly.

public function execute() {
 $this->dispatch( function ( $controller ) {
   $controller->bindWorker( "repeat", "onRepeat" );
   …
   $controller->queue( "repeat", 42 );
 } );
}
public function onRepeat( int $len ) : string { … }

2. Or, require the method to have a certain annotation.

public function execute() {
 $this->dispatch( function ( $controller ) {
   …
   $controller->queue( "repeat", 42 );
 } );
}

/** @workerEvent repeat */
public function repeatWorker( int $len ) : string { … }

If this makes the overall boilerplate too long, we could potentially reduce that in turn from the other end. For example by making execute() already implemented and making the dispatch closure a pre-bound method, like so:

public function dispatcher( $controller ) { // called from final public ParallelMaintenance::execute
 $controller->bindWorker( "repeat", "onRepeat" );
 …
 $controller->queue( "repeat", 42 );
}

public function onRepeat( int $len ) : string { … }

public function dispatcher( $controller ) {
 …
 $controller->queue( "repeat", 42 );
}

/** @workerEvent repeat */
public function repeatWorker( int $len ) : string { … }

This RFC is approved after Last Call per today's TechCom meeting

Looks like a little bitrot has hit this patch, with the deprecation of resetServiceForTesting outside unit tests the closing of services ends up failing.

I'll see if I can either work around that or switch it to use the exec controller in place of the fork controller, which won't require shutting down connections from the parent process. Also will update the callback name binding per notes above.

Change 451099 had a related patch set uploaded (by Brion VIBBER; owner: Brion VIBBER):
[mediawiki/core@master] WIP: ParallelMaintenance, MediaWiki\Parallel helper classes

https://gerrit.wikimedia.org/r/451099

Seem to have worked around the deprecation issue by simply not shutting down the lock manager or file group, but will test more thoroughly to see if that breaks anything later...

For the API change, I've gone with an explicit bindWorker on ParallelMaintenence that takes the event name and a callable instead of automagic'ing anything.

This allows for using local methods easily enough but preserves search-and-replace and should work better for static analysis/refactoring tools. It does not enforce a particular naming convention, and could even use closures if one really desires though I would avoid it for legibility.

public __construct() {
  // general setup, then:
  $this->bindWorker( 'repeat', [ $this, 'repeatWorker' ] );
}

public repeatWorker( $data ) {
  return str_repeat( $data['str'], $data['count'] );
}

public execute() {
  $this->dispatch( function ( \MediaWiki\Parallel\IController $controller ) {
    $controller->queue( 'repeat', [ 'str' => '*', 'count' => 42 ], function ( $result ) {
      var_dump( $result );
    } );
  } );
}