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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi’s picture

Issue summary: View changes
Status: Active » Postponed
Berdir’s picture

Happy to test this on my installation profile to figure out what breaks for me.

klausi’s picture

Title: Don't rebuild the route on ModuleInstaller::install() » Don't rebuild the route on ModuleInstaller::install() (30% installer speedup)
Status: Postponed » Active

Unpostponing.

dawehner’s picture

Status: Active » Needs review
FileSize
15.95 KB

Let's see whether we can also get rid of all that calls.

Status: Needs review » Needs work

The last submitted patch, 4: 2451665-4.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +Performance
FileSize
588 bytes
23.2 KB

Let's fix it.

Time to install drupal with patch

       25.12 real        20.18 user         1.24 sys

Time to install drupal without patch

       34.14 real        27.67 user         1.46 sys

Status: Needs review » Needs work

The last submitted patch, 6: 2451665-6.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
18.01 KB
5.19 KB

Merged things together with a different patch.

Fabianx’s picture

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
@@ -232,10 +225,8 @@ public function destruct() {
-    if (!isset($this->yamlDiscovery)) {
-      $this->yamlDiscovery = new YamlDiscovery('routing', $this->moduleHandler->getModuleDirectories());
-    }
-    return $this->yamlDiscovery->findAll();
+    $discovery = new YamlDiscovery('routing', $this->moduleHandler->getModuleDirectories());
+    return $discovery->findAll();

Wasn'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?

Fabianx’s picture

dawehner’s picture

Well, 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.

dawehner’s picture

FileSize
16.52 KB
1.49 KB

Forgot to remove the error test completely.

klausi’s picture

Status: Needs review » Needs work

Looks good and almost ready!

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
@@ -232,10 +225,8 @@ public function destruct() {
-    if (!isset($this->yamlDiscovery)) {
-      $this->yamlDiscovery = new YamlDiscovery('routing', $this->moduleHandler->getModuleDirectories());
-    }
-    return $this->yamlDiscovery->findAll();
+    $discovery = new YamlDiscovery('routing', $this->moduleHandler->getModuleDirectories());
+    return $discovery->findAll();

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?

Berdir’s picture

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

Berdir’s picture

Ok, 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.

dawehner’s picture

Thank you @berdir.

Berdir queued 12: 2451665-12.patch for re-testing.

The last submitted patch, 12: 2451665-12.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
19.56 KB

Let's try.

Status: Needs review » Needs work

The last submitted patch, 19: 2451665-19.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.27 KB
3.61 KB

What a horrible reroll.

klausi’s picture

Status: Needs review » Needs work

Cool, now let's also add the comment I pointed to in #13 and the CR draft.

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.39 KB
646 bytes

Wrote a CR

klausi’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

OK, 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!

Fabianx’s picture

Let 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?

klausi’s picture

No, 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.

dawehner’s picture

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?

Exactly.

Fabianx’s picture

The change record is fine now :). RTBC + 1

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This 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!

  • alexpott committed ae3dc62 on 8.0.x
    Issue #2451665 by dawehner: Don't rebuild the route on ModuleInstaller::...

Status: Fixed » Closed (fixed)

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