Skip to content
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.

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

YAML routing loader should be cached in production. #7368

Closed
TomiS opened this issue Mar 14, 2013 · 12 comments
Closed

YAML routing loader should be cached in production. #7368

TomiS opened this issue Mar 14, 2013 · 12 comments

Comments

@TomiS
Copy link

TomiS commented Mar 14, 2013

While we were recently creating a high-traffic website on Symfony 2.0.x we found out a quite severe performance problem in production. We use YAML config in all of our bundles and it seems that YAML routes are parsed on every request even when using APC cache in production. We did some performance analysis and this really is a major performance issue.

A code example below shows how we worked around the problem in our project but due to the fact that cache key is prefixed with a hard-coded value routes_ we understand this example is not mergeable to Routing component as is. So that's why no PR.

In my opinion YAML routes should be APC cached by default if ApcUniversalClassLoader is used as autoloader. Other option might be to somehow convert all YAML configs to PHP arrays and store them to app/cache during cache warmup for prod use.

I'd really like to see something like this before 2.3 LTS because I really think these kind of performance issues really cripple any LTS even if the framework is otherwise very good.

ps. This issue is only about YAML routes but naturally all (YAML) configs should be somehow cached to gain maximum performance.

<?php
namespace Acme\DemoBundle\Loader;

use Symfony\Component\Routing\Loader\YamlFileLoader;

/**
 * ApcCachedYamlFileLoader loads and cachesYaml routing files.
 *
 */
class ApcCachedYamlFileLoader extends YamlFileLoader
{
    /**
     * Loads a Yaml file.
     *
     * Magic happens in parent, added just caching layer
     *
     * @param string $file A Yaml file path
     * @param string $type The resource type
     *
     * @return RouteCollection A RouteCollection instance
     *
     * @throws \InvalidArgumentException When route can't be parsed
     *
     * @api
     */
    public function load($file, $type = null)
    {
        // Key in cahe is routes_filepath_type
        $key = 'routes_' . $file. $type;

        // Checking if collection is in cache
        if ($collection = apc_fetch($key)) {
            return $collection; // Returning cached collection
        }

        // Loading collection with YamlFileLoader (parent) and caching it
        $collection = parent::load($file, $type);
        apc_store($key, $collection);

        return $collection;
    }
}
@Tobion
Copy link
Member

Tobion commented Mar 14, 2013

Normally the yaml route definitions should not be loaded every request because the generator and matcher are dumped. See PhpGeneratorDumper and PhpMatcherDumper. Did you change some configs or made customizations to the default setup?

@stof
Copy link
Member

stof commented Mar 14, 2013

Or are you calling $router->getRouteCollection() at runtime (which is indeed not cached as its goal is to rebuild the cached versions) ?

@TomiS
Copy link
Author

TomiS commented Mar 14, 2013

Thanks for quick reply @Tobion and @stof

I don't think we've made any significant configuration changes. I did a grep over our code base and outside of Symfony core getRouteCollection is only called in AllowedMethodsRouterLoader.php of FOSRestBundle version 0.10.0:

We did implement a custom router (for subdomains) but it does not alter the behavior of getRouteCollection. At the moment we're wondering if this misbehaviour could be related to subdomains anyway in which case using Symfony 2.2 should solve this (maybe).

@bendavies
Copy link
Contributor

@stof what is the alternative to $router->getRouteCollection()?

@mpdude
Copy link
Contributor

mpdude commented Mar 16, 2013

Try to use ConfigCache to cache what you built from the RouteCollection, if that is possible. So you don't need to fetch the collection once your cache is primed.

@stof
Copy link
Member

stof commented Mar 19, 2013

@bendavies you should not access the RouteCollection at runtime. If you have some code doing it, you should refactor it as it would be a flawed implementation of your feature.

@bendavies
Copy link
Contributor

@stof thanks, but say i wanted a list of all routes at runtime, how would i go about it?

@mpdude
Copy link
Contributor

mpdude commented Mar 20, 2013

Grab it and use ConfigCache to Cache it. The list of Resources you need is available on the collection.

@stof
Copy link
Member

stof commented Mar 20, 2013

@bendavies why do you want the list of all routes ?

@bendavies
Copy link
Contributor

i don't.
Imagine a theoretical situation where...um...i'm creating some admin interface for an application and i want to list all route uris in the app. something like that.

does it matter why?
I just want to know what the correct way of doing it it.

@TomiS
Copy link
Author

TomiS commented Mar 26, 2013

Ok. I'll close this now. I'll reopen if I find more proof of Symfony misbehaving. At this point it seems too probable it's an issue in our setup.

@TomiS TomiS closed this as completed Mar 26, 2013
@benbender
Copy link

I think indeed this is either a bug or a failure in the documentation. Without this bug I would not know that this is a "flawed" way of implementation. I think the behaviour should be clearly documented on the RouterInterface or the Router should use the configured caches in production.

kimlai added a commit to kimlai/RestHalBundle that referenced this issue Aug 23, 2016
The current implementation uses the `Router::getRouteCollection` method,
which loads all routes. If you use yaml to define your routes, it will
parse all route files, which has a pretty big performance impact on a
large application with many routes.

This method is actually intended to be used to warm up the cache and
nothing else (the result is cached in memory, but every request will
read the configuration with no cache involved).

See symfony/symfony#7368 (comment)
for a discussion on this topic.

We could implement a router that can do what we need efficiently, but
given that we don't use the `method` attribute, let's just remove it
completely.

Note : Blackfire says that this change leads to a -200ms improvement in
an app with 260 routes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants