Sync FileCacheStorage with changes from phpstan-src #498

staabm · 2021-07-23T13:22:44Z

as can be seen in #461 (comment) the FileCacheStorage is a major bottleneck while running tests on windows - I guess the same is true on other OSes.

This PR removes unnecessary temp-file handling, because smartFileSystem->dumpFile is already a atomic operation which does magic temp-path file handling behind the scenes. therefore we don't need to do the same things on the rector side.

TomasVotruba · 2021-07-23T13:33:29Z

Thanks for the PR 👍

Could you share some basic performance comparison before/after? Just so we have an idea how "much"

staabm · 2021-07-23T13:41:30Z

I can see a 25-35% perf improvement on my workstation, depending on load.

TomasVotruba · 2021-07-23T16:24:58Z

Current design is based on cache in PHPStan, and I would like to keep it as close the original to make the maintenance easier. So I want to be sure it has added value, before we change the design.

I've checked the numbers for CI here and in another PR and there is no perf change. Could you demonstrate it in Github Actions I could check?

staabm · 2021-07-23T23:59:02Z

Current design is based on cache in PHPStan, and I would like to keep it as close the original to make the maintenance easier

on phpstan side this class evolved already and does no longer use a smartFileSystem. This might also fix the same problem this PR is trying to fix.

I've checked the numbers for CI here and in another PR and there is no perf change.

Also made this observation. I guess the optimization kicks in when doing another run (which doesn‘t happen in CI because we always start from scratch); but might still be usefull for local test runs.

Any idea how to best reproduce CI like performance? Any way to clear all caches or similar?

Could you demonstrate it in Github Actions I could check?

will reproduce on monday when back at the workstation

TomasVotruba · 2021-07-24T00:25:38Z

on phpstan side this class evolved already and does no longer use a smartFileSystem. This might also fix the same problem this PR is trying to fix.

Could you share the diff?

Any idea how to best reproduce CI like performance? Any way to clear all caches or similar?

I would simply run the CI and compare final times before and after your change. Nothing special.

will reproduce on monday when back at the workstation

👍

staabm · 2021-07-24T07:12:11Z

Side idea: maybe it would make sense to have a in-memory CacheStorage for CI to get rid of slow IO

TomasVotruba · 2021-07-24T11:40:23Z

If the numbers are better, why not :)

staabm · 2021-07-25T09:13:14Z

Phpstan upstream version alread contains a CacheInterface and a in-memory impl
https://github.com/phpstan/phpstan-src/tree/master/src/Cache

in other words: using the latest upstream impl should provide everything we need. It seems phpstan even uses this classes internally exactly like proposed

staabm · 2021-07-26T08:16:12Z

Current design is based on cache in PHPStan, and I would like to keep it as close the original to make the maintenance easier. So I want to be sure it has added value, before we change the design.

I just updated the implementation to be in line with the phpstan-src upstream FileCacheStorage.

using a in-memory impl to speedup CI is a possible followup.

I would simply run the CI and compare final times before and after your change. Nothing special.

I compared a few builds. the numbers are highly insconsistent, so its pretty hard to tell whether the testsuite is fast/slow because of a change within a PR or whether there is some other external factor which makes the numbers change.

from a logical point of view with the changed FileCacheStorage a lot less file-renaming is going on.
this is also visible from blackfire profiles.

the build is currently failing because of "style" problems:

  ------------------------------------------------------------------------
  packages/Caching/ValueObject/Storage/FileCacheStorage.php:29
 ------------------------------------------------------------------------
  - '#"@\\mkdir\(\$directory, 0777\)" is forbidden to use#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  packages/Caching/ValueObject/Storage/FileCacheStorage.php:36
 ------------------------------------------------------------------------
  - '#Use custom exceptions instead of native "InvalidArgumentException"#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  packages/Caching/ValueObject/Storage/FileCacheStorage.php:47
 ------------------------------------------------------------------------
  - '#Use explicit names over dynamic ones#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  packages/Caching/ValueObject/Storage/FileCacheStorage.php:48
 ------------------------------------------------------------------------
  - '#Array destruct is not allowed\. Use value object to pass data instead#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  packages/Caching/ValueObject/Storage/FileCacheStorage.php:71
 ------------------------------------------------------------------------
  - '#Array destruct is not allowed\. Use value object to pass data instead#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  packages/Caching/ValueObject/Storage/FileCacheStorage.php:77
 ------------------------------------------------------------------------
  - '#"@\\var_export\(new \\Rector\\Caching\\ValueObject\\CacheItem\(\$variableKey, \$data\), \\true\)" is forbidden to use#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  packages/Caching/ValueObject/Storage/FileCacheStorage.php:80
 ------------------------------------------------------------------------
  - '#"%s" in sprintf\(\) format must be quoted#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  packages/Caching/ValueObject/Storage/FileCacheStorage.php:83
 ------------------------------------------------------------------------
  - '#"@\\rename\(\$tmpPath, \$path\)" is forbidden to use#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  packages/Caching/ValueObject/Storage/FileCacheStorage.php:87
 ------------------------------------------------------------------------
  - '#"@\\unlink\(\$tmpPath\)" is forbidden to use#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  packages/Caching/ValueObject/Storage/FileCacheStorage.php:89
 ------------------------------------------------------------------------
  - '#Use custom exceptions instead of native "InvalidArgumentException"#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  packages/Caching/ValueObject/Storage/FileCacheStorage.php:95
 ------------------------------------------------------------------------
  - '#Array destruct is not allowed\. Use value object to pass data instead#'
 ------------------------------------------------------------------------

since you wanted to be in line with phpstan-src I like to ask, which reported problems should be solved (and therefore will create differences between rector-src FileCacheStorage and phpstan-src FileCacheStorage )?

which can/should be phpstan-ignored?

staabm · 2021-07-26T08:21:43Z

packages/Caching/ValueObject/Storage/FileCacheStorage.php

@@ -23,70 +21,83 @@ public function __construct(
    ) {


where is the $directory configured which is used? when changing the impl we should add a v2 or similar so persisted cache information does not conflict with the new impl

Is this question still relevant or outdated? The original commit had changed

staabm · 2021-07-26T08:28:51Z

the adjusted implementation leads to ~200 less rename calls and a 10% speedup (32% of overall the test-runtime is spent on renaming files)

when running blackfire run --ignore-exit-status .\vendor\bin\phpunit .\packages-tests\

packages/Caching/ValueObject/Storage/FileCacheStorage.php

TomasVotruba · 2021-07-26T10:57:05Z

Could you check last PHPStan fails? I think it's ready then

staabm · 2021-07-26T11:56:28Z

@TomasVotruba should be good to go

phpstan.neon

samsonasik · 2021-07-26T11:59:40Z

packages/Caching/ValueObject/Storage/FileCacheStorage.php

+            return;
+        }
+        @\unlink($tmpPath);
+        if (\DIRECTORY_SEPARATOR === '/' || !\file_exists($path)) {


have you tried in different OS ? DIRECTORY_SEPARATOR is / in macOS:

➜ ~ php -a Interactive shell php > echo DIRECTORY_SEPARATOR; / php > var_dump(\DIRECTORY_SEPARATOR === '/'); bool(true)

Above code will make always throw exception.

this implementation is taken 1:1 from phpstan.

I guess this condition is meant as a non-windows feature detect.
As this class - as is - works in phpstan for months without further bugfixes, I feel this is good enough.

That possibly a dead code then, true || false will result true, true || true will result true, see https://3v4l.org/pKcY1

I can't tell the details why phpstan has implemented it like that.

Some comment would be useful so we don't use this blindly in the future

samsonasik · 2021-07-26T12:25:58Z

Unrelated to the PR, could you use same user as PR, eg PR from @clxmstaab use @clxmstaab in commit, or @staabm use @staabm in next PR ?

staabm · 2021-07-26T12:28:45Z

Unrelated to the PR, could you use same user as PR, eg PR from @clxmstaab use @clxmstaab in commit, or @staabm use @staabm in next PR ?

I am working on different workstations with different users. why is this "mix" of user/submitter a problem?

In case I could only work from one of these computers my feedback/contribution loop would get longer.

samsonasik · 2021-07-26T12:33:06Z

I see. Just to make easier to verify that the commit is from the PR author so it can be verified to the commit author if there is something that need to be clarified.

TomasVotruba · 2021-07-26T12:57:30Z

packages/Caching/ValueObject/Storage/FileCacheStorage.php

+                return null;
+            }
+            return $cacheItem->getData();
+        })($key, $variableKey);


This is very confusing to read. Not sure if args are on left or right.
Could you extract function to assign line and invoke it on 2nd?

its the way how phpstan does this things.

all these nitpicks are great, but it goes against intially mentioned goal:

Current design is based on cache in PHPStan, and I would like to keep it as close the original to make the maintenance easier.

after we apply all 'rector rules' the diff between the phpstan impl and ours here gets bigger and bigger.

Ok, you're right. Let's give it a go

fwiw, I opened a upstream PR to simplify this on phpstan-src

phpstan/phpstan-src#593

Cool! 👍

This is too much magic for me. I understand only objects and methods :D

TomasVotruba · 2021-07-26T12:58:22Z

packages/Caching/ValueObject/Storage/FileCacheStorage.php

     * @param mixed $data
+     * @return void


It seems these duplicate the PHP code

TomasVotruba · 2021-07-26T12:59:19Z

packages/Caching/ValueObject/Storage/FileCacheStorage.php

-        $firstDirectory = sprintf('%s/%s', $this->directory, Strings::substring($keyHash, 0, 2));
-        $secondDirectory = sprintf('%s/%s', $firstDirectory, Strings::substring($keyHash, 2, 2));
+        $firstDirectory = sprintf('%s/%s', $this->directory, substr($keyHash, 0, 2));
+        $secondDirectory = sprintf('%s/%s', $firstDirectory, substr($keyHash, 2, 2));


TomasVotruba · 2021-07-26T13:16:42Z

Thank you Marcus 👍

staabm changed the title ~~Perf optimize FileCacheStorage~~ Sync FileCacheStorage with changes from phpstan-src Jul 26, 2021

clxmstaab added 5 commits July 26, 2021 09:56

perf optimize FileCacheStorage

ea36aec

sync FIleCacheStorage impl with upstream phpstan

fa3b02f

update comment

5edf306

Update FileCacheStorage.php

8e848cc

use Rector\Caching\ValueObject\CacheItem

6823d1a

clxmstaab force-pushed the cleanup-file-cache-storage branch from b6eaf6f to 6823d1a Compare July 26, 2021 07:56

staabm commented Jul 26, 2021

View reviewed changes

packages/Caching/ValueObject/Storage/FileCacheStorage.php Show resolved Hide resolved

clxmstaab added 9 commits July 26, 2021 13:23

fix phpstan: use CachingException

e1d0fee

fix phpstan: use smartFileSystem

0ccb973

re-added nearly everything again

1eb741b

Update FileCacheStorage.php

97b01b6

remove String::substring()

08f64a7

Update FileCacheStorage.php

b0be077

ignore phpstan errors

38da1ee

fix path

cd54f0c

Update phpstan.neon

3344e4a

staabm commented Jul 26, 2021

View reviewed changes

phpstan.neon Show resolved Hide resolved

samsonasik reviewed Jul 26, 2021

View reviewed changes

TomasVotruba reviewed Jul 26, 2021

View reviewed changes

TomasVotruba merged commit 6478ec1 into rectorphp:main Jul 26, 2021

staabm deleted the cleanup-file-cache-storage branch July 26, 2021 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync FileCacheStorage with changes from phpstan-src #498

Sync FileCacheStorage with changes from phpstan-src #498

staabm commented Jul 23, 2021

TomasVotruba commented Jul 23, 2021

staabm commented Jul 23, 2021 •

edited

TomasVotruba commented Jul 23, 2021

staabm commented Jul 23, 2021 •

edited

TomasVotruba commented Jul 24, 2021

staabm commented Jul 24, 2021

TomasVotruba commented Jul 24, 2021

staabm commented Jul 25, 2021

staabm commented Jul 26, 2021

staabm Jul 26, 2021

TomasVotruba Jul 26, 2021

staabm commented Jul 26, 2021 •

edited

TomasVotruba commented Jul 26, 2021

staabm commented Jul 26, 2021

samsonasik Jul 26, 2021 •

edited

staabm Jul 26, 2021 •

edited

samsonasik Jul 26, 2021

staabm Jul 26, 2021

TomasVotruba Jul 26, 2021

samsonasik commented Jul 26, 2021

staabm commented Jul 26, 2021

samsonasik commented Jul 26, 2021

TomasVotruba Jul 26, 2021

staabm Jul 26, 2021

TomasVotruba Jul 26, 2021

staabm Jul 26, 2021

TomasVotruba Jul 26, 2021

TomasVotruba Jul 26, 2021

TomasVotruba Jul 26, 2021

TomasVotruba commented Jul 26, 2021

Sync FileCacheStorage with changes from phpstan-src #498

Sync FileCacheStorage with changes from phpstan-src #498

Conversation

staabm commented Jul 23, 2021

TomasVotruba commented Jul 23, 2021

staabm commented Jul 23, 2021 • edited

TomasVotruba commented Jul 23, 2021

staabm commented Jul 23, 2021 • edited

TomasVotruba commented Jul 24, 2021

staabm commented Jul 24, 2021

TomasVotruba commented Jul 24, 2021

staabm commented Jul 25, 2021

staabm commented Jul 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm commented Jul 26, 2021 • edited

TomasVotruba commented Jul 26, 2021

staabm commented Jul 26, 2021

samsonasik Jul 26, 2021 • edited

Choose a reason for hiding this comment

staabm Jul 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsonasik commented Jul 26, 2021

staabm commented Jul 26, 2021

samsonasik commented Jul 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba commented Jul 26, 2021

staabm commented Jul 23, 2021 •

edited

staabm commented Jul 23, 2021 •

edited

staabm commented Jul 26, 2021 •

edited

samsonasik Jul 26, 2021 •

edited

staabm Jul 26, 2021 •

edited