Posted by catch on February 1, 2013 at 1:24pm
19 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | Performance |
Issue Summary
Sort-of follow-up to #1058720: Lazy load modules which I've marked as won't fix.
Now there's the extension handler, there's the possibility to load all modules only when hooks are invoked (ideally only if there's an implementation of a hook).
This would allow module loading as well as potentially some of the system configuration data loading to be skipped for requests that either don't need to invoke any hooks at all, or invoke one or two hooks which aren't implemented.
Comments
#1
Wonder how much this will blow up.
#2
Given how early the module handler initializes, that's not as shocking as I first thought. :-)
I suppose the real test will be moving the loadAll() call to the first invoke call. That's when things really start to asplode.
#3
At the moment that won't actually work, since a lot of hook invocations loop over the result of module_implements() then use $function or cufa() themselves.
However what we can do is loadAll() as soon as a non-empty implementation is found, that should catch 99% of cases where the modules are needed and would mean no modules loaded when there's a request with a single hook (say hook_init()) with no implementations at all, see what the bot says about this one.
#4
The last submitted patch, lazier.patch, failed testing.
#5
#3: lazier.patch queued for re-testing.
#6
The last submitted patch, lazier.patch, failed testing.
#7
I have some questions and thoughts:
* I'm still concerned about modules bringing a procedural API with them, that could be used before first module invokation?
* If the first module hook invokation remains hook_init() this has no sense because at the first hook_init() call during "full bootstrap" everything will be loaded anyway?
* Making the lazy loading lazier will also enfore the if() check to be done more frequently, at each getImplementationInfo() call, does this really worth the shot to have this check *always* running even when loaded? I know it's a simple if() but it's a simple if() that will be called *a lot*
* Wouldn't it be simpler to leverage hook groups and moving all hooks implementations to additional files in order to empty .module files whenever possible in order to minimize the "module load all" operation performance impact instead? This would also mean to do smarter groups so that we wouldn't never ever load hook_menu(), hook_flush_cache() etc... by, for example, grouping all "cache related" hooks altogether
#8
Modules only get to interact with the system via hooks and bundles. There are procedural callbacks but those are also controlled. If a modules provides a bundle that calls it's own procedural API then in most cases tough if it breaks.
We need to phase out hook_init(), per #1029460: Find a better hook than hook_init() for drupal_add_js() and drupal_add_css(). Most implementations of it only make sense for requests to full pages, for which there's hook_page_build() and similar.
Also lazy initialization will potentially help with issues like #534594: registry_rebuild() and system_rebuild_module_data depend on all modules being loaded regardless of the performance impact - modules are a service so they should be treated like one.
I'd be much more concerned about the drupal_container() calls in module_invoke_all() or any number of other more serious performance issues in Drupal 8, of which there are many documented. If we're remotely serious about lightweight bootstrap in Drupal 8 then we need to try to get rid of all of _drupal_bootstrap_code(). For example this patch should remove one database query from cached page requests using the internal page cache.
That's significantly more work, and it's work that's likely better spent moving things to CMI/plugins etc. which has the same end effect of getting it out of the module file. Also hook_hook_info() is really messy - you can't register dynamic hooks and the patches to implement it for all modules have filled me with dread.
#9
All your answers sounds right, indeed, thanks.
#10
I started looking into this to see if it was just another case of "stupid installer". Unfortunately it's not. The fix for the installer is simple but once I got past that I kept running into more places where module code is assumed to be loaded in pre-hook-invocation scenarios. The most significant place is in old-style menu callbacks and the fix for this is to add a 'module' column to the menu_router table so that we can load the .module file before trying to call the page callback.
But then there are various other places where module files are assumed to be loaded, e.g. the EntityFormController calling
field_attach_load()and apparently various other widgets and things that get called before the first time any module hook is invoked. A lot of these can be dealt with by passing the module_handler to the service in question.And then there's this in the theme() function, which is often called before the first time a hook is invoked:
<?php// If called before all modules are loaded, we do not necessarily have a full
// theme registry to work with, and therefore cannot process the theme
// request properly. See also _theme_load_registry().
if (!drupal_container()->get('module_handler')->isLoaded() && !defined('MAINTENANCE_MODE')) {
throw new Exception(t('theme() may not be called until all modules are loaded.'));
}
?>
I'm sure these problems can be overcome and am happy to work on this issue - just want to get some feedback on whether it's worth persevering given the extent of the changes required. See the interdiff for how far I went with this. No point in getting testbot to test the patch yet though as there is still much borkage :-/
#11
For page callbacks, we don't need an extra column. When we're building up the menu_router information we have the $module name available, so just default the "page file" file to be the .module file if not otherwise specified. It's included via a require_once(), so it should work fine. (And I hope we still get to remove that system entirely, but it's not gone yet.)
That said, the number of other function-based APIs we have floating around is still ugly. :-( Perhaps we should use this issue as a canary to ferret them out for prioritization for OO conversion, but not actually expect it get this committed any time soon? (Eg, "hey look, we need to make field_attach_load() and friends into an OO system next.")\
#12
Opened #1921160: Field module should implement entity hooks that's been planned for ages but I think it's usually me and yched bringing it up in random issues.
I won't release Drupal 8 with two router systems, but Crell's idea for an interim solution for hook_menu() sounds good.
For theme() the only reason that hunk is there is because the theme registry can get corrupted if it's built without all modules loaded. If we remove that line then modules should be lazy loaded, their theme hooks found, and all will be well in the world. I'm sure that's not going to be quite as easy as this, but that's one of the main goals of this issue to at least make it possible to remove those bizarre conditions.