Page MenuHomePhabricator

Parallelize substages of `arc diff`
Open, NormalPublic

Description

See PHI84. See T4281 for earlier context. See D18606 for some additional discussion.

When we run arc diff, we typically spend time doing these things, more or less sequentially:

  • Prompting the user to answer various questions.
  • Launching $EDITOR to ask the user for a commit message or update message.
  • Running linters (roughly arc lint).
  • Running unit tests (roughly arc unit).

Currently, these steps run sequentially. In theory, much of this work can happen in parallel instead.

T4281 was an earlier effort to parallelize some of this work. It relied on using the parent process as a sort of server, and the subprocesses as sort of clients, and letting the "server" give the "clients" access to the terminal when they needed to prompt the user. This was very "clever" and also very fragile and hard to debug (i.e., users reported unreproducible hangs until I ripped the whole thing out). Although we may have fixed some of the problems with this model in the meantime, some of the problems are also fairly fundamental.

Currently, there are some legitimate dependencies between these steps. Some of these we can likely extract by adjusting how the workflow works; others may require more finesse.

In particular, lint can emit any kind of patch, and may instruct arc to edit files in a way which changes their behavior (for example, you can write a linter which replaces every file with a haiku about pasta). If lint modifies files, unit tests which passed before the modifications may fail after the modifications. If we run lint and unit in parallel, then apply the lint fixes, and do not re-run unit tests, we may upload a bad change with metadata that says "tests pass". Realistically, it is generally safe to assume that lint does not break unit tests, but we can't be certain this is true in the general case.

A couple of general technical capability questions:

  • Can we actually write a PhutilConfirmFuture which doesn't block? (The answer should be "yes".)
  • Can we avoid filling the output buffer in subprocesses by testing if writes to stdout would block? (This should also be a "yes".)

I broadly expect to:

  • Restructure lint and unit so they can operate in a --subprocess sort of mode which just hands back the results without acting on them.
  • Do stdout testing in those subprocesses to prevent stalling on the stdout buffer.
  • Keep all command-and-control logic in the main process. We can switch this to futures where it makes sense, but if the subprocesses don't block this doesn't really matter.

As with other infrastructure changes, this is probably 30 diffs that do nothing and then five lines of actual changes.

I think this is not directly adjacent to other planned arc refactoring, although at least some amount of cleanup is likely inevitable and that should further the cause of T10038, etc.

Event Timeline

Can we avoid filling the output buffer in subprocesses by testing if writes to stdout would block? (This should also be a "yes".)

This seems to work as expected. I wrote a block.php and a work.php:

block.php
<?php

while (true) {
  sleep(1);
}
work.php
<?php

require_once 'scripts/init/init-script.php';

$nonblocking = !empty($argv[1]);

if ($nonblocking) {
  $stdout = fopen('php://stdout', 'wb');
  stream_set_blocking($stdout, false);
}

while (true) {
  $work = str_repeat('x', 4096);
  if ($nonblocking) {
    phutil_fwrite_nonblocking_stream($stdout, $work);
  } else {
    echo $work;
  }

  fprintf(STDERR, '.');
}

When invoked as php -f work.php | php -f block.php, a few dots are printed out and then things hang forever, as expected: the stdout buffer fills up and the work.php process blocks in echo.

When invoked as php -f work.php -- 1 | php -f block.php to enable the nonblocking test, work does not hang.

When either variant of work.php is run standalone, their behavior is identical.

So we should be able to do something like this in lint and unit subprocesses to let them keep working even if the parent process is busy prompting the user in a blocking way or waiting for $EDITOR to exit, without needing a client/server level of cleverness.

ftdysa added a subscriber: ftdysa.Sep 26 2017, 5:11 PM

I think I'm going to land this stuff on the experimental branch since there's also some other related work which is better targeted there, and things are probably going to get worse before they get better.

sakura added a subscriber: sakura.Oct 2 2017, 8:09 PM
alexmv added a subscriber: alexmv.Jan 16 2018, 4:16 PM
scp awarded a token.Jun 27 2018, 4:20 PM
scp added a subscriber: scp.
epriestley moved this task from Backlog to vNext on the Arcanist board.Sep 24 2018, 3:35 PM