-
-
Notifications
You must be signed in to change notification settings - Fork 108
perf: Improve performance of Box::collectRemainingFiles()#1560
perf: Improve performance of Box::collectRemainingFiles()#1560theofidry merged 1 commit intobox-project:mainfrom
Conversation
The ->notPath() filter of Symfony Finder is either accepting a "literal path" or a regular expression and converts each literal path to a regular expression and then filters the file stream by applying each regex individually. This leads to O(bufferedFileNames^2) calls to preg_match(), well over 20 million for PHPStan.
Fix this by performing a simple in_array() check on the buffered files. This brings down the time to compile PHPStan from 2:35 to 0:25 on my machine.
I have verified that the resulting PHPStan Phar hash is identical before and after this change.
Fixes #1551.
Tideways Comparison showing the improvement: https://app.tideways.io/share/trace/55089586-d71b-4fe5-a00a-08482ad2df06/compare/3abe5b8b-a960-4c0a-9c43-437a3dc23630/callgraph (link valid for 1 month, screenshot for posterity).
or a regular expression and converts each literal path to a regular expression
and then filters the file stream by applying each regex individually. This
leads to O(bufferedFileNames^2) calls to `preg_match()`, well over 20 million
for PHPStan.
Fix this by performing a simple `in_array()` check on the buffered files. This
brings down the time to compile PHPStan from 2:35 to 0:25 on my machine.
I have verified that the resulting PHPStan Phar hash is identical before and
after this change.
Fixes #1551.
e449e70 to
36860ae
Compare
|
Wow I didn't realise that, awesome, thanks! |
|
Wild. Appreciate this, Tim. @theofidry, you still keen to keep this one behind a feature flag now we have this improvement in place? I don't mind either way but happy to remove the need for the flag if we feel this is now in a better spot. |
|
If we're confident the perf impact is <10-20% I'm fine with making it the default and making the existing flag a noop option. Actually this could already be assessed without any other changes. We know have a few projects for which we know the impact was quite big and for which we can compare the build with and without the flag. |