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.
Problem/Motivation
In contrast to D7 we do load a lot of files before page cache, even we don't actually need them.
Proposed resolution
Move the loads to preHandle and see how much we gain.
Remaining tasks
User interface changes
API changes
- Remove logging from ControllerResolver.
- Add method to DrupalKernel to load legacy includes.
Beta phase evaluation
Issue priority | Major because it improves performance by 25% for cached pages |
---|---|
Prioritized changes | The main goal of this issue is performance. |
Comment | File | Size | Author |
---|---|---|---|
#30 | 2392787-28.patch | 9.86 KB | alexpott |
#25 | 2392787-25.patch | 9.97 KB | Anonymous (not verified) |
#22 | 2392787-22-interdiff.txt | 694 bytes | Anonymous (not verified) |
#22 | 2392787-22.patch | 9.99 KB | Anonymous (not verified) |
#20 | 2392787-20-interdiff.txt | 1.28 KB | Anonymous (not verified) |
Comments
Comment #1
dawehnerSo the result is the following:
Before
After
Comment #3
dawehnerLet's move drupal_get_path() to bootstrap.inc to get around that.
Comment #5
BerdirSome more numbers, based on my testing, a page cache response in HEAD is 4.2ms (with the page cache no config thing enabled), with #2392433: Stream wrappers are registered before page cache, the same change as here + the removal of the injected logger from ControllerResolver (this part will be part of the next patch), it is 3.1ms
With all those changes, we're executing 2 queries, for the page cache entry and the cache tags checks for it.
That's an improvement of 25%.
Comment #6
dawehnerBefore
After
Comment #7
dawehnerComment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedi like the idea, seems like a simple change that should work.
if it doesn't, we should fix the things that stop it working.
Comment #10
dawehnerThat was a simple fix, maybe someone has a cleaner solution though.
Comment #12
Wim Leers99% of this patch is trivial moving around of code. If testbot says it's green, and I can still install Drupal manually, then it's good to go.
The only potentially contentious change is this one:
This is okay because it is only necessary for this code in
\Symfony\Component\HttpKernel\Controller\ControllerResolver
:… which is done only for DX, but that in fact also happens in
HttpKernel::handleRaw()
:So if this costs us performance, and we have an equivalent to maintain a good DX, this doesn't actually change anything.
Comment #13
BerdirShould we remove the logging code in ControllerResolver completely? If we never inject the logger, then keeping that code is a bit pointless, especially as we already deal with it one layer above?
Comment #14
Wim LeersThat's a good point. Minor, but good. Let's do that.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedlike this?
Comment #16
Wim LeersYes, exactly like that. Thanks!
Comment #17
alexpottI'm wondering if we should move this into a method so that if we ever get to the point that Drush does not need to fake a request it can do something cleanly. Tying all this includes into the internal implementation of preHandle feels wrong.
Comment #18
Fabianx CreditAttribution: Fabianx commentedRTBC +1 once #17 is addressed, a helper function makes a lot of sense.
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedok, so what should we call the method? loadLegacyCode() ? loadOldDrupalCodeOhHowIMissThee() ?
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedok, i chose loadLegacyIncludes().
Comment #21
dawehnerMeh, I guess we have to add the method to the DrupalKernelInterface?
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedmkay then.
Comment #23
Wim LeersThis should now become
{@inheritdoc}
.Comment #24
tstoecklerRe #17: Not objecting, but it is still the case that (among others) the UrlGenerator expects a RequestContext and just fatals if it's not set, so I think Drush (and any other external scripts) not having to call
preHandle()
is a pipe dream at this point. We just have to accept the fact thatpreHandle()
is the equivalent ofDRUPAL_BOOTSTRAP_FULL
now...Comment #25
Anonymous (not verified) CreditAttribution: Anonymous commentedmkay, added inheritdoc.
Comment #26
dawehner+1
Comment #28
dawehnerJust a usual reroll.
Comment #29
alexpottUpdated summary.
Comment #30
alexpottxpost that somehow deleted @dawehner's patch :(
Comment #31
webchickWow, according to the issue summary, this sounds great! :) Moving over to catch for sign-off since it deals with bootstrap performance.
Comment #32
catchThis looks good, although loading those files on page cache requests is a bad regression from Drupal 7 so it's sadly not an improvement as such.
This doesn't need profiling, just common sense.
Committed/pushed to 8.0.x, thanks!
Comment #34
Wim LeersNote: see #1 and #6 for profiling. This was also profiled as part of #2392433: Stream wrappers are registered before page cache, which is where this problem was discovered IIRC.