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

module:route-provider command should be removed as it causes performance problems #851

Open
ederuiter opened this issue Oct 9, 2019 · 5 comments

Comments

@ederuiter
Copy link

commented Oct 9, 2019

Hi,

We found out that using multiple route service providers (one per module) causes performance issues. This is due to the fact that the (cached) routes are re-loaded for each RouteServiceProvider. In our test-suite this causes delays due to createApplication taking 60ms where 40ms is spent in reloading routes.

According to the Laravel developers the RouteServiceProvider should not be instantiated more than once (see laravel/framework#30171 ).

We solved our performance issues by using loadRoutesFrom in the main ServiceProvider of each module.

To prevent other users of this package from having the same issues, I suggest to remove the module:route-provider command as it promotes usage of multipe RouteServiceProviders and thus leads to performance issues. If you want this, I can provide a PR to remove the module:route-provider command (or to replace it with a hint to use loadRoutesFrom)

@nWidart

This comment has been minimized.

Copy link
Owner

commented Oct 9, 2019

Hello,

Hm this is interesting, I've not seen this performance it in Asgardcms which has multiple modules, and in applications using it with many more modules.

What does this loadRoutesFrom method look like when you load your routes?

I don't think it'll look very good since it's more that just loading one file.

Edit: I misread, thought Dries told to override it, not use it, duh :D .

Still it looks very rough.

Couldn't we just call these in the main service provider

Route::middleware('web')
            ->namespace($this->moduleNamespace)
            ->group(__DIR__ . '/../Routes/web.php');

Just tested that and seems to work too, however I'm assume that wouldn't allow for caching...

@ederuiter

This comment has been minimized.

Copy link
Author

commented Oct 10, 2019

Hi,

We saw it in our profiling of our test-suite. Using multiple RouteServiceProviders (~20) results in calling Symfony\Component\Routing\CompiledRoute::__unserialize 1.3 million times .. when using loadRoutesFrom instead of the RouteServiceProviders the CompiledRoute::__unserialize method is called only 75 thousand times. So for us rewriting the RouteServiceProviders to loadRoutesFrom improved the runtime of our test-suite by a factor 2.

Regarding caching: the ServiceProvider::loadRoutesFrom method takes that into account. It won't load anything if the routes are already cached. And you can use app()->routesAreCached() to add the check yourself.

Furthermore there is no need to override the loadRoutesFrom method, you only have to call it.
So what we have looks like this:

use Illuminate\Support\ServiceProvider;

class ModuleServiceProvider extends ServiceProvider
{
    public function register()
    {
        $this->loadRoutes();
    }

    public function boot()
    {
        //Do your route bindings here
    }

    private function loadRoutes(): void
    {
        if (app()->routesAreCached()) {
            return;
        }

        foreach (File::glob($this->getModuleRoutePath() . '*.php') as $fileName) {
            $this->loadRoutesFrom($fileName);
        }
    }

    private function getModuleRoutePath()
    {
        return $this->getModulePath() . DIRECTORY_SEPARATOR . $this->moduleRoutesPath . DIRECTORY_SEPARATOR;
    }
}

And all your route middleware/prefixes configuration can be moved into your route files:

Route::group([
    'middleware' => ['web', 'auth', 'firewall']
], static function (Router $router) {
    $router->resource('projects', ProjectController::class, [
        'only' => ['index', 'show', 'edit', 'update'],
    ]);
});
@nWidart

This comment has been minimized.

Copy link
Owner

commented Oct 10, 2019

Hm thanks for those findings, very useful!

I wonder if it'd be more performant to define a list of route files "manually", instead of globbing over the directory.

The only downside I see with this is we won't have the namespace "injected" in the file (not super bad since it's nice to be explicit, but still).

@ederuiter

This comment has been minimized.

Copy link
Author

commented Oct 10, 2019

Due to the if (app()->routesAreCached()) check the glob is only performed when the routes are not cached, so to speedup the routing initialization you can simply use the route cache. Alternatively a static list is also very easily implemented.

For the namespace injection, you could probably use something like this:


    private function loadRoutes(): void
    {
        if (app()->routesAreCached()) {
            return;
        }

        app(Router::class)
            ->namespace($this->moduleNamespace)
            ->group(static function(Router $router) {
                foreach (File::glob($this->getModuleRoutePath() . '*.php') as $fileName) {
                    require $fileName;
                }
            });
    }

and in your route files you can use

/** @var Router $router */
$router->group([
    'middleware' => ['web', 'auth', 'firewall']
], static function (Router $router) {
    $router->resource('projects', ProjectController::class, [
        'only' => ['index', 'show', 'edit', 'update'],
    ]);
});
@ndhan93

This comment has been minimized.

Copy link

commented Oct 17, 2019

how about this ?
just replace boot on RouteServiceProvider

public function boot()
{
    //parent::boot();

    if (!$this->routesAreCached()) {
        $this->loadRoutes();
    }
}

because we have laravel default RouteServiceProvider (in app\Providers) to load cached route (if route cached), so we can skip it on module route provider

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.