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
feat: add parallel cache support #7131
feat: add parallel cache support #7131
Conversation
src/Cache/FileHandler.php
Outdated
return; | ||
} | ||
|
||
if (getenv('PHP_CS_FIXER_EXPERIMENTAL_PARALLEL_CACHE')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idea for future (out of this PR scope) - we can detect that file was modified since it was read initially, and do the backilling based on that indicator, without env var.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implemented in 2aef0f8
ker@d ~/github/PHP-CS-Fixer λ ./php-cs-fixer list-files --config=.php-cs-fixer.dist.php | xargs -n 400 -P 3 ./php-cs-fixer fix --config=.php-cs-fixer.dist.php --path-mode intersection
Loaded config default from ".php-cs-fixer.dist.php".
Using cache file ".php-cs-fixer.cache".
Loaded config default from ".php-cs-fixer.dist.php".
Using cache file ".php-cs-fixer.cache".
Loaded config default from ".php-cs-fixer.dist.php".
Using cache file ".php-cs-fixer.cache".
string(34) "
73993 !!! FRS !!! READ 1689350710"
string(34) "
73994 !!! FRS !!! READ 1689350710"
string(34) "
73995 !!! FRS !!! READ 1689350710"
Fixed 0 of 212 files in 7.380 seconds, 20.000 MB memory used
string(31) "
73995 !!! FRS !!! DECISION ???"
array(3) {
["property"]=>
int(1689350710)
["os"]=>
int(1689350710)
["should backfill"]=>
bool(false)
}
string(28) "
73995 !!! FRS !!! SAVE FILE"
string(40) "
73995 !!! FRS !!! AFTER SAVE 1689350727"
Fixed 0 of 400 files in 23.354 seconds, 22.000 MB memory used
string(31) "
73993 !!! FRS !!! DECISION ???"
array(3) {
["property"]=>
int(1689350710)
["os"]=>
int(1689350727)
["should backfill"]=>
bool(true)
}
string(27) "
73993 !!! FRS !!! BACKFILL"
string(28) "
73993 !!! FRS !!! SAVE FILE"
string(40) "
73993 !!! FRS !!! AFTER SAVE 1689350743"
string(31) "
73994 !!! FRS !!! DECISION ???"
array(3) {
["property"]=>
int(1689350710)
["os"]=>
int(1689350743)
["should backfill"]=>
bool(true)
}
string(27) "
73994 !!! FRS !!! BACKFILL"
string(28) "
73994 !!! FRS !!! SAVE FILE"
string(40) "
73994 !!! FRS !!! AFTER SAVE 1689350748"
Fixed 0 of 400 files in 27.835 seconds, 22.000 MB memory used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if there should be internal logic for handling parallelisation in cache, when parallelisation itself is possible only using external tooling and we can't determine if it's used or not in current run.
doc/usage.rst
Outdated
export PHP_CS_FIXER_EXPERIMENTAL_PARALLEL_CACHE=1 | ||
php php-cs-fixer.phar list-files --config=.php-cs-fixer.dist.php | xargs -n 10 -P 8 php php-cs-fixer.phar fix --config=.php-cs-fixer.dist.php --path-mode intersection -v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export PHP_CS_FIXER_EXPERIMENTAL_PARALLEL_CACHE=1 | |
php php-cs-fixer.phar list-files --config=.php-cs-fixer.dist.php | xargs -n 10 -P 8 php php-cs-fixer.phar fix --config=.php-cs-fixer.dist.php --path-mode intersection -v | |
PHP_CS_FIXER_EXPERIMENTAL_PARALLEL_CACHE=1 php php-cs-fixer.phar list-files --config=.php-cs-fixer.dist.php | xargs -n 10 -P 8 php php-cs-fixer.phar fix --config=.php-cs-fixer.dist.php --path-mode intersection -v |
IMHO it shouldn't be exported, but defined only for this particular run, that is supported by external parallelisation. Exporting will keep that value and will use that feature also for regular run (without xargs
) and I am not sure if it should be like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var_dump(">>>", getenv('PHP_CS_FIXER_EXPERIMENTAL_PARALLEL_CACHE')); die;
your proposal is returning
string(3) ">>>"
bool(false)
some playground
ker@d ~/github/PHP-CS-Fixer λ cat x.php
<?php
echo json_encode([basename(__FILE__) => getenv('ENV_FOO')]);
ker@d ~/github/PHP-CS-Fixer λ cat y.php
<?php
echo json_encode([basename(__FILE__) => getenv('ENV_FOO')]);
ker@d ~/github/PHP-CS-Fixer λ ENV_FOO=TEST php x.php
{"x.php":"TEST"}
ker@d ~/github/PHP-CS-Fixer λ ENV_FOO=TEST php x.php | php y.php
{"y.php":false}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not thought about pipes and variable visibility. It should be something like FOO=bar bash -c 'somecommand someargs | somecommand2'
(source), in this case variable is available on each level of the pipeline. It makes it a little bit more complicated, but enables feature for single run instead of doing it globally. I won't fight for it, just a suggestion 🙂.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it makes it overcomplex and I'm ok to have env exported. I assume that one either will want it each time, or never - and if they need sometimes, they will figure out the way.
bash is also less portable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
solved in #7131 (comment)
ker@d ~/github/PHP-CS-Fixer λ ./php-cs-fixer list-files --config=.php-cs-fixer.dist.php | xargs -n 400 -P 3 ./php-cs-fixer fix --config=.php-cs-fixer.dist.php --path-mode intersection Loaded config default from .php-cs-fixer.dist.php. Using cache file .php-cs-fixer.cache. Loaded config default from .php-cs-fixer.dist.php. Using cache file .php-cs-fixer.cache. Loaded config default from .php-cs-fixer.dist.php. Using cache file .php-cs-fixer.cache. string(34)
src/Cache/FileHandler.php
Outdated
} | ||
var_dump("\n".getmypid().' !!! FRS !!! SAVE FILE'); | ||
ftruncate($handle, 0); | ||
fwrite($handle, $cache->toJson()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder about putting @
to silence warning and check !== false
for every method here... any better suggestion?
This reverts commit 10f2de7.
3233885
to
4d67ada
Compare
4d67ada
to
d25c533
Compare
This comment was marked as outdated.
This comment was marked as outdated.
go away stale bot, we look for brave reviewer |
d25c533
to
6a70a58
Compare
…RIMENTAL_PARALLEL_CACHE
6a70a58
to
5b227da
Compare
I don't review PRs with CI failures 😆. Unless it's PoC and I'm asked for the feedback 😉. |
Not saying who would be that reviewer ;) We were discussing idea internally and I opened it as POC, if we conclude on value to finish it (or not) - #7131 (review) - happy to continue. But not worth to continue with everything technical to conclude "nah, we do not like the idea itself". |
Also, it was green before you asked for solving this ;) |
efd8135
to
c9ee48a
Compare
3b4dc19
to
98a2c72
Compare
No description provided.