[PoC][WiP] Caching controller information for annotations #17933

linaori · 2016-02-26T13:39:08Z

_DO NOT MERGE_
Branch I'm working on: https://github.com/iltar/symfony/tree/feature/cached-controllers

What is this about?

Controllers and Annotations. We use Controllers in every web application and the best practices recommend us to use Annotations to configure them.

Make your controller extend the FrameworkBundle base controller and use annotations to configure routing, caching and security whenever possible.

So what about `@Template()`?

Don't use the @Template() annotation to configure the template used by the controller. The @Template annotation is useful, but also involves some magic. We don't think its benefit is worth the magic, and so recommend against using it.

While I disagree with some points, I agree with the fact that Annotations are really useful in Symfony (and in general).

So what about `@ParamConverter()`?

Use the ParamConverter trick to automatically query for Doctrine entities when it's simple and convenient.

Again, a recommendation to use annotations, awesome!

PS. The paramconverter uses a lot more magic than @Template() (but that's non of my business)

So what is the problem?

Overhead and complexity.

First off, you need the SensioFrameworkExtraBundle to get this to work. This will require an extra package which is not maintained in the core and follows a different release scheme.

Second, the logic in here is quite complex and cumbersome, not to mention slow! This is executed every single kernel.controller event, which means every sub-request and I often have at least 5 sub-requests on a page.

Afaik there have been discussions about wanting to integrate this into the core and to be honest, I understand why this should not be in there. However, I would like to have the functionality in here for several reasons:

Easier to extend
Easier to maintain (those who contribute to the FWEB know what I mean)
Follows the same release cycle
Best practices can be implemented without having to use a non symfony/ vendor package! In my opinion, if you recommend it, it should be in the core.

So what's my solution?

When routing cache is loaded, it already uses reflection to read Annotations regarding the @Route, however the actual implementation is in the SensioFrameworkExtraBundle. At this point, the metadata of the controllers could also be extracted and cached away. This means that during runtime, no additional reflection classes/methods or Annotation reading has to be done.

This PR contains the changes I made including some benchmarking (I hope I did it correct) in the unit-test provided. One thing I have not yet added is argument scanning (say for the Request $request parameter or @ParamConverter()), but is a piece of cake to add.

The benchmark is done with different test cases in both the old and the new setup. The old setup is where the SensioFrameworkExtraBundle does it for every single kernel.controller and the new tests where it's cached away via serialize().

Note that this is not the actual caching strategy but can be designed if this would be actually implemented, could provide different default strategies for this in a key/value storage. The classes are not complete and are missing functionality. This is merely the storage idea I had.

What will this solve?

It will create a generic extension point for people to add custom annotations to their actions. Take for example the ControllerExtraBundle, which has to do a lot of extra logic each request. The same goes for the FrameworkExtraBundle.
By having the ConfigurationAnnotation in the core, all symfony/sensio annotations can use it
The annotation adapter and adapter factory will allow everyone to create their adapter which can be cached away automatically. The PR features one for the Route and one for the ConfigurationAnnotation as example.
It will improve run-time performance for every single kernel.controller (~2ms per subrequest) for just the resolving, this is excluding any performance gains from annotation specific listeners!
It will make maintaining the logic behind the annotation reading a lot less complex and will reduce a lot of code required for each annotation to work, avoiding a lot of bugs and pull requests on multiple repositories.

Basic concept

Each public method in a controller gets scanned and all annotations are stored in the MethodMetadata. The controller annotations, name and MethodMetadata-s are stored in a MethodMetadata. Every single piece of code that needs to know something about the controller annotations (service="...", route information, security rules etc) can be read from this cached variant via a special interface, most likely something like $metadataRepository->get(ControllerName::class);.

current situation, a few controllers, annotations and reflection loaded each request
 * Executed 1000 times: 2233 ms
   - 5 controllers
   - 30000 annotations total
   - 2.23 ms/request


current situation, many controllers, annotations and reflection loaded each request
 * Executed 10 times: 2182 ms
   - 500 controllers
   - 30000 annotations total
   - 218.2 ms/request
   - 0.44 ms/sub-request


new situation, few controllers, annotations and reflection loaded during cache warmup
 * bootstrap executed in 5 ms
   - 4 controllers
   - 30 annotations
 * Executed 1000 times: 112 ms
   - 5 controllers
   - 0.11 ms/request


new situation, many controllers, annotations and reflection loaded during cache warmup
 * new bootstrap executed in 342 ms
   - 500 controllers
   - 3000 annotations
 * New Executed 1000 times: 20139 ms
   - 500 controllers
   - 20.14 ms/request

Serializing everything in 1 file is a bad thing, but I'm no caching expert. You could also write it away to %kernel.cache_dir%/metadata/controller/md5(Controller::class), but that's up to a caching layer.

A bunch of files are copied over from the SFEB, they will ofc not be included here.

It would be really nice to have (most of) the Annotations in the core so the best-practices can be followed without having to install additional bundles outside of the Symfony vendor namespace.

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets	~
License	MIT
Doc PR	~

/cc @mmoreram as author of the ControllerExtraBundle your opinion is valued :)

javiereguiluz · 2016-02-26T14:08:56Z

If we move ParamConverters to FrameworkBundle we'll solve #1547, which is the oldest pending Symfony issue (@beberlei opened it in July 2011 !!)

linaori · 2016-02-26T14:15:54Z

@javiereguiluz That's one of the things I'd like to achieve (Functionality wise) but the current implementations are too cumbersome and badly performing as it's not being cached.

In fact, every annotation in the SFEB can be moved to the core once this is done as they will be a lot more efficient and the listeners can be a lot more efficient as well. Adding custom annotations will be no problem at all afterwards, which allows the annotations to be utilized a lot more.

linaori · 2016-02-29T15:15:46Z

I was looking to implement this in a generic fashion. Ideally I would make it a component which is configured by the framework bundle. Right now I'm not really sure where to put it.

The metadata part is controller specific; HttpKernel?
The Route annotation is already in the Routing component - bit odd but okay
The event listeners could also fit in the HttpKernel.
@Security could go into the Security Component + Bundle (configuration)
@Cache could go into the HttpKernel (HttpCache dir?)
@Method could go with the @Route
@ParamConverter could also go in the HttpKernel
@Template doesn't have a specific dependency, it can be used with several engines.. FrameworkBundle?
Where to put the service definitions, also the HttpKernel?

For each annotation I can provide a factory+adapter to get them to hook in on the caching so I do not need the ConfigurationInterface, but would be nice to use. I want to avoid having dependencies on the HttpKernel just

Ideally I would keep them together to make it easier for the developer to find them, but that's not possible unless I add everything in the Framework Bundle. Another option is to add the basics to the HttpKernel:

ClassMetadata
MethodMetadata
ConfigurationAnnotation/ConfigurationInterface
The layer to fetch the info that belongs to a controller

The problem here is that all the annotation loading is done in the routing component, which isn't used at all in Symfony, just the FrameworkExtraBundle so I will have to move that to the routing component. Luckily symfony/routing is already in the dev-requirements, so adding it shouldn't be too hard.

Would it be an idea to extract the annotation loading of routes (or config) to another component or bundle? This entire functionality could be in a Controller component which depends on the Routing/Httpkernel.

I would love to have your thoughts @javiereguiluz @fabpot (and anybody else willing to help me).

stof · 2016-02-29T18:46:06Z

Follows the same release cycle

This actually means that adding a new feature will not be available for people relying on LTS releases, while they can just upgrade the bundle today. So I would rather list this as a drawback

In my opinion, if you recommend it, it should be in the core.

This is a bad argument. We recommend many packages which are outside the core, precisely because it gives more flexibility in maintaining them (and in case you did not notice, MonologBundle itself is not in the core anymore since Symfony 2.1 or 2.2, but we still recommend it)

linaori · 2016-02-29T18:56:26Z

Follows the same release cycle

This actually means that adding a new feature will not be available for people relying on LTS releases, while they can just upgrade the bundle today. So I would rather list this as a drawback

I guess you can indeed see it both ways. With semver you have the benefit you don't have to rely on LTS anymore and fixes are backported. Yes you have faster releases with another bundle, however this bundle is less maintained or doesn't get the same care as the framework does.

In my opinion, if you recommend it, it should be in the core.

This is a bad argument. We recommend many packages which are outside the core, precisely because it gives more flexibility in maintaining them (and in case you did not notice, MonologBundle itself is not in the core anymore since Symfony 2.1 or 2.2, but we still recommend it)

I've poorly stated this, sorry for the confusion. I mean the core as in vendor namespace. Right now this different compared to the monolog bundle, where it's still a symfony package. The current features are in the sensiolabs namespace.

Instead of adding this to "the core", I could also add it to a stand-alone symfony-"vendored" package. If desired I can also apply those changes on the FrameworkExtraBundle simply for the sake of performance.

Personally I would like to see this in Symfony though.

stof · 2016-02-29T19:02:29Z

@iltar we have other examples of recommended libraries in other vendor namespaces:

twig/twig
monolog/monolog
incenteev/composer-parameter-handler (added in the SE to respect the best practice of not committing the parameters.yml file)

And the list could go on.

The maintenance issue on FrameworkExtraBundle is rather than @fabpot is the only maintainer on it rather than involving some other Sensio guys too. But this could be solved

linaori · 2016-02-29T19:15:48Z

@stof That would be a quick win for both the framework and my attempt to make a PR

Edit: additionally the PR template could be taken along and a more clear development plan/roadmap would help a lot too. I can make a PR for the template tomorrow (one of the many todo's).

This PR was merged into the 3.0.x-dev branch. Discussion ---------- Remove the @template owner after kernel.view The issue is most likely that the owner is somehow cached or still referred at a point where it cannot be cleared properly. This will fix the reference but is not a permanent fix. This is something I can properly fix with symfony/symfony#17933. | Q | A | ------------- | --- | Branch | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #402 | License | MIT | Doc PR | ~ Commits ------- 0da6408 Remove the @template owner after kernel.view

linaori · 2016-03-01T11:35:02Z

@stof I could make the controller metadata rather simple and extend on it in the SensioFrameworkExtraBundle. Another case that would benefit from cached metadata would be the ControllerResolver.

linaori · 2016-03-02T11:09:34Z

I've done some refactoring locally, this is the result I get based on a cache warmer that uses the route collection:

// ProfilerController:
function toolbarAction(Request $request, $token, string $test1, array $test2 = [])

All that remains now is to implement a caching layer and using this in the ControllerResolver of the framework bundle. Annotations are optional and will only be read if the annotation_reader service is available. My first goal is to make sure the ControllerResolver uses this cache when the _controller parameter in the request is a string which avoids reflection calls runtime. This also means the cache warm-up will fail if a controller cannot be found (fail as early as possible).

PS. here's one with annotations

linaori · 2016-03-02T11:10:00Z

Oops, that was not supposed to be closed...

edit: I've added the branch I'm working on if you're interested to see the changes: https://github.com/iltar/symfony/tree/feature/cached-controllers

@wouterj

…iltar, HeahDude) This PR was merged into the 3.1-dev branch. Discussion ---------- Added an ArgumentResolver with clean extension point | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #17933 (pre-work), #1547, #10710 | License | MIT | Doc PR | symfony/symfony-docs#6422 **This PR is a follow up for and blocked by: #18187**, relates to #11457 by @wouterj. When reviewing, please take the last commit: [Added an ArgumentResolver with clean extension point](4c092b3) This PR provides: - The ability to tag your own `ArgumentValueResolverInterface`. This means that you can effectively expand on the argument resolving in the `HttpKernel` without having to implement your own `ArgumentResolver`. - The possibility to cache away argument metadata via a new `ArgumentMetadataFactory` which simply fetches the data from the cache, effectively omitting 1 reflection call per request. *Not implemented in this PR, but possible once this is merged.* - The possibility to add a PSR-7 adapter to resolve the correct request, avoids the paramconverters - The possibility to add a value resolver to fetch stuff from $request->query - Drupal could simplify [their argument resolving](https://github.com/drupal/drupal/blob/8.1.x/core/lib/Drupal/Core/Controller/ControllerResolver.php) by a lot - etc. The aim for this PR is to provide a 100% BC variant to add argument resolving in a clean way, this is shown by the 2 tests: `LegacyArgumentResolverTest` and `ArgumentResolverTest`. /cc @dawehner @larowlan if you have time, can you check the impact for Drupal? I think this should be a very simple change which should make it more maintainable. Commits ------- 1bf80c9 Improved DX for the ArgumentResolver f29bf4c Refactor ArgumentResolverTest cee5106 cs fixes cfcf764 Added an ArgumentResolver with clean extension point 360fc5f Extracting arg resolving from ControllerResolver

nicolas-grekas · 2016-04-17T15:12:02Z

@iltar should we close this one and #1547 now that #18308 has been merged?

linaori · 2016-04-17T15:25:55Z

@nicolas-grekas I will close this one as I can still reference to it even when closed. Regarding the other one; "#1547 opened on Jul 6, 2011 by @beberlei", I'd say it's time to close it 😄

[PoC] Caching controller information for annotations

27d50bf

carsonbot added the Status: Needs Review label Feb 26, 2016

javiereguiluz added Feature FrameworkBundle labels Feb 26, 2016

linaori mentioned this pull request Mar 1, 2016

Remove the @Template owner after kernel.view sensiolabs/SensioFrameworkExtraBundle#404

Merged

linaori closed this Mar 2, 2016

linaori reopened this Mar 2, 2016

DavidBadura mentioned this pull request Mar 7, 2016

[PropertyAccess] Allow custom methods on property accesses #18016

Closed

4 tasks

linaori mentioned this pull request Mar 16, 2016

Extracting the argument resolving from the ControllerResolver #18187

Closed

merk mentioned this pull request Mar 16, 2016

Template guessing no longer works with @View FriendsOfSymfony/FOSRestBundle#1395

Closed

linaori mentioned this pull request Mar 25, 2016

Added an ArgumentResolver with clean extension point #18308

Merged

linaori mentioned this pull request Apr 4, 2016

[RFC] Add a new kernel event after resolving controller arguments #18362

Closed

linaori closed this Apr 17, 2016

linaori deleted the poc/controller-metadata branch February 8, 2019 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PoC][WiP] Caching controller information for annotations #17933

[PoC][WiP] Caching controller information for annotations #17933

linaori commented Feb 26, 2016

javiereguiluz commented Feb 26, 2016

linaori commented Feb 26, 2016

linaori commented Feb 29, 2016

stof commented Feb 29, 2016

linaori commented Feb 29, 2016

stof commented Feb 29, 2016

linaori commented Feb 29, 2016

linaori commented Mar 1, 2016

linaori commented Mar 2, 2016

linaori commented Mar 2, 2016

nicolas-grekas commented Apr 17, 2016

linaori commented Apr 17, 2016

[PoC][WiP] Caching controller information for annotations #17933

[PoC][WiP] Caching controller information for annotations #17933

Conversation

linaori commented Feb 26, 2016

What is this about?

So what about @Template()?

So what about @ParamConverter()?

So what is the problem?

So what's my solution?

What will this solve?

Basic concept

javiereguiluz commented Feb 26, 2016

linaori commented Feb 26, 2016

linaori commented Feb 29, 2016

stof commented Feb 29, 2016

linaori commented Feb 29, 2016

stof commented Feb 29, 2016

linaori commented Feb 29, 2016

linaori commented Mar 1, 2016

linaori commented Mar 2, 2016

linaori commented Mar 2, 2016

nicolas-grekas commented Apr 17, 2016

linaori commented Apr 17, 2016

So what about `@Template()`?

So what about `@ParamConverter()`?