Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Try to parallelize linting with amphp #4838

Closed
wants to merge 11 commits into from
3 changes: 2 additions & 1 deletion composer.json
Expand Up @@ -14,9 +14,10 @@
}
],
"require": {
"php": "^5.6 || ^7.0",
"php": "^7.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will work on how the compat with old php can/should look like, after I get it working on current php versions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to see #4810.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't want PHP 5, then why do we even need a parallel linter? We can use the tokenising linter on PHP 7?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we even need a parallel linter? We can use the tokenising linter on PHP 7?

I had the theory that tokenized linting in parallel process is faster then doing it in a single process

"ext-json": "*",
"ext-tokenizer": "*",
"amphp/parallel": "^1.2",
staabm marked this conversation as resolved.
Show resolved Hide resolved
"composer/semver": "^1.4",
"composer/xdebug-handler": "^1.2",
"doctrine/annotations": "^1.2",
Expand Down
5 changes: 4 additions & 1 deletion src/Linter/Linter.php
Expand Up @@ -32,10 +32,13 @@ final class Linter implements LinterInterface
public function __construct($executable = null)
{
try {
$this->sublinter = new TokenizerLinter();
// XXX feature detect the pooled linter
$this->sublinter = new PooledLinter();
// $this->sublinter = new TokenizerLinter();
} catch (UnavailableLinterException $e) {
$this->sublinter = new ProcessLinter($executable);
}
var_dump(get_class($this->sublinter));
}

/**
Expand Down
94 changes: 94 additions & 0 deletions src/Linter/PooledLinter.php
@@ -0,0 +1,94 @@
<?php

/*
* This file is part of PHP CS Fixer.
*
* (c) Fabien Potencier <fabien@symfony.com>
* Dariusz Rumiński <dariusz.ruminski@gmail.com>
*
* This source file is subject to the MIT license that is bundled
* with this source code in the file LICENSE.
*/

namespace PhpCsFixer\Linter;

use Amp\Parallel\Worker\DefaultPool;
use Amp\Parallel\Worker\Environment;
use Amp\Parallel\Worker\Task;

class PooledLinter implements LinterInterface
{
private $processPool;

public function __construct()
{
$this->processPool = new DefaultPool();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would allow a Amp\Parallel\Worker\Pool instance to be passed via the constructor. If one isn't passed, use the one returned from Amp\Parallel\Worker\pool().

}

public function __destruct()
{
yield $this->processPool->shutdown();
}

/**
* {@inheritdoc}
*/
public function isAsync()
{
return true;
}

/**
* {@inheritdoc}
*/
public function lintFile($path)
{
return new PooledLintingResult($this->processPool->enqueue($this->createTaskForFile($path)));
}

/**
* {@inheritdoc}
*/
public function lintSource($source)
{
return new PooledLintingResult($this->processPool->enqueue($this->createTaskForSource($source)));
}

private function createTaskForFile($path) {
return new class($path) implements Task {
private $path;

public function __construct($path)
{
$this->path = $path;
}

/**
* @inheritDoc
*/
public function run(Environment $environment) {
$linter = new TokenizerLinter();
return $linter->lintFile($this->path);
}
};
}

private function createTaskForSource($source) {
return new class($source) implements Task {
private $source;

public function __construct($source)
{
$this->source = $source;
}

/**
* @inheritDoc
*/
public function run(Environment $environment) {
$linter = new TokenizerLinter();
return $linter->lintSource($this->source);
}
};
}
}
60 changes: 60 additions & 0 deletions src/Linter/PooledLintingResult.php
@@ -0,0 +1,60 @@
<?php

/*
* This file is part of PHP CS Fixer.
*
* (c) Fabien Potencier <fabien@symfony.com>
* Dariusz Rumiński <dariusz.ruminski@gmail.com>
*
* This source file is subject to the MIT license that is bundled
* with this source code in the file LICENSE.
*/

namespace PhpCsFixer\Linter;

use Amp\Parallel\Worker\Task;
use Amp\Promise;
use Symfony\Component\Process\Process;

/**
* @internal
*/
final class PooledLintingResult implements LintingResultInterface
{
/**
* @var bool
*/
private $isSuccessful;

/**
* @var Promise
*/
private $promise;

public function __construct(Promise $promise)
{
$this->promise = $promise;
}

/**
* {@inheritdoc}
*/
public function check()
Copy link
Contributor Author

@staabm staabm Feb 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should return :void

{
if (!$this->isSuccessful()) {
// on some systems stderr is used, but on others, it's not
throw new LintingException('error 213', 890);
//throw new LintingException($this->process->getErrorOutput() ?: $this->process->getOutput(), $this->process->getExitCode());
}
}


private function isSuccessful()
{
if (null === $this->isSuccessful) {
$this->isSuccessful = yield $this->promise;
}

return $this->isSuccessful;
}
}