Problem/Motivation

We saw in #2470679: [meta] Identify necessary performance optimizations for common profiling scenarios that routing for e.g. /node/1 is quite a bit slower in D8. A simple optimization we could do, is to handle the most frequently requested routes in PHP code only (i.e. not talking to the DB), and return early. Thus, only using the DB for less commonly accessed routes.

Proposed resolution

Refactor the routing process so we have a chained route provider, and simplify the remaining flow to removed the chained router, etc.

Remaining tasks

User interface changes

API changes

Comments

catch’s picture

Asked this on irc yesterday. What happens if:

node/%node is in PHP.

node/1 is registered explicitly as a separate route, and is not in PHP.

(node/add and node/%node are probably a better example, except that node/add wouldn't be a valid path for node/%node).

That doesn't make this impossible, but it might mean we have a restriction that all paths in a top level directory/path root are either in PHP or not.

Also it's a different optimization but #2406117: potentially cache parts of routing would be a higher-level cache we could add (compared to this) which would also help with route lookup, although that has its own complexities too.

dawehner’s picture

(node/add and node/%node are probably a better example, except that node/add wouldn't be a valid path for node/%node).

Symfony kinda solves that by adding a regular expression for the %node example.

In general this seems to be an optimization, that is tricky to be implemented as a generic case, but rather could be applied really easy on
custom base, as you have more knowledge available.

If we wanna do that, maybe we could write a decorator for RouteProvider, that people on custom sites could adapt things more easy ...
note: We also want to make it possible to reuse that chaining for other routing implementations like mongo.

larowlan’s picture

Pity the dump to .htaccess option doesn't work

fabianx’s picture

We can still do this when we compile routes statically to PHP on runtime without problems ...

As we then can check for all path-prefixes and group all same matching paths together ...

pwolanin’s picture

So - IMHO the issue here is really to change the architecture of routing a bit to make ths easy, not necessarily implement the PHP dumping

pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

Title: ChainedRouteProvider which subclasses RouteProvider but returns early for most common routes » ChainedRouteProvider which subclasses RouteProvider but could return early for most common routes
pwolanin’s picture

Title: ChainedRouteProvider which subclasses RouteProvider but could return early for most common routes » Create a ChainedRouteProvider which subclasses RouteProvider but could return early for most common routes
Crell’s picture

I got a bit lost part way through this thread... why subclass anything or wrap anything? I'm fully on board with adding an earlier DB-free router; ChainRouter is in there for exactly that reason. But piling more wrappers onto the RouteProvider doesn't seem like a wise idea to me. That's the wrong place to add this sort of flexibility. Honestly I'm a little concerned that we've put enough into the current router as is that leveraging ChainRouter may be difficult.

Although part of me is quite curious what would happen if we swapped out the RoutingProvider with one that does PHP dump for everything. I have no idea what would happen, honestly. :-) It might be worth investigating.

dawehner’s picture

Well @crell, the problem is that the chained router architecture is just not exactly what we need ... as it would entirely bypass
our upcasting logic, I mean sure, in case we have a really limited list of routes, we might be able to just hardcode the entity loading, but its something which was not taken into account, if you ask me.

Crell’s picture

dawehner: Yep, that's exactly the sort of thing I mean by "Honestly I'm a little concerned that we've put enough into the current router as is that leveraging ChainRouter may be difficult.". There's a couple of ways to addressing that:

1) For those routes we put in the "early/fast" router, we hard code appropriate logic. This is probably faster, but more brittle and less site-flexible.

2) Use another DynamicRouter instance for the early/fast router, too, and wire *the exact same enhancers and filters and stuff" into that one. Stateless services make this easy. :-) If there's a filter or enhancer that we can't reuse that way, it's because we did it wrong. This would be the more robust option, but may not buy us as much on performance since we'd still be loading many of the same services.

Also remember that the way ChainRouter works, each "failed" router results in a 404 exception. Creating and throwing an exception is non-free in PHP; it costs several times a function call.

Unfortunately I don't know any way to determine the best approach here other than writing it a bunch of different ways and benchmarking the hell out of it. :-)

catch’s picture

Category: Bug report » Task

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

This could be also a usecase for an experimental module

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

Priority: Major » Normal

Not sure how major this is anymore. Also note we're removing ChainRouter in #2810303: Reunite the router: One router to rule them all

dawehner’s picture

IMHO such a module is most of the time custom. You can make bigger assumptions there so its actually worth it. Generalizations will be hard, given that upcasting for example might be needed for specific usecases.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

I'm honestly not sure its really worth even trying that.

wim leers’s picture

Status: Active » Closed (won't fix)