Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

FileSize
26.08 KB

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

dmitrig01’s picture

What benefits does this bring us?

chx’s picture

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

Anonymous’s picture

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

kbahey’s picture

Subscribing ...

kbahey’s picture

Title: total module system revamp » Total module system revamp
Status: Needs review » Needs work

Patch applies with 2 offsets.

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

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

So, something is borked ...

chx’s picture

Status: Needs work » Needs review

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

Anonymous’s picture

subscribing. will review tomorrow.

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

Anonymous’s picture

Status: Needs review » Needs work

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

i consistently get:

85 passes, 0 fails and 0 exceptions.

on clean cvs HEAD, and:

82 passes, 3 fails and 0 exceptions.

with the patch.

fails on:

Module "aggregator" is enabled. at [/home/justin/code/drupal/7/registry/modules/system/system.test line 43]
Module "translation" is enabled. at [/home/justin/code/drupal/7/registry/modules/system/system.test line 88]
Module "locale" is enabled. at [/home/justin/code/drupal/7/registry/modules/system/system.test line 88]
chx’s picture

Status: Needs work » Needs review

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

pwolanin’s picture

subscribe

Anonymous’s picture

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

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

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

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

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

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

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

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

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

+  drupal_load('module', 'comment');

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

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

+  include_once './includes/module.inc';
+  include_once './includes/common.inc';
+  include_once './includes/file.inc';
+  drupal_load('module', 'system', 'modules/system/system.module');

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

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

chx’s picture

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

dopry’s picture

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

webchick’s picture

subscribing

moshe weitzman’s picture

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

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

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

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

BioALIEN’s picture

Status: Needs review » Needs work

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

RobLoach’s picture

I have to have a look at this.

moshe weitzman’s picture

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

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

samirnassar’s picture

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

After I rerolled the patch I attempted to install Drupal.

Issues:

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

seven instances of:

notice: Undefined variable: data in /var/www/drupal/drupal7/test2/includes/bootstrap.inc on line 1322.

Right before installation is completely done I get:

Sixteen reports of:

notice: Undefined variable: data in /var/www/drupal/drupal7/test2/includes/bootstrap.inc on line 1322.

Post installation everything seems to work.

  • Logging out
  • Logging back in
  • Creating an article

Along with a varying number of Notices regarding $data.

catch’s picture

steamedpenguin - the patch didn't make it.

samirnassar’s picture

samirnassar’s picture

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

catch’s picture

FileSize
31.47 KB

rerolled from root ;)

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

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

Anonymous’s picture

steamedpenguin: thanks for taking this up.

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

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

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

catch’s picture

Justin, patchless.

Anonymous’s picture

/me tries again...

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

samirnassar’s picture

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

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

boombatower’s picture

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

Run the SimpleTests.

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

chx’s picture

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

RobLoach’s picture

chx’s picture

Status: Needs work » Needs review
FileSize
31.9 KB

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

chx’s picture

Rerolled against HEAD.

dmitrig01’s picture

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

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

Everything else looks fine.

Damien Tournoud’s picture

FileSize
504.35 KB
485.86 KB

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

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

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

chx’s picture

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

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

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

kbahey’s picture

Damien

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

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

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

nedjo’s picture

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

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

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

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

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

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

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


/**
 * Drupal is bootstrapped in phases with each phase loading only the minimal
 * code required to provide a specified set of functionality.
 *
 * The highest phase of bootstrapping, DRUPAL_BOOTSTRAP_FULL, loads a set
 * of required API files as well as a minimal set of required modules.
 *
 * After bootstrapping, code is loaded on demand based on information in the
 * registry. When a hook is called, all module files implementing the hook
 * are loaded. Similarly, when a function is tested for, it is loaded if 
 * available. Menu callback functions are tested for and so loaded as needed.
 *
 * @see module_implements()
 * @see drupal_function_exists()
 */
 
 

Suggested documentation for function _drupal_bootstrap()


/**
 * Carry out bootstrapping for a single phase.
 *
 * If a phase has an associated hook, e.g., hook_boot(),
 * any modules implementing that hook will be loaded at this time.
 */

Suggested documentation for _drupal_bootstrap_full()


/**
 * Call initialization routines, set a header, and load all files required for
 * a full Drupal bootstrap.
 *
 * Loads a set of required API include files and the minimum required
 * module files.
 */
 

Suggested changed documentation for function module_implements()()


/**
 * Determine which modules are implementing a hook and load their files.
 *
 * ....
 */

More comments on the code and documentation:

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

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

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

Great work chx.

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

nedjo’s picture

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


/**
 * Implementation of hook_init().
 */
function book_init() {
  drupal_add_css(drupal_get_path('module', 'book') . '/book.css');
}

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

pwolanin’s picture

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

chx’s picture

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

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

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

chx’s picture

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

chx’s picture

Reroll against HEAD.

chx’s picture

Added a missing )

nedjo’s picture

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

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

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

Damien Tournoud’s picture

We are nearly there, chx!

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

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

-ContactSitewideTestCase 129 passes, 0 fails, 0 exceptions
+ContactSitewideTestCase 99 passes, 66 fails, 0 exceptions
-ForumTestCase 386 passes, 0 fails, 0 exceptions
+ForumTestCase 278 passes, 163 fails, 0 exceptions
-RegistryParseFileTestCase 3 passes, 0 fails, 0 exceptions
+RegistryParseFileTestCase 3 passes, 0 fails, 1 exception
-SyslogTestCase 17 passes, 0 fails, 0 exceptions
+SyslogTestCase 17 passes, 0 fails, 1 exception
-TrackerTest 112 passes, 0 fails, 0 exceptions
+TrackerTest 110 passes, 2 fails, 0 exceptions
-TranslationTestCase 60 passes, 0 fails, 0 exceptions
+TranslationTestCase 60 passes, 0 fails, 1 exception
-TriggerContentTestCase 368 passes, 0 fails, 0 exceptions
+TriggerContentTestCase 332 passes, 72 fails, 0 exceptions
-UploadTestCase 121 passes, 4 fails, 0 exceptions
+UploadTestCase 117 passes, 12 fails, 0 exceptions

The Upload test looks funny, but others seems reliable.

Damien Tournoud’s picture

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

-UploadTestCase 125 passes, 0 fails, 0 exceptions
+UploadTestCase 120 passes, 9 fails, 0 exceptions

And the detailed list of exceptions:

---- RegistryParseFileTestCase ----
Exception       Warning registry.inc    113             Missing argument 3 for _registry_parse_file(), called in /var/www/7.x/includes/tests/registry.test on line 31 and defined
---- SyslogTestCase ----
Exception       Warning registry.inc    85              file_get_contents(./modules/syslog/syslog.install): failed to open stream: No such file or directory
---- TranslationTestCase ----
Exception       Warning registry.inc    85              file_get_contents(./modules/translation/translation.install): failed to open stream: No such file or directory
chx’s picture

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

chx’s picture

Huh, accidentally I upped a cruftier version.

RobLoach’s picture

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

chx’s picture

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

Damien Tournoud’s picture

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

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

chx’s picture

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

cburschka’s picture

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

cburschka’s picture

Status: Needs review » Needs work

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

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

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

cburschka’s picture

Status: Needs work » Needs review

I fail at patching.

Anonymous’s picture

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

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

catch’s picture

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

moshe weitzman’s picture

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

cburschka’s picture

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

chx’s picture

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

minesota’s picture

subscribe

cburschka’s picture

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

Anonymous’s picture

RE: #59

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

chx’s picture

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

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

Damien Tournoud’s picture

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

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

Invoke a hook in a particular module.

So I don't like seeing stuff like that:

module_invoke('block', 'list', $region);
module_invoke('taxonomy', 'get_term', $tid);
module_invoke('taxonomy', 'del_term', $form_state['values']['tid']);

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

For example: in run_tests.sh, I see:

   module_invoke('simpletest', 'get_all_tests');
   $test = new $test_class($test_id);
   [...]

... while I will prefer:

   drupal_load('module', 'simpletest');
   simpletest_get_all_tests();

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

@@ -585,6 +585,7 @@ function file_save_upload($source, $vali
     $errors = array();
     foreach ($validators as $function => $args) {
       array_unshift($args, $file);
+      drupal_function_exists($function);
       $errors = array_merge($errors, call_user_func_array($function, $args));
     }

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

    if ($callback && drupal_function_exists($callback)) { [...] }

Is this a meaningful review?

cburschka’s picture

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

sun’s picture

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

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

Awesome job, chx!

cburschka’s picture

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

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

Dries’s picture

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

moshe weitzman’s picture

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

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

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

Dries’s picture

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

chx’s picture

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

catch’s picture

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

cburschka’s picture

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

call_user_func_array(FALSE, array());
Warning: call_user_func_array(): First argument is expected to be a valid callback, '' was given in /home/.jordon/cburschka/- on line 2

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

webchick’s picture

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

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

cburschka’s picture

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

Damien Tournoud’s picture

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

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

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

We should benchmark this and looks closely at the results.

Dries’s picture

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

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

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

cburschka’s picture

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

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

nedjo’s picture

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

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

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

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

Crell’s picture

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

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

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

nedjo’s picture

@crell: I agree with the issues you flag.

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

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

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

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

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

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

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

Crell’s picture

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

For hook_init et al, I see two alternatives.

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

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

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

nedjo’s picture

@Crell

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

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

Crell’s picture

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

cburschka’s picture

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

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

---

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

dmitrig01’s picture

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

Dries’s picture

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

kbahey’s picture

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

chx’s picture

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

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

chx’s picture

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

catch’s picture

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

chx’s picture

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

catch’s picture

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

chx’s picture

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

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

catch’s picture

Quick visual review.

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

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

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

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

Damien Tournoud’s picture

I completely agree with catch.

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

=== modified file 'includes/file.inc'
--- includes/file.inc	2008-07-05 18:34:29 +0000
+++ includes/file.inc	2008-07-13 22:14:10 +0000
@@ -585,6 +585,7 @@ function file_save_upload($source, $vali
     $errors = array();
     foreach ($validators as $function => $args) {
       array_unshift($args, $file);
+      drupal_function_exists($function);
       $errors = array_merge($errors, call_user_func_array($function, $args));
     }

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

+  if (!$hook) {
+    // Only write this to cache if the implementations data we are going to cache
+    // is different to what we loaded earlier in the request.
+    if ($cache && $implementations != $cache->data) {
+      cache_set('hooks', $implementations, 'cache_registry');
+    }
+    return;
+  }

What is the use of the !$hook here?

+  // If we are in maintenance mode but managed to bootstrap fully then we can
+  // use the registry.
+  if (defined('MAINTENANCE_MODE') && drupal_bootstrap()) {
     $implementations = array();
     foreach (module_list() as $module) {
       if (module_hook($module, $hook)) {
+        $implementations[] = $module;
+      }
+    }
+    return $implementations;
+  }

Why not using cached values while in MAINTENANCE_MODE?

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

+      // If we are not given a file but have the database loaded already,
+      // we try to look it up.
+      if (!$file && !defined('MAINTENANCE_MODE') && ($result = db_fetch_object(db_query("SELECT filename, module, module_filename FROM {registry} WHERE name = '%s' AND type = 'function'", $function)))) {
+        $file = $result->filename;
+        $module_filename = $result->module_filename;
+        $module = $result->module;

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

+  // If we are in maintenance mode but managed to bootstrap fully then we can
+  // use the registry.
+  if (defined('MAINTENANCE_MODE') && drupal_bootstrap()) { }

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

chx’s picture

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

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

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

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

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

Damien Tournoud’s picture

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

moshe weitzman’s picture

A very nice cleanup! Here is another code review:

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

catch’s picture

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

nedjo’s picture

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

chx’s picture

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

chx’s picture

Comment fixes.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

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

Dries’s picture

Status: Reviewed & tested by the community » Needs work

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

moshe weitzman’s picture

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

cburschka’s picture

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

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

chx’s picture

Status: Needs work » Needs review

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

Dries’s picture

Status: Needs review » Needs work

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

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

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

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

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

bjaspan’s picture

subscribe

cburschka’s picture

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

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

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

catch’s picture

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

kbahey’s picture

FileSize
57.94 KB

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

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

Benchmarks to follow shortly ...

kbahey’s picture

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

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

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

Of course, menu module is enabled.

So, is this something that needs to change?

kbahey’s picture

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

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

Document Path:          /
Document Length:        4632 bytes

Concurrency Level:      5
Time taken for tests:   52.85409 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      5196000 bytes
HTML transferred:       4632000 bytes
Requests per second:    19.20 [#/sec] (mean)
Time per request:       260.427 [ms] (mean)
Time per request:       52.085 [ms] (mean, across all concurrent requests)
Transfer rate:          97.42 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   100  259  56.6    257     538
Waiting:      100  250  56.6    248     538
Total:        100  259  56.6    257     538

Percentage of the requests served within a certain time (ms)
  50%    257
  66%    279
  75%    291
  80%    300
  90%    325
  95%    360
  98%    396
  99%    441
 100%    538 (longest request)

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

Document Path:          /
Document Length:        4632 bytes

Concurrency Level:      5
Time taken for tests:   52.278253 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      5196000 bytes
HTML transferred:       4632000 bytes
Requests per second:    19.13 [#/sec] (mean)
Time per request:       261.391 [ms] (mean)
Time per request:       52.278 [ms] (mean, across all concurrent requests)
Transfer rate:          97.06 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   1.7      0      50
Processing:   100  260  48.6    260     468
Waiting:      100  251  49.9    251     468
Total:        100  260  48.8    260     468

Percentage of the requests served within a certain time (ms)
  50%    260
  66%    278
  75%    294
  80%    301
  90%    320
  95%    341
  98%    369
  99%    389
 100%    468 (longest request)
kbahey’s picture

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

mem.info:

name = Memory
description = Displays peak and end memory
package = Development
core = 7.x

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

files[] = mem.module

mem.module

// $Id$

function mem_exit() {
  if (function_exists('memory_get_usage')) {
    $curr = memory_get_usage(TRUE);
  }

  if (function_exists('memory_get_peak_usage')) {
    $peak = memory_get_peak_usage(TRUE);
  }

  if ($curr && $peak) {
    drupal_set_message("Current bytes: $curr, Peak: $peak");
  }
}
moshe weitzman’s picture

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

kbahey’s picture

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

I used the following configuration:

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

No APC

Without the patch

Document Path:          /
Document Length:        176 bytes

Concurrency Level:      5
Time taken for tests:   48.367945 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      739000 bytes
HTML transferred:       176000 bytes
Requests per second:    20.67 [#/sec] (mean)
Time per request:       241.840 [ms] (mean)
Time per request:       48.368 [ms] (mean, across all concurrent requests)
Transfer rate:          14.91 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    95  241  49.6    241     450
Waiting:       95  231  51.2    228     444
Total:         95  241  49.6    241     450

Percentage of the requests served within a certain time (ms)
  50%    241
  66%    261
  75%    275
  80%    282
  90%    302
  95%    323
  98%    349
  99%    378
 100%    450 (longest request)

With the patch

Document Path:          /
Document Length:        15963 bytes

Concurrency Level:      5
Time taken for tests:   60.766066 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      16505000 bytes
HTML transferred:       15963000 bytes
Requests per second:    16.46 [#/sec] (mean)
Time per request:       303.830 [ms] (mean)
Time per request:       60.766 [ms] (mean, across all concurrent requests)
Transfer rate:          265.25 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   117  303  52.9    302     528
Waiting:      106  277  49.8    277     492
Total:        117  303  52.9    302     528

Percentage of the requests served within a certain time (ms)
  50%    302
  66%    322
  75%    337
  80%    346
  90%    366
  95%    390
  98%    428
  99%    460
 100%    528 (longest request)

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

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

Trying to view a node gives this error:

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

I have this in the devel_generate.info file:

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

And the first .inc has the said hook.

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

kbahey’s picture

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

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

Document Path:          /
Document Length:        15963 bytes

Concurrency Level:      5
Time taken for tests:   60.710667 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      16505000 bytes
HTML transferred:       15963000 bytes
Requests per second:    16.47 [#/sec] (mean)
Time per request:       303.553 [ms] (mean)
Time per request:       60.711 [ms] (mean, across all concurrent requests)
Transfer rate:          265.49 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   115  302  54.5    301     511
Waiting:      105  277  51.1    279     457
Total:        115  302  54.5    301     511

Percentage of the requests served within a certain time (ms)
  50%    301
  66%    325
  75%    340
  80%    351
  90%    371
  95%    389
  98%    419
  99%    441
 100%    511 (longest request)

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

Any ideas as to why?

kbahey’s picture

Here is a possible clue as to why ...

With the patch, devel says:

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

Then on subsequent page views, we get:

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

Without patch, devel says:

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

Then on subsequent page views, we get:

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

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

All this without APC again.

kbahey’s picture

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

  if (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update') {
    //module_invoke_all('init');
  }

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

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

Document Path:          /
Document Length:        15448 bytes

Concurrency Level:      5
Time taken for tests:   60.952371 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      15947000 bytes
HTML transferred:       15448000 bytes
Requests per second:    16.41 [#/sec] (mean)
Time per request:       304.762 [ms] (mean)
Time per request:       60.952 [ms] (mean, across all concurrent requests)
Transfer rate:          255.49 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   118  303  53.8    305     468
Waiting:      105  277  51.6    277     454
Total:        118  303  53.8    305     468

Percentage of the requests served within a certain time (ms)
  50%    305
  66%    327
  75%    342
  80%    349
  90%    370
  95%    391
  98%    424
  99%    437
 100%    468 (longest request)

Any other scenarios?

catch’s picture

Did you remove hook_help() at the same time?

kbahey’s picture

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

catch’s picture

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

kbahey’s picture

Thanks.

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

Results are still the same though ...

Document Path:          /
Document Length:        15448 bytes

Concurrency Level:      5
Time taken for tests:   60.542955 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      15947000 bytes
HTML transferred:       15448000 bytes
Requests per second:    16.52 [#/sec] (mean)
Time per request:       302.715 [ms] (mean)
Time per request:       60.543 [ms] (mean, across all concurrent requests)
Transfer rate:          257.22 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   116  301  57.3    303     519
Waiting:      105  274  53.5    277     505
Total:        116  301  57.3    303     519

Percentage of the requests served within a certain time (ms)
  50%    303
  66%    322
  75%    340
  80%    348
  90%    369
  95%    400
  98%    421
  99%    445
 100%    519 (longest request)
cburschka’s picture

I just noticed something I don't understand.

Without the patch, you report this test outcome:

Document Path:          /
Document Length:        176 bytes

With patch, you report this:

Document Path:          /
Document Length:        15448 bytes

(alternatively 15963 bytes)

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

Pardon me if I'm missing something obvious.

kbahey’s picture

You are of course right.

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

So here are the results without the patch:

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

Document Path:          /
Document Length:        15174 bytes

Concurrency Level:      5
Time taken for tests:   59.784452 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      15673000 bytes
HTML transferred:       15174000 bytes
Requests per second:    16.73 [#/sec] (mean)
Time per request:       298.922 [ms] (mean)
Time per request:       59.784 [ms] (mean, across all concurrent requests)
Transfer rate:          256.00 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   116  298  53.4    297     508
Waiting:      104  270  51.4    270     495
Total:        116  298  53.4    297     508

Percentage of the requests served within a certain time (ms)
  50%    297
  66%    317
  75%    330
  80%    338
  90%    362
  95%    394
  98%    423
  99%    437
 100%    508 (longest request)

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

Document Path:          /
Document Length:        15689 bytes

Concurrency Level:      5
Time taken for tests:   60.382950 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      16231000 bytes
HTML transferred:       15689000 bytes
Requests per second:    16.56 [#/sec] (mean)
Time per request:       301.915 [ms] (mean)
Time per request:       60.383 [ms] (mean, across all concurrent requests)
Transfer rate:          262.49 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   3.0      0      68
Processing:   116  300  53.9    300     512
Waiting:      106  275  51.7    274     456
Total:        116  301  54.0    300     512

Percentage of the requests served within a certain time (ms)
  50%    300
  66%    321
  75%    336
  80%    346
  90%    370
  95%    391
  98%    426
  99%    445
 100%    512 (longest request)

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

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

cburschka’s picture

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

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

kbahey’s picture

Re: memory usage.

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

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

sun’s picture

FileSize
58.03 KB

I gave this a shot.

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

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

Just re-rolled.

sun’s picture

After some more testing and clearing cache_registry manually, I get

Fatal error: Call to undefined function system_region_list() in .\includes\theme.inc on line 1788

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

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

cburschka’s picture

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

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

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

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

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

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

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

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

moshe weitzman’s picture

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

catch’s picture

Status: Needs work » Needs review

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

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

sun’s picture

Status: Needs review » Needs work

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

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

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

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

/**
 * registry_load_path_files
 */
function registry_load_path_files($return = FALSE) {
sun’s picture

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

Thus, only c) is remaining.

cburschka’s picture

Whoops. I fell for the same error.

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

chx’s picture

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

kbahey’s picture

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

Without the patch, with APC

Document Path:          /
Document Length:        15689 bytes

Concurrency Level:      5
Time taken for tests:   34.868390 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      16231000 bytes
HTML transferred:       15689000 bytes
Requests per second:    28.68 [#/sec] (mean)
Time per request:       174.342 [ms] (mean)
Time per request:       34.868 [ms] (mean, across all concurrent requests)
Transfer rate:          454.57 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    64  173  50.5    167     456
Waiting:       59  151  46.9    144     434
Total:         64  173  50.5    167     456

Percentage of the requests served within a certain time (ms)
  50%    167
  66%    189
  75%    198
  80%    203
  90%    229
  95%    264
  98%    305
  99%    352
 100%    456 (longest request)

With the patch, with APC

Document Path:          /
Document Length:        15963 bytes

Concurrency Level:      5
Time taken for tests:   35.514639 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      16505000 bytes
HTML transferred:       15963000 bytes
Requests per second:    28.16 [#/sec] (mean)
Time per request:       177.573 [ms] (mean)
Time per request:       35.515 [ms] (mean, across all concurrent requests)
Transfer rate:          453.84 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    66  176  47.6    168     462
Waiting:       61  153  45.0    147     411
Total:         66  176  47.6    168     462

Percentage of the requests served within a certain time (ms)
  50%    168
  66%    193
  75%    201
  80%    206
  90%    227
  95%    259
  98%    296
  99%    343
 100%    462 (longest request)

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

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

chx’s picture

Status: Needs work » Needs review
FileSize
176.78 KB

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

catch’s picture

Status: Needs review » Needs work

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

chx’s picture

Status: Needs work » Needs review
FileSize
179.09 KB

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

catch’s picture

Status: Needs review » Needs work

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

chx’s picture

Status: Needs work » Needs review
FileSize
179.61 KB

Baaaaaaaaaah!

chx’s picture

FileSize
162.86 KB

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

catch’s picture

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

kbahey’s picture

Without the patch, APC is on.

Document Path:          /
Document Length:        16339 bytes

Concurrency Level:      5
Time taken for tests:   36.90880 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      16881000 bytes
HTML transferred:       16339000 bytes
Requests per second:    27.71 [#/sec] (mean)
Time per request:       180.454 [ms] (mean)
Time per request:       36.091 [ms] (mean, across all concurrent requests)
Transfer rate:          456.76 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    66  179  54.1    179     388
Waiting:       61  159  50.6    155     367
Total:         66  179  54.1    179     388

Percentage of the requests served within a certain time (ms)
  50%    179
  66%    200
  75%    212
  80%    219
  90%    247
  95%    271
  98%    302
  99%    327
 100%    388 (longest request)

With patch from #147, APC is on.

Document Path:          /
Document Length:        16743 bytes

Concurrency Level:      5
Time taken for tests:   36.243167 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      17285000 bytes
HTML transferred:       16743000 bytes
Requests per second:    27.59 [#/sec] (mean)
Time per request:       181.216 [ms] (mean)
Time per request:       36.243 [ms] (mean, across all concurrent requests)
Transfer rate:          465.72 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   2.1      0      64
Processing:    68  180  51.0    182     411
Waiting:       63  164  48.8    160     388
Total:         68  180  51.1    182     411

Percentage of the requests served within a certain time (ms)
  50%    182
  66%    199
  75%    207
  80%    212
  90%    236
  95%    269
  98%    312
  99%    339
 100%    411 (longest request)
kbahey’s picture

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

Without the patch, APC on:

top
  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
 3299 www-data  20   0  265m  26m  11m S    0  1.3   0:05.80 apache2
 3298 www-data  20   0  264m  25m  11m S    0  1.3   0:05.87 apache2
 3300 www-data  20   0  264m  25m  11m S    0  1.3   0:06.05 apache2
 3301 www-data  20   0  263m  25m  11m S    0  1.2   0:05.91 apache2
 3302 www-data  20   0  263m  24m  10m S    0  1.2   0:05.76 apache2
 3304 www-data  20   0  257m  18m  10m S    0  0.9   0:05.61 apache2
 3306 www-data  20   0  257m  18m  10m S    0  0.9   0:05.47 apache2
 3307 www-data  20   0  257m  17m  10m S    0  0.9   0:05.32 apache2
 3308 www-data  20   0  257m  17m  10m S    0  0.9   0:05.26 apache2
 3309 www-data  20   0  257m  17m  10m S    0  0.9   0:05.13 apache2
 3296 root      20   0  255m 9924 4924 S    0  0.5   0:00.03 apache2

With the patch, APC on, and after restarting apache

top
  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
 3801 www-data  20   0  263m  25m  11m S    0  1.2   0:05.93 apache2
 3805 www-data  20   0  262m  23m  11m S    0  1.2   0:06.17 apache2
 3804 www-data  20   0  261m  22m  11m S    0  1.1   0:06.04 apache2
 3806 www-data  20   0  260m  20m  10m S    0  1.0   0:05.94 apache2
 3814 www-data  20   0  258m  18m  10m S    0  0.9   0:05.14 apache2
 3815 www-data  20   0  258m  18m  10m S    0  0.9   0:05.30 apache2
 3809 www-data  20   0  257m  18m  10m S    0  0.9   0:05.81 apache2
 3810 www-data  20   0  257m  17m  10m S    0  0.9   0:05.60 apache2
 3813 www-data  20   0  257m  17m  10m S    0  0.9   0:05.20 apache2
 3812 www-data  20   0  257m  17m  10m S    0  0.9   0:05.23 apache2
 3800 root      20   0  255m 9924 4924 S    0  0.5   0:00.03 apache2

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

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

Without the patch:

  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
 6329 www-data  20   0  212m  17m 3656 S    0  0.9   0:10.30 apache2
 6330 www-data  20   0  212m  17m 3656 S    0  0.9   0:10.40 apache2
 6347 www-data  20   0  212m  17m 3636 S    0  0.9   0:09.39 apache2
 6332 www-data  20   0  212m  17m 3588 S    0  0.9   0:09.88 apache2
 6333 www-data  20   0  212m  17m 3588 S    0  0.9   0:09.99 apache2
 6343 www-data  20   0  212m  17m 3568 S    0  0.9   0:10.00 apache2
 6344 www-data  20   0  212m  17m 3568 S    0  0.9   0:09.74 apache2
 6346 www-data  20   0  212m  17m 3568 S    0  0.9   0:09.38 apache2
 6348 www-data  20   0  212m  17m 3568 S    0  0.9   0:09.17 apache2
 6349 www-data  20   0  212m  17m 3568 S    0  0.9   0:09.14 apache2
 6327 root      20   0  202m 9624 4696 S    0  0.5   0:00.03 apache2

With the patch:

  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
 6850 www-data  20   0  212m  17m 3576 S    0  0.9   0:09.85 apache2
 6851 www-data  20   0  212m  17m 3576 S    0  0.9   0:09.51 apache2
 6853 www-data  20   0  212m  17m 3576 S    0  0.9   0:09.18 apache2
 6854 www-data  20   0  212m  17m 3576 S    0  0.9   0:09.30 apache2
 6855 www-data  20   0  212m  17m 3576 S    0  0.9   0:09.38 apache2
 6836 www-data  20   0  211m  17m 3576 S    0  0.8   0:09.92 apache2
 6837 www-data  20   0  211m  17m 3576 S    0  0.8   0:09.72 apache2
 6838 www-data  20   0  211m  17m 3576 S    0  0.8   0:09.84 apache2
 6839 www-data  20   0  211m  17m 3576 S    0  0.8   0:09.86 apache2
 6856 www-data  20   0  211m  17m 3576 S    0  0.8   0:09.36 apache2
 6833 root      20   0  202m 9624 4696 S    0  0.5   0:00.03 apache2

There is no difference in overall Apache size without APC.

And here are the no APC performance results:

Without APC, without the patch:

Document Path:          /
Document Length:        16339 bytes
Concurrency Level:      5
Time taken for tests:   61.134970 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      16881000 bytes
HTML transferred:       16339000 bytes
Requests per second:    16.36 [#/sec] (mean)
Time per request:       305.675 [ms] (mean)
Time per request:       61.135 [ms] (mean, across all concurrent requests)
Transfer rate:          269.65 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   115  304  67.0    307     595
Waiting:      105  277  63.2    281     574
Total:        115  304  67.0    307     595

Percentage of the requests served within a certain time (ms)
  50%    307
  66%    329
  75%    349
  80%    361
  90%    384
  95%    410
  98%    435
  99%    468
 100%    595 (longest request)

No APC, With the patch

Document Path:          /
Document Length:        16743 bytes
Concurrency Level:      5
Time taken for tests:   59.776748 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      17285000 bytes
HTML transferred:       16743000 bytes
Requests per second:    16.73 [#/sec] (mean)
Time per request:       298.884 [ms] (mean)
Time per request:       59.777 [ms] (mean, across all concurrent requests)
Transfer rate:          282.37 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   113  298  64.3    302     509
Waiting:      102  272  62.0    275     495
Total:        113  298  64.3    302     509

Percentage of the requests served within a certain time (ms)
  50%    302
  66%    324
  75%    338
  80%    346
  90%    373
  95%    398
  98%    432
  99%    456
 100%    509 (longest request)

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

cburschka’s picture

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

nedjo’s picture

FileSize
617 bytes

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

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

kbahey’s picture

Status: Needs review » Needs work

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

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

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

       Resp Time   Trans Rate   Trans     OKAY    Failed   Elap Time   Concurrent
w/out       0.41        24.62    2962     2962        0      120.31         9.98
with        0.39        25.59    3085     3085        0      120.56         9.95

Shaves about 20 milliseconds from each request.

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

       Resp Time   Trans Rate   Trans     OKAY    Failed   Elap Time   Concurrent
w/out       0.88        11.31    1362     1362        0      120.40         9.96
with        0.85        11.75    1407     1407        0      119.73         9.96

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

Xano’s picture

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

merlinofchaos’s picture

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

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

Crell’s picture

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

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

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

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

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

skilip’s picture

following

catch’s picture

Version: 7.x-dev » 8.x-dev

Moving to D8.

catch’s picture

Priority: Critical » Major

Downgrading all D8 criticals to major per http://drupal.org/node/45111

yoroy’s picture

Subscribe. What's the status? Is this still something acionable or does it need rethinking first. Anything re-usable in here for D8? Keep this major?

sun’s picture

Priority: Major » Normal
Status: Needs work » Closed (won't fix)

Looking at some of the more recent patches, almost all contained change proposals have either been tried in other issues and already deemed to be bogus, or attempt to apply changes to subsystems that have been changed differently in the meantime.

Given that the performance benchmarks didn't yield any measurable improvements, and also, given the rather blurry issue title lacking a focused scope, I think we can close this issue.

In case there are still tidbits in this patch that are not covered in other issues already, let's create more focused issues for them.