fix(symfony): metadata cache is broken in dev/prod

[emmanuelballery]

Q	A
Branch?	main
Tickets	#4975
License	MIT

Hi!

Cache seems broken in 3.0.0. Metadatas are not stored in the filesystem nor in APCu, resulting in poor performances (see https://github.com/api-platform/core/issues/4975#issuecomment-1251668523).

Cache services are provided by each metadata XML file located in src/Symfony/Bundle/Resources/config/.

• api_platform.cache.metadata.property => metadata/property.xml
• api_platform.cache.metadata.resource => metadata/resource_name.xml
• api_platform.cache.metadata.resource_collection => metadata/resource.xml
• api_platform.cache.route_name_resolver => metadata/api.xml
• api_platform.cache.identifiers_extractor => seems unused
• api_platform.elasticsearch.cache.metadata.document => elasticsearch.xml

In ApiPlatformExtension, these files are loaded in registerMetadataConfiguration but then each cache is overrided in registerCacheConfiguration for an ArrayAdapter instead of the usual cache.system.

[soyuka]

Thing is, in development mode we want symfony to clear the cache based on the mtime. Maybe that we should use ArrayAdapter only in development ? I must say I'm not that familiar with how Symfony handles cache.

[emmanuelballery]

Thing is, in development mode we want symfony to clear the cache based on the mtime. Maybe that we should use ArrayAdapter only in development ? I must say I'm not that familiar with how Symfony handles cache.

But currently this destroys non debug environment. I updated my PR to only target non debug envs.

For debug env, it hurts a lot. I overrided api platform caches to go to the filesystem and will cache clear when my schema will change.

---

To add to the strangeness, CachedResourceMetadataCollectionFactory has a local cache implemented.

It:

• looks for a local cache entry in $this->localCache
• then fallbacks to CacheItemPoolInterface (meaning anArrayAdapter) which does the same as the first step but in a different array
• fallbacks to creating the real collection

So we store the metadatas in 2 stores for no real benefit.

A clean solution could be to remove this $this->localCache and only depend on CacheItemPoolInterface then work on a strategy that is ok per environment:

• non debug cache could rely on cache.system (what my commmit does) and be a chain of:
  • ArrayAdapter
  • ApcuAdapter
  • FilesystemAdapter
• debug cache could be a chain of:
  • ArrayAdapter
  • (...) something that check mtime but based on what files?

[soyuka]

(...) something that check mtime but based on what files?

But symfony already has this on cache.system IIRC for dev environments.

[emmanuelballery]

Symfony cannot tell that the cache used for ResourceMetadataCollectionFactoryInterface has to be reloaded PER api resource. It's all or nothing - except if we create more specific caches.

I'm ok to work on improving the dev cache (probably by testing performances first). Maybe in another PR?

[webda2l]

As @emmanuelballery I'll remove the localCache from CachedResourceMetadataCollectionFactory (and so add symfony/cache as dependancy in require, instead current require-dev) And I'll remove completely the $this->registerCacheConfiguration($container); function of ApiPlatformExtension, let SF deal with the default cache.system that work well. Then, it let user configure this cache.system with their needs, by ex:

framework:
    cache:
        # APCu (not recommended with heavy random-write workloads as memory fragmentation can cause perf issues)
        system: cache.adapter.apcu

when@dev:
    framework:
        cache:
            # system: cache.adapter.array           Disabled because lower perf than the default cache.adapter.system

A distinct cache 'api_platform' (extends app.system) could be maybe added to the default app & system, to avoid modifying other use of the cache.system like above.

[emmanuelballery]

As I mentionned in the related issue, you don't have to change the cache.system as it would impact all you app. Overriding api platform cache is enough.

https://github.com/api-platform/core/issues/4975#issuecomment-1253617780

[dunglas]

Hitting a local cache in memory will always be faster than calling the cache component. I would keep it (we also follow the same pattern in Symfony components btw).

[emmanuelballery]

Hitting a local cache in memory will always be faster than calling the cache component. I would keep it (we also follow the same pattern in Symfony components btw).

I agree and I do it myself. But If ApiPlatform forces the cache to be an ArrayAdapter (and it does right now), then this second cache store is useless as it basically does the same thing but in a different way.

---

I did suggest to change things around the cache but the main concern of this PR is that v3 is not production ready as is. I tried to route these cache changes in another PR, but failed. We are all - myself included - speaking of improvements when the real current fix is not pushed yet.

I'd suggest to focus on fixing non debug environments (or prod) here, which my commit does; and I'll happily bench the chain of ResourceMetadataCollectionFactoryInterface in another PR to see what could be improved for a better DX.