Add PhpArrayAdapter to cache metadata #1196

ossinkine · 2020-08-20T16:38:17Z

Closes #1186

cc @ostrolucky @dmaicher @nicolas-grekas

I see two weak places in this implementation:

DoctrineMetadataCacheWarmer extends AbstractPhpFileCacheWarmer which also extended by ValidatorCacheWarmer, SerializerCacheWarmer and AnnotationsCacheWarmer (everywhere PhpArrayAdapter used) but it's marked as internal, so BC breaks possible here. I can avoid extending this class but a lot of code will be copy-pasted.
Since PhpArrayAdapter requires Symfony cache I wrap Doctrine cache with DoctrineAdapter. If Doctrine cache is DoctrineProvider which wrap a Symfony cache we get a redudant chain of adapters. For example the chain PhpArrayAdapter -> DoctrineAdapter -> DoctrineProvider -> RedisAdapter can be simplified by removing DoctrineAdapter and DoctrineProvider: PhpArrayAdapter -> RedisAdapter. I've tried to create a compiler pass which remove this redundancy but get list of entity managers in compiler pass a little bit difficult. And maybe we can implement this simplification in Symfony framework bundle globally for all such cache chains.

What is your opinion about this?

nicolas-grekas · 2020-08-25T10:35:35Z

AbstractPhpFileCacheWarmer is internal

That can be changed, PR welcome on symfony/symfony to discuss it there.

DoctrineAdapter -> DoctrineProvider

This would be better implemented on the bundle imho, as that would allow releasing the logic in sync with the feature provided here.

…ic (ossinkine) This PR was submitted for the 3.4 branch but it was merged into the 5.2-dev branch instead. Discussion ---------- [FrameworkBundle] Make AbstractPhpFileCacheWarmer public | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | Deprecations? | no | License | MIT Introducing the PhpArrayAdapter was a great feature. It has proven itself well in the production. Actually, using the adapter is closely related to warming up the cache for it. FrameworkBundle has an AbstractPhpFileCacheWarmer for conveniently creating a custom warmer. But using it outside of the FrameworkBundle becomes more complicated because it is marked as internal. I propose making it public and allow it to be used outside of the FrameworkBundle. For example, I've proposed to cache Doctrine metadata using PhpArrayAdapter and the implementation uses AbstractPhpFileCacheWarmer doctrine/DoctrineBundle#1196 Commits ------- a0fb442 Make AbstractPhpFileCacheWarmer public

ostrolucky · 2020-08-29T11:48:30Z

DependencyInjection/DoctrineExtension.php

+        $cacheWarmerDefinition->addTag('kernel.cache_warmer');
+    }
+
+    private function registerMetadataPhpArrayCache(string $entityManagerName, ContainerBuilder $container): void


I would personally prefer to not unnecessarily register new service if its only purpose is to inject it to warmer. Service should be instead just inlined, IMHO. Otherwise we are unnecessarily increasing public API surface.

doctrine.orm.default_metadata_cache is the alias which refers to the cache service. How can I create inline service and refer the alias to it?

ostrolucky · 2020-08-29T11:51:18Z

DependencyInjection/Compiler/PhpArrayCachePass.php

+/**
+ * Removes redundant decorators such DoctrineAdapter -> DoctrineProvider -> AdapterInterface.
+ */
+class PhpArrayCachePass implements CompilerPassInterface


We would prefer not having to maintain this in bundle forever. Can you please contribute this to Symfony core too? That way you will also get better feedback, as I'm not exactly confident about these internals.

As I understand @nicolas-grekas has another opinion. Since this is just optimization we can merge without it.

Nicolas didn't mean it doesn't make sense to contribute to Symfony, he said it needs to be merged to doctrine bundle anyways, as otherwise this feature would be available in next minor Symfony version only. But yeah I am also ok with just removing it for now, would make it easier to merge

ostrolucky

Tests are DI only, very simplistic. Can we have test which asserts that cache is hit when loading metadata and not generated again? Also, can you provide some numbers so everyone can see what's the expected improvement?

Also, we probably need to mark a test as legacy so CI passes.
And of course, needs to be rebased as it has conflicts.

ossinkine · 2020-08-31T09:11:37Z

Tests don't passed because AbstractPhpFileCacheWarmer marked as internal, @group legacy does not hide the errors

nicolas-grekas · 2020-08-31T09:13:09Z

Hmm, I didn't consider this aspect.
I'd be fine removing the annotation on branch 3.4 (4.4?) on Symfony.

@nicolas-grekas

… (ossinkine) This PR was merged into the 3.4 branch. Discussion ---------- [FrameworkBundle] Make AbstractPhpFileCacheWarmer public | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | Deprecations? | no | License | MIT The same as #37951 but with 3.4 as base branch, see doctrine/DoctrineBundle#1196 (comment) cc @nicolas-grekas Commits ------- b82d9a2 Make AbstractPhpFileCacheWarmer public

ostrolucky · 2020-10-26T19:38:00Z

Any chance for finishing this @ossinkine?

ossinkine · 2020-10-26T19:44:43Z

@ostrolucky I can remove PhpArrayCachePass which optimizes cache adapter chaining right now and later publish PR to Symfony but I don't what tests you requested and I don't how much time it will take

ostrolucky · 2020-10-26T19:50:50Z

Sounds good enough to me!

ossinkine · 2020-10-27T15:33:09Z

@ostrolucky I've removed the commit with optimizations, rebased the branch from the master and fixed code style issue except the one, I think proposed fix is a bad idea

ostrolucky · 2020-10-31T22:04:42Z

How about this. Does that work for you @ossinkine?

stof · 2020-10-31T22:27:42Z

get list of entity managers in compiler pass a little bit difficult.

this could actually be simple. We can add a tag on the services and use that tag in the compiler pass

DependencyInjection/Configuration.php

Tests/DependencyInjection/DoctrineExtensionTest.php

dmaicher · 2020-11-01T09:36:48Z

DependencyInjection/Configuration.php

@@ -688,6 +688,13 @@ private function getOrmCacheDriverNode(string $name): ArrayNodeDefinition
                ->scalarNode('pool')->end()
            ->end();

+        if ($name === 'metadata_cache_driver') {


this means we also need to create a new recipe for 2.2?

See https://github.com/symfony/recipes/blob/master/doctrine/doctrine-bundle/1.12/config/packages/prod/doctrine.yaml#L4 (it's used for 2.x currently as well via symlink)

ossinkine · 2020-11-01T17:42:24Z

@ostrolucky since PHP Array Cache adapter is a decorator for another adapter I'm not sure metadata_cache_driver should be depreacated. Or you suppose there can't be cache misses?

ostrolucky · 2020-11-01T22:06:04Z

If PHP Array Cache adapter is a decorator for another adapter, what happens right now when metadata_cache_driver is not set? I think ArrayAdapter is used then, so we should probably make sure same continues happening in 3.0.

greg0ire

Please state the obvious. It would be great not to have to read several issues and PRs on different repositories to understand this.

DependencyInjection/Configuration.php

DependencyInjection/DoctrineExtension.php

We decorate the existing metadata cache of each entity manager with a PHP Array Cache when not in debug mode. That cache is warmed up by getting all the metadata for all entities.

Since metadata is not supposed to be discovered at runtime, the fallback cache, which is the one configured through this key, is never supposed to be used. There is no point in configuring it.

greg0ire

I pushed new commits with explanations that might be completely wrong, please review and amend them if necessary.

stephanvierkant · 2020-11-10T11:51:36Z

It isn't clear to me what I've to change in my application and I'm not the only one: https://stackoverflow.com/questions/64741823/symfony-4-4-metadata-cache-driver-configuration-key-deprecation-notice

Can someone explain why this change was needed and how we should replace metadata_cache_driver? If I understand, I can update the Symfony docs (if needed).

ostrolucky · 2020-11-10T11:56:30Z

metadata_cache_driver configuration needs to be removed. Change is needed because defining own metadata_cache_driver is useless from now on.

dmaicher · 2020-11-10T18:35:41Z

DependencyInjection/DoctrineExtension.php

+        $phpArrayCacheDecoratorServiceId = $decoratedMetadataCacheServiceId . '.php_array';
+        $phpArrayFile                    = '%kernel.cache_dir%' . sprintf('/doctrine/orm/%s_metadata.php', $entityManagerName);
+
+        $container->register(DoctrineMetadataCacheWarmer::class)


@ossinkine actually this is not working properly for multiple entity managers. I just tried it on one of my bigger projects that uses 2 managers.

Since DoctrineMetadataCacheWarmer::class is the service id it will only work for the last manager and overwrite any previous definitions

ossinkine force-pushed the issue-1186 branch from 7813b6a to 1f069b9 Compare August 20, 2020 17:09

ossinkine mentioned this pull request Aug 26, 2020

[FrameworkBundle] Make AbstractPhpFileCacheWarmer public symfony/symfony#37951

Merged

ostrolucky reviewed Aug 29, 2020

View reviewed changes

ostrolucky requested changes Aug 29, 2020

View reviewed changes

ostrolucky added the Status: Needs Work label Aug 29, 2020

ossinkine force-pushed the issue-1186 branch 2 times, most recently from b66fa48 to 66a7930 Compare August 31, 2020 09:03

ossinkine mentioned this pull request Sep 2, 2020

[FrameworkBundle] Make AbstractPhpFileCacheWarmer public symfony/symfony#38036

Merged

ostrolucky linked an issue Oct 26, 2020 that may be closed by this pull request

Add PhpArrayAdapter to cache metadata #1186

Closed

ostrolucky added the Status: Waiting feedback label Oct 26, 2020

ossinkine force-pushed the issue-1186 branch 3 times, most recently from 092c8dc to 7d2d76d Compare October 27, 2020 15:15

ostrolucky removed Status: Waiting feedback Status: Needs Work labels Oct 27, 2020

ossinkine force-pushed the issue-1186 branch 3 times, most recently from c88e9d2 to bb74383 Compare October 31, 2020 13:45

ostrolucky force-pushed the issue-1186 branch from 7b28e7e to c925eed Compare October 31, 2020 21:59

ostrolucky added this to the 2.2.0 milestone Oct 31, 2020

ostrolucky force-pushed the issue-1186 branch from c925eed to 58f56fe Compare October 31, 2020 22:08

stof reviewed Oct 31, 2020

View reviewed changes

DependencyInjection/Configuration.php Outdated Show resolved Hide resolved

Tests/DependencyInjection/DoctrineExtensionTest.php Outdated Show resolved Hide resolved

ostrolucky force-pushed the issue-1186 branch from 58f56fe to f1a5aca Compare October 31, 2020 23:52

dmaicher reviewed Nov 1, 2020

View reviewed changes

ostrolucky force-pushed the issue-1186 branch from f1a5aca to 160b5c1 Compare November 1, 2020 23:02

ostrolucky requested review from alcaeus and greg0ire November 3, 2020 12:07

greg0ire reviewed Nov 4, 2020

View reviewed changes

DependencyInjection/Configuration.php Show resolved Hide resolved

DependencyInjection/DoctrineExtension.php Show resolved Hide resolved

ossinkine and others added 2 commits November 4, 2020 19:52

Add PhpArrayAdapter to cache metadata

728a3b8

We decorate the existing metadata cache of each entity manager with a PHP Array Cache when not in debug mode. That cache is warmed up by getting all the metadata for all entities.

Deprecate "metadata_cache_driver" configuration key

3e2946e

Since metadata is not supposed to be discovered at runtime, the fallback cache, which is the one configured through this key, is never supposed to be used. There is no point in configuring it.

greg0ire force-pushed the issue-1186 branch from 160b5c1 to 3e2946e Compare November 4, 2020 19:10

greg0ire approved these changes Nov 4, 2020

View reviewed changes

ostrolucky merged commit ae25798 into doctrine:master Nov 7, 2020

ostrolucky mentioned this pull request Nov 10, 2020

EntityListener not called since 2.2.0 under prod env only #1249

Closed

stephanvierkant mentioned this pull request Nov 10, 2020

Can't clear metadata with DoctrineBundle 2.2.0 (prod) #1251

Closed

dmaicher reviewed Nov 10, 2020

View reviewed changes

dmaicher mentioned this pull request Nov 10, 2020

fix service ids for DoctrineMetadataCacheWarmer #1254

Closed

ostrolucky mentioned this pull request Nov 10, 2020

Revert "Add PhpArrayAdapter to cache metadata" #1255

Merged

ossinkine mentioned this pull request Nov 13, 2020

Fix bugs in PhpArrayAdapter introduction #1260

Merged

dmaicher mentioned this pull request Apr 29, 2021

Support setting PSR-6 caches with ORM 2.9+ #1332

Merged

dmaicher mentioned this pull request Aug 30, 2021

Undo deprecation of metadata_cache_driver configuration #1393

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PhpArrayAdapter to cache metadata #1196

Add PhpArrayAdapter to cache metadata #1196

ossinkine commented Aug 20, 2020

nicolas-grekas commented Aug 25, 2020

ostrolucky Aug 29, 2020

ossinkine Aug 31, 2020

ostrolucky Aug 29, 2020

ossinkine Aug 31, 2020

ostrolucky Sep 4, 2020

ostrolucky left a comment

ossinkine commented Aug 31, 2020

nicolas-grekas commented Aug 31, 2020 •

edited

ostrolucky commented Oct 26, 2020

ossinkine commented Oct 26, 2020

ostrolucky commented Oct 26, 2020

ossinkine commented Oct 27, 2020

ostrolucky commented Oct 31, 2020

stof commented Oct 31, 2020

dmaicher Nov 1, 2020

ostrolucky Nov 1, 2020

ossinkine commented Nov 1, 2020

ostrolucky commented Nov 1, 2020 •

edited

greg0ire left a comment

greg0ire left a comment

stephanvierkant commented Nov 10, 2020

ostrolucky commented Nov 10, 2020

dmaicher Nov 10, 2020

Add PhpArrayAdapter to cache metadata #1196

Add PhpArrayAdapter to cache metadata #1196

Conversation

ossinkine commented Aug 20, 2020

nicolas-grekas commented Aug 25, 2020

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

ostrolucky left a comment

Choose a reason for hiding this comment

ossinkine commented Aug 31, 2020

nicolas-grekas commented Aug 31, 2020 • edited

ostrolucky commented Oct 26, 2020

ossinkine commented Oct 26, 2020

ostrolucky commented Oct 26, 2020

ossinkine commented Oct 27, 2020

ostrolucky commented Oct 31, 2020

stof commented Oct 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ossinkine commented Nov 1, 2020

ostrolucky commented Nov 1, 2020 • edited

greg0ire left a comment

Choose a reason for hiding this comment

greg0ire left a comment

Choose a reason for hiding this comment

stephanvierkant commented Nov 10, 2020

ostrolucky commented Nov 10, 2020

Choose a reason for hiding this comment

nicolas-grekas commented Aug 31, 2020 •

edited

ostrolucky commented Nov 1, 2020 •

edited