Dump dependency container and re-use it - 35% faster test-suite by staabm · Pull Request #3809 · rectorphp/rector-src

staabm · 2023-05-12T07:38:37Z

Before

$  php vendor/bin/phpunit rules-tests/TypeDeclaration/Rector/
PHPUnit 10.1.3 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.5
Configuration: C:\dvl\Workspace\rector-src-staabm\phpunit.xml

...............................................................  63 / 417 ( 15%)
............................................................... 126 / 417 ( 30%)
............................................................... 189 / 417 ( 45%)
............................................................... 252 / 417 ( 60%)
............................................................... 315 / 417 ( 75%)
............................................................... 378 / 417 ( 90%)
.......................................                         417 / 417 (100%)

Time: 00:32.574, Memory: 1.13 GB

OK (417 tests, 475 assertions)

After

➜  rector-src git:(dump-container) php vendor/bin/phpunit rules-tests/TypeDeclaration/Rector/
PHPUnit 10.1.3 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.18
Configuration: /Users/staabm/workspace/rector-src/phpunit.xml

...............................................................  63 / 417 ( 15%)
............................................................... 126 / 417 ( 30%)
............................................................... 189 / 417 ( 45%)
............................................................... 252 / 417 ( 60%)
............................................................... 315 / 417 ( 75%)
............................................................... 378 / 417 ( 90%)
.......................................                         417 / 417 (100%)

Time: 00:08.483, Memory: 1.04 GB

OK (417 tests, 418 assertions)

running the whole test-suite it comes down to

Before php vendor/bin/phpunit 160.22s user 28.24s system 98% cpu 3:11.11 total
After php vendor/bin/phpunit 109.70s user 13.60s system 97% cpu 2:06.40 total

staabm · 2023-05-12T07:49:03Z

I wonder why these failling tests depend on ContainerBuilder instead of ContainerInterface. they are not meant to modify the container but just read from it, right?

it seems the PR works for all tests but e2e. ~~I don't have an idea yet why~~

seems these are a few use cases which require ContainerBuilder because they use some more in-depth methods which are not reqiured for regular rector end-user use-cases

.github/workflows/tests.yaml

staabm · 2023-05-12T09:10:44Z

the failling CI job is unrelated. created a upstream issue composer-unused/composer-unused#514

src/Kernel/RectorKernel.php

tests/Validation/Collector/EmptyConfigurableRectorCollector/ConfigurableArrayMissingTest.php

staabm · 2023-05-12T11:01:06Z

its reproducible a lot faster locally then before. In CI it seems it got slower then before.. :-/.. still investigatin

samsonasik

The commit with double quote: Revert "debug infos" should be changed to single quote or no quote to avoid error on build scoped.

staabm · 2023-05-12T14:22:17Z

in case this works in the near future I will squash everything together.

atm I don't know why it gets slower on CI, but gets faster on my mac and also my windows laptop :/

src/Kernel/RectorKernel.php

staabm · 2023-05-12T16:05:49Z

perf wise we reached the goal. we are equal fast in CI and locally. I have updated the benchmark in the description

src/Util/FileHasher.php

src/Util/Hasher.php

staabm · 2023-05-13T11:29:03Z

src/Kernel/RectorKernel.php

+    /**
+     * @var string
+     */
+    private const CACHE_KEY = 'kernel-v4';


when changing dependencies of registered services without also changing existing config files - like e.g. in #3817 -, one need to increase this key to invalidate existing permanent compiled DI containers

see e.g. https://github.com/rectorphp/rector-src/actions/runs/4966591084/jobs/8888109775 what happens when you forget to change the cache key

as soon as you have a need to adjust config files, cache will invalidate automatically

with ecf3dc6 I have added a new case, which automatically invalidates the compiled container caches, when exceptions are thrown while getting services out of the container - to improve DX so we don't need to update the RectorKernel::CACHE_KEY for most operations

Ok, I see this note

TomasVotruba · 2023-05-15T15:53:14Z

packages/Testing/PHPUnit/AbstractTestCase.php

+            // clear compiled container cache, to trigger re-discovery
+            RectorKernel::clearCache();
+
+            throw $e;
+        }


This seems hacky. What is it need for and when it happens? It might be hiding a bug we have

We can adress it later, to keep this PR merged 👍

TomasVotruba · 2023-05-15T15:54:00Z

src/Kernel/RectorKernel.php

+        return $container;
+    }
+
 }


Much better, thank you 👍

TomasVotruba · 2023-05-15T15:54:49Z

Let's ship it... thank you Markus for tremendous work on this one 👏

samsonasik · 2023-05-15T16:39:49Z

@staabm it seems cause error in tests when add new param to __construct, see https://github.com/rectorphp/rector-src/actions/runs/4982896946/jobs/8919207651#step:6:26

samsonasik · 2023-05-15T16:44:29Z

src/Kernel/RectorKernel.php

+    /**
+     * @var string
+     */
+    private const CACHE_KEY = 'v7';


should it be updated when add new param to __construct ? see error https://github.com/rectorphp/rector-src/actions/runs/4982917436/jobs/8919255393#step:6:26

There was 1 error: 1) Rector\Core\Tests\Issues\DowngradeNewInArrayInitializerPromotion\DowngradeNewInArrayInitializerPromotionTest::test with data set #0 ArgumentCountError: Too few arguments to function Rector\Core\PhpParser\Parser\InlineCodeParser::__construct(), 3 passed in /tmp/rector/kernel-v7-b923df1ea94cad9fb5b38891cbfaa566ed8e88d7e8551c1c3c978857f94fc876.php on line 3246 and exactly 4 expected

staabm · 2023-05-15T18:06:51Z

It will auto-repair on the next commit

see #3809 (comment)

samsonasik · 2023-05-15T18:19:12Z

It seems it not auto repair, I need to increase the key b4ad93c

samsonasik · 2023-05-15T19:39:17Z

@staabm here first commit that error: https://github.com/rectorphp/rector-src/actions/runs/4982896946/jobs/8919207651

here second commit that still error https://github.com/rectorphp/rector-src/actions/runs/4982917436/jobs/8919255393

so need to increate key cache to fix it b4ad93c

staabm · 2023-05-15T19:44:29Z

I will have a closer look tomorrow. Thx

samsonasik · 2023-05-15T20:06:46Z

@staabm it seems also make e2e test on rector/rector repo error, see https://github.com/rectorphp/rector/actions/runs/4982533394/jobs/8918337344

samsonasik · 2023-05-15T20:09:03Z

@staabm I tried "rector/rector": "dev-main#bd630a0db2db355484ee4d5a99850facb2323f37" in laminas-servicemanager-migration tests and got error as well:

➜  laminas-servicemanager-migration git:(1.13.x) ✗ vendor/bin/phpunit 
PHPUnit 9.6.8 by Sebastian Bergmann and contributors.

EEEEEEEEEEEEEEEEEEE                                               19 / 19 (100%)

Time: 00:00.039, Memory: 16.00 MB

There were 19 errors:

1) LaminasTest\ServiceManager\Migration\Rector\Class_\ImplementsFactoryInterfaceToPsrFactoryRector\AutoImportRenameUseTest::test with data set #0 ('/Users/samsonasik/www/laminas...hp.inc')
Error: Class "RectorPrefix202305\Symplify\SmartFileSystem\SmartFileSystem" not found

/Users/samsonasik/www/laminas/laminas-servicemanager-migration/vendor/rector/rector/src/Kernel/CachedContainerBuilder.php:40
/Users/samsonasik/www/laminas/laminas-servicemanager-migration/vendor/rector/rector/src/Kernel/RectorKernel.php:112
/Users/samsonasik/www/laminas/laminas-servicemanager-migration/vendor/rector/rector/src/Kernel/RectorKernel.php:55
/Users/samsonasik/www/laminas/laminas-servicemanager-migration/vendor/rector/rector/packages/Testing/PHPUnit/AbstractTestCase.php:38
/Users/samsonasik/www/laminas/laminas-servicemanager-migration/vendor/rector/rector/packages/Testing/PHPUnit/AbstractRectorTestCase.php:60

Step to reproduce:

git clone git@github.com:laminas/laminas-servicemanager-migration.git
cd laminas-servicemanager-migration
composer require rector/rector:dev-main#bd630a0db2db355484ee4d5a99850facb2323f37
vendor/bin/phpunit

samsonasik · 2023-05-15T20:13:20Z

For reference error since rectorphp/rector@bd630a0 build from this PR.

samsonasik · 2023-05-15T20:18:14Z

I create an issue for that so it tracked rectorphp/rector#7939

samsonasik · 2023-05-16T04:13:23Z

@staabm I created PR #3865 to fix phpunit error on using rector/rector:dev-main with verify VersionResolver::PACKAGE_VERSION still @package_version@

The increase cache_key is different issue to resolve tho :), I will let you check

staabm commented May 12, 2023

View reviewed changes

.github/workflows/tests.yaml Outdated Show resolved Hide resolved

staabm commented May 12, 2023

View reviewed changes

src/Kernel/RectorKernel.php Outdated Show resolved Hide resolved

staabm commented May 12, 2023

View reviewed changes

tests/Validation/Collector/EmptyConfigurableRectorCollector/ConfigurableArrayMissingTest.php Show resolved Hide resolved

staabm mentioned this pull request May 12, 2023

Fix WorkerCommandLineFactoryTest #3811

Merged

clxmstaab force-pushed the dump-container branch from 54d82fa to 9e41adb Compare May 12, 2023 11:01

staabm force-pushed the dump-container branch 2 times, most recently from a975cad to 814a7ca Compare May 12, 2023 14:02

samsonasik requested changes May 12, 2023

View reviewed changes

samsonasik reviewed May 12, 2023

View reviewed changes

src/Kernel/RectorKernel.php Outdated Show resolved Hide resolved

staabm changed the title ~~Dump dependency container and re-use it~~ Dump dependency container and re-use it - 35% faster test-suite May 12, 2023

staabm marked this pull request as ready for review May 12, 2023 16:48

staabm requested a review from TomasVotruba as a code owner May 12, 2023 16:48

staabm force-pushed the dump-container branch 2 times, most recently from 5b2b6f0 to 01bee16 Compare May 12, 2023 17:28

samsonasik approved these changes May 12, 2023

View reviewed changes

staabm commented May 13, 2023

View reviewed changes

src/Util/FileHasher.php Outdated Show resolved Hide resolved

staabm force-pushed the dump-container branch 2 times, most recently from 9ffc4d2 to 9c8656f Compare May 13, 2023 06:44

staabm marked this pull request as draft May 13, 2023 06:46

samsonasik reviewed May 13, 2023

View reviewed changes

src/Util/Hasher.php Outdated Show resolved Hide resolved

staabm force-pushed the dump-container branch from 9c8656f to 3f8b241 Compare May 13, 2023 11:13

staabm commented May 13, 2023

View reviewed changes

staabm marked this pull request as ready for review May 13, 2023 11:29

samsonasik approved these changes May 13, 2023

View reviewed changes

staabm force-pushed the dump-container branch from 7fe9d0d to c017e5b Compare May 13, 2023 11:57

staabm added 2 commits May 14, 2023 20:07

Discard changes to packages/Caching/Detector/ChangedFilesDetector.php

c538985

Discard changes to packages/Testing/PHPUnit/AbstractTestCase.php

a4f7e36

staabm marked this pull request as draft May 14, 2023 18:07

staabm and others added 4 commits May 14, 2023 20:08

Discard changes to rules/Renaming/NodeManipulator/ClassRenamer.php

4dc72e9

Delete FileHasher.php

178306f

Merge branch 'main' into dump-container

5655d4f

Update RectorKernel.php

0bbaaa3

staabm marked this pull request as ready for review May 14, 2023 18:31

staabm added 3 commits May 15, 2023 09:55

Automatically compiled containers on getService() exceptions

ecf3dc6

Merge branch 'main' into dump-container

41160ea

Merge branch 'main' into dump-container

32102e4

samsonasik approved these changes May 15, 2023

View reviewed changes

TomasVotruba reviewed May 15, 2023

View reviewed changes

TomasVotruba approved these changes May 15, 2023

View reviewed changes

src/Kernel/RectorKernel.php

return $container;

}

}

Copy link
Copy Markdown

Member

TomasVotruba May 15, 2023

Choose a reason for hiding this comment

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

Much better, thank you 👍

TomasVotruba approved these changes May 15, 2023

View reviewed changes

TomasVotruba merged commit 3bdd519 into rectorphp:main May 15, 2023

staabm deleted the dump-container branch May 15, 2023 15:57

samsonasik reviewed May 15, 2023

View reviewed changes

samsonasik mentioned this pull request May 15, 2023

rector/rector:dev-main cause phpunit error test rectorphp/rector#7939

Closed

Uh oh!

Conversation

staabm commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staabm commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

staabm commented May 12, 2023

Uh oh!

Uh oh!

Uh oh!

staabm commented May 12, 2023

Uh oh!

samsonasik left a comment

Choose a reason for hiding this comment

Uh oh!

staabm commented May 12, 2023

Uh oh!

Uh oh!

staabm commented May 12, 2023

Uh oh!

Uh oh!

Uh oh!

staabm May 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staabm May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samsonasik May 15, 2023

Choose a reason for hiding this comment

Uh oh!

TomasVotruba May 15, 2023

Choose a reason for hiding this comment

Uh oh!

TomasVotruba May 15, 2023

Choose a reason for hiding this comment

Uh oh!

TomasVotruba commented May 15, 2023

Uh oh!

samsonasik commented May 15, 2023

Uh oh!

samsonasik May 15, 2023

Choose a reason for hiding this comment

Uh oh!

staabm commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samsonasik commented May 15, 2023

Uh oh!

samsonasik commented May 15, 2023

Uh oh!

staabm commented May 15, 2023

Uh oh!

samsonasik commented May 15, 2023

Uh oh!

samsonasik commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samsonasik commented May 15, 2023

Uh oh!

samsonasik commented May 15, 2023

Uh oh!

samsonasik commented May 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

staabm commented May 12, 2023 •

edited

Loading

staabm commented May 12, 2023 •

edited

Loading

staabm May 13, 2023 •

edited

Loading

staabm May 15, 2023 •

edited

Loading

staabm commented May 15, 2023 •

edited

Loading

samsonasik commented May 15, 2023 •

edited

Loading