Drupal rebuilds the router on each module installation. This is useful when adding modules once Drupal has been installed.
However, it is causing a lot of CPU cycles when Drupal is installing and the routes may be rebuilt over and over. Here's a screenshot from a Blackfire profile shaal created while we were debugging the Umami demo's installation time. The router is rebuilt 55 times before Drupal has finished being installed.
Here is the current code:
if (!\Drupal::service('router.route_provider.lazy_builder')->hasRebuilt()) {
// Rebuild routes after installing module. This is done here on top of
// \Drupal\Core\Routing\RouteBuilder::destruct to not run into errors on
// fastCGI which executes ::destruct() after the module installation
// page was sent already.
\Drupal::service('router.builder')->rebuild();
}
Adding a check to drupal_installation_attempted ()
greatly improves the installation time. Here is the comparison when we check if Drupal is installed first
if (!drupal_installation_attempted() && !\Drupal::service('router.route_provider.lazy_builder')->hasRebuilt()) {
// Rebuild routes after installing module. This is done here on top of
// \Drupal\Core\Routing\RouteBuilder::destruct to not run into errors on
// fastCGI which executes ::destruct() after the module installation
// page was sent already.
\Drupal::service('router.builder')->rebuild();
}
Comment | File | Size | Author |
---|---|---|---|
#56 | drupal-core-improve-installer-performance-8.9.x-backport-3086307-56.patch | 20.99 KB | mxr576 |
#48 | 3086307-48.patch | 20.96 KB | alexpott |
#48 | 45-48-interdiff.txt | 3.28 KB | alexpott |
#45 | 3086307-45.patch | 20.87 KB | alexpott |
#45 | 42-45-interdiff.txt | 2.44 KB | alexpott |
Comments
Comment #2
mglamanThis adds the installation check before rebuilding routes.
Comment #4
shaalFixing deprecation warning in tests.
Comment #5
mglamanI don't know how reliable the times are... but
#4: 58 min build duration;
8.8.x commit test: 1 hr 1 min build duration;
random issue: 59 min build duration;
so it might not be easy to test based on DrupalCI build times.
Comment #6
shaalI ran some blackfire.io tests using DDEV.
time blackfire run drush8 si demo_umami -y
1) Blackfire graph without the patch:
https://blackfire.io/profiles/a8f30835-bec9-4e70-90d9-cb3737f133c8/graph
2) Blackfire graph with patch: #4
https://blackfire.io/profiles/fe5231f0-b122-4109-bfeb-5b7d7685d643/graph
3) Blackfire comparison graph between the 2 previous runs
https://blackfire.io/profiles/compare/5693f6b6-85b4-41f7-9369-e1ef12525f...
TLDR: it took 12% less time for demo_umami to install after applying patch #4.
Comment #7
BerdirI'll try to test this as well a bit, a bit worried that it might break some weird module that tries to access routes e.g. in hook_install(), a think a bunch of them tend to do that.
One thing that I would suggest is to also just compare "time drush8 si demo_umami -y". blackfire, like any profiler, tends to increase fast function call costs a lot, I've seen total time execution growing by 3x or more in tests. Just the total time tends to be a better comparison. depending on how many fast function calls are inside the route rebuild vs outside, it speed it up less or theoretically even more.
Comment #8
BerdirOk, two positive results from testing.
a) Our massive install profile with 133 enabled extensions at the end installed just fine with with this patch, no errors. Only tested throug drush and not the UI, though.
b) Install time decreased significantly.
Before: 2m46.589s
After: 2m6.020s
That's 25% faster.
Comment #9
mglamanI've considered this, as well. The main thing I noticed is the call count went from 55 to 4.
Is routing considered "stable" in hook_install? Also, this is called after that I believe. So hook_modules_installed may be the hook most affected.
😱oh my, that is nice
Comment #10
andypost25% is impressive!
As CR said install hooks must rebuild router if they need fresh list
So this looks more like bugfix
Comment #11
andypostComment #12
shaalre-rolled patch #4
Testing real time installation (without blackfire)
drush si demo_umami -y
before -
97 seconds
after this patch -
84 seconds
Comment #14
chr.fritschI tried this patch with Thunder 3.
Sadly I was not able to pass the installation. It failed with
Comment #15
BerdirCan you try to get a full backtrace for this? SqlContentEntityStorage.php:847 is just where we catch the original exception and re-throw it, try a print (string) $e; or so there.
Comment #16
chr.fritschSure. Here it is. Not installing the pathauto module would fix it. Performance improvement is then from 1:39m to 1:07m
Comment #17
BerdirHm, I also have pathauto, not for users though. But that doesn't seem related to pathauto, it's just passed throuh to its methods as we override computeValue(), but then just continues to the parent? I don't quite see why disable pathauto fixes that.
Comment #18
mglamanInteresting for #16. I wonder if that means the Umami demo would also raise exceptions during installation if pathauto was configured (because it generates content during the install process, which would run pathauto.)
When saving a non-new entity, the PathFieldItemList tries to build a route:
So if any content is generated during the installation which sets a path, this error would occur.
Perhaps we should adjust the patch to have routes rebuild at the end of each of these install tasks?
Or does this prove there may be unintended consequences.
Comment #19
mglamanActually, I wonder if an implementation of \Drupal\Core\Entity\EntityTypeListenerInterface::onEntityTypeCreate just needs to rebuild routes. Then new entity routes provided by route providers would exist, solving the pathauto problem.
Proposal:
The patch as is, and a new event subscriber for routing that subscribes to \Drupal\Core\Entity\EntityTypeEvents::CREATE. There is \Drupal\Core\Entity\EntityTypeEventSubscriberTrait available to simplify the subscribe.
On \Drupal\Core\Entity\EntityTypeEventSubscriberTrait::onEntityTypeCreate and \Drupal\Core\Entity\EntityTypeEventSubscriberTrait::onEntityTypeDelete we should rebuild the router.
Comment #20
andypostWhile working on help topics stabilization (search module integration) we faced that sometimes install/uninstall caused fatals on route rebuild
I did not touch it deep but we filed separate issue #2901590: Investigate route rebuilding problem wrt Search module
Comment #21
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIMO, ~20% (average of #8 and #12) is a significant enough improvement to warrant raising the priority to Major, including that in the issue title, and tagging it as a Performance issue.
Also categorizing as a task due to the information that's in #10. If/when we discover other BC considerations, let's add them to the issue summary.
Comment #22
tstoecklerAnother possible solution would be to not rebuild the router during installation at all, but catch a
RouteNotFoundException
from withinRouteProvider::getRouteByName()
and then rebuild the router on-demand. This assumes that a) we don't catch theRouteNotFoundException
in::getRouteByName()
from within the installer (or at least not a lot) as the additional rebuild would be bad for performance then and b) that even in complex scenarios the number of times a missing route will be encountered (as is the case with Pathauto per the above) is small enough that the additional cost of throwing the exception is outweighed by the saved router rebuilds.Here's a WIP patch that does that. I verified that it fixes the issue with Pathauto noted above, but I didn't do any performance tests.
Comment #24
tstoecklerHmm... I guess there can be the case then when we never actually get around to rebuilding the router. Not sure why only the multilingual test is failing, but this should hopefully fix it. Still WIP, but hopefully something that can be used to validate any potential performance benefits.
Comment #25
mglamanCircling back, I just encountered #14 with the patch from #12.
Due to preSave op on a User and ensureComputedValue fro Drupal\\pathauto\\PathautoFieldItemList. It's not due to pathauto, though.
This is in the path module's path field list class.
Comment #26
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedPatch #24 is working with Drupal 8.8.0
Thank you
Comment #27
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedAfter a number of testing cases.
Reverted this patch #24 and #12
Waiting to untile it been committed to Drupal core
Extra modules which been installed in profiles may run into a number of issues
like devel, external links, dashboard, toolbar links
It could be the type of routes, dynamic, static, ...
I like this patch very much, It saves time on development with distributions!!
Comment #28
mglamanFollowing up here. Pathauto kills this when `user` is an enabled type. For my Pathauto borks when SiteConfigureForm saves uid 1
Comment #29
tstoecklerRe #27 and #28: Are the errors with patch #24? That is unclear to me.
Started some very basic profiling with Xdebug on this on the weekend. I only got so far, and the concrete times are not reliable because of Xdebug itself but also because I was doing other things on the machine while profiling and I only did one run each vs. having an average of multiple identical runs, etc. I do still think it's possible to make an assessment going by the relative numbers, however.
Overall installing the Umami profiles lead to 55 less router rebuilds with the patch. In 8.8.x
RouteBuilder::rebuild()
is called 57 times (which makes sense because Umami installs 50+ modules) which makes up 10.0% of execution time while with the patchNormalInstallerRouteBuilder::forceRebuild()
is called only 2 times which makes up 1.6% of execution time. This in itself is about an 8.5% reduction of the total execution time. The execution time as reported by Xdebug drops by 10%.I am assuming that most actual sites have at least that many modules and I assume that many sites have double the amount of modules due to contrib and custom modules. That would make the savings even more significant.
So would still love to see some less handwavy profiling from someone more adept at this and maybe some input from people working on bigger distributions, but I think that at least validates that the patch does what it says on the label and provides a significant reduction of installation time.
And of course would love to have some input on the concrete approach, as well.
Comment #31
catchIf we're concerned about hook_modules_installed(), we could try to do the same thing that's proposed in #3115543: Defer running of hook_modules_installed() during updates and defer it to after the final route rebuild.
Note that #3157866: Remove unnecessary route rebuild from installer is going in the opposite direction (also for a performance improvement) but that's not really incompatible - we'd have to revert that patch in here and it gives us a better optimised baseline to compare against.
Comment #32
alexpottYay for more profiling of the installer. Also we should look to see how #2488350: Switch to a memory cache backend during installation has affected this.
I think we also should do #3157866: Remove unnecessary route rebuild from installer and #3157954: Remove unnecessary route rebuild from tests first.
Comment #33
alexpottI really like the idea of deferring hook_modules_installed until we install the profile. That should be when the router is properly built for the first time during an install.
I think there might be a neater way to achieve what we want here. During module install we swap out the route provider for the lazy builder...
\Drupal::getContainer()->set('router.route_provider', \Drupal::service('router.route_provider.lazy_builder'));
What we could do is to this in the \Drupal\Core\Installer\NormalInstallerServiceProvider and then routes would only be rebuilt during an install when someone tries to use a route.
Comment #34
alexpottHere's a build that tries to avoid the rebuilds using the lazy router provider all the time during the installer. It's a different approach so no interdiff. With this patch installing umami goes from 57 route rebuilds to 4. And we can definitely reduce that by one with some of #3157866: Remove unnecessary route rebuild from installer.
Comment #35
tstoecklerWow, very impressive! That is certainly a lot nicer than #24.
Comment #36
alexpottSo #34 was not quite right - but the idea works :)
Here's a fixed patch with testing for interactive and non-interactive installs.
Still need to add a test for accessing a route in a hook_modules_installed() implementation during an install.
Comment #37
alexpottWithout the decorated service in NormalInstallerServiceProvider and ModuleInstaller...
InstallerTest fails (installing testing install profile):
InstallerPerformanceTest fails (installing standard install profile):
Comment #39
Berdir@alexpott is it possible that the number of rebuilds depends on the number of batch steps we have? that would make for an ugly random fail.
Comment #40
alexpott@Berdir I don't think so. We're only rebuilding if the routing system is actually used. I don't think \Drupal\Core\Routing\RouteBuilder::destruct() is not called at all during installation.
Comment #41
alexpottFixing the test fail.
Comment #42
alexpottAnd here we go it is possible to only build the router once during an instal :) - with this patch the standard install will only do it once.
Comment #43
mglaman@alexpott this looks great 🙌
Curious why we need this? Aren't we in NormalInstallerServiceProvider? Is this just in case someone has swapped the service,
per
But even then, do we need to check if it was this exact class?
s/builder/building ?
I also tripped up "the router for this is.."
Maybe "building the router for this route is unnecessary work"?
Comment #45
alexpottThanks for the review @mglaman.
Fixing the broken test and addressing #44.2
re #44.1 This is because of \Drupal\Core\Installer\InstallerServiceProvider - which is for the super-super early installer (before the system module is around). This is why #36 failed - because using this route provider there results in the router rebuild from being fired - and the menu tree being created :) - this will happen on the redirect to core/install.php from the frontpage.
Comment #46
borisson_I had two small questions, but I'm not sure if they are relevant. If they aren't this is rtbc as far as I'm concerned.
If we add another installer next to super early and normal, this comment would be outdated. Do we need to change this comment?
This will increase performance during install time, but are we sure that does not decrease performance during normal operation?
Comment #47
alexpottThanks for the review @borisson_ I think #46.1 is not too important it is unlikely we'll introduce yet another installer service provider - famous last words and all but the 2 should be enough.
Re #46.2 - that's a good point. Atm InstallerKernel::installationAttempted() is very cheap (but it is a method call... but we can do better. We can have a special installer version of the lazy router builder and avoid this completely for runtime.
Comment #48
alexpottFixing #46.2
Comment #49
borisson_The (relevant) remark I had earlier is fixed.
The performance of the installed application should not change compared to the current situation and installation should be a lot faster, this issue seems like a clear winner :) RTBC.
Comment #50
alexpottAlso I realised that the lazy route provider is only used in module installation so the performance improvement of #48 over #45 is irrelevant for regular runtime. It is a bit neater in terms of conditions and things to think about so #48 seems preferable.
Comment #51
catchCommitted e0d5786 and pushed to 9.1.x. Thanks!
Comment #54
xjmComment #55
ressa CreditAttribution: ressa at Ardea commentedThanks a lot for making this improvement, performance is so much better, cutting the default install time in half, and reducing Umami profile install time with roughly 25%. Testing with Lando 3.0.16 in Ubuntu 20.04:
Default installation
time drush site-install -y
Umami profile installation
time drush site-install demo_umami -y
Comment #56
mxr576I created a 8.9.x backport of #52 when I tested this fix as a possible solution for an issue that I reported in Argument 2 passed to Drupal\jsonapi\Routing\Routes::Drupal\jsonapi\Routing\{closure}() must be an instance of Drupal\jsonapi\ResourceType\ResourceType, NULL given. This patch did solve the issue completely but reduced warnings by ~80%. See comment #163 and #165.
Comment #57
smustgrave CreditAttribution: smustgrave at Mobomo commented#3038995: Remove unnecessary router rebuild from Standard installation closed as a duplicate