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

Is 'api.routes' instance required? #1733

Open
bslinger opened this issue Jun 9, 2020 · 5 comments
Open

Is 'api.routes' instance required? #1733

bslinger opened this issue Jun 9, 2020 · 5 comments

Comments

@bslinger
Copy link

bslinger commented Jun 9, 2020

Q A
Bug? no
New Feature? no
Framework Laravel
Framework version 7.14.1
Package version 3.0.0
PHP version 7.4.6

We were seeing drastic performance degradation after upgrading from Laravel 5.6, Dingo v2.0.0, PHP 5.6 to Laravel 7.14, Dingo v3.0.0, PHP 7.4, and when profiling the code Dingo was slowing things down a lot, even when the routes were cached.

After digging in further today, I narrowed it down to the \Dingo\Api\Routing\Router::setAdapterRoutes method, in particular the 'api.routes' instance being set by \Dingo\Api\Routing\Router::getRoutes which runs \Dingo\Api\Routing\Router::createRoute on every defined route and was taking multiple seconds on every request - I couldn't find anywhere this instance was being used, so created my own version of the Router class to avoid performing that second step and performance is vastly improved while seemingly not having affected functionality.

I just wanted to check in here and see if anybody with more in-depth knowledge of Dingo might let me know if I'm missing anything here?

@specialtactics
Copy link
Member

Those functions are there to maintain compatibility with Laravel's routing interface, they are definitely used (but not in every request, they need to be called by something).

I think there are many other reasons that should be done again for compatibility's sake - it won't be obvious until something breaks.

However I don't really get (unless you are massively exaggerating) what that operation takes "seconds" for you - I've never seen it take so much as 100ms.

Can you show me some profiling info or something?

@bslinger
Copy link
Author

bslinger commented Jun 9, 2020

Under load it is taking multiple seconds, and is definitely the bottleneck causing high CPU usage when we deploy our new code under production conditions. Here's a NewRelic trace (which I unfortunately can't download in structured format):

image (24)
image (25)
image (26)

Is there a way I could set it up to lazily create these routes then? From what I can see, when the routes are not cached it only creates a \Dingo\Api\Routing\Route object for the matched route, via \Dingo\Api\Http\Middleware\PrepareController::handle, but when getRoutes is called via \Dingo\Api\Routing\Router::setAdapterRoutes it creates one for every route in the system, which in our case is approximately 235 routes.

@specialtactics
Copy link
Member

Route caching doesn't actually work/improve performance, this has been discussed at length on the laravel github.

It seems to me like what you are seeing is the routing overhead of a framework under load conditions, nothing more. I thought you meant it takes seconds in general, of course routing is a bottleneck if you don't have enough resources for the traffic.

All routes have to be created before they can be assessed for which to pick, and I do recommend you disable route caching, because it degrades performance.

@bslinger
Copy link
Author

Can you elaborate on route caching not working? It seems to work fine to me and removes the need for us to iterate over a number of files to create the available routes on every request. Is there an issue you can reference with more information about it?

My point was that under similar load on Laravel 5 and Dingo 2.0, the routing was not causing this bottleneck. When using route caching, this section of the Dingo Router is the bottleneck. When not using route caching, finding the routes takes more time than it used to as well, though that seems to be more of a Laravel issue.

@specialtactics
Copy link
Member

Hey @bslinger I don't have a link, unfortunately, but there were 2 discussions on the laravel github about it.

Both Laravel's routing and the underlying Symfony routing have changed a lot (in fact there was a major Symfony version upgrade between 5 and 7), so it has not stayed the same.

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

2 participants