Performance: Split .module files by moving hooks

Yeah, go Crell! :) Subscribing to test this patch.

Note: install completely fails, wsod, looking into it now.

This patch should make the testbot happy - at least for install - hope I didn't forgot any module.
Update still gives wsod, that's for later this day.

Edit: including menu.registry seemed to be needed because otherwhise, your initial menu will look very weird :)

Nice. Will run some benchmarks later.

Changing status to get results from bot.

Lovely. This can be justified just by code cleanliness. Any benchmarked speed bonus is nice but not a prerequisite IMO.

@crell update.php still gives a wsod, I'll try to look into it today - but if someone else catches it, be my guest :)

Marking to needs work for update.php

Ok, did some investigation, _theme_build_registry in theme.maintenance.inc returns no theming functions at all with this patch. I'm off for two hours now, I have some time later, again, that shouldn't stop anyone else of course :)

Sorry, I didn't catch the update.php bug. :( But I see another point:

Do we really need menu.registry.inc in install.php?
I threw it out and my install worked well.

If there should be a reason to include it though, the $module_list array keys should be consistent:

+  $module_list['menu_registry']['filename'] = 'modules/menu/menu.registry.inc';
+  drupal_load('module', 'menu_registry');

instead of:

+  $module_list['menu']['filename'] = 'modules/menu/menu.registry.inc';
+  drupal_load('module', 'menu');

Didn't mean to change status.

@pancho: I made the code more consistent. If you leave menu out, the install will be ok, but links to Main menu, Navigation, Secondary menu will be in your root navigation tree.

Pach attached fixes update.php and also a small typo in profile.info.

Cool! You caught that little bastard! :)
However, wouldn't it be cleaner to add require_once DRUPAL_ROOT . '/modules/system/system.registry.inc'; to update.php (line 650) instead of having include_once DRUPAL_ROOT . '/modules/system/system.registry.inc'; in _module_implements_maintenance()?
We obviously need it only for update.php and not in other cases of maintenance mode. It seems to work here.

Concerning menu.registry.inc in install.php:
I'm not sure if I got what you mean, but on my freshly installed site, everyting seems to be fine, though I had menu_registry thrown out of install.php. Maybe you or someone else can check that?

Moving include to update.php, makes sense and still works :)
Someone else should test play with leaving out menu.registry from install.php and see what happens.
There is a png attachment which shows what I get.

+  drupal_load('module', 'system_registry');
+  drupal_load('module', 'menu_registry');

Hum. We have system_registry and a menu_registry modules, now?

+require_once DRUPAL_ROOT . '/modules/system/system.registry.inc';

This needs a comment at the minimum. But a discussion would be better. Why not moving the theme function defined in system_theme() somewhere else? For example: automatically add them in theme.inc, where it would make full sense? The less we keep of that absurd system module, the better.

@pancho: I made the code more consistent. If you leave menu out, the install will be ok, but links to Main menu, Navigation, Secondary menu will be in your root navigation tree.

This looks like a bug in the module installation code.

@pancho: I made the code more consistent. If you leave menu out, the install will be ok, but links to Main menu, Navigation, Secondary menu will be in your root navigation tree.

This looks like a bug in the module installation code.

Ok, this is linked to the fact that module_implements() has a special case in maintenance mode in which it doesn't use the registry.

The special case for the menu module is just wrong: this can potentially break *any* module hook_enable(), including contrib ones if installed in an installation profile.

What we would need is to change the if (defined('MAINTENANCE_MODE')) check in module_implements() to something like if (defined('MAINTENANCE_MODE') && drupal_get_bootstrap_phase() < DRUPAL_BOOTSTRAP_FULL), and insure that the registry is fully built as soon as we enter the BOOTSTRAP_FULL mode (ie. we will probably need to add a registry_rebuild() in drupal_system_install().

Simply adding following line in install.php on line 37 also fixes install, anyway, do install & update need more than the theming functions defined in system_theme() ?

  require_once DRUPAL_ROOT . '/modules/system/system.registry.inc';

@swentel: In #21, I was discussing the menu.registry.inc case.

Those two pseudo modules system_registry and menu_registry look like hacks IMO. Are they really needed?

Also // $Id: $ should be // $Id$ on new files.

Other than that, the patch looks nice.

Also would be good to rename those "registry style" hooks following this pattern: {module}_register_{items}. For example:

{module}_register_blocks() for blocks
{module}_register_menu_items() for menu items
{module}_register_permissions() for permissions
{module}_register_themes() for theme hooks
...

this would add consistent name conventions and would differentiate those "registry style" hooks from other hooks.

Issue: #357321: Rename hook_permission() into hook_permission_info() for consistency

subscribe

This is one of those patches we will likely get much faster in when we

1) start with one prototype implementation

2) get that committed

3) roll sub-sequent patches (in this issue) until all of core is converted (perhaps 2-3 modules at a time)

Otherwise, this beast of a patch needs to be _carefully_ re-rolled all over the time, ensuring that no other commit changed any of those hook implementations.

So let's start with the key module.

- Added hook_perm(). Equally invoked on two pages only (admin and admin/user/permissions).

- Only added manual inclusion of system.registry.inc in install.php and update.php. Tested installation manually and could not replicate that previously mentioned menu error (was most likely a bug caused somewhere else).

- system.registry.inc needs to be manually included, because install.php and update.php rely on the path 'batch', and we can't rely on the registry during installation and upgrades.

- FWIW, by appending drupal_set_message('IN'); to system.registry.inc you will see that the registry works and the message is only output on pages that invoke one of the registry hooks. (oh wow! ;)

Before someone assumes this could be set back to needs work... ;)

I'm not sure either, but I tend to prefer #34 though.

btw: Equally fun: #467606: Move system's trigger functionality away

We really need to benchmark the memory usage. Shared hosts and dedicated servers alike use a code cache so ...if we make the life of a code cache harder then that's not good.

Via devel. There may be better ways to do this.

To me it looks like it makes 0 difference with APC. Without APC there's a very small consistent memory improvement on the front page.

On admin/build/modules the memory usage varies between requests and I can't get useful information - included below though.

Given this is just a couple of hooks the numbers seem about right to me. I think we need to proceed with this, and then do this again (or ideally something a bit more robust if someone knows how) after 5-6 patches have landed.

Restarted apache before and after applying the patch (and obviously to switch APC on and off).

With APC - identical results:
HEAD and patch - front page:
Memory used at: devel_boot()=1.5 MB, devel_shutdown()=5.75 MB, PHP peak usage=6.5 MB.

HEAD and patch on /admin/build/modules:
Memory used at: devel_boot()=1.64 MB, devel_shutdown()=12.09 MB, PHP peak usage=20 MB.

Without APC:

Front page:
HEAD:
61 files included
devel:Memory used at: devel_boot()=3.45 MB, devel_shutdown()=19.96 MB, PHP peak usage=20.75 MB.
PQP: 20.93MB

Patch:
61 files included
Memory used at: devel_boot()=3.45 MB, devel_shutdown()=19.86 MB, PHP peak usage=20.75 MB.
PQP:  20.83MB


/admin/build/modules - not measurable at this point  since it's all over the place:
HEAD:
Memory used at: devel_boot()=3.48 MB, devel_shutdown()=27.94 MB (up to 29.18 MB), PHP peak usage=36.75 MB.
64 files included

Patch:
65 files included
Memory used at: devel_boot()=3.48 MB, devel_shutdown()=27.76 MB, PHP peak usage=35.75 MB.
Memory used at: devel_boot()=3.48 MB, devel_shutdown()=27.93 MB, PHP peak usage=36.5 MB.

To me, this looks like a micro-optimization. It would be great if we could illustrate the memory usage improvement before adding new rules and complexity.

Dries. It's a micro-optimisation because we're only moving system_menu() and system_theme() for an initial patch. As far as I can see this saves us in the region of 100k of memory when APC isn't enabled.

Total file size parsed (via PQP) is 1.59MB vs. 1.57Mb with the patch applied.

Both PQP and devel report about 100k memory saved.

If we trust those numbers, then we're getting 100kb of memory saving for 20kb of code moved and it ought to add up very quickly. The patch tested is 30k, Crell's first patch on this issue was 225k. Looks to me like just by moving hook_menu() and hook_theme() for core modules we'd save in the region of 3-500kb. With 50-100 contrib modules installed (assuming they all conformed to the pattern), then that easily begins to add up to several MB of memory, and just by moving hook_menu() and hook_theme().

There's also the trigger patch which sun posted, hook_perm() a few posts above, and probably most CUD and some other hooks could be moved before we start to see too much trade off in terms of file inclusion.

The only way to test this is to try it. Either we roll and commit 5-10 similar patches quickly so that they can be maintained and rolled back individually, or someone will have to roll another monster patch, and we hope it doesn't break in time to test it and commit it. I'll try to test either, but I'm not volunteering to roll the 250k patch.

If we don't commit this, or it turns out not to have these gains, then I think we need to reconsider the registry entirely. The complexity is there, not in cut and pasting code around.

You're right. I reconsider my position. :)

OK so if you're happy committing this. I'll do a similar test once we've committed 4-5 similar patches. Then we'll 1. find out if those numbers were right in the first place 2. see if it actually adds up to anything worth having.

4-5 patches should be easy to revert if it doesn't look good, but it's probably enough to be comfortable going ahead with more conversions if we can get better verification of the gains.

I reconsidered. I want to make it one shot. But only if you let me... :P

There are things that should only be done once. ;)

Constantly re-rolling something that can be done by a machine is not the type of job we should have to deal with.

Put attached registry.php into the root directory of D7 and invoke it, i.e. http://yoursite/registry.php. Afterwards create a patch.

Now I want to see some benchmarks.

Both attached patches need to be applied to make this work.

Sorry - since I only later found out that I could also auto-add files to CVS and invoked that unconditionally, field_sql_storage.registry.inc was mistakenly added to the .info file.

New patch without that. However, lessons learned: Except of one module, all modules in core implement registry functions.

All core modules enabled (thanks for the reminder sun).

79 files included.

Front page and node/n are the same for file includes. We make the default files loaded in HEAD c. 800k smaller. This equates to just over 500k memory savings.

Very small increase in requests per second - might be margin of error, certainly not less.

HEAD:
2.07 MB total file size
Memory used at: devel_boot()=4.56 MB, devel_shutdown()=26.85 MB, PHP peak usage=27.75 MB.
3.48 [#/sec]

Patch:
1.99 MB total file size.
Memory used at: devel_boot()=5.14 MB, devel_shutdown()=26.32 MB, PHP peak usage=27.25 MB.
3.51 [#/sec]

I also did a quick poll in #drupal, asking people to use PQP to get no. files + total file size for some D6 sites with lots of contrib:

Michelle's site:
366 files 23.93mb

beeradb's site:
28.04mb, 224 files. 106 modules

agentrickard's site:
20.94 MB 180 files. This is 82 modules.

Moonshine's site:
site with 91 active modules (+ devel and pqp) 16.15MB 264 files

If we're dealing with c.2mb with just core, the multiplying that by 10 or 15 means we could end up with savings of around 5-7.5MB on sites with lots of contrib. Additionally, we're only removing a small percentage of core (800kb out of 2mb), whereas in contrib it's likely that hook_menu() and similar are a higher proportion overall (since contrib generally doesn't have files like system.module or common.inc to worry about with a few exceptions).

proper status.

The results of these benchmarks (thanks, catch!) made me a bit disappointed, but also made me "play" and think about the entire challenge.

What I did in the past hours is trial & error only, to get a feeling of the maximum limits we can achieve here.

If you want to skip this evaluation, read on where it says "BREAK". If you don't skip, bear in mind that you need to reverse the logic and meaning of everything that is said after you've read the evaluation.

Now, to test the maximum performance increase, I tried a different approach:

- Enable all core modules + admin_menu + devel + pqp.
- Don't load all modules by default.
- Only load the module that "owns" the active menu item.
- Move hook_boot, hook_init, hook_help, hook_exit into $module.bootstrap.inc files.

So essentially, and this is utopian (!), I wanted to load the bare minimum only. To achieve this, I had to additionally put some drupal_function_exists() resp. module_invoke() [which still seems to be the proper function to me] calls into those $module.bootstrap.inc files and shuffle a few other functions around where required.

$preg_hooks = array(
  'boot',
  'init',
  'help',
  'exit',
);
$filename_target = '@name.bootstrap.inc';

To start with, system.module and filter.module (primarily because of filter_xss_bad_protocol()) are required on all pages. Thus, I couldn't completely remove module_load_all() in _drupal_bootstrap_full(). I altered menu_execute_active_handler() to determine the owning module of a menu router item via preg_match(), since we do not seem to store that info yet.

Afterwards, I quickly realized that almost all modules were still loaded on all pages, because of *_alter hooks. So, we are loading many modules for hooks that test whether they may apply, but most often, they do not. Conclusion: Move them, so we load those, but not the modules.

$preg_hooks = array(
  '\w+_alter',
);
$filename_target = '@name.alter.inc';

That made an improvement, but one thing we really have to be careful about when doing benchmarks is that there must not be any PHP notice/warning during the request. Otherwise, watchdog() along with error handling is triggered, which invokes and loads at least dblog.module (more, if all core modules are enabled).

However, some modules were still being loaded without any obvious reason. To identify from where and why they were loaded, I put the following directly into .module files:

echo "<pre>"; var_dump(debug_backtrace()); echo "</pre>\n";

That uncovered that most modules were loaded because of

- arbitrary info hooks, such as hook_node_info() or hook_field_info()
- hook_elements()

I originally assumed that those would be cached already (because they end in _info() ?), but they are not. To eliminate those:

$preg_hooks = array(
  '\w+_info',
  'elements',
);
$filename_target = '@name.bootstrap.inc';

Obviously, ^^ that's wrong. Should be $module.registry.inc in reality (if they were cached).

Again, this approach is not feasible, because it requires too many drupal_function_exists() calls to load files for required functions.

Effectively, we want to reverse what I did here.

BREAK

The maximum performance gain with all core modules being enabled (but not really loaded) is:

Plain HEAD:  2.09 MB Total size, 894.180 ms Execution Time, 17.02 MB Memory Used, 75 Files
Hacked HEAD: 1.55 MB Total size, 603.790 ms Execution Time, 11.95 MB Memory Used, 80 Files

Note that 12 MB is a magic number. IIRC, shared hosts use that default PHP memory_limit setting.

Crell mentioned a master plan in IRC that I wasn't aware of yet, but makes perfectly sense:

The plan is to continue to load .module files by default, but not require the presence of a .module. Anything that is called indirectly can be moved out of .module... where the optimal place to put it is, that's still up for debate.
Functions that will be called directly.
node_load() should be in node.module.
node_node_load() can be in node.nodes.inc
That means many many many modules won't need a .module file.

So, effectively, the stuff that remains in my .module files is cruft we can move into includes. Everything else is more often needed. This is where you need to reverse the thinking if you read the above.

What we want in .module files is:

- hook_boot()
- hook_init()
- hook_exit()
- hook_help()
- hook_*alter()

What we still don't want to have in .module files is:

- hook_elements(); we can cache that info.
- hook_*info(); should be cached already, probably a bug.

Those should live in $module.registry.inc instead.

As of now, we still have a few exceptions:

- system.module: ships with many essential functions, but also many that are not needed on regular pages (e.g. trigger/actions)

- filter.module: is a bloat of its own. I don't want to release D7 without cleaning up that mess. The entire module should not have to be loaded on most pages.

- Locale module is like system.module. Note that Translation module is not.

Additionally, in terms of file size, we unconditionally load:

- common.inc: 143 KB
- theme.inc: 67 KB
- menu.inc: 90 KB
- file.inc: 68 KB
- form.inc: 107 KB (that would be a very easy one)

as well as (which don't make sense to me, since they're infrequently needed):

- pager.inc: 17 KB
- tablesort.inc: 9 KB
- mail.inc: 17 KB
- actions.inc: 12 KB

If you do the math, then you'll see that we're needlessly loading 530 KB on all pages that may only need 10% of that stuff. So there is another 25% performance increase (in addition to the 30%) only waiting for us. (On that note, core-committer note: We introduced this fine registry for D7. Let's not release with a half-baked registry. That would make no sense at all. It's about properly introducing features, not deadlines.)

The last paragraph and math becomes more important if you consider Drupal sites that do not have, for example, any publicly facing forms. Ultimately, we have to find a mean usage of functions and hooks throughout all Drupal sites, while not hindering "the more advanced" ones too much.

Overall conclusion: This is what we want to do. Starting with registry hooks and other, rarely used hooks (such as actions). It follows the principle of what I am trying to do in http://drupal.org/project/js for AJAX/AHAH callbacks that only need certain functions and/or data, but in a much cleaner way and for all requests Drupal serves.

I'm appending a list of issues that we badly need to tackle, because they disturb the whole functionality:

- drupal_function_exists() does not use drupal_load() - bad for debugging, very inconsistent.
- hook_node_info(), hook_field_info() is not cached.
- node_invoke('form') is called on all pages for administrative users due to menu_tree_page_data('management').
- hook_forms() is invoked for all forms, loads entire modules, possibly having no effect at all.
- Stuff like node_types_rebuild() does not have to be loaded all the time:

@@ -4242,7 +4244,7 @@ function drupal_flush_all_caches() {
   // Rebuild content types, menu will be rebuilt as well.
-  node_types_rebuild();
+  module_invoke('node', 'types_rebuild');

@justinrandell: I think the only reason to leave those files in, is to avoid the extra file include - on the assumption that both the .module and the .inc are going to get loaded on most requests anyway. But it's probably a detail we can sort out down the line.

To understand why we want to leave certain, essentials hooks in .module files, I took the time to prepare a readable backtrace of what is being called from where and why.

I intentionally chose a simple, administrative settings page, because that highlights a page that - basically - involves 1 module only. We are loading countless instead. I believe that this is where we should start to look at what should be moved where and why -- and which hook invocations we should simply eliminate (by caching data of _info hooks), and which hooks are essential to all pages (given that one would think that a simple settings page like this shouldn't require them).

Let's see what the bot thinks about this.

Also attached:

- registry.php: Fairly completely rewritten to do all of core in one shot. Contains the current proposal for .module splits into .inc files.

- rebuild.php: Required if you want to run registry.php manually and play with it. Rebuilds the registry.

While I'm trying to get the bot pass: Please have a look at registry.php and the hook => target associations in there. I think they're pretty solid already, so please let me know if I missed something.

Another point up for debate is: registry.php optionally allows us to re-order hooks while moving them. Enabled for a few hook sets already.

A few small things (I haven't even got to running tests yet).

The menus are out of place and the navigation menu is showing up with edit links for each menu. See: http://img.skitch.com/20090522-e8a67k6g6485ty69pna8rqpuhw.png

After clearing the cache I still get an extra menu with management and user menu items in the right place: http://img.skitch.com/20090522-kut468frrua4fy89m69ceuuc4h.png

Here's a summary of what is changed by registry.php:

module.registry.inc:
- hook_menu()
- hook_theme()
- hook_flush_caches()

module.actions.inc:
- hook_action_info()
- All action-related functions
- hook_mail()

module.user.inc:
- hook_perm()
- hook_user_after_update()
- hook_user_cancel()
- hook_user_categories()
- hook_user_form()
- hook_user_login()
- hook_user_insert()
- hook_user_operations()
- (various user operation callbacks)
- hook_user_register()
- hook_user_submit()
- hook_user_update()
- hook_user_timezone() # system only
- hook_user_validate()

module.block.inc
- hook_block_list()
- hook_block_configure()
- hook_block_save()

module.node.inc
- hook_node_access_records()
- hook_node_delete()
- hook_node_delete_revision()
- hook_node_insert()
- hook_node_prepare()
- hook_node_presave()
- hook_node_type()
- hook_node_update()
- hook_node_validate()

module.comment.inc
- hook_comment_delete()
- hook_comment_insert()
- hook_comment_publish()
- hook_comment_unpublish()
- hook_comment_update()
- hook_comment_validate()
- hook_comment_view()

module.taxonomy.inc
- hook_taxonomy() // @todo Lonely left over, should be (and is) hook_taxonomy_vocabulary_delete() instead.
- hook_taxonomy_term_insert()
- hook_taxonomy_term_update()
- hook_taxonomy_term_delete()
- hook_taxonomy_vocabulary_insert()
- hook_taxonomy_vocabulary_update()

module.search.inc
- hook_node_update_index()
- hook_ranking()
- hook_search()
- hook_search_preprocess()
- hook_search_page()
- hook_update_index()

module.cron.inc
- hook_cron()

module.xmlrpc.inc
- hook_xmlrpc()
- (various Blog API functions specific to blogapi.module)

module.form.inc
- hook_form_FORM_ID_alter()
- (NOT hook_form_alter())
- Do manually afterwards: All form handler functions, including _validate(), _submit(), _after_build(), etc.

In general, this is what is NOT moved:

- hook_init(), hook_boot(), hook_exit()
- hook_X_load()
- hook_X_view()
- API functions that are called directly, such as node_load() or user_cancel().
- hook_link(), required for rendering nodes and comments
- hook_help()
- hook_page_alter(), hook_js_alter(), hook_*_alter(), usually invoked on all pages.
- Everything else that is invoked on most pages.
- hook_*_info() - we need to made them cached. When this is done, they will be moved to module.registry.inc.
- hook_elements() - when it will be cached. When this is done, they will be moved to module.registry.inc.

So my biggest concern with this patch is balancing the performance improvements (btw, let's see some benchmarks here in the issue, please. I saw you post them on IRC...) with the developer experience aspect. What we want is to establish an easily-identifyable pattern so that module developers always know where to put their hooks. If module developers are confused about what goes where, they'll just opt for sticking everything in .module since that's what they've always done and we won't see the performance improvements replicate throughout contrib.

For the most part, this looks pretty straight-forward:

- If it has to do with loading/viewing, leave it in .module.
- If it's an API function called elsewhere, leave it in .module (this was the same rule as D6)
- If it's a X_alter_X function, leave it in .module.
- If it's a "low-level" function called on every page like hook_init(), hook_boot(), and hook_exit(), leave it in .module.

The other ones though are a bit weird. hook_link() is not intuitive. hook_help() is not intuitive. hook_X_info() is really not intuitive, and I'm tempted to mark this as postponed until those are movable to X.registry.inc.

The other thing that worries me is that this list represents A LOT of possible .inc files to keep in your head as a module developer:

1. actions
2. admin
3. block
4. cron
5. form
6. node
7. pages
8. registry
9. search
10. user
11. taxonomy
12. xmlrpc

It *almost* maps to a "use a .inc file for each module's hooks you're implementing" except that cron, form, and xmlrpc don't map like that. And admin, pages, and registry are more "meta" file names.

Let's spend some time thinking about developer experience and how we might establish some easier patterns/rules for developers that they can apply to any module they write.

Those changes to system.module (only the last, specific to it) - we leave out.

webchick was so kind to enlighten me: We have this nice list on http://api.drupal.org/api/group/hooks/7 already (which I didn't realize we have).

Those 7 fails are due to #469768: PASS.. Once that is in, this will pass. Attached is the new registry.php script that skips this new error_test.module.

Further, I realized that - due to the awesome design of the registry - the registry.php script can be applied at any time to an entire Drupal site, it works recursively, and will thereby also auto-convert any contrib modules that may exist. That aside, catch suggest to also commit this script to Deadwood/Coder module to automate this during module upgrades.

Better title.

Also, on the issues webchick raised:

• It is important that we take these optimizations serious. This is not micro-optimization - I'm expecting a performance increase of approx. 20% by this patch. We simply load too much code on all requests without needing it at all.

  That said, my development box is a bit broken currently, so I can't do proper benchmarks. I hope someone can take that job over.

• Often asked: What do others do? - Exactly the same. I would say that many developers coming from phpBB, vBulletin, Joomla and others are already used to code that's splitted into functionality.
• hook_link() belongs to loading/viewing/rendering of something, so I think it matches the pattern for leaving it in .module files.
• hook_help() is a bit hard. If Help module is disabled on sites, then we load many code on those sites that is not used at all. OTOH, if it is enabled, then we are more or less doubling the amount of include files loaded on all pages. That's a very tough issue.
• $module.registry.inc might not be a good name at all. $module.info.inc might be easier to grasp - less "meta", because actually, all hooks in there should really be suffixed with _info().
• includes/form.inc, btw, is the second largest include file we load (after common.inc). I wonder whether we really need everything in there on all requests.
• After working on this, $module.pages.inc and $module.admin.inc do not look right to me. Because they both serve the same purpose, but in different areas. $module.pages-admin.inc would be more logical (and also group them nicely together). In reality, those most often map to menu callbacks, so I think to continue the logic, they would have to be called $module.menu.inc and $module.menu-admin.inc. That way, menu handlers would also be more in line with $module.form.inc, which equally contains form handlers.
• This is not the end. As stated in #56, we can do a lot more after this patch. Specifically, and in correlation with #469698: Establish naming convention for form IDs, we should move all form handler functions into $module.form.inc. Form API uses drupal_function_exists() for all handlers already, so those can effectively live whereever we want (but just not in the .module file).

Also, I'm re-stating the list of issues we need to tackle independently:

• drupal_function_exists() does not use drupal_load() - bad for debugging, very inconsistent.
• hook_node_info(), hook_field_info() is not cached.
• node_invoke('form') is called on all pages for administrative users due to menu_tree_page_data('management').
• hook_forms() could be moved into $module.form.inc, but is invoked for system_theme_form, and thus, loaded on all pages, which makes no sense at all.
• node_types_rebuild() and related functions should live either in node.registry.inc or in node.node.inc.

Any takers for these? Let me know here or in IRC, and I'll replace/add issue links in this list.

Add tag.

Just to reiterate, the baseline performance of Drupal 7 in PHP is considerably worse than it is in Drupal 6, and there's no other way to make this particular 20% gain. So while we should try to find a good pattern, it's easy to move one or two hooks around once the initial patch is in.

hook_link() needs to go and die #451272: Rename or remove or do *something* with hook_link()

hook_help() needs to go and die #299050: Help System - Master Patch

If we release Drupal 7 with those two still existing then it will overshadow the rest of the release. Please let's not postpone this on either of those (if this gets postponed on the help patch, which is postponed on another issue which doesn't even have a patch at the moment, then we're shooting ourselves in the face).

hook_X_info() - yes we should make them consistent/cached. Is there an issue for that yet?

I also agree with Crell that the number of modules implementing all the various hooks here is likely to be extremely small - hence they'll end up with a handful of includes at most. And with an automated script provided it's easy to sort out anyway.

If no one else does I'll try to run benchmarks by the end of tomorrow, but won't have much time today. We need to benchmark with all core modules enabled, with and without APC + check number of files included and memory usage. If it really is 20% then just hitting a page with devel running ought to be enough.

#469768: PASS. went in, so this should pass.

Btw, #310467: Slimmer hook_theme() could eliminate the first 2 hunks in this patch that need to be applied manually. Can I get some reviews/feedback over there?

Note that we still have to solve the Navigation menu issue. I have no clue what happens there, but I can only guess that it must be some code that does not properly use the registry.

Also, one amendment: Since we have this script now and "re-rolling" this patch from scratch takes about 5 minutes, we can discuss a valid pattern for moving hooks and functions pretty lengthy. And I would really like us to do so. The script allows us to think about this split at a higher level before moving any stuff around. Bear in mind that we (more or less) lose CVS history for all functions that are moved. I appreciate that webchick raised this issue. I would like to see more feedback like Crell's - and we can work on fixing all related issues in the meanwhile.

This is made with a large amount of awesomesauce. I've been pushing some modules in Drupal 6 to use modulename.apimodulename.inc, and this absolutely fixes it for core in Drupal 7. Subscribing and looking forward to seeing the benchmarks. Way to go!

Benchmarks on the latest patch, all core modules enabled. 10 nodes on front page with taxonomy terms and path aliases. In all cases restarted apache then hit the front page a few times to ensure caches were fresh.

Notably there's a very small improvement with APC - as long as we don't make things worse with opcode caches I think we're fine there. Without APC it's about 3mb memory saved. Devel says anything up to 30% improvement in page execution times, ab is a bit more conservative at 6-8% - although I only benchmarked node pages for this - we'd probably be loading even less code on user pages for example.

No APC.
HEAD: 
2.88 [#/sec]
Executed 106 queries in 45 milliseconds.Page execution time was 455.5 ms.
Memory used at: devel_boot()=2.68 MB, devel_shutdown()=25.06 MB, PHP peak usage=25.75 MB.

Patch.
3.06 [#/sec]
Executed 106 queries in 44.25 milliseconds.  Page execution time was 394.02 ms.
Memory used at: devel_boot()=2.67 MB, devel_shutdown()=22.47 MB, PHP peak usage=23.25 MB.

With APC:
HEAD:
5.51 [#/sec]
Executed 106 queries in 56.05 milliseconds.  Page execution time was 241.29 ms.
Memory used at: devel_boot()=0.71 MB, devel_shutdown()=5.76 MB, PHP peak usage=6.5 MB.

Patch.
5.56 [#/sec]
Executed 106 queries in 60.41 milliseconds. Page execution time was 279.38 ms.
Memory used at: devel_boot()=0.75 MB, devel_shutdown()=5.7 MB, PHP peak usage=6.5 MB.

node/1
HEAD
3.53 [#/sec]
Executed 50 queries in 23.01 milliseconds. Page execution time was 311.08 ms.
Memory used at: devel_boot()=2.68 MB, devel_shutdown()=24.57 MB, PHP peak usage=25.25 MB.


Patch:
3.8 [#/sec]
Executed 51 queries in 23.96 milliseconds.  Page execution time was 280.2 m
Memory used at: devel_boot()=2.68 MB, devel_shutdown()=21.98 MB, PHP peak usage=22.75 MB.


To illustrate just how important this is - the Drupal 6 site my D7 benchmarking site is upgraded from:
DRUPAL 6 no APC:
node/1
4.68 [#/sec]
Executed 53 queries in 24.88 milliseconds. Page execution time was 249.94 ms.
Memory used at: devel_init()=2.24 MB, devel_shutdown()=19.42 MB.

So looks to me like a decent improvement, and closes about 40% of the current gap n PHP performance between D6 and D7.

Subscribe.

Summing up, it seems we have three kinds of functions:

- very likely to be called on any page load (eg. hook_form_alter, hook_init)
- only called on a small set of pages (eg. theme_hook, page_callback)
- cached and therefore only called on cache reset (eg. hook_menu)

A pattern that focuses exclusively on performance would put the first kind all into .module. If they're going to be loaded anyway, including a separate code file may cost time instead of saving it. The second kind would be grouped into sets most likely to be called together (eg. hook_form, hook_form_validate, hook_form_submit, theme_hook_form). The third kind would be in one separate file, since most registry rebuilds are run together.

This pattern might not be ideal for DX, however - developers might appreciate a more semantic approach of where to put functions, like $module.forms.inc, or $module.alter.inc or $module.theme.inc.

As Crell stated, we have some tradeoff between semantic refactoring and performance refactoring.

(As a matter of personal taste, I rely on the API function reference and rarely care where functions actually live - also, slow sites have given me far more trouble than APIs. So I tend more toward the "performance" route. But that's opinion.)

As a matter of fact: I was stuck finding functions in HEAD a few times already. You no longer have hook_menu() where you can lookup in which file a function lives. However, the performance increase (by far) outweighs this issue, and we should rather think about helping developers to predict the location of a function (by using a grokable pattern), but also, since we have the registry in place, I think we could add an on-site widget for developers (admin_menu, perhaps) to lookup functions.

Since Arancaytar mixed things a bit up (possibly due to my reversed analysis), I'll try to revamp the proposal.

ok, also 10 nodes on front, some taxonomy, 50 users and all core modules except upload which gives a lot of notices. Don't have APC though right now, ab with 100 requests.

/node without patch
3.02 [#/sec]
Page execution time was 327.31 ms. Executed 79 queries in 29.46 milliseconds.
Memory used at: devel_boot()=1.3 MB, devel_shutdown()=10.73 MB, PHP peak usage=11 MB.

/node with patch
3.29 [#/sec]
Page execution time was 286.23 ms. Executed 79 queries in 27.67 milliseconds.
Memory used at: devel_boot()=1.31 MB, devel_shutdown()=9.51 MB, PHP peak usage=10 MB.

/node/1 without patch
3.31 [#/sec]
Page execution time was 281.03 ms. Executed 45 queries in 27.85 milliseconds.
Memory used at: devel_boot()=1.3 MB, devel_shutdown()=10.56 MB, PHP peak usage=10.75 MB.

/node/1 with patch
3.61 [#/sec]
Page execution time was 261.56 ms. Executed 45 queries in 33.77 milliseconds.
Memory used at: devel_boot()=1.31 MB, devel_shutdown()=9.35 MB, PHP peak usage=9.75 MB.

/user/1 without patch
3.29 [#/sec]
Page execution time was 271.65 ms. Executed 31 queries in 19.4 milliseconds.
Memory used at: devel_boot()=1.3 MB, devel_shutdown()=10.21 MB, PHP peak usage=10.5 MB

/user/1 with patch
3.88 [#/sec]
Page execution time was 237.97 ms. Executed 31 queries in 17.74 milliseconds.
Memory used at: devel_boot()=1.31 MB, devel_shutdown()=9 MB, PHP peak usage=9.25 MB.

/admin/settings/site-information without patch
3.53 [#/sec]
Page execution time was 353.78 ms. Executed 29 queries in 67.14 milliseconds.
Memory used at: devel_boot()=1.3 MB, devel_shutdown()=10.91 MB, PHP peak usage=11.25 MB.

/admin/settings/site-information With patch
3.89 [#/sec]
Page execution time was 280.88 ms. Executed 30 queries in 43.02 milliseconds.
Memory used at: devel_boot()=1.31 MB, devel_shutdown()=9.7 MB, PHP peak usage=10 MB.

I agree with Arancaytar and Crell on performance vs. DX. Trying to squeeze cycles out of an overloaded server, memory limit errors - these are terrible developer experience. For that matter, waiting ages for pages to load is bad user experience. Having to grep a folder instead of one file for a function is really, really low in the scale of hard to grok things about Drupal.

Also since the only reason we're doing this is /for/ performance, there's no point doing some weird hybrid version based on what we think might make sense semantically - what makes sense is loading less code.

So far, I really like what I see here on my box.

The pattern gets a bit hairy with Node API, i.e. custom node type hooks... :( I'd consider them all in $module.node.inc, but if you think about it:

- hook_view could also work in $module.pages.inc

- hook_form|_validate|_submit could also work in $module.form.inc

- hook_insert|_update|_delete is what definitely belongs into $module.node.inc

I still prefer $module.node.inc for all. Just wanted to mention that we have one edge case in core. (the only one I was able to determine thus far)

To be continued...

Eat this.

I'd support $module.node.inc. It also sounds better than form.inc and pages.inc from the performance aspect.

When you open a node for editing, you call _load and _form (etc.). When submitting, you call _form again (FAPI rebuilds a form before executing), then _validate, then _view or _submit depending on if you previewed. The page you land on then uses _load and _view and others. It's a group whose members are frquently called together.

I'll patch, test and see if I can sniff out what testbot didn't like.

These are the tests in question:

Advanced search form	32	1	0
Bike shed	17	1	0
Block availability	50	3	0
Block functionality	144	3	0
Field form tests	114	21	2
Trigger content (node) actions	423	9	6

These are the failures I was able to replicate (35 out of 38):

Fail       Other      field.test                     975    Entity was created
Fail       Other      field.test                     1064    The response is an object
Fail       Other      field.test                     1065    Response status is true
Fail       Browser    field.test                     1034    Parsed page successfully.
Fail       Browser    field.test                     1034    Widget 0 is displayed and has the right value
Fail       Browser    field.test                     1035    Parsed page successfully.
Fail       Browser    field.test                     1035    Widget 0 has the right weight
Fail       Browser    field.test                     1034    Parsed page successfully.
Fail       Browser    field.test                     1034    Widget 1 is displayed and has the right value
Fail       Browser    field.test                     1035    Parsed page successfully.
Fail       Browser    field.test                     1035    Widget 1 has the right weight
Fail       Browser    field.test                     1034    Parsed page successfully.
Fail       Browser    field.test                     1034    Widget 2 is displayed and has the right value
Fail       Browser    field.test                     1035    Parsed page successfully.
Fail       Browser    field.test                     1035    Widget 2 has the right weight
Fail       Other      field.test                     1039    Widgets are displayed in the correct order
Fail       Browser    field.test                     1040    Parsed page successfully.
Fail       Browser    field.test                     1040    New widget is displayed
Fail       Browser    field.test                     1041    Parsed page successfully.
Fail       Browser    field.test                     1041    New widget has the right weight
Fail       Browser    field.test                     1042    Parsed page successfully.
Fail       Other      search.test                    286    Article node is not found with POST query and type:article.
Fail       Other      search.test                    223    Help text is displayed when search returns no results.
Fail       Other      search.test                    419    "Your search yielded no results" found
Fail       Other      search.test                    423    "Your search yielded no results" found
Fail       Other      search.test                    429    "Your search yielded no results" found
Fail       Other      trigger.test                   45    Make sure the page has actually been created
Fail       Other      trigger.test                   48    Make sure the publish post action fired.
Fail       Other      trigger.test                   45    Make sure the page has actually been created
Fail       Other      trigger.test                   45    Make sure the page has actually been created
Fail       Other      trigger.test                   48    Make sure the make post sticky action fired.
Fail       Other      trigger.test                   45    Make sure the page has actually been created
Fail       Other      trigger.test                   45    Make sure the page has actually been created
Fail       Other      trigger.test                   48    Make sure the promote post to front page action fired.
Fail       Other      trigger.test                   45    Make sure the page has actually been created

Thanks for listing the failing tests, Arancaytar! Let's fix those issues in #471278: Prepare module split and continue the high-level discussion here.

Now the patch in #471278: Prepare module split passed (of course). Though field attach/form tests should still fail with the split patch, and I have little idea how to fix them.

I know that this is an unfortunate situation, because #471278: Prepare module split will always pass without the split patch. However, I absolutely do not want to mix any other code changes into this monster patch. IMHO, the .module split patch should be 100% automated, so we can be sure that nothing in those functions is altered. Otherwise, this would mean that someone would have to review those 1.17 MB, which is utopian.

To be honest, this patch doesn't sit well with me ... but I'm obviously happy to discuss this more. However, given that my gut has been nagging about this for a while, I figured I'd speak up.

1. We only see a 6-8% improvement in optimal conditions. You only get that if you don't have APC enabled and when you can't cache the page. So all things considered, for most real-life scenarios, it is probably only going to speed up the site by 1-2% across page views. Plus, we have to acknowledge that all big traffic sites have APC enabled. Unlike settings up reverse proxies and whatnot, enabling APC is easy.
2. When we worked on the registry patch, we were promised "tremendous performance improvements" but so far these have not materialized yet. I don't have proof for this but if you subtract the extra overhead _introduced_ by the registry from the 6-8% gain, this might be a null operation. See #325665: Cache registry lookups and #298600: DBTNG: make module_implements work regardless of bootstrap phase for examples of overhead introduced by the registry. I realize my comment might tick people off, but I do think we need to be critical about the extra complexity (being) introduced by the registry, and weigh that with the gains. I can't help but feel like we introduced complexity, and now we're scrambling to truly take advantage of it. Yes, I think extensive splitting is scrambling (but normal intuitive splitting is not) ...
3. This patch takes an incremental step to more splitting, but it sounds like follow-up patches would split things even more. Personally, I disagree that performance is more important than developer experience in this case. I work on core almost every day, and I don't like the idea of having to deal with lots of files and scattered functions -- frankly, it makes my Drupal life less fun. It's not like it is a bad API hidden in dark corner that only frustrates me once in a while, no, this is going to affect all my Drupal work. There is a sane limit to how you should break a module's functions. My personal preference is 1 files for small modules, 2-3 files for medium sized modules, and 3-6 files for large modules. The proposal in this issue seems to take things beyond the sane limits, and it might only be a start. Some of the DX comments in this issue seem to brush this off as minor but I disagree. I'll be affected by this every day and I want Drupal work to be fun and intuitive.
4. To be frank, there might be more important performance patches to work on. Some of the other performance patches can give us a bigger bang for our buck, and they do not degrade the developer experience.

Thoughts:

Use a single .module file for development work.
Create a tool that automates the splitting of the .module file.

Perhaps this splitting happens automatically when you install the module.

Additionally, this splitting could be turned on or off, depending on if you need it. Sort of like disabling css and js aggregation when you're doing development work.

The most performance is gained if all php-files are joined in one + APC (eAccelerator) it brings near x10 - x20 times

But for common case - shared hosting is much better hold them separated (system.module hold a lot of forms that rarely used - actions, perfomance .... but this one loaded at every request)

ZF use this technique on production - all files joined + op-code cache

here example from russian forum - http://forum.dklab.ru/comments/nablas/49OptimiziruemZagruzkuPhp-kodaV22R...

# ab -n 100 -c 10

Clean PHP :
- separate files loaded:
Requests per second: 3.69 [#/sec] (mean)

- joined 5-Mb:
Requests per second: 4.85 [#/sec] (mean)

xCache
- separate files loaded:
Requests per second: 9.41 [#/sec] (mean)

- joined 5-Mb:
Requests per second: 34.54 [#/sec] (mean)

eAccelerator
- separate files loaded:
Requests per second: 14.04 [#/sec] (mean)

- joined 5-Mb:
Requests per second: 42.86 [#/sec] (mean)

1. Not all hosts have an opcode cache. One of our goals is to bring more Drupal installations to the masses. My own benchmark results on my development box have been vastly different (between 20-30%) and the overall memory consumption decreased by 4 MB (24%) on most pages. Crell mentioned that ab is testing the full time for an entire HTTP request, while Devel and other tools measure the processing of PHP.
2. The registry is the successor of the menu page callback registry and theme registry we already introduced in D6. #310467: Slimmer hook_theme() removes that remaining duplicate registry for theme functions, so developers no longer need to define 'file' in hook_theme().
3. The last patch is no longer incremental. It also moves functions that follow no naming pattern, which the script could not match. Btw, we have no benchmarks on that one yet. Only theme functions are remaining - though I could improve the script to allow to move them as well (into the file of their corresponding content builder functions).
4. I would be cautious with a statement that having everything in one .module file makes work with Drupal better. For example, it is absolutely no fun to work in bloated modules like node.module and user.module. Most often you need to grep those 100 kB files to find a function anyway. I would counter-argue: Separating functions into includes that follow a logical pattern can also improve DX with Drupal.
5. After all, I'm still happy that we really discuss this in a broader scope now. We wouldn't have if we had moved registry-style functions only. And, if the resulting decision should be to ignore the performance on hosts that don't use an opcode cache, then I would argue that we have to move functions in $module.admin.inc and $module.pages.inc back into their respective module files as well. Currently, those partially moved functionalities are the most confusing thing to me when doing Drupal development work.

@Crell - in order to convince Dries (and others like me who are mostly nodding with him), you have to address his data concern. He said 6-8% improvement under optimal conditions, and 1-2% under real world conditions (not sure if this is page speed or memory).

I'll agree that code registry has so far been disappointing. I think I even said "tremendous performance improvement" when it was under review. The killer patch which made it worthwhile got hung up on some really hard problems - #259412: Total module system revamp. Parts of that patch have been incorporated though so it is not so bad.

The registry adds a lot of complexity to the code, and mostly benefits those sites without an opcode cache. That is to say, it benefits folks who can't possibly care much about performance since they are ignoring a 9x speedup (don't quibble - it is the biggest optimization by far). There is only so much you can do to help the ignorant. Please let's not get into the poverty argument - VPS hosting is so cheap now. Managed hosting is a bit more, but often doable.

and at the third step make joined file with common part of bootstrap-code to load this once not as many files

I glad to try join all core files in one and test but code-registry is mistery for me

On the DX issue, I use vi from terminal for everything. If I'm not sure where a function lives, I do grep -r "function function_name" * from the root of my Drupal install, then vi /foo/bar/bar.module - if it's a huge module like user.module I then have to search within vim for the function itself or scroll down 4000 lines. If instead I can grep -r "function user_node_load" * and go to modules/user/user.node.inc and see all my lovely node related functions right there - that doesn't seem any worse to me at all (not necessarily a lot better, but no worse).

On speaking to sun in irc he suggested we do one more re-roll and benchmark that which makes sense to me. Also if anyone knows how to measure memory usage of apache processes (as opposed to what PHP measures itself), now would be a good time to speak up - I still feel like there should be a small memory gain for APC users if the APC cache itself doesn't have to deal with a lot of unused files.

Also a note that we should move hook_requirements() and probably hook_schema() into their own includes so huge .install dont' get loaded on admin.

Two additional facts that may not be clear to everyone yet:

1. This .module split (currently) moves all CUD in CRUD to include files. This means: On regular page views, we load the same amount of files as we do now. Only when something "happens", we load more (but only the appropriate code for "something").
2. Opcode caches usually support garbage collection. Whenever a site does not need certain hooks, and those hooks are moved away, the opcode cache will flush that garbage over time. Trivial examples are triggers/actions and XMLRPC implementations.

I'll try to run some tests to show Apache memory usage soon. I usually use eAccelerator so I'll run them with and without it enabled.

One additional point (that checking Apache memory usage should show) is that potential memory savings may allow a higher max clients, and ultimately higher concurrency on the same webserver (assuming the database is up to the load).

This patch needs to be evaluated "system as a whole" with more than what ab2 -c1 -n500 will reveal.

Attached is the latest version of registry.php.

All further fixes that were moved into #471278: Prepare module split have been commented out (and remarked in the script).

Subscribe.

In the meantime, registry.php contains a list of @todos -- bugs in Drupal core, some of them unrelated to this patch, but only uncovered due to this patch.

Did some more testing, with APC only this time, all core modules enabled.

ab -c1 n1000 - benchmarks are the same, but also looked at APC usage and apache processes via top.

With APC:
HEAD:

Apache processes (from top) - virt 249m, 29M res, 16m shr

(one process is res 56m, shr 25m, rest same as above)


Patch:

Apache processes virt 249m, res  28m, shr 14m

(one process is res 50, shr 22, rest same as above)

I've attached apc.php screenshots - these are from restarting apache, running the benchmarks, looking at apc.php. Obviously as more pages are visited on the site, the patched memory usage is going to increase as the includes get picked up - but if you have files which are very rarely or never used (say actions hooks on some sites) - then it's likely there'd be some long term savings, especially with contrib added in.

To me this looks like about a 1M memory saving with APC, at least in the 'just started up' state. If you double or treble the number of modules installed, then I reckon we're looking at 2-5M per process (given the bulk of code in core isn't contained in hooks, and lot of code in contrib is).

If you are running php as mod_php in Apache, you will see the first process to handle a request with inflated memory size, because it is the one that loaded and parsed the initial scripts. Other processes will be smaller because they fetch the cached op codes from shared memory.

A more accurate memory figure would be what the performance module (part of devel) would say.

You can select either summary or detail, in the database, and see a before and after to compare.

If you are running php as mod_php in Apache ... Other processes will be smaller because they fetch the cached op codes from shared memory.

Wrong. I thought that too until I actual tried to verify it. The opcode cache is NOT shared across processes. Every Apache process builds up its own copy. They all end up using the same amount of memory after they've each compiled more or less all the files. I verified this with APC, XCache, and eAccelerator.

If that's the case, then judging by my tests, I think we'll see small memory improvements in APC with this even after apache processes have been running for a while - because dead or very rarely used code isn't going to be loaded at all in the case of individual processes.

Yes, I think that's true: we'll see small memory usage improvements since very rarely used code won't "ever" be loaded (Apache processes die after a max # of requests so "ever" is a reasonable approximation).

The fact that opcode caches do not share memory like everyone (including me) seems to assume they do does not seem relevant to this issue. Of course, it is extremely relevant to Drupal hosting in general and I sure would like to take the time to dig into the PHP internals to find out why the memory is not being shared...

I am really wondering what advantages this patch would bring me as a person hosting a Drupal site:

1) the idea is to use less memory per process
2) yet, I have to cater for it to use the full amount of memory because if an admin works the site, all code still can be included.
3) From that follows that I can't use php.ini to restrict the available memory because admins shouldn't get WSODs
4) The only use case that I can see is that you force admins to e.g. use admin.mysite.com and normal users stay on mysite.com and configure both sites differently in apache/ php.ini.
5) Isn't that just a fringe use case?

Larry, you look at the individual page request. I am looking at an apache proess over time.

Larry told me that he considers the non-opcode case.

I don't think we should optimize for the non-opcode case.

Subscribe

I suggest to create a new issue that reverts the page callback split from D6 and moves all functions back into .module files.

Er. What?

What's so unclear? Dries, moshe, as well as killes don't see any benefit in this split.

If we do not split, then the existing split does not make any sense, too.

And if we do not split, then the registry can be reverted.

The good news: Makes many issues in the queue obsolete.

Then that simply means more work needs to be done to either sway their opinions, or come up with a compromise. Cold, hard data like benchmarks helps. Then we can get out of "what if" land and into "this is what actually happens" land.

Also, on a more personal note... sun, I don't understand this ugly anger from you the last day or so. It feels like you are shutting down discussion everywhere you go, and yet this very discussion is the critical piece for our peer review process to function. If there's something specific bothering you, I'm on IRC all hours that I'm awake, and always open for a chat.

Contrary to your usual habits, just saying that they are wrong and closing comments won't work this time.

...and yet this very discussion is the critical piece for our peer review process to function. ...

Well I agree with sun here:

The non-APC benchmarks already on this issue show decent memory and page execution savings - apparently not enough of an improvement though for Dries to think it's worth doing the split (and to be honest I wasn't particulary impressed by the results I got in testing - although seems some people got slightly better results than me).

The APC improvements are at best marginal, and may well not make any measurable difference to memory usage on real sites at all.

If we only care about optimizing of the opcode case, then there's no point in doing this, or the registry, or the page split - so we should revert all those changes and just have .module files. If we care about the non-opcode case, but aren't convinced enough by the data provided that it's a sufficient gain to make the change, then we should still revert the registry and the page split.

@webchick there's plenty of benchmarks already on this issue, from different people with different configurations - what other benchmarks do we need?

As I pointed in #94 there's a big performance degradation if modules splited in small peaces and opcode cache used - mostly caused by memory fragmentation.

So I think better focus on producing 2-tarballs -one joined into .modules only (for opcode) and another for non-opcode is splited as it now.

Unfortunately it seems like optimizing for one case directly impacts and degrades performance on the other case. There's a lot to factor in here.

We can go forward with the split which will decrease performance in environments that use an opcode cache. It will also save on some memory. How much memory is saved comes down to how much of Drupal's code is loaded over the average lifespan of an Apache child process. Looks like the benchmarks above are placing it at a couple to several megs per process (depends on contrib)

However, going forward with the split will definitely help cases where an opcode cache isn't being used. This can most often be seen in situations where PHP is being invoked through mod_suphp or mod_fastcgi (or straight CGI if you're crazy :P). Personally, the host I'm on has PHP 4 and PHP 5 side-by-side using mod_php for PHP 4 and mod_suphp for PHP 5. Since PHP 4 is a piece of crap and more and more modules require PHP 5, the site I manage is using mod_suphp and would stand to benefit from the split.

I also use mod_suphp on my personal dev server because it reduces the annoyances of having to dance around permissions.

From the developer experience perspective, I'm definitely fine with splitting modules apart as it creates additional organization, but that's just a personal preference.

Running with andypost's idea, in an ideal world we'd be able to do both. I'm not very familiar with the packaging system that goes into creating module tarballs, but it seems like it would be possible to have a module be distributed as it is today but with the addition of a modulename.optimized or something. It would basically just be all the code containing module files concatenated into one file and would be loaded on systems that are using an opcode cache (which we'd have to detect).

That said, I'm just thinking big here. I'm not in-tune enough with the various core systems (registry, module loading routines, etc.) to know if that's possible.

Improving opcode cache case as well via registry: I'm not sure whether it would work, but technically, the registry makes Drupal know of all its own code and functions. If it's true that opcode caches work faster with one huge LOB instead of many included files, then I could imagine that production sites - where code rarely changes - could use something like this:

/**
 * Implementation of hook_registry_alter().
 */
function apc_registry_alter() {
  $files = db_query("SELECT DISTINCT filename FROM {registry_file}");
  $lob = 'opindex.php';
  array_unshift($files, 'index.php');
  foreach ($files as $file) {
    file_put_contents(DRUPAL_ROOT . '/' . $lob, file_get_contents(DRUPAL_ROOT . '/' . $file), FILE_APPEND);
  }
  db_query("UPDATE {registry} SET filename = 'opindex.php'");
}

<IfModule mod_rewrite.c>
  # Rewrite URLs of the form 'x' to the form 'opindex.php?q=x'.
  RewriteCond %{REQUEST_FILENAME} !-f
  RewriteCond %{REQUEST_FILENAME} !-d
  RewriteCond %{REQUEST_URI} !=/favicon.ico
  RewriteRule ^(.*)$ opindex.php?q=$1 [L,QSA]
</IfModule>

Whatever we do, we can't leave the non-op-code cache case untended to. We have been progressively using more memory. Until recently we could run core in the default 8MB of php. Now 16MB is required. Add a few of the popular contrib and that is not enough.

If we want Drupal to continue to grow, we can't forget about shared hosting.

Personally, I stopped using shared hosting years ago, and all of our clients are on dedicated and VPS, and all have op code caches. But we still need to strike a balance between that and shared hosts.

Op code caches (e.g. APC) still issue lstat() for each file to load, and the split hurts here because there are more files to check, even if the compiled code is in memory. Run strace on a php process at some point to see that in action. Or ping me offline and I will fetch the output for you.

The registry and code split still benefits the shared hosting case, by reducing memory consumption, e.g for non admin pages. So it is not a mutually exclusive case of either/or.

Going forward, I like the idea of "compiling Drupal" into a big BLOB that makes op-code caches happier. Sun's snippet above are promising. It can also be a script that does that. chx had something to that effect earlier.

This way, we keep shared hosting sort of under control, while those who have op code cache can go further with some extra steps.

To reprise what I wrote in #93,

It seems like this splitting of module files could be automated, and selectively turned on or off. Include comments in the code that indicate where a split point should occur, and the name of the file to split it to.

Then, on installation, if splitting is enabled, the file can be automatically parsed for split points, and split out into the required parts.

Splitting for those who need it, no splitting for those who don't.

@JohnForsythe

I like that! And it makes writing modules simpler, we all still write everything in a single .module. (but perhaps less readable, because of the sheer size of each file?)

Challenge is that a new hook_menu() will have to be created by the splitting script, to say which callback is in which file.

But all in all seems doable.

+1 for for moving hooks that are only called when new things are installed, and when cache is rebuilt. Double plus one. As for anything that goes further, don't make me recite a quote about premature optimization. Hardware is a lot cheeper than drupal devs.

Let's continue this issue with splitting for shared hosts.

Suppose we should file another issue for merge.

Now #459980: Rework admin/settings/performance so we can put merge functionality into this settings or leave for contrib

Reminder: Properly splitting the code with the registry - and thereby removing all hard-coded filepath inclusions - is the only way to apply any kind of dynamic merging (or splitting) later on.

I thought more about sun's comment in #118, and I've opened an issue to examine what the consequences of that would actually look like, whether there's other alternatives etc. #497118: Remove the registry (for functions). fwiw despite the fact I came up with some reasonable arguments on the issue, overall I'd rather see this one marked fixed. What I don't want to see is this one in limbo like it is now without a wider discussion of why we ever committed the registry in the first place if we're not going to use it as it was intended.

Citing Crell:

Also, I hold that a single 5000 line file is /worse/ DX than 5 1000
line files.

I would agree, if it were a logical split, ie. from an OO point of view it would make totally sense. However, the proposed approach splits modules at rather arbitrary points. In #101 sun wrote we "move all CUD in CRUD to include files". From a developer's POV, this *is* arbitrary. As much as I am for better performance, IMO this approach will make DX worse.

#497118: Remove the registry (for functions) is about to happen.

registry-split.patch queued for re-testing.

Resetting status.