Two things that stick out:

1. Quering the database for routes. We need to look at caching this, possibly using CacheCollector?

Most requests only need to check a handful of routes (unless it's the toolbar tree), most routes on the site are admin paths that hardly ever get visited. Cache routes by role or something?

2. Request::create() takes over 3ms all by itself. What about Request::duplicate(), or don't mock the request for router access check since that doesn't really make sense anyway.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Issue tags: +MenuSystemRevamp

In menu_item_route_access(), we use the mocked request for two things.
First, we are given node/3/edit and need to turn that into node_page_edit and array('node' => 3).

Even if we were passed the route and the params already broken up, AccessManager::checkNamedRoute() does almost the exact same thing as we do.

It comes down to AccessManager::check() wanting a route and a request.

pwolanin’s picture

Crell had an idea about putting a subset of commonly accessed routes like those for nodes and users into PHP so it would not hit the DB.

The request creation being that slow seems odd - does that include upcasting entities?

dawehner’s picture

There was one issue which suggested to load all routes of the rendered menu tree with one sql query, another win of not relying on paths but on route names and route parameters: #2058845: Pre-load non-admin routes

Even if we were passed the route and the params already broken up, AccessManager::checkNamedRoute() does almost the exact same thing as we do.

Well sure, it was designed to replace the call to menu_item_route_access :)

The request creation being that slow seems odd - does that include upcasting entities?

No it does not. Request itself is just a gigantic beast. In order to speed it up we could indeed use duplicate() and set $request->server->REQUEST_URI for ourselves.

catch’s picture

Having two router stores - basically admin + non-admin is worth a look, have discussed that before in irc several times but I don't think there's an issue.

@pwolanin: yes it doesn't include any upcasting etc., it's just slow. Note I brought this up as a performance issue nearly a year ago now. It's only going to get worse as more conversions happen.

pwolanin’s picture

FileSize
1.62 KB

ok, so looking at the Request code, using clone or duplicate clones all the contained objects, so it should be safe to use/abuse that new object.

I just create a little phpunit as a stub for comparison (attached as patch)

for 10000 iterations

.Drupal\Tests\RequestSpeed::testCreate elapsed: ~2.6 sec

.Drupal\Tests\RequestSpeed::testDuplicate elapsed: ~0.7 sec

So duplicating does seem ~3x faster. But the time scales are much shorter than what tim mentioned - 0.3 ms not 3 ms.

catch’s picture

I had 15 calls to Request::Create() at over 3ms total for the request on the page I tested. That's close enough to 0.3ms per call so we may be talking about the same thing. If not it'd be worth double checking both ends.

dawehner’s picture

Status: Active » Needs review
FileSize
6.05 KB

I added a small helper class which does everything what ::create() does and extended the speed test from peter:

Drupal\Tests\RequestSpeed::testCreate elapsed: 1.1852490901947

Drupal\Tests\RequestSpeed::testDuplicate elapsed: 0.23370409011841

Drupal\Tests\RequestSpeed::testDuplicateWithUri elapsed: 0.41815710067749
dawehner’s picture

Another test with xdebug explicit disabled:


Drupal\Tests\RequestSpeed::testCreate elapsed: 1.239452123642

Drupal\Tests\RequestSpeed::testDuplicate elapsed: 0.24371004104614

Drupal\Tests\RequestSpeed::testDuplicateWithUri elapsed: 0.47629404067993
dawehner’s picture

FileSize
1.72 KB

that is the code i used

pwolanin’s picture

I like the idea, but the method name seems a little long?

dawehner’s picture

There we go: just using duplicate.

Status: Needs review » Needs work
Issue tags: -MenuSystemRevamp

The last submitted patch, request_duplicate-2078855-10.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review

#11: request_duplicate-2078855-10.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, request_duplicate-2078855-10.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review

#11: request_duplicate-2078855-10.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, request_duplicate-2078855-10.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
Issue tags: +MenuSystemRevamp

#11: request_duplicate-2078855-10.patch queued for re-testing.

pwolanin’s picture

So I realize it's copied from symfony, but this line is really hard to read:

+    $server['REQUEST_URI'] = $components['path'] . ('' !== $query_string ? '?' . $query_string : '');
dawehner’s picture

There we go.

pwolanin’s picture

Can we just use an if() ?

The combination of the compressed ternary with appending '?" is making me sad.

e.g.

if ($query_string !== '') {
  $query_string = '?' . $query_string
}
dawehner’s picture

I am fine with that (beside the missing semicolon ;))

pwolanin’s picture

Status: Needs review » Needs work

I'm finding the variable naming confusing.

Can we call it $post if it holds the POST data instead of $request and perhaps the new object should be called just $request?

pwolanin’s picture

Status: Needs work » Needs review
Issue tags: +Performance
FileSize
6.2 KB
2.51 KB

I think this variable naming is clearer?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Yeah this always confused me.

catch’s picture

Would be good to profile the front page before/after so we can see how much difference this makes in practice. Need to give the patch one more once-over but in general it looks great!

I've spun off the route querying which is the most expensive part to #2084675: Quering the database for router items is slow (access checking).

catch’s picture

Title: menu_item_route_access() is slow » Change notce: Request::create() is slow, use a helper to duplicate instead
Category: bug » task
Status: Reviewed & tested by the community » Active

Committed/pushed to 8.x, thanks!

We should document the addition of the request duplicate helper, this isn't the only place that could benefit from thatt.

dawehner’s picture

Status: Active » Needs review
larowlan’s picture

Can we get the full namespace of RequestHelper? Other than that, looks the goods.

pwolanin’s picture

Title: Change notce: Request::create() is slow, use a helper to duplicate instead » Request::create() is slow, use a helper to duplicate instead
Status: Needs review » Fixed

looks like it's there now.

ParisLiakos’s picture

so i opened https://github.com/symfony/symfony/issues/8998 because imho this should be pushed upstream

pwolanin’s picture

As a side note, I've been trying to use this with the breadcrumbs conversion and it doesn't behave totally as expected. In particular there are points where the language handling is wrong generating link if we tweak this code to do this right(?) think and remove the attributes from the parent request.

dawehner’s picture

Added a followup: #2090293: Fix RequestHelper

Automatically closed -- issue fixed for 2 weeks with no activity.