Make Drupal smaller: move page callbacks to their own include files

Let's stay simple for now. I am absolutely against making this an array but not now. Let's not make this issue even more clouded by hammering everything in. Small moves (tells the man who has written the kitchen sink...).

Also take a look at the locale clean-up patch -- it could take advantage of this patch.

The main motivation of this patch is performance, IMO, so let's much sure to run some benchmarks.

Simple benchmarks:

Timing:

Without APC, without patch. Drupal HEAD front page, 10 nodes, menu block. 10 reloads, time is ms from devel.module
697.36
701.08
696.91
694.29
694.51
693.44
694.14
712.12
694.59
693.39

Average: 697.18 ms
Median: 694.55 ms

Without APC, with patch. Drupal HEAD front page, 10 nodes, menu block. 10 reloads, time is ms from devel.module
632.03
635.61
632.31
634.34
631.92
631.11
631.37
630.79
633.11
641.22

Average: 633.381 ms
Median: 632.17 ms

Savings using average: 9.15%
Savings using median: 8.98%

Without APC, without patch. Drupal HEAD 'create content' page. 5 reloads, time is ms from devel.module

379.94
375.25
375.34
379.4
374.92

Average: 376.97 ms
median: 375.34 ms

Without APC, with patch. Drupal HEAD 'create content' page. 5 reloads, time is ms from devel.module

309.57
308.25
309.73
310.21
310.41

Average: 309.63 ms
Median: 309.73 ms

Savings using average: 17.86%
Savings using median: 17.48%

With APC on, the page times were close enough to identical to not bother posting.

My conclusions from the statistics above:

The savings is static; meaning the biggest gains will be seen on the pages doing the least work. This is not at all surprising, but the statistics confirm that.

On a page that basically does very little, we see an 18% performance increase from splitting up just 4 modules. There is still significant work to be done to node.module, taxonomy.module and perhaps filter.module for just the core installation.

For sites not using APC, this is a major benefit. Not all sites can use APC.

These savings are significant which makes this patch really worth pursuing.

Do we want guidelines how these files need to be split? And do we want to programatically enforce (some of) these guidelines? I'm not saying we should, but it might help to get consistency and predictability across all modules. Food for thought.

:(
The metrics are not as impressive as I'd have hoped.
Funny thing is, I'd built my module exactly like this over a year ago, specifically shoving the admin/* options off into their own little include to avoid them coming in for every page load.

Looks like it wasn't really worth the effort. Premature optimization :)

Pity, it sounds like a clever idea. Managing this bifurcated code however is probably more trouble than the win it may produce.

subscribe

My gut doesn't is a bit worried about this direction. I think that modules use each other in lots of neat ways right now. And we'll start finding in our callbacks that certain functions are not available when we want them. And then modules will start loading each others includes and it is gonna get messy.

Also, it is currently really simple and easy to write a small include script which does anything you want just by including bootstrap.php and drupal_bootstrap(), and then your own logic. This script introduces a dependency on menu system. This risk would be mitigated if we had a single function call like "load all" that setup the current environment for us.

Perhaps we introduce a new bootstrap phase DRUPAL_BOOTSTRAP_NORMAL which loads the includes as suggested in this patch. This is the standard bootstrap. Any script that needs access to unloaded stuff can do a DRUPAL_BOOTSTRAP_FULL in order to get it.

This will require some changes from this patch. Core needs to know how to load everything. I think the way toward that end is to standardize on a few include names and situations when they are loaded.

We need to also be careful about overcomplicating stuff.

Our modules used to be a single file (plus a .mysql file) that had everything in it. Then we introduced .install, then .info. Some modules already have .inc.

With this patch, we can overcomplicate things and have 7 includes for a module, ...etc.

Having admin and other makes sense. Anything over that is overkill, at least initially. Think of the entry level developers, ...etc.

This is not to say that I am against the patch. No, I am for it.

But as Dries said we need guidelines, and for those we can say start by having admin and non-admin incs only, and introduce others only as needed, and only a few advanced use cases should do that.

Then we see how it goes for one release cycle and revise the guidelines and/or the code as necessary.

One other thing that this patch should benefit is memory utilization. It is no longer possible to run Drupal with a few modules in 8M like we used to be able to. This patch should help in this direction on shared hosts.

See. I actually disagree with the fears of making things overly complicated.

Now, this may be the environ I come from -- and Dries is pretty familiar with that environ as well -- where things are broken up MUCH more granularly.

I find it very difficult to organize the code in .module files; and to find code once a module gets beyond a certain point. I think it's actually *very* good to split things up logically; it becomes a great deal more clear.

Moshe: It's very important that 'API' functions remain available, or that there are ways for API to become available. It may be as simple as modules exposing a function that loads in appropriate includes (this is probably optimal) such as Views has been doing with the 'views_load_cache()' function, or it may be wrappers that auto-load things as well (this can create extra performance problems though when files try to be included too often).

Since this particular patch only includes pages, the guidelines are fairly simple:

Your pages go in a modulename.pages.inc -- if your module provides a lot of pages and there is an easy way to split them obviously, do that. The only obvious split for existing core modules I can think of (and 99% of the modules I know of) is to separate the admin pages from the regular pages. But even that split isn't very useful for core modules. The only one I can think of that provides significant non-admin pages is...node.module.

dman: I think these numbers are quite significant. I think it's important to remember that what we have in this patch is a highly incomplete solution. It doesn't address all of the modules that are in use, nor does it take the next step and separate out hooks and such, nor does it remove theme_* functions to other files (or templates) which is work that also needs to be done.

I found an 18% savings on a page that essentially does nothing; we know from Rasmus' talk (and my own profiling which agrees with his numbers) that 50% of a simple page is taken up by loading modules. That means that, with a little more work, we could probably turn that 18% into 35%, simple ensuring that modules are as trim as possible. We might be able to further enhance things so that the core include/ files are slimmer too. I think this patch is a big step but it is not the whole trip, and we have to keep in mind that seeing a 35% to 40% load time reduction will take a lot more effort.

I also think that what I didn't look into was the amount of memory Drupal uses per load, and that's another thing we should be looking at. Drupal uses too much memory, and reducing the amount of code loaded will also help that problem.

BTW, I want to throw in my vote that 'includes' become 'page file' which matches the syntax used in other, similar patches that allow registration of something to include a filename, and be singular. I don't see much benefit to multiple-include-at-registration, it only complicates matters. Also, I think we should make pathing consistent with the theme registry, where the path adding is automatic but another variable can be used to override the guessing.

Memory usage, via devel:

Prior to patch:
* Memory used at devel_init(): 6.27 MB
* Memory used at devel_shutdown(): 6.49 MB

With patch:
* Memory used at devel_init(): 4.74 MB
* Memory used at devel_shutdown(): 4.97 MB

I think we can get that down a whole lot farther without adding significant complexity.

Earl said:

It may be as simple as modules exposing a function that loads in appropriate includes (this is probably optimal) such as Views has been doing with the 'views_load_cache()' function

Right. Thus all your API functions are going to have to live in .module, right?

Perhaps the menu part of this patch isn't needed at all? Couldn't modules organize themselves so that less code is included on every request? I can see some benefit to the menu system auto-loading an admin include but thats about it.

Assuming we proceed, I like Earl's suggestion about having menu system guess the right path. We could go further and have it guess the right include file. For example, a callback function named aggregator_page_sources would trigger an automatic load attempt of page.inc in the block directory. 'page' comes from the second part of the callback function name.same for aggregator_admin_settings(). if the guessed filename does not exist, it is not loaded (obviously).

It seems to me that together with the revamped menu system (and also, but in lesser degree, FAPI 3) will make Drupal 6 much, much faster than Drupal 5. Throw in JS aggregation and then Drupal 6 will blow Drupal 5's performance away :)

Subscribing!

@Crell - You are perpetuating a myth about file_exists(). It is very fast - we have benchmarked this many times. Bust out your profiler and take a look at 1000 requests or so. The OS caches this. Anyway, I proposed adding exactly one file_exists() call. That a drop in the bucket compared to our current number. Further, noone is imposing function names. These are conventions that a module author can choose to use. That's how Rails works, and I rather like it. Conventions are powerful. We can agree to disagree on this though.

My earlier point stands - we can decrease the amount of code loaded on each page without touching menu system. Just requires some refactoring into include files.

@Crell - You are perpetuating a myth about file_exists(). It is very fast - we have benchmarked this many times. Bust out your profiler and take a look at 1000 requests or so. The OS caches this. Anyway, I proposed adding exactly one file_exists() call. That a drop in the bucket compared to our current number. Further, noone is imposing function names. These are conventions that a module author can choose to use. That's how Rails works, and I rather like it. Conventions are powerful. We can agree to disagree on this though.

My earlier point stands - we can decrease the amount of code loaded on each page without touching menu system. Just requires some refactoring into include files.

Let me spin Moshe's idea further for a bit! If we impose a naming proposal modulename_includename_whatever_you_want then module_invoke can easily check for includename.inc besides the module. So modules can still call each other, there is very, very little additional code needed but we still gain what we wanted. What say you?

I can't help loving this idea.

- Wim: as it stands, I don't think Drupal 6 is faster than Drupal 5. I might be wrong though.

- all: what do to others think about chx's proposals.

I really like this idea. It's cleaner than the 'include' menu property and much more powerful.
If modules have to use module_invoke it would enable us to even put the API of an module in an include and it will be only loaded if used! really great! That could really save us a lot of memory!

However I think the current patch misses a module_exists() check or disabled modules might be also invoked.

I like the concept. It has a feel-good code factor to it.
I'd specifically be happy to see the admin chunks split from the display chunks of module code so pageloads are cheaper.
However I wouldn't want to have to know so much about the feature I'm calling in someone elses module to know I should have pre-loaded their dependancies. This is probably an issue further beyond the current 'callback' refactoring proposal.

The mem stats merlinofchaos posted are very significant, as we know that mem problems have been plaguing shared-host users for a while.

It can easily get complex, so this discussion on conventions is very important.
Like I say. it's a feel-good approach, so I'm keen.

Some notes about the patch. If you look at the dates of the posts it was written in ten minutes. It's a proof of a concept, showing how easy to code it. It is specifically written for non-hook invokes as hooks are handled by http://drupal.org/node/116165 . So what you do is, say module_invoke('modulename', 'includename_whatever_you_want'). Moving this function out of module_invoke into to a separate function and getting menu_execute_active_handler to use it is peanuts. Becuase of 'by module' admin pages we already considered storing the defining module in the menu table...

I'm not convinced that this is the right way. In your example of 'system_admin_theme_settings' the .inc file is supposed to be 'module/admin.inc', BUT ...
1. the usual naming pattern nowadays is module/module_admin.inc and this patch would result in heavy renaming in cvs (what actually is not possible, we will loose the whole history of our files in cvs) and
2. you can't have 2 different admin includes - e.g. module_admin_theme.inc and module_admin_block.inc. Thats IMO a big -1 on this approach.

I think we shouldnt try to 'guess' the include file from the function name or we would need to try different include files on each call of this function to solve the problem of function names with multiple underscores in it.

How many extra includes are we talking about for a typical page load in that latest patch?

Despite assurances that this a fast operation, I am concerned. We need benchmarks.

I seriously hate repeating myself. I have not shown down anything with a vengeance. I said "small moves" -- not now, please. We (will) have enough problems without implementing everything in one patch.

I am not entitled to comment much on my own idea, aren't I :) ? There is no need for a system update at this point, HEAD->HEAD updates are unsupported.

I think this comment is wrong?

+ *   is available, call drupal_include('system', 'admin_theme_settings').  This function will then
+ *   include system/admin.inc if it exists.

It would include system/system.admin.inc, wouldn't it ?

Furthermore it would be neat, if it would be possible to just use drupal_include($function) - and the right include will be included..
If we do that, one could use drupal_include('system_admin') to include a file - which is quite intuitive if the file is also called 'system_admin'.inc. As an affect we could also remove all the 'module' properties from the menu and form code as it it could do just drupal_include($callback). Furthermore we could stay using module_invoke_all there.

The problem with that is - that there might be conflicting module names like node, node_foo, .. - perhaps drupal_include could try through all possible module combinations until it finds a file (or not..)? Usually there shouldn't be much possible module names, so the behavior won't change much in comparison to the proposed one.

What do you think about that?

I think, people shouldn't have to worry about this much.
What about using module_list() to find matching module names? That would minimize the possible matching module names..

if function already exists, stop.
if eek is a module and eek/eek_ack.inc exists, include it and stop.
if eek_ack is a module and eek_ack/eek_ack_urk.inc exists, include it and stop.

or we go through the whole list of modules and determine if the function name begins with the module name..
or [...]
I don't know what's faster, but I think there are ways to get this done efficient - I agree that performance is important here.

For the possibility of overlapping include names, I think they are not possible as drupal_include only uses the substring until the first '_' - so there is now way to include node_admin_bla.inc for the node module.
In the end we would have some more logic in drupal_include(), but things are simpler for anyone using it.

I don't understand the confusion about module names. If everyone uses module_invoke(), there is no ambiguity. Thats what was in chx' original patch.

I haven't read every message in this thread closely - sorry. Though I wish some people were more terse. Brevity is beautiful.

sry for confusing you..
We were talking about the possibility of doing drupal_include($module_include) as well as drupal_include($function) instead of drupal_include($module, $include). That would make identifying the right module for menu and form callbacks unneeded.

With some guidance from Crell and merlinofchaos, I've started doing some splitting of ecommerce files and I'd like to offer some feedback.

I would love to see more code split up into .page.inc, .form.inc and .admin.inc and so on. The obvious benefit being a smaller load. I also agree with merlinofchaos (#13), ie. "context delineated" code is much easier to learn from.

To address the problem of functions-on-demand, why not a much simpler approach?:

  drupal_include_all($module);

The called module implements a hook:

function foo_include_all() {
  include_once(drupal_get_path('module','foo') .'/foo.admin.inc');
  include_once(drupal_get_path('module','foo') .'/foo.form.inc');
}  

Sure, you could extend this to:

  drupal_include_all($module, $type = 'form'); // or whatever suits you

but I can't see how adding that complexity is very useful. Because:

1. The .module files should still contain the most useful functions
2. Many modules may have no more than 1 additional .inc
3. Arguably the .page.inc and .admin.inc (that way I see them being used) would rarely be of interest to other modules.

@moshe, I suggested moving my code out to a separate function. But then again, I suggested storing module names for menus so there we do not have a problem.

forms are a problem. but even that does not mandate writing so many words that was written in this issue, that's true, I can't follow. This issue is getting to the point where I just close up, sum up where we are and open another one. If forms are a problem then either use hook_forms are use two underscores after the module name, there is no other solution. What other problems we have in short?

I didnt have time for a review, but I took a quick look at the code. The concept of 'file, filepath' is very flexible and should help us to easily split up larger .module files. +1 on the idea! Its also generic enough (in contrast to the 'magical' patch above) that we can convert modules already broken into smaller pieces (e.g. views, cck, ... in contrib) to use the new concept without the need to reorganize all the code. There are various modules which dont fit 100% into the '.forms.inc/.amin.inc/etc.' concept. So I'm really thankful that we still have the chance to organize the code ourselves and therefore optimize it for a particular module. I also like that the 'file' field inherits from the parent items and the 'filepath' field can be omitted for callbacks within the same module. I will try to provide a full review as time permits.

However supporting this patch doesnt mean that we should stop thinking about a solution of inter-module API calls.

FYI, a version of this patch: http://drupal.org/node/137767 should go in soon (I hope) which will require a complete overhall of the menu system code you have.

@Crell - I think it would be very useful to have a standardized method for putting callbacks in separate files like this. Set it critical if you think it is- someone else can change it back if they disagree.

However, I think that not too much should be separated out. Admin pages, yes, certainly. Page callbacks or other functions that might be re-used by other modules might better stay in the main module file. Some suggestions/guidelines need to be documented too.

One thing to make sure of it that the file path is rebuilt when menu_rebuild is called (I'll look at the patch in more detail after the rewrite for multiple menus). Also, hard coding of the path should not be accepted, since otherwise everything will break if the module is moved.

I think there is an alternative for filenames: the filename can be set as something relative to the module file.

So, if you set in something like hook_menu:
"module" => "foo", "filename" => "foo_admin.inc"
OR
"module" => "foo", "filename" => "includes/foo_admin.inc"

Then when the system module (via menu.inc) needs to include a file it can do:
include_once(drupal_get_path('module', $item->module) .'/'. $item->filename);

Note, I think it's useful in this context to be able to set "module" in hook_menu - obviously it could default to the module implementing the hook.

@Crell: That's sort of what the second version did with the magic naming
No, its not. As you explicitly specify the module, there is no 'magic' in pwolanin's idea! The only difference to your last patch is, that the developer doesnt need to call drupal_get_path('module', $item->module) himself. We would still have "complete control over how to organize things". We should eliminate this source of trouble where module developers can hardcode the full path (/modules/mymodule/...) instead of using drupal_get_path(). No more no less.

Minor comment- to be more similar to other .inc files, why not name system.admin.inc as system_admin.inc? For example, node module has content_types.inc.

Also, it seems that even for this initial example, more should go in the .inc file, like function system_admin_menu_block()?

Subscribing - great patch :)

+    if (! $item->access) {
+      return MENU_ACCESS_DENIED;
+    }
+    else {
+      if ($item->file) {
+        include_once($item->file);
+      }
+      return call_user_func_array($item->page_callback, $item->page_arguments);
+    }

Please turn this around:

if ($item->access) { // instead of !$item->access
}
else {
}

Other than that, this looks ready. (And yes, let us use dots, please -- it's consistent with database.mysql.inc etc.)

Looks good to me, though it probably means I'll have to do a manual merge with this patch to get more of the menu system working: http://drupal.org/node/145058

Committed! :)

Marking this 'code needs work' until this has been properly documented in the handbook. Thanks guys! :-)

Oops - missed this one and another is system module I think.

  $items['node/add'] = array(
    'title' => 'Create content',
    'page callback' => 'system_admin_menu_block_page',
    'access callback' => '_node_add_access',
    'weight' => 1,
  );

I can add this to my menu.inc patch: http://drupal.org/node/145058, or do you want a separate patch?

here's the patch for node module alone

node/add dies with a white screen of death after this patch. next page refresh shows the following error:

warning: call_user_func_array() [function.call-user-func-array]: First argumented is expected to be a valid callback, 'system_admin_menu_block_page' was given in /Users/wright/drupal/cvs/6/core/includes/menu.inc on line 313.

@dww - see patch above.

ah, ok, it wasn't clear that patch was for this problem. sorry about that. ;) just tested the patch and can confirm it fixes the bug. RTBC, indeed. thanks.

Committed to CVS HEAD. Thanks for the patch and the documentation.

this patch causes problems on MySQL 5 because it uses a default value for the file field preventing the table from being created.

here's a patch for the .install file. it looks like the insert query always supplies a default value.

I think the MySQL 5 issue depends on how you run it- the error only occurs in STRICT mode, which is only on by default in some installs/OS's.

also, here: http://drupal.org/node/144765#comment-248367

Moshe says that require_once() is preferred to include_once() - it that worth changing? (I can add it to this patch: http://drupal.org/node/145058)

ok, added "require_once" to the patch here: http://drupal.org/node/145058

That patch deals with lots of critical menu functionality- reviews please!

setting back to RTBC- the patch to fix the .install still awaits.

I've committed the .install fix.

Perhaps I missunderstand something, but after going through the files in 5.1 and following the entire bootstrap process, I came to the conclusion that the greatest amount of memory, and page load time comes when "all" the modules are loaded so that drupal can create a map that links the URL to the FUNCTION that needs to be executed.

I started a post off in the forums about this, and was lead over to this project page.

I am very curious, or would like feedback (please understand I am new to drupal, and not an expert...just giving input here to both learn what is going on, as well as figure out if this would be possible, and reasonable to help Drupal be faster and use less resources) on this concept.

Rather then loading every single module that is "enabled" so that drupal can do it's thing and map the URLS to the functions, why couldn't the "enabling" of the module, or the "installing" of the module (ie...madule_name.install) take care of this...permanently. In this way there would be NO NEED to load any module other then module(s) needed to perform the work for the selected url.

Let me explain what I mean.

WHen a person goes the admin area, and to the module section and selects to "install" or "enable" a module, the module would have code in the module_name.install file that not only created the tables and so forth as they do now, but would also write to a very specific and dynamic table that is purely for the purpose of mapping the url to it's function as well as listing any other module, taxonomy, action, and so forth that it depends on.

Then during the bootstrap process, rather then loading up each and every module that is enable and installed, Drupal would simply go to the table look up the drupal path, select the appropriate function, and load all files (or the modules) that the main function being called would need.

Now I know that the way I explained leaves out a lot, like well, you wont always know what files need to be included so that wont work. I am simply asking for you all to look at the "concept"....it can be worked on from there.

But having one table that has the url and the callback function ALREADY mapped, would eliminate the need to load every module. Instead Drupal would know already exactly what function to call, what file it is in, what the path is, and what other modules it must load as it loads the main callback function as we would have already defined all this as an array in our .install file.

array(

'path' => 'mymodule',
'mapped to' => 'function_to_be_called'
'depends on' => array(
'second_module',
'third_module',
'taxonomy',
'included_file_name',
),
'whatever else we need in this table' => 'whatever'
);

Something of this nature would exist in the install file to write to the mapping table.

Then Drupal would only need to lookup the path and the function and whatever the callback depends on.

Now only the needed module, plus all modules that the callback needs to perform its operation is loaded, and nothing else is...very lightweight, very memory friendly, and allows for a much fast framework.

If menu.module is enabled, as the admin makes changes, or custamizations to the menu, again, it would be wrote to this dynamic table so that the new menu is already mapped out and no need to load every module. I have not played with this yet, I was just simply running through the code to see what is being done.

ALso since the menu hook is part of what determines access rights and such, these also could be written to the table so it can do a quick check to see if the person has access before it even loads the rest of the table, if not, no need to load the table at all, just display access denied as it is currently setup.

If the user has rights, do a querry for the url / function / dependencies )other modules it depends on....nodes etc.) then call the callack function, process the data, send it to the theme to be themed, return it back to the browser to be displayed.

I just want to know "why" this COULD WORK, not the reasons it can not work....or ways to MAKE IT WORK, rather then ways to not make it work. Eliminating extra modules being loaded would be a huge increase in performance since your not carrying all the extra weight around with you....it's all in a table that is changed as you enable / disable a module or when the system admin comes in and changes the menu itmes up a bit.

I just do not see a reason why Drupal MUST load every module to create the map, when the map could be created when the modules themselves are installed/enabled.

Secondly,
The table could be a "starting point". What I mean is drupal could come in an access the table, then IF it did not find what it needed, and ONLY IF it did not find what it needed, it then continues the normal bootsrap process by loading every module and creating an on the fly map for itself as it does now.....in other words it would default back to the normal bootstrap process if it does not find everything it needs.

Again, perhaps I missunderstand the bootstrapping a bit and just need to be educated...interested to see what you all have to say. Thanks.

Crell,

I appolagize and didn't realize how things were setup here. I am sorry, feel free to close this.

Will be any back port for drupal 5?

@sinasalek: No. This is a major API change to the menu system. Drupal 5 (and Drupal 6, at this point) are stable releases where no new features or API changes are added.