Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#23 | 2078855-21-23.increment.txt | 2.51 KB | pwolanin |
#23 | request_clone-2078855-23.patch | 6.2 KB | pwolanin |
#21 | request_clone-2078855-21.patch | 6.13 KB | dawehner |
#21 | interdiff.txt | 762 bytes | dawehner |
#19 | request_clone-2078855-19.patch | 6.11 KB | dawehner |
Comments
Comment #1
tim.plunkettIn
menu_item_route_access()
, we use the mocked request for two things.First, we are given
node/3/edit
and need to turn that intonode_page_edit
andarray('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.Comment #2
pwolanin CreditAttribution: pwolanin commentedCrell 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?
Comment #3
dawehnerThere 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
Well sure, it was designed to replace the call to menu_item_route_access :)
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.
Comment #4
catchHaving 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.
Comment #5
pwolanin CreditAttribution: pwolanin commentedok, 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.
Comment #6
catchI 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.
Comment #7
dawehnerI added a small helper class which does everything what ::create() does and extended the speed test from peter:
Comment #8
dawehnerAnother test with xdebug explicit disabled:
Comment #9
dawehnerthat is the code i used
Comment #10
pwolanin CreditAttribution: pwolanin commentedI like the idea, but the method name seems a little long?
Comment #11
dawehnerThere we go: just using duplicate.
Comment #13
pwolanin CreditAttribution: pwolanin commented#11: request_duplicate-2078855-10.patch queued for re-testing.
Comment #15
pwolanin CreditAttribution: pwolanin commented#11: request_duplicate-2078855-10.patch queued for re-testing.
Comment #17
pwolanin CreditAttribution: pwolanin commented#11: request_duplicate-2078855-10.patch queued for re-testing.
Comment #18
pwolanin CreditAttribution: pwolanin commentedSo I realize it's copied from symfony, but this line is really hard to read:
Comment #19
dawehnerThere we go.
Comment #20
pwolanin CreditAttribution: pwolanin commentedCan we just use an if() ?
The combination of the compressed ternary with appending '?" is making me sad.
e.g.
Comment #21
dawehnerI am fine with that (beside the missing semicolon ;))
Comment #22
pwolanin CreditAttribution: pwolanin commentedI'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?
Comment #23
pwolanin CreditAttribution: pwolanin commentedI think this variable naming is clearer?
Comment #24
dawehnerYeah this always confused me.
Comment #25
catchWould 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).
Comment #26
catchCommitted/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.
Comment #27
dawehnerhttps://drupal.org/node/2086077
Comment #28
larowlanCan we get the full namespace of RequestHelper? Other than that, looks the goods.
Comment #29
pwolanin CreditAttribution: pwolanin commentedlooks like it's there now.
Comment #30
ParisLiakos CreditAttribution: ParisLiakos commentedso i opened https://github.com/symfony/symfony/issues/8998 because imho this should be pushed upstream
Comment #31
pwolanin CreditAttribution: pwolanin commentedAs 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.
Comment #32
dawehnerAdded a followup: #2090293: Fix RequestHelper