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.

Files: 
CommentFileSizeAuthor
#23 2078855-21-23.increment.txt2.51 KBpwolanin
#23 request_clone-2078855-23.patch6.2 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 59,126 pass(es).
[ View ]
#21 request_clone-2078855-21.patch6.13 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,582 pass(es).
[ View ]
#21 interdiff.txt762 bytesdawehner
#19 request_clone-2078855-19.patch6.11 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,009 pass(es).
[ View ]
#19 interdiff.txt787 bytesdawehner
#11 request_duplicate-2078855-10.patch6.03 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,588 pass(es).
[ View ]
#11 interdiff.txt2.87 KBdawehner
#9 RequestSpeed.txt1.72 KBdawehner
#7 request_duplicate-2078855-7.patch6.05 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,398 pass(es).
[ View ]
#5 request-time.txt1.62 KBpwolanin
Screen Shot 2013-09-01 at 2.26.42 PM.png120.78 KBcatch
Screen Shot 2013-09-01 at 2.27.05 PM.png138.16 KBcatch

Comments

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.

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?

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.

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.

StatusFileSize
new1.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.

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.

Status:Active» Needs review
StatusFileSize
new6.05 KB
PASSED: [[SimpleTest]]: [MySQL] 58,398 pass(es).
[ View ]

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

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

StatusFileSize
new1.72 KB

that is the code i used

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

StatusFileSize
new2.87 KB
new6.03 KB
PASSED: [[SimpleTest]]: [MySQL] 58,588 pass(es).
[ View ]

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.

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.

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.

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

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

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 : '');

StatusFileSize
new787 bytes
new6.11 KB
PASSED: [[SimpleTest]]: [MySQL] 59,009 pass(es).
[ View ]

There we go.

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
}

StatusFileSize
new762 bytes
new6.13 KB
PASSED: [[SimpleTest]]: [MySQL] 58,582 pass(es).
[ View ]

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

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?

Status:Needs work» Needs review
Issue tags:+Performance
StatusFileSize
new6.2 KB
PASSED: [[SimpleTest]]: [MySQL] 59,126 pass(es).
[ View ]
new2.51 KB

I think this variable naming is clearer?

Status:Needs review» Reviewed & tested by the community

Yeah this always confused me.

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).

Title:menu_item_route_access() is slowChange 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.

Status:Active» Needs review

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

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

looks like it's there now.

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

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.

Added a followup: #2090293: Fix RequestHelper

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