[Proposal] Improve route resolution with indexed lists

[Frondor]

I find the way routes are being resolved from the store actually can be improved (in speed and CPU work) by implementing some lists, following the laravel source. This way, calls like the ones the actual edge global route('nameOrHandler') does resolves way more faster and cheaper. Actually, it iterates on every route just to find the last registered one (imagine a lot of calls to this function in multiple views), and at the same time, it does quite some work checking if the string passed is a route name / url path / controller action, etc...

See https://github.com/adonisjs/adonis-framework/blob/556a2135443c7930de2d2a9d3f8049eef5dd63b8/src/Route/Store.js#L141 Basically, Store.find does some extra work, that could be avoided if we adopt laravel helpers like action()

Proposal:

• Simplify route resolution, implementing indexed lists. For a live example see laravel's route collection https://github.com/laravel/framework/blob/master/src/Illuminate/Routing/RouteCollection.php
• Leverage handler resolution from route('name'), so it works with Store.getByName, which uses Store.namedList.
• Create action('routeHandler') to work with Store.getByAction, which uses Store.actionList

See this gist for how the indexed collection may work, take in mind its just the idea: https://gist.github.com/Frondor/2fd5b612a4948cfea23207b387c10d7e (press ctrl + f and look for {proposal} lines to jump faster in the code) Notice the two methods at the end, those can be really useful for plugin development, so you don't need to keep doing wasteful iterations over the _routes array

• I'm aware the middlewares are being bound by the route name actually (or not?), It should be using the route handler name (action) instead, so something like this could be implemented, and performance improved. Also, route names should be defined implicitly only.

I'm open to more ideas in order to improve the proposal and start working on a PR.

[thetutlage]

All ideas seems micro performance improvements and I don't think this can really help the overall performance.

If you can share some benchmarks, where the request/second improves with this, then we can look more into it.

Also being compatible with Laravel is not a goal

[Frondor]

I know you're not aiming for the mirror framework, and that's totally right. But since it is inspired by, and adopts the laravel conventions almost everywhere (not to mention the benefit you get for SEO), most basic things are expected to work just like it worked in laravel (for developers coming from it, in my humble opinon). Like setting routes to the group's base, which actually can't be achieved because of #812 and how middlewares work with route names instead of handler names.

I understand people can see it as "micro performance improvement", but I think we should take in mind we are working in the router, which is the bottleneck of the app, and its not a bad idea to improve it as much as we can. I quote myself again

Actually, it iterates on every route just to find the last registered one (imagine a lot of calls to this function in multiple views), and at the same time, it does quite some work checking if the string passed is a route name / url path / controller action, etc...

For a benchmark I would need to code the whole feature, which would take quite some days actually, since I'm trying to finish my first adonisjs app, but I can show you how expensive the current method is, to resolve a route in the bottom of the stack.

Call Store.find to resolve the last (registered) route in the stack on a 70 routes application: https://jsperf.com/lodash-find-vs-referenced-lists-vs-indexed-lists (although a bit more complex, indexed lists are way more performant because of array pointers being faster than object literals)

While creating the jsPerf tests, I noticed how complex the actual find method is, I trully believe all those checks need to be leveraged to different methods (byName, byHandler, etc..). Besides of using route lists or not, although lists would improve the performance significantly from my point of view.

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.