8000 [FrameworkBundle] enable metadata cache when annotation is disabled by bastnic · Pull Request #49679 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[FrameworkBundle] enable metadata cache when annotation is disabled #49679

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 31, 2023
Merged

[FrameworkBundle] enable metadata cache when annotation is disabled #49679

merged 1 commit into from
Mar 31, 2023

Conversation

bastnic
Copy link
Contributor
@bastnic bastnic commented Mar 13, 2023 •
edited
Loading
Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #...
License MIT
Doc PR symfony/symfony-docs#...

When using Annotations, annotations are cached at AnnotationLoader level.
Which is cleared when entities are changed. So the dev experience is optimal.

[ClassMetadataFactory.php](vendor/symfony/serializer/Mapping/Factory/ClassMetadataFactory.php") on line 51:
[Symfony\Component\Serializer\Mapping\Loader\LoaderChain](vendor/symfony/serializer/Mapping/Loader/LoaderChain.php&line=28#line28) {#543 ▼
  -loaders: array:1 [▼
    0 => [Symfony\Component\Serializer\Mapping\Loader\AnnotationLoader](vendor/symfony/serializer/Mapping/Loader/AnnotationLoader.php&line=33#line33) {#544 ▼
      -reader: [Doctrine\Common\Annotations\PsrCachedReader](vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/PsrCachedReader.php&line=22#line22) {#262 ▼
        -delegate: [Doctrine\Common\Annotations\AnnotationReader](vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/AnnotationReader.php&line=20#line20) {#263 ▶}
        -cache: [Symfony\Component\Cache\Adapter\PhpArrayAdapter](vendor/symfony/cache/Adapter/PhpArrayAdapter.php&line=32#line32) {#277 ▶}
        -debug: true
        -loadedAnnotations: array:14 [▶]
        -loadedFilemtimes: array:4 [▶]
      }
    }
  ]
}

When using yaml files, there is no cache at the loader level so I added in the past the same cache as for the prod env, as the metadata are effectively cleared when using only yaml config files.

#35109

The regression introduced by my patch is for people that do not use mapping files but use annotations.

#41961

But now, we are in the opposite situation: no cache for people using mapping files but not annotations.

On a current project it means loading 83 yaml files for each dev requests. It's not good at all.

A simple local fix is to add that in a dev services files.

serializer.mapping.cache_class_metadata_factory:  
    class: 'Symfony\Component\Serializer\Mapping\Factory\CacheClassMetadataFactory'  
    decorates: 'serializer.mapping.class_metadata_factory'  
    arguments: ['@serializer.mapping.cache_class_metadata_factory.inner', '@serializer.mapping.cache.symfony']

image

A solution in Symfony could be:
1/ only yaml/xml mapping files (enable_annotations: false) : cache like prod => that what I did in this PR, as it fixes the current perf regressions on my different projects. There is no cache on yaml/xml file as soon as annotation is enabled (which is the default)
2/ add a cache at reader level for yaml/xml loader
3/ add a cache cleaner at metadata level when annotation are enabled

@carsonbot carsonbot added this to the 5.4 milestone Mar 13, 2023
nicolas-grekas
nicolas-grekas approved these changes Mar 14, 2023
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This patch works for me as a fix for the regression in 5.4

This means the serializer cache is not auto-invalidated when using yaml/xml. That was the case before, and is an acceptable tradeoff given the perf drop of parsing all files in every requests (in dev).

For 6.3 or up, we could add some auto-invalidation mechanism based on resource checkers, like we do for the container. That'd mean allowing loaders to declare their resources and the cache metadata factory to use those resources to trigger a prune on the pool when needed.

If you're OK with that, can you please open an issue so that we keep track of the need?

@bastnic
Copy link
Contributor Author
bastnic commented Mar 14, 2023

This means the serializer cache is not auto-invalidated when using yaml/xml. That was the case before, and is an acceptable tradeoff given the perf drop of parsing all files in every requests (in dev).

No no, it is!

@nicolas-grekas
Copy link
Member

(please check test failures, they look related)

@bastnic bastnic force-pushed the feature/metadata-cache branch 2 times, most recently from 090ea0b to f1bf23d Compare March 25, 2023 21:17
@bastnic
Copy link
Contributor Author
bastnic commented Mar 25, 2023

@nicolas-grekas tests are fixed now.

@nicolas-grekas
Copy link
Member

In standalone mode, tests still fail, can you have a look? (aka when running them from inside the FWB directory.)

@nicolas-grekas nicolas-grekas force-pushed the feature/metadata-cache branch from f1bf23d to 1773dff Compare March 31, 2023 08:25
@nicolas-grekas
Copy link
Member

Thank you @bastnic.

@nicolas-grekas nicolas-grekas merged commit 9d6fdbf into symfony:5.4 Mar 31, 2023
This was referenced Mar 31, 2023
Merged
Merged
@bastnic bastnic mentioned this pull request Mar 31, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug FrameworkBundle Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
3 participants
@bastnic @nicolas-grekas @carsonbot
0