Total module system revamp

Oh. I left in one debugging chunk. Replaced it with in-line comments and added more comments to module_implements.

What benefits does this bring us?

removes the "load all module on bootstrap" and simplified a lot of module.inc . Lower memory footprint, bigger speed, simpler code.

Subscribing. Thanks for the work chx. Unfortunately I'm out of development cycles for the rest of this month. :(

Subscribing ...

Patch applies with 2 offsets.

However, it prevents one from logging into the site after running install.php on an empty database. The welcome email is not sent, and when going to the home page, you are not logged in. If you try, the password is never accepted.

Reversing the patch and running install.php on a fresh database works fine.

So, something is borked ...

I have seen what you describe -- it means schema is broken so user_save does not work -- however this has been fixed before patch submission and I rechecked this right now and it indeed works. Anyone else can reproduce this? If so, then how do you install? I have tried with an existing settings.php and without one so I am at a loss of how this error resurfaced. The drupal_bootstrap change and the if (defined('MAINTENANCE_MODE') && drupal_bootstrap()) { check in module_implements is responsible for the schema to work. Do you see the instal files in registry SELECT * FROM registry WHERE filename LIKE '%install'; and the schema implementations SELECT * FROM registry WHERE name LIKE '%schema';?

subscribing. will review tomorrow.

for what its worth, installing on a fresh cvs HEAD + patch worked for me.

real review tomorrow, but marking this as code needs work until we can figure out why the patch causes three failures in the System -> Module List Functionality tests.

i consistently get:

85 passes, 0 fails and 0 exceptions.

on clean cvs HEAD, and:

82 passes, 3 fails and 0 exceptions.

with the patch.

fails on:

Justin, I will check this more throughly but even without looking at the code, variable_get is static cached and module_list variable is not reloaded. So it's more the test that fails not the code. Of course it needs to be fixed and I will somehow but I do not want to deter others from review.

subscribe

@chx: i couldn't get you in #drupal or on IM, so i'll try to catch you later to discuss this.

- overall, yay for no more module_load_all in the bootstrap - once that's gone, it will be easier to find the bits of drupal that load too much code and slim them down :-)

- +1 to removing the bootstrap special cases from module_list and elsewhere

- this patch WSOD's on me quite a lot (particularly in the admin section, admin/build/block, admin/content/comment ...), so setting this back to code needs work

- the failing tests are due to changes in the function signature of module_list, which the EnableDisableCoreTestCase::assertModules hasn't been updated for. the modules enabled by simpletest are not showing up in module_exists, even though they are set to installed in system table. this is a side effect of the patch keeping the module list in the global $conf array - not sure of the best way, see comments below

- i think the .info files part of the patch, to add .install files to the list of files to parse, should go in a separate patch. that looks like an omission that should be corrected straight away regardless of how this patch goes

- same for the change to includes/theme.inc - IMO that's a simple separate patch to cleanup what we missed with the initial registry patch

- i don't like the variable_set('module_list', ...); idea. until we get something like the a static cache facility, i'd prefer to keep the refresh flag to module_list. i think its more obvious than making keeping a variable in $conf that gets rebuilt as a side-effect of a call to module_rebuild_cache.

- what's this drupal_load call for? why doesn't the registry handle this?

- i don't follow the comments next to the check for drupal_bootstrap:
+ // If we are in maintenance mode but managed to bootstrap fully then we can
+ // use the registry.
+ if (defined('MAINTENANCE_MODE') && drupal_bootstrap()) {
why do we need this now? is it because we remove the module_load_all from _drupal_bootstrap_full?

- its not obvious to me why a registry rebuild now require the following code loaded:

commenting these lines out didn't seem to cause any issues for me.

- enabling "Site building -> Testing" menu item for simpletest module doesn't appear on the modules build page after enabling the simpletest module. the item only appears on the next refresh.

the variable is because variables are cached, we save one query here . if you want to roll em separate, feel free. the comment module include is needed because i ripped out the manual install includes and that piece there uses a constant from comment module. about module_implements, if we got as far as a full bootstrap --that's what the code checks-- then we do not need the fallback any more, we have the registry to use.

+1 to the additional argument to variable_set(), I wrote this same variable_set 3 weeks ago with an argument name of persistent. As for the rest of the code I have no comment currently. I will try to include this in my benchmarking day next week.

subscribing

This is a fantastic patch. It builds so nicely on the new code registry. This is will be the most important performance patch for D7. Its also quite a nice cleanup as it ditches many special cases like bootstrap_hooks(), module_load_all_includes(),

I noted a few bugs when clicking around:
- warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, '_search_menu' was given in /Users/mw/htd/pr/includes/menu.inc on line 502.
- Call to undefined function _block_rehash() in /Users/mw/htd/pr/modules/block/block.admin.inc on line 19
- warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'trigger_access_check' was given in /Users/mw/htd/pr/includes/menu.inc on line 502.
- Fatal error: Call to undefined function openid_complete() in /Users/mw/htd/pr/modules/openid/openid.pages.inc on line 35 (when clicking on 'openid identities in profile')

From a quick code review:
- Add doxygen for new $file param for drupal_function_exists()
- This syntax looks slightly a bit awkward: drupal_load('module', 'system', 'modules/system/system.module'). i guess not enough of drupal is available to avoid hard coding that long path?

FYI for testers - In order to get this working, I had to rebuild them menu manually in index.php

The patch provided is over 3 weeks old and as per Justin's and Moshe's posts above, setting to CNW.

I have to have a look at this.

This still applies with some fuzz but has more small buglets than I care to fix. Hopefully chx or someone else can get back to it.

the WSODs reported by justin are just NOTICES. I suspect his error settings are not helpful for patch review.

As Moshe asks he is given... a reroll of a stale patch against HEAD.

After I rerolled the patch I attempted to install Drupal.

Issues:

at install.php?locale=en&profile=default I get:

seven instances of:

Right before installation is completely done I get:

Sixteen reports of:

Post installation everything seems to work.

• Logging out
• Logging back in
• Creating an article

Along with a varying number of Notices regarding $data.

steamedpenguin - the patch didn't make it.

Durn it all to heck. Lost 6 hours of patch review.

rerolled from root ;)

Also wrapped $data in isset() to remove that notice. Whether that's the best approach I have no idea, but that notice is gone at least.

Still needs #16 dealt with. And there's a lot of failures in system and menu tests.

steamedpenguin: thanks for taking this up.

attached a patch without the test/ test2/ directory prefixes in the patch file.

also, fixed warnings about undefined $data variable in registry_cache_hook_implementations.

@moshe: the WSOD are still there, clean CVS + this patch. there are still parts of drupal that are written as if the all enabled modules are loaded by the bootstrap that we haven't covered via the registry.

Justin, patchless.

/me tries again...

and fails, so here's a link...

thanks justinrendell and catch. I'll remember to patch from within.

Can someone with more code-foo write up a quick list of things patch testers need to test. It makes it easier to not just re-roll stale patches but perhaps also add fixes.

> Can someone with more code-foo write up a quick list of things patch testers need to test.

Run the SimpleTests.

Almost all should pass, should hit 100% very soon. For list of ones that don't and issues related to them see http://drupal.org/node/243773#comment-873403

Simplified the code flow a bit. I will check tests soon.

The variable stuff might overlap with #145164: New variable defaults to improve performance and help developers.

I didna ran all o' tests but ran a few and they a-workin'. batch.inc had uncoverted function_exists calls, causing mucho grief. THat's fixed now. Yay.

Rerolled against HEAD.

I've been asked to do a code review, so I'll do it file-by-file

- batch.inc
No problems spotted
- bootstrap.inc
1st hunk: When would db_set_active not be there? And if it woudln't, why not use drupal_function_exists? This could use a code comment
Everything else: looks good.
- common.inc:
2nd hunk: + // Load all essential modules <-- missing a period
Everything else: good.
- module.inc:
2nd hunk: When would you want modules not sorted by filename and weight, but rather by module name?
Everything else is fine.
- registry.inc:
Second parameter of module_implements seems to be a boolean, yet you are passing NULL.

Everything else looks fine.

Ok, I was unable to do a fresh install with that patch (the admin user won't be saved for some reason). Modifying the DB to enter worked, thou :)

Tests failing before:
- Core filters
- Poll create
- Search engine ranking (this is specific to my MySQL 5.1 installation)
- Upload user picture
- Cache expire test

Tests failing after:
- Blog API functionality
- Blog functionality
- Block functionality
- Site-wide contact form
- Personal contact form
- Forum functionality
- Core filters
- Comment functionality
- Menu item creation/deletion
- Path alias functionality
- Poll create
- Test single fields
- Test select field
- Test date field
- Test field weights
- Bike shed
- Search engine ranking
- Tracker
- Trigger content (node)
- Cache expire test

Damien and Dmitri thanks for your reviews! I am still working on this but it can't hurt to post a much better version. There are more points where we run drupal_function_exists now and I will audit all of them. Also, we store the module file name for every function in the registry and load the module as needed -- I believe the module writers are keeping common functionality in the module so if we load an include of a module then the module itself also needs to be loaded.

Also, as the only place we used sorted information was the system module page and there we run uasort($files, 'system_sort_modules_by_info_name'); anyways, I deleted all sorting from module.inc.

Fresh install worked for Dmitri and myself -- I have no clue what's Damien's problem.

Damien

I have the same problem, and was never able to complete an install with any version of this patch (have not tried in the last several days).

What happens is that the installation finishes, but you get the login boxes instead of being logged in.

Anyone facing this too? What info would help reproduce this?

Amazing patch. Great inline code comments. The registry shines.

The patch changes the overall logic of how and when code is loaded, and so maybe could use some more high-level documentation to make the new flow clear.

Previously it was clear enough, if clunky: all .module files are always loaded for all enabled modules.

Now we have on demand loading of module files as well as their includes. The key logic seems to be in the enhancements to module_implements() and drupal_function_exists(), along with tweaks to the menu system.

* On bootstrap, the minimum required modules are loaded.
* When hooks are called, all modules implementing them are loaded.
* Any call to drupal_function_exists() will load that function if available, along with its associated module file.
* The menu system passes callbacks through drupal_function_exists() so each page initializes with its required files.

Is that more or less right? If so, some suggested documentation additions.

Suggested additional documentation for function drupal_bootstrap(). (Is this the best place to put a brief summary of the overall process of loading module code on demand? Does this documentation already exist elsewhere?)

Suggested documentation for function _drupal_bootstrap()

Suggested documentation for _drupal_bootstrap_full()

Suggested changed documentation for function module_implements()()

More comments on the code and documentation:

function module_load()
- PHPDoc comments need to be updated, e.g., "Load required modules." Maybe the function name should be module_load_required().
- Could we call drupal_required_modules() here and then unset system from the array? Why isn't block module enabled? Is it no longer required? Should we remove it from drupal_required_modules()? A comment at least would be good.

function module_list()
- The static variables are leftovers and can be deleted.

install_main(), variable_set('module_list', ...)
- I didn't immediately follow why we set this temporarily. A comment before the line would ge good.

Great work chx.

(I haven't installed/tested so no comments on the reported issues.)

hook_init() looks problematic under the new approach. If we continue to invoke it in DRUPAL_BOOTSTRAP_FULL, we'll get lots of modules always loading, even though really they don't need to, since hook_init() implementations tend to be for actions needed only if the module is present (like loading CSS or JS files). e.g.,

I'm suspecting we need a change in logic here--hook_init() should be invoked for each module only after the module is loaded.

@nedjo - yes, that looks problematic. Perhaps there needs to be a different way for modules that want their CSS or JS added on every page?

• On bootstrap, the minimum required modules are loaded.
• When hooks are called or inquired about, all modules implementing them are loaded. This means module_hook, module_implements, module_invoke, module_invoke_all and even module_exists.
• Any call to drupal_function_exists will load that function if available, along with its associated module file.
• The menu system passes callbacks through drupal_function_exists so each page initializes with its required files.

hook_init will need to be split further? Likely. Attached patch is a result of me looking over core for $function calls. I will look over every call_user_func_array tomorrow.

I can't repro the install problem. If you can then please give me file level access, SSH key is here.

hook_init, move that to a module.init.inc and move on. I have added comments where it felt necessary. I did not touch drupal_bootstrap though -- high level documentation belongs to the handbook IMO. We could move all the documentation to drupal_bootstrap if we do this. I looked at every call_user_func_array -- we are good.

Reroll against HEAD.

Added a missing )

Yes. hook_init etc. is a side issue that should be dealt with in future follow up patch. There will be many other refinements we'll need to take full advantage of load on demand. We'll need to look at all hooks invoked by module_invoke_all().

hook_help for example is like hook_init--we won't want all modules that implement it to always load. We'll want to eliminate hook_form_alter() hooks wherever possible by converting them to the form-id-specific versions. Etc. Maybe we'll need something like another version of module_invoke_all() that invokes all that are already loaded rather than all that are enabled.

But all of this is detail. First we need to get this patch tested and committed.

We are nearly there, chx!

I'm still having the issue regarding the update of the admin user at the end of the installation.

On the test front (read: (-) = before the patch, (+) = after the patch):

The Upload test looks funny, but others seems reliable.

With a cleaner test environment (looks like simpletest does not work with clean urls disabled...), the same as above is true, except for the Upload test:

And the detailed list of exceptions:

After meticulously comparing installing and testing procedures with Damien it turned out that he unchecked the update module install... this lead to the temporary module list install.php set persist until the very end and that meant that when module_implements iterated the modules for schema it found only system and filter. That's very bad. I have added $permanent to variable_del too and deleted our temporary setting at the appropriate place. Install was always a mess of temporary hacks to work around the nonexsistence of database. Can't wait for PDO::pgsql to come along and solve it. The other bugs still need addressing. Can't be hard, I guess.

Huh, accidentally I upped a cruftier version.

The word "cruftier" should be used more.... This patch is looking awesome. I hope to have time for a good review on Monday.

Contact tests failed because contact.install was missed from contact.info. My bad. Easy fix. But, syslog got a syslog.install added which does not exist. Same with translation. I added a default to $module so registry tests now pass. Forum needed a "few" module_invokes thrown around so that it does not try to call taxonomy_foo without taxonomy module even loaded. Heh. Tracker needed a comment module load. Trigger and upload needs further love. If someone else wants, just follow the test manually, you will be greeted with a Fatal error undefined function error at some point. Which simpletest really should catch but that later.

@chx: in [1] I suggested to pull PHP errors, warning and notices from the tested site and show them in the simpletest report. An old patch is in [2] if you want to get the general idea. We might want to push that idea further.

[1] http://drupal.org/node/243532#comment-868123
[2] http://drupal.org/files/issues/243532-simpletest-error-reporting.patch

After some consideration and discussion with Morbus I am going to change module loading so that if a module loaded then the modules it depends upon is also loaded.

I'm getting fatal errors on admin/build/modules and a missing system_region_list function on admin/build/testing. But I'm not using a clean checkout, so this says little.

I am unable to install a new site with the patched code.

Notice: Undefined index: form in includes/form.inc on line 1349

Fatal error: Unsupported operand types in includes/form.inc on line 505

I fail at patching.

After some consideration and discussion with Morbus I am going to change module loading so that if a module loaded then the modules it depends upon is also loaded.

Storing the dependencies in the system table row might have some benefit then?

I had a bit of a look through trigger.test and I'm starting to wonder if it's a latent bug in the test rather than a bug in the patch itself. Will keep poking around, but no fatal errors/undefined functions yet.

After some discussion with chx and some deep meditation, I agree that we need to just load dependant modules as part of module_load as chx suggests in #53. The main sticky point is insuring that all constants are available when needed and that references work properly - module_invoke() is no help here.

Try as I might, I can't get hook_menu to be invoked in contrib modules anymore. This doesn't seem to be finished yet, but I don't know whether contrib modules have to do something differently or this just doesn't work yet.

@Arancaytar, after #54-56 and the fact that most core works flawlessly, I tend to doubt your bug reports. If you can find a module where the menu is not called on enabling it, then tell me. How would the tests pass otherwise...?

subscribe

While .module files were already supposed to be declared in .info, it seems that until this patch lands, modules can work without doing so. This patch makes it essential, so that's what caused my problem.
Looks okay on my test site now.

RE: #59

What about moving the module constants and global reference initialization to a module.boot file?

global reference initialization? Wow. Do you know PHP or just brainfart in my issue?

Edit: The issue is that module_invoke uses func_get_args. As reference passing happens if the function header contains reference notation, as we do not have them, passing by value happens. Read and learn before following up, please. (Yes, with PHP5 you can give even reference arguments to have defaults but then you then can't pass in constants just variables. Also, defines are still a problem even if we rewrite module_invoke to use that)

I did a visual review of the patch, because I have no access to my test environment from here.

I have one remark regarding the (ab)use of module_invoke(). I'm strongly against this: it's looks ugly to me and adds several new layers of indirection. The documentation of module_invoke is unambiguous:

Invoke a hook in a particular module.

So I don't like seeing stuff like that:

Agreed, most of them might not be necessary if we load dependencies unconditionally. But still, we should avoid using module_invoke('module', 'myfunction', $arguments...) because module_myfunction($arguments) is easier to read, simpler and more efficient.

For example: in run_tests.sh, I see:

... while I will prefer:

I also don't like drupal_function_exists() all by itself on a line, like here:

Last idea: we could modify drupal_function_exists to return FALSE if called with an empty argument, so that we can drop the first part of:

Is this a meaningful review?

I'm with you on drupal_function_exists. With a name like that, you do not expect the function to do anything, just to return a boolean.

Calling another module's function via module_invoke() is perfectly valid. No need to change this.

drupal_function_exists() is indeed misleading. However, that was introduced in the registry patch, not in this one, and can be altered in another issue.

Awesome job, chx!

Did I forget to mention that? Sorry, criticism comes more naturally than praise. This is indeed a great patch!

I guess this is now just missing the dependency loading you plan to put in (#53, #57-#59).

I agree with everything Damien said in #66. I only skimmed the code, but a more detailed review is forthcoming (after we fixed the issues in #66).

I think we need to learn to love module_invoke(). The doxygen for it should read "Load the file where this function exists and then execute it". You can't just call functions anymore because we are lazy loading files. Damien suggested calling drupal_load('module', taxonomy') first but thats a bit less robust. Those functions might move to an include file within taxonomy. Worse, taxonomy might split into 2 differently named modules. module_invoke() really is the most robust way to make function calls into files which are not your own. Perhaps we abbreviate it to mi() if the we deem the name too long.

We already had this same conversation about drupal_function_exists(). It does return a boolean as you expect. Further, it is a drop-in replacement for function_exists() we have our own version because we need to consult the registry before we can say that a function does not exist in our drupal instance. We already discussed the name for this function in the initial registry patch. If folks want to keep doing that, please use a different issue.

I agree that directly calling functions is slightly more readable but we have to suffer a little readability in order to benefit from the massive speedup that the code registry allows.

Moshe, a trade-off between performance and developer experience seems reasonable as long the performance gain is "massive" (to use your own words).

We could rename it to simply d() and make it return the function name itself (casts to TRUE anyways) and FALSE if it does not exist. call_user_func_array(d($function), $args); is now possible, if you need a guard then if (d($function)) $function(). And finally, there is nothing more, you do not call $function() without a guard.

I'd be in favour of d() - it'll be like not putting strings in without t().

call_user_func_array(d($function), $args);

It removes the fatality, but not the error. We probably won't get around the if(), which means that d() can return a pure boolean value.

t() stands for "translate"
l() stands for "link"
d() stands for..?

I'd go with f() stands for "function call" personally. Be careful about the DX for new developers.

Would this code pattern really become ubiquitous such that all function calls are passed through this loader, or could functions at least assume that other functions in their own code files, etc. are present?

It looks like we have to objectives here: reducing Drupal memory footprint (by loading less code), and improving its speed.

I'm not convinced of the benefit of the registry on memory footprint reduction: Drupal 7.x simply can't be installed with 16 Mo memory anymore (see #281405: Drupal 7.x can't be installed with memory_limit=32M). I hope that we do better with that patch.

On the other side, pure performance gains are not obvious here: true, we are loading less code but these gains are not that big (mostly thanks to op-caching), but we are also adding indirection and overhead to many function calls.

We should benchmark this and looks closely at the results.

I've looked some more at this patch today, and developer experience issues aside, I think it looks fine. I think the following things need to happen:

- Talk more about how we could optimize the developer experience. Personally, I don't think d() addresses the issues raised in #66.
- We should carefully benchmark the memory footprint improvements.
- We should carefully benchmark the performance improvements.

All of these sound fun to work on. ;-)

but we are also adding indirection and overhead to many function calls.

Functions that get called together frequently should then be placed in a single file, so they don't need to use indirection...

At present, we probably won't see a lot of improvement in our benchmarks, at least for Drupal core, because we'll need a significant amount of tuning first. For example, most or all core modules implement hook_help, so presumably will be loaded when theme('help') is called.

I was previously thinking we could wait before doing such tuning, but now I guess we at least need:

(a) different treatment for hook_init, so that it's called for a given module after that module is loaded. Without that change, all modules implementing hook_init will be loaded at full bootstrap, won't they?

(b) a new method, module_invoke_loaded() or similar, that invokes not all modules returned by module_list() but only those that are already loaded. This is what should be called for e.g. hook_help, to avoid loading all modules. Part of this would be tracking what we have already loaded. Possibly we use another temporary variable, module_loaded.

@nedjo: I don't quite get what the value of module_invoke_loaded() is. Loaded or not, we don't care, just "do X and let Drupal figure it out". What is already loaded will change depending on the situation (which page callback it is called from, what weirdness someone is doing and calling your function out of its expected environment, etc.), and you do not want the behavior of your code to be that unpredictable.

As far as benchmarks, the only decent benchmarks for this sort of work I know of are the ones I did a few months ago for Drupal 6: http://www.garfieldtech.com/blog/benchmark-page-split

There's definitely a benefit even to just splitting off all hook implementations, so perhaps we just start with that and see?

@crell: I agree with the issues you flag.

But here's the issue. 32 out of 41 core modules implement hook_help, so - since we invoke hook_help for all modules on every page - they will always load. Do we always need them? No. Usually, we only need help if the module itself is present. So, it looks to me, a lot of the benefit of this amazing patch is lost off the top on this one hook.

hook_init is similar--usually we need it only if the module is already going to be loaded. It's primarily a "define or do stuff needed by this module" sort of hook, not a "provide what other modules need" variety.

But you're right. Calling a "only if it's already loaded" hook could wreak havoc since e.g. we'll miss modules that happen to load later in the page load process.

Maybe we need to define a point when all modules that are going to load are assumed to have done so and so it's possible to call the limited set of hooks that require only already loaded modules?

Or maybe we need a set of hooks that are called for each module after that module is loaded? For hook_help, we'd need to cache the results and retrieve them at the end. (Though, again, what is the end?)

Or maybe we need to somehow use caching for e.g. hook_help, like we do with other similar "define or do stuff needed by this module" hooks like hook_theme and hook_forms?

Again, this twist won't break anything. Certainly we could wait and address it later. But meantime I suspect it will significantly impact the benchmarking results.

hook_help needs to die in favor if advanced_help or whatever the GSoC project working on it comes up with. :-)

For hook_init et al, I see two alternatives.

1) A convention of $module.init.inc for boot, init, and exit hooks. Drupal calls module_invoke_all('init') and just those small files load and run. That is what is supposed to happen.

2) We always load the .module files as now, and things that you know for a fact will always get called (or that you want always available, like direct-call API functions) live there. You then know those are always available.

I guess I don't really see why it gets more complex than that. It's just a matter of finding the right pattern of cut and paste. Once the registry initializes, the rest should just take care of itself, especially since the registry implements caching anyway.

@Crell

Thanks, I'm following this better now and I'm satisfied that one or another of the solutions you suggest will work fine.

Here's one of the points I wasn't fully grokking: since hook_implementations no longer need to be in .module files (and since we have the registry and on demand loading), we can significantly reduce .module file sizes so that loading them isn't anything like the hit it now is.

Yep, that was exactly the original intent back in November. :-) Of course, I petered out of the registry patch toward the end and chx and moshe picked it up, so I don't know if they've run across problems since then that change that. I don't want to speak for them on the current incarnation.

2) We always load the .module files as now, and things that you know for a fact will always get called (or that you want always available, like direct-call API functions) live there. You then know those are always available.

In that case, where do we move things like hook_perm and hook_menu? I'm not arguing against this alternative, just wondering where to put things that *won't* always be called, but are still almost always placed in .module.

---

Incidentally, there seems to be an unwritten convention that module.*.inc files are left up to the module developer, and "hardcoded" filenames like module.install don't have that extension. So we should decide whether the init file is hardcoded as module.init, or whether it is simply whatever file hook_init resides in.

About the hook_help issue - There is a summer of code project about the help system. One of the things that was done to help this was move help into hook_menu, meaning help is cached and only loaded unless really needed.

dmitrig01, can we start moving some of the help work into core to make it easier for this patch to be benchmarked? The sooner GSOC students start submitting their patches, the better. Don't wait until the last day ...

The problems I reported in #6 are now gone. A clean install using today's HEAD, and the patch in #51 works great.

This one implements autoloading of dependencies. I have not benchmarked yet, that will be only sensible after followup patches. But. As we do not load a boatload of modules there is no doubt that the memory footprint is smaller. Also, though there is a small overhead, even with an opcode cache, loading files also has a cost and two surely balances itself and again almost surely next to impossible to benchmark -- the cost of loading a file widely varies and, for example if you measure on your home computer running a single Apache then your modules are going to reside in the file system cache which might or might not be the case for your server always.

I ran a diffstat and compared file sizes and the size of Drupal does not change in any significant way.

The new variable needs to be built on install time so I moved the three new lines from system.admin.inc to module.inc. Now forum tests pass -- i checked manually before and that worked, but now tests work too. Added system.install to system.info which fixed the status page.

Still getting errors on trigger tests (72 fails). Otherwise everything except for the current known failures passes :)

There are two bugs, one in actions, one in trigger.install. trigger install calls actions_synchronize when not yet all modules are installed and it's totally unnecessary as install routines are calling actions_synchronize once install is done and done. But that's not engouh because actions_synchronize does not reset actions_list. So, I made two fixes: removed the needless actions_synchronize from trigger.install AND added reset to the actions_list call in actions_synchronize . Trigger tests happily pass. Note: if you were to add a trigger module to a profile you might see strange things because of this, even in D6. This part I will post separately as a bugfix for D6.

This does indeed pass all tests (that don't fail in HEAD). Will have a look through the code later.

I would like to do the drupal_function_exists => f rename later. Note that there are very very few not nice calls, one single line d_f_e in file.inc, one module_load in drupal_profile. Other than that, it's quite elegant.

Edit: I meant the Drupal profile, default.profile.

Quick visual review.

The extra module_invoke() calls which damien identified in #66 are all gone, except ni theme.inc and run-tests.sh. I'm assuming these have to be left in because they're outside modules? If so then there's no explosion of module_invoke in contrib so it ought to cover the DX concerns there.

As others have said, we already have drupal_function_exists() - extra calls to it in this patch are only areas which got missed in the original registry patch. So I think it's fine to leave a name change etc. to a different issue.

I like the change from module_list() to a variable, that's nice, and module_list() was nasty.

All the code comments look good, to be ultra-nitpicky in the doxygen for drupal_bootstrap "If left empty, then this function only returns the array of phases left."- would prefer "If called without parameters..".

I completely agree with catch.

On a visual review, here are some remaining concerns/comments (much smaller than before!):

Is $function guaranteed to exist here? If not, why not embracing the call_user_func_array() call?

What is the use of the !$hook here?

Why not using cached values while in MAINTENANCE_MODE?

Lastly, the constant MAINTENANCE_MODE seems to be used as a synonym of "the database is loaded":

Shouldn't we have something more explicit? Something like db_loaded()? This would avoid some crazy things like this:

I think we are very close to be ready to commit this :) Awesome job, chx!

Is $function guaranteed to exist here? If not, why not embracing the call_user_func_array() call? <= well, previously we took it granted. I just load it.

Edit: if you mean call_user_func_array(drupal_function_exists(... then that's exactly what i asked for above to be done in a next patch.

What is the use of the !$hook here? <= internal stuff, when you can module_implements() like that, no arguments, it writes its cache back. Many Drupal functions have similar get/set behaviours, but i did not want to write a module_implements_cache_write wrapper when it's internal anyways.

Why not using cached values while in MAINTENANCE_MODE? <= who said that cached values are there?? you mean static cache? in maint. mode you are so much better off without static caching , less things to go stale while juggling with modules.

Lastly, the constant MAINTENANCE_MODE seems to be used as a synonym of "the database is loaded": <= maybe. Way out of scope for this patch.

@chx: thanks for your answers. I don't have any objection anymore.

A very nice cleanup! Here is another code review:

* "Whether to set/unset the variable permanently or just for the time of this page" => "Whether to persist this variable value after the current request."
* this function could still be useful: module_load_all_includes($type, $name = NULL). not a big deal though. if core doesn't need it, i guess we could wait and see.
* consider a dedicated function for writing the implementations cache. module_implements() is not an obvious way to do that. (also used when rebuilding registry)
* It is slightly funny that hook_init() finally became harmless in D6 but once again will become "try not to use this" in D7. I'm not complaining, just noting it. i will add this to the api docs.

If hook_init() becomes harmful, we need another spot to put drupal_add_css() calls into. A high number of modules have a hook_init implementation only for loading their css files.

Really, these hook_init() implementations look plain wrong. Moving the hook_init() discussion to #285348: Move hook_init() implementations for JS/CSS to theming functions.

I considered and voted against a separate function for writing implementations cache -- it's not something people should call anyways. I have my ideas how to provide a possibility to load in your css and stuff. But that later. If noone has any serious consideration then...?

Comment fixes.

I'm now happy, and so are the other reviewers AFAICT.

I'd still like to see us benchmark these changes. Can we switch Drupal's memory requirements back to 8MB? What potential speed-ups are we looking at?

Fair enough ... The memory and performance gains are much stronger when you start piling on Contrib modules as most sites now do. So benchmarking core is not so representative. Alas, hardly any modules will actually run on D7 right now so I don't know that we can test that way.

Hen or egg problem. Performance improvements have to be done long before code freeze, but can't be benchmarked properly until contrib authors port their stuff. And like hell will contrib authors chase core while it's hot, porting the same code many times - I'm one and I know I wouldn't. ;)

Perhaps we need a devel generator that generates benchmarking code instead of data. A hundred little hook implementations in a dozen contrib modules, doing things like sorting big arrays, querying the database and loading big blobs of data into memory.

that's hardly "code needs work", this needs a review, a benchmark review :) if noone else then i will try to run ab... and recording memory usage meanwhile.

I think some of you need to update your mental model about how people use Drupal. ;) There are different things to look at here:

• The majority of the Drupal users use a stock Drupal core with a couple of contributed modules. They don't add more than 5 contributed modules. What does this patch mean for them? The fact that someone only uses a few modules, does not imply that he/she has a small site. Quite the contrary, I know sites in this category with a couple million forum topics or hundreds of thousands of taxonomy terms. Performance is absolutely critical for them.
• Some people build complex Drupal sites and have a lot of modules enabled. Without doubt, these people are in the minority, but they are a very important minority nonetheless. What does this patch mean for them? Again, I know sites in this category that are really small and that are not concerned about the performance of their Drupal site. At the same time, I know sites in this category that are concerned about performance.

In other words, we need to care about both (1) and (2) because there is NO CORRELATION between (a) the number of modules installed and (b) the site's performance requirements.

If this patch slows down sites in category (1) and we can't demonstrate significant gains in category (2), this will be difficult to commit. So, for this patch to be acceptable it cannot slow down core. If it actually speeds up core, it would be a no-brainer but I haven't seen (recent) evidence of that.

In other words, it makes perfect sense to rigorously test the impact of this patch on just Drupal core. If it doesn't pass the performance tests, we should go back to the drawing board and revisit some design choices.

subscribe

I can't be sure if it is related, but I'm getting these notices on the main module admin page:

* notice: Undefined index: package in modules/system/system.admin.inc on line 673.
* notice: Undefined index: #value in modules/system/system.admin.inc on line 776.

Edit: Culprit is the recently committed #229129: System module page *seriously* broken patch.

It's not. I get the same with database TNG installed - pretty sure it's #229129: System module page *seriously* broken

If you try to reverse the patch (using patch -p0 -R < ... then module.inc will have rejects and it cannot be reversed cleanly.

This attached is a re-roll that can be safely reversed for those doing tests multiple times.

Benchmarks to follow shortly ...

There is a notice: Undefined index: #value in ../HEAD/drupal/modules/system/system.admin.inc on line 776.

Then, when you try to enable devel, you get this:

You must enable the Menu module to install Devel.
Would you like to continue with enabling the above?

Of course, menu module is enabled.

So, is this something that needs to change?

Here are benchmarks, but they show little improvement. There are no contrib modules enabled (see my comment above about devel requiring menu). Just the base install.

Withpout the patch, no APC, PHP 5.2.4, one article on the front page

With the patch, no APC, PHP 5.2.4, one article on the front page

This is a small module to display the peak memory for every page load.

mem.info:

I assume with the patch, it needs this line, although I saw no output from it:

files[] = mem.module

mem.module

At minimum, you need to comment out module_invoke_all(help) wherever that happens. Ideally also do same for init hook. Otherwise, all modules are getting loaded. See nedjo's comments earlier.

I hacked the devel and devel generate modules to remove hook_init() and hook_help() and the .info to remove the menu module dependency.

I used the following configuration:

500 generated nodes, with 10 comments each.
5000 comments in total.
200 users (apart from 1 and anon)

No APC

Without the patch

With the patch

Seems like a 4 request per second different AGAINST the new patch, which does not make sense.

This did not seem right, so upon investigation, I found that the comments were not generated for the generated nodes with the new patch, which explains this since less work is done.

Trying to view a node gives this error:

Fatal error: Call to undefined function devel_generate_nodeapi() in ../HEAD/drupal/modules/node/node.module on line 703

I have this in the devel_generate.info file:

files[] = devel_generate.module
files[] = devel_generate.inc
files[] = devel_generate_batch.inc

And the first .inc has the said hook.

chx, what I am doing wrong, or how should devel_generate change to fix this? I already removed hook_help() from it.

Actually, I found out that the database does indeed contain the correct number of nodes, comments and users (500, 5000, and 202). I disabled devel_generate and I don't have the nodeapi error anymore.

Ran the test again with the patch, no APC, and this is what I get:

So, indeed the new patch is slower (20 vs 16 req/sec, 48 vs 60 ms time per request).

Any ideas as to why?

Here is a possible clue as to why ...

With the patch, devel says:

Executed 113 queries in 45.25 milliseconds. Page execution time was 175.31 ms.

Then on subsequent page views, we get:

Executed 93 queries in 11.21 milliseconds. Page execution time was 118.8 ms.

Without patch, devel says:

Executed 136 queries in 44.26 milliseconds. Page execution time was 177.53 ms.

Then on subsequent page views, we get:

Executed 73 queries in 9.54 milliseconds. Page execution time was 113.25 ms.

So the initial number of queries is more in the old scenario, but less with the newer one. However, subsequent page loads have more queries than the old scenarios. PHP execution is also a bit more?

All this without APC again.

These are the results with the patch, but with this line in includes/common.inc commented out.

That is, we do not invoke hook_init on any module.

The results are still the same (16 requests per second).

Any other scenarios?

Did you remove hook_help() at the same time?

Where would that be? I searched in common.inc but could not find it.

menu.inc apparently: menu_get_active_help() called by theme_help() in theme.inc

Thanks.

I added a return ''; at the begining of that function, and ran the test again.

Results are still the same though ...

I just noticed something I don't understand.

Without the patch, you report this test outcome:

With patch, you report this:

(alternatively 15963 bytes)

176 bytes looks characteristic for a fatal error response (parsing/missing function). Did you actually visit the unpatched test site in the browser to make sure it's running correctly? I guess that crashing halfway through could make Drupal finish faster than normal... ;)

Pardon me if I'm missing something obvious.

You are of course right.

The 176 bytes is a fatal error message from devel_generate, which I now disabled.

So here are the results without the patch:

Without the patch, APC off, with both hook_init() and hook_help() disabled.

Without the patch, APC off, with both hook_init() and hook_help() enabled (just like HEAD is), the results are:

So, we can conclude that all the 4 scenarios are the same performance wise.

- Without the patch.
- Without the patch, and hook_init() and hook_help() disabled.
- With the new patch.
- With the new patch, and hook_init() and hook_help() disabled.

So there is no improvement in terms of runtime, but at least there is no significant slow-down either.

Which is pretty good news, considering that the intent of this patch was (afaik) to decrease the memory footprint, not the execution time. It saves the parsing of code files, not the execution, after all. Is there a benchmarking tool that can show the memory usage of PHP?

Re: memory usage.

See #118. There is a module that worked before this patch that would display the peak memory for PHP.

If someone can make it work with the patch, that would be useful.

I gave this a shot.

- The patch is quite hard to test, since update 7006 (registry) is altered. So you need two db dumps to test it properly.
- Commented out module_invoke_all('init') in common.inc and added an early return in menu_get_active_help().
- Maximum execution time as well as memory usage does not change after applying the patch.

So I went ahead, enabled all core modules and inserted an echo in forum.module - which is still output on all pages. I guess this means that something else is still loading all modules.

Just re-rolled.

After some more testing and clearing cache_registry manually, I get

on all paths. The error only disappears by either visiting "admin/build/modules" or "devel/cache/clear".
After doing so, the very same error appears on "node/1". It seems that all other paths are fine, even "node". cache_registry now contains an entry for "node", but no entry for "node/1" and I'm unable to recover from this error (on this path).

Totally aside from that:
- registry_load_path_files() needs some PHPdoc.

1.) Confirmed that forum.module is first included during bootstrap with module_invoke_all('init'), but during menu_execute_active_handler when it is commented out.

2.) Tracked the second inclusion down to registry_load_path_files, which essentially pre-includes files that the cache says are required for this menu path. While testing, the registry cache had to be continually cleaned because every page load cached forum.module as a required file for this menu path.

3.) Tracked the third inclusion to _theme_build_registry() (calling hook_theme).

4.) Tracked a fourth inclusion to theme_get_settings() (calling node_get_types).

5.) Tracked a fifth inclusion to theme('blocks').

So here's how I understand it: When the theme registry is rebuilt, all modules implementing hook_theme are loaded. That's as it should be.

HOWEVER, Drupal then remembers that "hey, I had to load this file the last time I was on that page, I'd better load it again now". That doesn't make any sense - the last time Drupal built that page, it had to rebuild the theme registry and load far more code than it usually does. That extra code shouldn't be reloaded!

4 and 5, however, show that we have only laid the foundation for our intended performance boost. This patch will make it possible to load only the files that are needed, but it turns out that most of the files are needed when things like hook_blocks or hook_help or hook_node_info are invoked. The solution is either to aggressively cache this (makes sense for hook_help and hook_node_info, which are pretty static) or to refactor these hook implementations into their own files.

good catch ... in addition, menu rebuild prevent a cache write too. we need an easy way to mark a request as invalid for registry cache.

Given we know the patch isn't a performance slowdown now, it should probably go back to CNR.

There's already patches around for hook_block() and the SoC project for hook_help(), seems like some of the rest could be done independently of this patch.

Well, performance is one thing, but in its current state the registry does not seem to be failsafe, as mentioned in #133. Maybe I was too vague there:

a) If, for any reason, no cache data is available at all, Drupal spits out a fatal error on all pages until manual interaction takes place (visiting admin/build/modules).

b) In combination with a), there might also be a path mapping problem that resulted in above mentioned fatal error on all paths below node (e.g. node/1, node/add). This is still happening in my test system. There is a cache entry for node in cache_registry, but no entry for any other path below node.

c) Even if a) and b) are caused by something completely different and cannot be replicated by someone else, registry_load_path_files() definitely needs some PHPdoc:

oh well. I'm totally sorry. Reverting changes for module_invoke_all('init); in common.inc seems to fix a) and b) - albeit I have no idea why that is.

Thus, only c) is remaining.

Whoops. I fell for the same error.

Well, it seems that at least one of those core hook_init implementations is essential. Unsurprising, though I'm not sure which it is.

I suspect that it's the fact of firing a hook before menu_execute_active_handler is what's important not any hook_init implementation. Checking this theory is easy, fire "ini1" instead of "init" and see.

Just in case someone is interested, here are the results with APC enabled.

Without the patch, with APC

With the patch, with APC

The patch is consistent in that it provides the same performance, i.e. no degradation.

hook_init() and hook_help() are enabled, like in HEAD for both tests.

Note that this patch comes straight from #105 -- I have rerolled and added a few more features, one to stop cache on building, two I moved build-only code from system and user and some actions code from system too. I do not know whether this small amount of code movement around is enough -- if you benchmark this further then please post only the summary not the whole table, attach those please as a text file, because it makes the already long issue very very long. I doubt that we can make Drupal not load our modules but we can move as much code as possible from the most important modules.

/admin Call to undefined function _system_theme_data() system.module line 520

Try this please. Update module again but this time you had it on and I off.

Same problem in system.install, line 351 - when running simplestests.

Baaaaaaaaaah!

Accidentally the adaptive path cache patch got mixed into this -- sorry -- catch ran all tests and he got only the 4 path failures from that patch -- so all the tests pass.

All tests now pass. Still needs benchmarking/memory profiling.

Without the patch, APC is on.

With patch from #147, APC is on.

This is overall memory utilization at Apache's level after running the above benchmarks.

Without the patch, APC on:

With the patch, APC on, and after restarting apache

There is a minor difference in favor of the patch (see the RES column).

Without APC, which is what shared hosts most likely will have:

Without the patch:

With the patch:

There is no difference in overall Apache size without APC.

And here are the no APC performance results:

Without APC, without the patch:

No APC, With the patch

I need to get the mem.module working so we can get a more accurate per page memory picture.

Is this issue still alive, or did it fail to bring the desired performance boost and get scrapped?

Likely the main potential performance gains will require further refactoring of Drupal and so aren't captured in the existing tests.

Likely it would be useful to test the performance with this additional patch applied in advance. This would prevent the call to hook_help() on every page, anticipating a future patch where we don't load on every page every module that provides help. Possibly there are other hook calls we should also prevent to get a fairer test.

The patch in #147 no longer applies, too many failures.

However, Nedjo's patch about help not being loaded helps on its own a bit.

10 concurrent users, APC is on, no page cache nor block cache, stress test for 2 minutes.

Shaves about 20 milliseconds from each request.

With APC off, the difference is more at 30 ms.

However, this is not something that can go in on its own ...

Just a small bump. Any chance you guys could get this wonderful patch in?

Note that the theme registry rebuilding (and any other similar registry rebuilding, such as _menu) that is considered a 'not common' operation could possibly be alleviated if registry can be easily modified to have some kind of flag that says "Uncommon loading is happening on this page, please do not use this page as indicative of whether or not to auto-load files".

That would require, theoretically, very little refactoring and might get us some of the performance enhancement we seek.

The problem is figuring out how to flag that. Possibilities:

1) Code in certain files is never pre-loaded. (Eg, $module.registry.inc)

2) On certain menu router entries, skip all cache learning. (Eg, admin/build/modules)

3) Certain hooks are never pre-loaded. (Eg, hook_menu())

4) What do we do with classes? (Like the DB, all of Views, I hope handlers...)

following

Moving to D8.