Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
The installer got a 30% performance regression due to unnecessary route rebuilds in #356399: Optimize the route rebuilding process to rebuild on write.
Proposed resolution
Do not actively invoke route rebuilds during the installer, but rather postpone them to the end of the page request. That way we increase the performance of installing a module, but this also has some side effects:
- Modules that rely on updated route information in
hook_install() / hook_modules_installed()
will have to call\Drupal::service('router.builder')->rebuildIfNeeded()/->rebuild();
themselves. - Some test cases need to call \Drupal::service('router.builder')->rebuild() themselves to make sure that the route information is up to date before they make requests. One typical example: The test enables a view and then calls to
$this->drupalGet('view-page')
. In the process of the test() function, the kernel terminate method is not called yet.
Remaining tasks
Commit the patch.
User interface changes
none.
API changes
Modules have to invoke router rebuilds themselves in hook_install() if (and only if) they need up to date route information immediately.
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff.txt | 646 bytes | dawehner |
#23 | 2451665-23.patch | 16.39 KB | dawehner |
#21 | interdiff.txt | 3.61 KB | dawehner |
#21 | 2451665-21.patch | 16.27 KB | dawehner |
#19 | 2451665-19.patch | 19.56 KB | dawehner |
Comments
Comment #1
klausiComment #2
BerdirHappy to test this on my installation profile to figure out what breaks for me.
Comment #3
klausiUnpostponing.
Comment #4
dawehnerLet's see whether we can also get rid of all that calls.
Comment #6
dawehnerLet's fix it.
Time to install drupal with patch
Time to install drupal without patch
Comment #8
dawehnerMerged things together with a different patch.
Comment #9
Fabianx CreditAttribution: Fabianx commentedWasn't the reasoning to leave this in that the container is rebuild anyway when the ModuleDirectories() thing changes?
Or is it just not needed anymore?
Comment #10
Fabianx CreditAttribution: Fabianx commentedComment #11
dawehnerWell, still, we need that in order to pass some of the tests which have the route builder stored as a local variable.
In general this is state, which should tried to be stored in just one location, if possible.
Comment #12
dawehnerForgot to remove the error test completely.
Comment #13
klausiLooks good and almost ready!
Since Fabianx asked we should document why we do not use the cached yaml discovery anymore: "Always instantiate a new YamlDiscovery object so that we always search on the up-to-date list of modules."
And we can start working on the CR draft. The issue summary already documents the side effects of this change we can copy.
@Berdir: any results on testing this on your install profile yet?
Comment #14
BerdirNot yet, I'm on beta7, so I will need to test this in combination with the previous patch, assuming it will even apply.
But we just noticed in a different project that we already changed the route rebuild from happening for each module before hook_install(), so the modules that are affected by this, e.g. captcha (which is displaying some messages which I think should just be dropped) are likely already broken.
Comment #15
BerdirOk, applying those patches was tough, but it worked.
Works fine for me, the comment about captcha is bogus, we had to switch to internal: a long time ago already because already before those two issues, the rebuild happened too late for it to work.
Install time for my install profile:
Beta7:
173.62 sec, 163.24 MB
After #356399: Optimize the route rebuilding process to rebuild on write:
269.12 sec, 162.9 MB
This patch:
198.99 sec, 162.98 MB
Somehow still a bit slower, but not that bad, and much better than before. And as mentioned, zero problems, so +1 to RTBC from me when the comment above is addressed.
Comment #16
dawehnerThank you @berdir.
Comment #19
dawehnerLet's try.
Comment #21
dawehnerWhat a horrible reroll.
Comment #22
klausiCool, now let's also add the comment I pointed to in #13 and the CR draft.
Comment #23
dawehnerWrote a CR
Comment #24
klausiOK, improved the CR a bit.
It seems that the drush integration has also been fixed in the meantime to call the Kernel Terminate event, so I removed that form the issue summary.
Looks RTBC to me now!
Comment #25
Fabianx CreditAttribution: Fabianx commentedLet me see if I understand this correctly (and please update the CR some):
- If I use setRebuildNeeded() then the routes are rebuild at the end of the request
- If I use rebuild() the routes are immediately rebuld
What I don't get is:
If I use setRebuildNeeded() and then call url() - will the routes be rebuilded or not?
I assume the answer is, the routes will use the old routing table and hence I need to call rebuild() myself?
Comment #26
klausiNo, the routes will never be rebuilt if you call Url(). But that is not changed in this issue, we did this in #356399: Optimize the route rebuilding process to rebuild on write. So that would be out of scope for this change record, would you like to add something to the CR "Route rebuilding moved to an explicit model" at https://www.drupal.org/node/2452735 ?
This issue is only about the install process.
Comment #27
dawehnerExactly.
Comment #28
Fabianx CreditAttribution: Fabianx commentedThe change record is fine now :). RTBC + 1
Comment #29
alexpottThis issue is a major task that will improve performance significantly and the disruption it introduces is limited. Per https://www.drupal.org/core/beta-changes, this is a good change to complete during the Drupal 8 beta phase. Committed ae3dc62 and pushed to 8.0.x. Thanks!