Create registry for module hooks

subscribing. This has been on my radar for awhie too.

nor access nor form is a hook , node module code needs change.

I did node changes.

I moved a lot of things from module.inc to a new module_admin.inc because module.inc is loaded even for cached pages in the DRUPAL_BOOTSTRAP_LATE_PAGE_CACHE phase. I have beaten the install system until it works with the new system, at the end of the day this mostly meant changing module_invoke to module_invoke_hook, while at it I changed a few more variable functions to a module_invoke_hook -- less code is better.

All in all, Drupal installs, when you enable a new module, its install runs. I tested book, story and page creation and display and worked.

And the patch.

What performance gain do we expect from this? (low, medium, high)?

Attempting a fresh install with the patch in #7, I get:
Fatal error: Call to undefined function cache_get() in ./includes/module.inc on line 364

I runned a million installs -- but , then my settings.php was filled in. Sorry! The new system does not rely on module_list to invoke things but on the registry so in the install.inc we need to fill in the registry cache with just enough information so that form API operates -- the elements hook. This let me remove the fixed_list from module_list because that's pretty much useless now. Yay, less code! I got warnings similar to http://drupal.org/node/131942 this, I fixed them.

Size of patch tells me I forgot module_admin.inc

I benchmarked it. The results are not worth posting. They vary so much. Somtimes its faster, sometimes it's slower

subscribe

I assume we've yet to take advantage of this patch to make Drupal faster. I think it has potential, but it would be great if we could try to evaluate/estimate that potential.

The code looks good but I haven't tested it. My only gripe are the module_invoke_all_refN functions -- these are somewhat ugly. Can't we get rid of those? And if so, at what cost?

I'm going to let this patch develop a bit more ...

The function signature needs to contain a reference marker -- so in normal circumstances you can't get rid of module_invoke_all_refN . We could change to passing around an array of parameters and using extract EXTR_REFR but I doubt that'd be nice. The module_invoke_all_refN are well hidden from the user, so I do not see that as a big problem.

The big thing here is the 'file' key which is yet unused but it'll let us make Drupal smaller we all know that even with a code cache -- it takes a lot of time to load all Drupal code.

I'm all for splitting up files so that we don't load code we don't need, and I think merlin's breakdown is a not-unreasonable starting point. However, let's not forget that the volume of code is not the only problem with a full load. Hitting the disk for a file is very expensive. If we decrease the amount of code that has to be parsed per page load by 20%, but at the same time increase the number of files that need to be stat-ed on each load by 20%, that's a net loss. It's better to load a bit more code than you need and keep the file count down. Where the exact trade off point is, only benchmarks will really say for sure.

And yes, I am talking about sites without an opcode cache. Those are the sites that would benefit most from reducing the load time, since they have the stat/compile cycle on every load. opcode cached sites will still get a performance boost from less code, but normal sites will benefit from less code and less stat/compile. I recommend we target those sites when benchmarking this.

In one of the menu issue threads, which I cannot find right now of course, I had suggested instead of having an $admin flag on a menu item having an include parameter, which was an array of files to load for that callback. It would look something vaguely like this:

$items['mymodule/some/path'] = array(
  'callback' => 'drupal_get_form',
  'callback arguments' => array('myform'),
  'include' => array(drupal_get_path('module', 'mymodule') . 'somefile.inc'),
);

somefile.inc would then include all of the functions involved in that form. The upside is that other modules could add to the include array in their hook_menu_alter if necessary. The downside is that it's non-automated and therefore different modules could potentially split things up differently. I suppose that could be a plus or a minus depending on how you look at it. Views UI and the Views Theme Wizard get renamed to .inc and otherwise don't really change, which is good, but some module authors could over-modularize their modules and balloon the stat calls.

I also think that foo_node.inc or foo.node would be preferable to foo_node.module, simply for readability. (Yes, foo.node. I did go there. :-) ).

I can see one use for allowing out-of-module includes: Shared libraries and API modules. Eg, only load token module if the callback is going to care; only load some piece of XML-RPC if we're going to be doing an XML-RPC call. That would also require that the callback be allowed to specify multiple files to load (eg, the file where the form is defined and the XML-RPC library).

If we need both out-of-module includes and multiple includes, then I don't really like splitting the file and path into separate properties. Matching them up becomes problematic. Having the drupal_get_path() call in hook_menu is also better for performance since hook_menu is now called only rarely while anything in the menu dispatcher would be called on every single page load.

Of course, again we need to then be careful about splitting things too far and ballooning our stat calls. It also raises the question of whether this sort of optimization is something Drupal should be doing or the developer should be doing.

Since the degenerate case is just one big .module file, we should avoid stat-as-fallback to avoid excess IO and excess function_exists() calls. (They're probably not very expensive, but they add up.) My feeling is that we should let Drupal do the "easy" stuff (e.g., .node files) and let the developer explicitly say otherwise (e.g., put admin forms in a separate file). We don't want to have too many naming rules, or else we limit our flexibility in the future and end up with a sites directory that resembles a .jar file. :-)

I agree that page callbacks themselves shouldn't be in /includes. I was talking more about shared libraries being there that would only sometimes be needed. pager.inc, tablesort.inc, xmlrpc.inc, etc. Presumably the page callback would know if it needs those.

If that's overdoing it, then yes we probably don't need to specify a path. We may still need multiple includes, however, so that value-add modules can value-add their includes as well. Eg, only include their form_alter_thisform() (assuming that patch goes in) function and additional validate and submit handlers for the page that has that form. I think that's a not unreasonable use case, if modules are going to be spinning out some forms to separate files anyway. Again, though, I don't know what the stat impact will be.

I'm just brainstorming at this point. :-)

I'm going to attempt to summarize a 2 hour conversation in IRC last night:

My main concern was that forcing code separation would vastly increase the learning curve for new developers to create modules. While splitting code along .install files lines make sense, as these are cleanly separated as "db manipulation code," separating hooks out from .module based on their frequency of calling strikes me as error prone and confusing. Merlinofchaos assured me that this would be a purely *optional* optimization, for clean code separation and that helped a lot...

Merlinofchaos and myself proceeded to experiment by taking the functions of block, system, user, and comment module, and grouping them according to the method outlined in this issue:
http://drupal.pastebin.us/24562 (block)
http://drupal.pastebin.us/24563 (system)
http://drupal.pastebin.us/24569 (user)
http://drupal.pastebin.us/24575 (comment)

Page callback code, menu access callbacks, form handlers and such take up a *tremendous* proportion of the code in a module file. Separating these out instantly cuts the size of a module file by at least a half, often times more like 3/4 or more.

Some modules also have a disproportionate amount of 'admin' callback code vs. 'normal' page callback code. It might make sense to separate these out further into page-admin.incs or so.

My only concern is I would like to keep all hook implementations together (apart from _install and _update_N hooks, which go in .install files, and _load, _insert, _form, etc. node hooks in module.node.inc files... this way you can instantly tell before installing the module that a module provides its own node type(s)). Splitting them apart too much makes it difficult to tell what a module actually does unless you look in 4 different spots.

For what I was talking about with splitting menu handlers out into separate conditional files, see this issue: http://drupal.org/node/140218

Needs reroll and if someone calls a module_invoke on something which is not a registered a hook then we need to fall back to the current behaviour.

Module splits are in, but more to this than that, so bumping to D7.

subscribe

subscribe

I think this thread has been supplanted by this one: http://drupal.org/node/221964