[WIP] Performance Improvement : Set name of system routes

[hissy]

Currently, 2.4% of execution time, 2.5% of CPU usage is wasted to generate route names, but nobody can see that name.
How about to set route names by default?
Package developers still can add custom routes without its name.

This PR is in progress. If the core team looks it's fine, I'll add the remaining route names.

[hissy]

422 routes exist but Router::addRoute called 845 times. It's also weird and maybe wasting resources

[mlocati]

@hissy: could you try to add this line:

$route->setCustomName($name);

right before this line:

https://github.com/concrete5/concrete5/blob/f22f8a6b78ee357dd07cf7d4963e6d0185e5e312/concrete/src/Routing/RouteGroupBuilder.php#L190

It seems to me that the route names are calculated twice: with this change it should be done only once.

Could you try to see if this makes some difference (that is, execution time, less useless calls)?

[hissy]

@mlocati I'll try it later, but we're generating names nobody can see more than 400 times at every request, even if that change works as expected.

[hissy]

@mlocati Number of calls dramatically reduced by adding $route->setCustomName($name);

[aembler]

I don't disagree that it'd be nice to improve performance... but this makes the routing in the core look a lot uglier by default.

Would it be better if we could simply cache these routes in some way, and leave the original registering of routes alone?

I think this deserves more discussion.

[mlocati]

With simply #9048 (comment) we can already reduce the time wasted for calculating route names twice.

[hissy]

Reducing 2.4% of execution time should be a big performance improvement, but yeah, this is not the only way to do that.
The current routing system also accessing File Locator many times, so the caching approach is better than this pull request.

[hissy]

We might be able to follow what the Symfony's Router class does. It caches UrlMatcher class.

https://github.com/symfony/routing/blob/3.4/Router.php#L265-L310

[aembler]

Yeah, that sounds like a good way forward.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions!

[hissy]

I'm working on it

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions!

[hissy]

Sorry, it's in my list

[aembler]

An interesting idea, but let's think about caching the router in some other ways.

[hissy]

Yeah, I'll try to cache UrlMatcher class like Symfony. Please close this PR

[mlocati]

I don't think it's worth caching it: please remark that we pre-filter the routes (see #7026 ), so that we actually need to compile just a tiny subset of them (is this done by Symfony too?)

The problem with caching is that routes may be added on the fly (for an example see https://github.com/concrete5/addon_community_translation/blob/1.0.1/controller.php#L277), and correctly handle this case may be a nightmare

[hissy]

I have to test it again with the recent develop branch, but it was horrible 2 years ago. We're generating route names nobody can see more than 400 times at every request

[hissy]

Tested with recent develop branch. Concrete still generating route names more than 1000 times even though nobody can see it. However, I can't find any good solution to improve this. Give up.