Currently module_implements relies on module_list (which in itself needs love), this patch makes it rely on the registry instead. This should make db_merge possible during bootstrap on postgresql which currently wont work.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Status: Needs review » Needs work

patch -p0 < module_implements.patch
patching file includes/bootstrap.inc
Hunk #1 succeeded at 820 (offset -11 lines).
Hunk #2 succeeded at 1430 (offset -11 lines).
patching file includes/common.inc
Hunk #1 succeeded at 1526 (offset 29 lines).
patching file includes/menu.inc
Hunk #1 FAILED at 1719.
1 out of 1 hunk FAILED -- saving rejects to file includes/menu.inc.rej
patching file includes/module.inc
patching file includes/registry.inc
patching file index.php
patching file modules/system/system.install
Hunk #1 succeeded at 1091 (offset -4 lines).

chx’s picture

Status: Needs work » Needs review
FileSize
14.39 KB

Reroll.

chx’s picture

FileSize
11.52 KB

For real :)

Dave Reid’s picture

Status: Needs review » Needs work

DB errors abound on my current test install (even after update.php), so I tried on a fresh install. Installed correctly and everything looks good except an error on the bottom of every page: Fatal error: Call to undefined function registry_cache_hook_implementations() in /home/davereid/Projects/drupal-head/includes/common.inc on line 1529. Tried running the system test. Registry parse file test fails 0/3 and stops:

Missing argument 3 for _registry_parse_file(), called in /home/davereid/Projects/drupal-head/modules/simpletest/tests/registry.test on line 32 and defined	Warning	registry.inc	119	_registry_parse_file()	
Undefined variable: contents	Notice	registry.inc	123	_registry_parse_file()	
Resource "registry_test_function72ce73c6deb5ef3b6de403e95f904adf" found.	Other	registry.test	35	RegistryParseFileTestCase->testRegistryParseFile()	
Resource "registry_test_classd10b5655b22218fe8b2cf4c444bbb23b" found.	Other	registry.test	35	RegistryParseFileTestCase->testRegistryParseFile()	
Resource "registry_test_interfacedfe2be8143c374ef1733cf36860b1cc2" found.	Other	registry.test	35	RegistryParseFileTestCase->testRegistryParseFile()	
chx’s picture

Status: Needs work » Needs review
FileSize
12.97 KB

That's an easy fix for common.inc, no idea at all how that call remained in there. I think I fixed the test as well -- it's also clear that we will need a new test. Meanwhile, reviews are still welcome.

Dave Reid’s picture

Status: Needs review » Needs work
FileSize
12.97 KB

Now getting following error on bottom of each page: Fatal error: Call to undefined function registry_cache_path_files() in /home/davereid/Projects/drupal-head/includes/common.inc on line 1530.

I've attached a revised patch. I think someone that has a little more understanding of the registry code to comment on the actual patch. I'll work on learning about it...

Dave Reid’s picture

Status: Needs work » Needs review

Err...didn't mean to change status.

chx’s picture

FileSize
13.43 KB

Well, #6 is wrong, that function should not be removed. The patch comes from total module revamp and this was an accident.

chx’s picture

FileSize
20.45 KB

Silly me, forgot to add the install files to the registry. Works pretty well!

Crell’s picture

Subscribe. Will try to review at some point.

chx’s picture

FileSize
21.46 KB

All tests now pass. And do you need any proof that patch my speeds up things? Check tracker.test. That was some test bug to hunt down. Little drop became a bit too fast :D

Crell’s picture

Status: Needs review » Needs work

I am either totally confused as to what you're trying to do or there are way too many patches rolled into one here. There's whitespace cleanup, adding .install files to the .info files, what looks like registry changes, and what looks like adding Yet Another Boolean Parameter (YABP). Only the last seems to have any relation to this issue, and I am actually against that approach in most cases for DX reasons.

IIRC from DBTNG, all that needed to be done for this issue was making module_list() itself not suck. Fix that to not be moronic and the rest fixes itself. Let's just do that and do the registry cleanup in another patch.

chx’s picture

Status: Needs work » Needs review
FileSize
20.88 KB

More kittens are saved by moving the tracker test patch to http://drupal.org/node/309951 . For dropping sort, there was no point in calling watchdog or menu in an alphabetical order so dropping the sort argument is just cleanup. Yes that's makes the patch non-kitten free, but it'd be rather daunting to submit a patch which patches out $sort from the current implementation of module_implements just to be replaced by this version the next day. If we are bent on keeping $sort I can put it back.

Crell’s picture

Status: Needs review » Needs work

I think we crossposted.

chx’s picture

Status: Needs work » Needs review

Sorry, missed Crell's reply. So this patch makes module_implements rely on the registry instead of module_list. Or you can say that we save some queries to the registry table when the hook registry for this router path is not yet cached. The patch needs to add the install files to the info otherwise hook_schema would fail as the registry wont contain them. To do this we needed a few registry.inc changes where the module name is passed all along. This way when we see a function name which starts with the name of the module, the rest can be a hook, so we store it as such. The rest of the registry changes is just removing what seemed to be cruft at the end of the day. Dunno what YABP are you talking of, really.

chx’s picture

Oh and by the way -- this patch paves the way for per hook weights.

webchick’s picture

This issue is a pretty good example of http://webchick.net/patch-reviewers-are-not-clairvoyant. chx spent about an hour with me in #drupal walking me through the patch. Here's a summary:

  • This patch supports a means of auto-discovering hooks implementations, based on naming conventions.
  • As it builds the list of functions for the registry, the registry table logs the module that the function came from (based on which .info file defined the file in which it resides).
  • If the function starts with the module name, the remainder of the function name gets logged in a hook column in the database. For example, for the function "node_list_permissions", "node" would get stored in the "module" column, "list_permissions" would get stored in the "hook" column.
  • When module_implements() returns a list of modules that implement a particular hook, it can bring up a list of functions indexed in the cache by hook, so it automatically knows which functions to call. Contrast that to what happens currently, where it needs to loop through all the functions and check them one by one in the registry (it calls module_hook(), which calls drupal_function_exists(), which is slow). In the case of a cache miss, the DB hits become 1 * (number of hooks called on the page), vs. (number of modules) * (number of hooks called on the page) times.
  • Modules (like Views) that implement hooks on other modules behalf will have to add their include files to the relevant .info file through the existing hook_system_info_alter().
webchick’s picture

Status: Needs review » Needs work

Now, for an initial pass.

  • It irks me in that "normalization nazi" sort of way that there is going to end up being cruft in the 'hook' column which are not hooks at all. I argued that the best way to deal with this is a hook registry, similar to the theme registry which contains the hook name, parameters, description, and so on. chx pointed out that the theme registry is a pain in the butt to deal with, and he's not wrong. The idea is that this patch takes the pain out of having to manually define hooks, while allowing us to can add additional metadata (such as weights) in a future patch for only those oddball hooks that need it. I still need to think about this some more.
  • On the whole, +1 to lines like this:
    -    foreach (module_implements('watchdog', TRUE) as $module) {
    +    foreach (module_implements('watchdog') as $module) {
    
  • However, the following lines make absolutely no sense to me:
    -  registry_cache_hook_implementations(FALSE, TRUE);
    +  module_implements();
    ...
    +  module_implements('', FALSE, TRUE);
       cache_clear_all('*', 'cache_registry', TRUE);
    

    At the very least they need comments, but it really is starting to feel like module_implements() is becoming this Frankenstein monster of a function -- it's module_build_hook_cache(), module_clear_hook_cache(), module_get_implementation_of_hook(), at least -- and should likely be split up.

  • I'd like to hear more of Crell's thoughts on this patch, having had a hand in the registry stuff. It sounded like he wasn't convinced up above.
  • + * @param $file
    + *   An associative array about the file. Only the module key is used.
    

    That parameter either needs WAY more comments, or it needs to just be $module.

chx’s picture

Status: Needs work » Needs review
FileSize
21.88 KB

module_implements split up, utility functions added, doxygen'd commented. That parameter is fixed.

RobLoach’s picture

Status: Needs review » Needs work
FileSize
26.34 KB

Was hitting:

Warning: Wrong parameter count for function_exists() in /var/www/drupal/head/includes/module.inc on line 449

Fixed that with:

/**
 * This is the maintenance version of module_implements.
 *
 * @param $hook
 *   The name of the hook (e.g. "help" or "menu").
 * @return
 *   An array with the names of the modules which are implementing this hook.
 *   Only enabled and already loaded modules are taken into consideration.
 */
function _module_implements_maintenance($hook) {
  $implementations = array();
  foreach (module_list() as $module) {
    $function = $module . '_' . $hook;
    if (function_exists($function)) {
      $implementations[] = $module;
    }
  }
  return $implementations;
}

Now I'm hitting:

notice: Use of undefined constant MODULE_IMPLEMENTS_CLEAR_CACHE - assumed 'MODULE_IMPLEMENTS_CLEAR_CACHE' in /var/www/drupal/head/includes/registry.inc on line 70.

chx’s picture

Status: Needs work » Needs review
FileSize
22.43 KB

Fixed.

RobLoach’s picture

Status: Needs review » Needs work

Hmmm,

notice: Undefined variable: refresh in /var/www/drupal/head/includes/module.inc on line 420.

.....

Fatal error: Cannot redeclare cache_get() (previously declared in /var/www/drupal/head/includes/cache.inc:17) in /var/www/drupal/head/includes/cache-install.inc on line 16
chx’s picture

Status: Needs work » Needs review
FileSize
23.1 KB

Readded $sort via another utility function after realizing help needs it.

sun’s picture

Re-rolled without unnecessary cruft (coding-style changes).

sun’s picture

Fixed some PHPdocs, and renamed _module_implements_sorted() to _module_implements_sort(), and changed $stored into $cache in _module_implements_sort() for better visual separation from $sorted.

sun’s picture

To test this patch on a existing site running HEAD, execute the following queries:

ALTER TABLE `registry` ADD `module` VARCHAR(255) NOT NULL, ADD `hook` VARCHAR(255) NOT NULL;
TRUNCATE `registry_file`;

and visit devel/cache/clear afterwards.

Testing code in custom.module:

function custom_boot() {
  cache_clear_all('hooks', 'cache_registry');
  module_implements(NULL, FALSE, TRUE);
  var_export(module_implements('schema'));
}
sun’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
28.4 KB

Removed some obsolete cruft from _registry_parse_files().
Fixed _element_info() returning NULL instead of an (expected) array if no elements are returned. (leading to a fatal error otherwise)

Please note that you still need to perform a complete registry rebuild after applying this patch.

Testing results using the example code in #26 (which should simulate the situation we have DIRECTLY after installation):

For D6:

array ( )

For D7 unpatched:

array ( )

For D7 patched:

array ( 0 => 'block', 1 => 'filter', 2 => 'node', 3 => 'system', 4 => 'user', 5 => 'comment', 6 => 'dblog', 7 => 'menu', 8 => 'simpletest', )

Simpletest results:
5799 passes, 5 fails, 0 exceptions

Like always, failed tests are only file related; see #315520: assertFilePermissions() not working on Windows

Aside from that, the implementation looks good, clever, and clean. Some parts might not be trivial, but make perfectly sense in the end.

So this patch should be RTBC.

The only remaining issue - NOT relevant/tied to this issue - is whether we can implement some failsafe algorithm, which automatically performs a complete registry rebuild if the stored data is empty, unavailable, or incomplete due to any reason, since users need to invoke drupal_flush_all_caches() somehow in such situations.

chx’s picture

Note that if registry is empty it rebuilds itself. You only had a problem because you ran an unpatched Drupal and then a patched one and the cached contents clashed. This can not happen normally.

drewish’s picture

subscribing

webchick’s picture

I've reviewed this patch 2-3 times now, and all the nit-picky things I can find to say about it have been fixed.

At an architectural level, I do have some concerns about the 'best guess' approach it takes to identifying hooks. But I understand that this is a trade-off in agility vs. accuracy... there are concerns about requiring yet another "registry" function with the theme registry, and possibly variable registry, etc.

However, I'd like to get Dries's two cents before committing this, since it seems like it crosses that treshold of "big" change.

Anonymous’s picture

i don't like the extra 'guess' hook data in the registry schema either.

here's an alternative for _module_implements_build that doesn't require any changes to the registry schema.

its a bit slower than chx's patch, but its simpler, and it is quicker than the current implementation.

/**
 * this is ugly, there's probably a DBNTG way to do it, but you get the idea.
 */
function _module_implements_build($hook) {
  $return = array();
  $candidate_functions = array();
  $modules = array();

  $result = db_query("SELECT name FROM {system} WHERE type = 'module' AND status = 1");
  while ($module = db_fetch_object($result)) {
    $candidate_functions[] = "'" . $module->name . '_' . $hook . "'";
    $modules[$module->name . '_' . $hook] = $module->name;
  }
 
  $result = db_query("SELECT name, filename FROM {registry} WHERE type = 'function' AND name IN (" . implode(',', $candidate_functions) . ")");
  while ($function = db_fetch_object($result)) {
    // We need to load the relevant file for this function.
    require_once DRUPAL_ROOT . '/' . $function->filename;
    registry_mark_code('function', $function->name);
    $return[] = $modules[$function->name];
  }

  return $return;
}
webchick’s picture

Status: Reviewed & tested by the community » Needs review

Sounds like this still needs discussion.

Anonymous’s picture

to make this easier, i've created add functions in .info files to the registry.

chx’s picture

Status: Needs review » Reviewed & tested by the community

No this does not, the IN (" . implode(',', $candidate_functions) . ")"); is slow and as you have more modules it gets slower. A simple query against an indexed hook column is vastly faster.

chx’s picture

FileSize
16.33 KB

Rerolled after the patch mentioned in #33 is in.

Anonymous’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
16.45 KB

updated patch always writes the module implementations to cache if we don't get any cache data for a request.

as it was, if we didn't get anything from the cache, then this snippet from module_implements

if ($cache && $implementations != $cache->data) {

would always be false, and we wouldn't cache when we should.

all tests pass, and db queries for a clean install, all modules enabled hitting node/1 with 1 comment are cut by 80%.

still don't like the hook guess approach, and i'll likely post an alternative soon, but this is a big win for cutting back queries, so even if i don't convince people that we don't need the hook guess stuff, i'd still like to see this go in.

setting to code needs review - someone else should validate the caching change.

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
16.36 KB

The change is good, yes but you can express it with a bit less code. Thanks. That drop in 80% of queries is just before the hook cache hits in but it's still great.

keith.smith’s picture

+ *   By default, modules are ordered by weight and filename, settings this
+ *   option to TRUE, module list will be ordered by module name.

This is minor enough that someone could do it before a commit so I'll leave this RTBC; I think "settings" in the comment above should perhaps be "setting".

Or something like "By default, modules are ordered by weight and filename. If this is option is TRUE, modules are ordered alphabetically by module name." might be just as clear, or more so.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Undefined index: lg_debug Notice database_test.test 1689 DatabaseLoggingTestCase->testEnableLogging()

webchick’s picture

Status: Needs work » Reviewed & tested by the community

Wrrrooonnnng issue. :) Sorry.

webchick’s picture

As I said to chx on IRC, this qualifies as a patch that I would like to get Dries's OK on before I commit it. It's kind of dirty, and he might have a better solution.

John Morahan’s picture

FileSize
3.65 KB

I tried to make use of this by turning custom_url_rewrite* into hooks, and ran into bizarre problems with simpletest while doing so. This line in tearDown() in drupal_web_test_case.php seemed to be the culprit:

      // Reload module list to ensure that test module hooks aren't called after tests.
      module_list(TRUE);

Should that be using the new module_implements(MODULE_IMPLEMENTS_CLEAR_CACHE) instead, or as well?

Anonymous’s picture

FileSize
16.3 KB

updated patch to keep up with HEAD. all tests pass.

@John Morahan - not sure about your question.

Anonymous’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
15.6 KB

simpler version, merged _module_implements_build and _module_implements_check and simplified module_implements.

all tests pass. setting back to code needs review to get some more eyes on the changes.

Anonymous’s picture

follow up comment following a request from chx to clarify what i changed.

i've modified the code so that for a cache miss for a given hook, we just call one function, _module_implements_build.

_module_implements_build pulls the hooks from the db, and loads the file for the hook into memory if its not already loaded.

this means we don't need the $loaded static or _module_implements_check helper.

so, just a simplification really. returning the favour for once, as chx usually simplifies my code ;-).

chx’s picture

Justin, that won't fly, the reason I had the loaded thing is that it's possible that the implementations are cached but the include file containing the hook is not loaded.

Damien Tournoud’s picture

Review of chx patch from #37:

@@ -1361,7 +1361,7 @@ function _element_info($type, $refresh =
     }
   }
 
-  return $cache[$type];
+  return isset($cache[$type]) ? $cache[$type] : array();
 }

What is the point of this (is that scope-creep)?

+function module_implements($hook, $sort = FALSE) {

The calling pattern of this function looks unecessary complicated. Why not replacing $sort with a bit mask $mode with MODULE_IMPLEMENTS_CLEAR_CACHE, MODULE_IMPLEMENTS_WRITE_CACHE and MODULE_IMPLEMENTS_SORT?

+    // Though drupal_function_exists itself checks function_exists,
+    // most of the time the function will exist because of the per router path
+    // hook cache so we can save lots of calls to drupal_function_exists.
+    if (!function_exists($function)) {
+      drupal_function_exists($function);
     }

What is the use of forcing the load of the function here?

I don't see the point of moving the whole _module_implements_check() in a separate function (it is called only once), it could be moved back to module_implements().

+    // We need to load the relevant file for this function.
+    drupal_function_exists($function->name);

In that case, this code should be removed.

+function _module_implements_sort($implementations, $hook) {
+  static $sorted = array(), $cache = array();

Here we are storing cached implementation in $cache just for the purpose of clearing those statics. I suggest moving those statics to module_implements().

chx’s picture

Moving to separate functions IMO increased the readibility.

chx’s picture

Re. form.inc , it can be omitted if you want, it only caused problems for sun because he applied the patch into an already installed Drupal. I removed six comments attempting to derail the issue over such a minute detail.

Both to Damien and Justin,

foreach (module_implements('foo') as $module) {
  $function = $module .'_foo';
  $function();
}

module_implements must ensure that the function you are likely to be calling in the near future is actually loaded.
About calling patterns, as you wish, really. I hope I can get back Justin in the issue and agree whether #43 or #44 is to be continued.

Anonymous’s picture

chx: #43. i missed the 'if its loaded from cache the implementations might not be in memory' part, so #44 is flawed.

Damien Tournoud’s picture

Status: Needs review » Needs work

We agreed with chx on the IRC that we could remove both _module_implements_check and _module_implements_build and factor them inside module_implements while keeping high readability.

This patch still misses two key parts:
- (1) the default ordering of hooks by weight and module name
- (2) a $sort parameter for _module_implements_maintenance

chx’s picture

Status: Needs work » Needs review
FileSize
14.47 KB

Here we come. I removed the form.inc patch so kindly please apply this and install after.

chx’s picture

FileSize
15.22 KB

Now, registry.test needed an update. Also, if you notice the 68 for the suffix length, well MySQL did not allow longer otherwise the index creation would fail. We can make the weight a smallint to win a few bytes and juggle with the module and the suffix but meh -- when did you use a hook longer than 68 characters?

keith.smith’s picture

+ *   By default, modules are ordered by weight and filename, settings this
+ *   option to TRUE, module list will be ordered by module name.

As in #38, I think "settings" should be "setting".

Damien Tournoud’s picture

- acked #60
- replaced $cache (the whole previous cache) to $cached_hooks (the count of previously cached implementations), to save memory

I think it would be better if we limit module names to 128 chars. That way, hooks could be 126 characters (because module + '_' + hook = 255, because function names are stored in VARCHAR(255)), and we would be better off here.

chx’s picture

I agree with Damien though PHP reference counting makes his memory argument moot (unless there is a new hook called) -- instead it's a performance improvement, and as Zend hashtables store their count storing the count in a variable is a rather speedy operation.

sun’s picture

Re-rolled,

- minor PHPdoc fixes for module_implements and _registry_parse_file.
- changed $hook === MODULE_IMPLEMENTS_CLEAR_CACHE tests to be type-agnostic, so invoking module_implements('', ...) does not result in a cleared cache.

Anonymous’s picture

is this safe?

  if ($hook === MODULE_IMPLEMENTS_WRITE_CACHE) {
    // Only write this to cache if we loaded new implementations.
    if (count($implementations) > $cached_hooks) {
      cache_set('hooks', $implementations, 'cache_registry');
    }
    return;
  }

what if we have a one module enabled and an another module disabled, and the count of hooks stays the same, but the list if different?

Damien Tournoud’s picture

@justinrandell: You have to understand that this function does a progressive caching of the hook. Implementations for a given hook are cached the first time the hook is called. That's the reason why we check for the number of implementations here. Module enabling/disabling is a completely different story. In that case, we simply empty the full cache of hook implementations.

Anonymous’s picture

@Damien: yep, my mistake.

Anonymous’s picture

watching the registry queries is much easier with the new db logging code :-)

with the patch at #63 we cut out a lot, but for a logged in user, all core modules enabled, loading node/1 with one comment, we're still at 12 registry queries with a fully loaded cache.

if we cache registry misses, this drops down to 5.

if people aren't to allergic to the idea, i'll roll a patch, else i can wait for a follow up issue.

chx’s picture

@justinrandell thanks for filing a separate issue at http://drupal.org/node/325665 ;)

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

ok, lets do the registry misses caching in a follow up.

here's an updated patch, with drupal_function_exists calling _registry_check_code('function', $function);. otherwise, its the same as at #63.

all tests pass.

now, how do we get Dries to weigh in on this issue?

Anonymous’s picture

FileSize
17.13 KB

attached patch this time.

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work

@justin: I really don't like those changes added a the last minute.

Your change broke the negative caching in drupal_function_exists(), because _registry_check_code() returns NULL on miss, not FALSE.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
17.6 KB

@Damien: good catch on NULL vs FALSE, attached patch fixes that, _registry_check_code return FALSE on a miss.

all tests pass.

Damien Tournoud’s picture

Opened #325994: Reduce system.name to 128 chars for the suggested change to the module name length.

kbahey’s picture

Status: Needs review » Needs work

I tried to benchmark using this patch, to see if it speeds up things, using an existing Drupal 7 site (from last Friday or so), but I get the following error when trying to access any page:

PDOException: SELECT module FROM {registry} WHERE type = 'function' AND suffix = :hook ORDER BY weight, module - Array ( [:hook] => init ) SQLSTATE[42S22]: Column not found: 1054 Unknown column 'module' in 'field list' in module_implements() (line 447 of ../includes/module.inc).

And there are no pending updates in update.php.

So, I emptied the database, loaded the Drupal 6 database, and tried to run update.php, but I get another error and can't proceed:

An unrecoverable error has occurred. You can find the error message below. It is advised to copy it to the clipboard for reference.
Please continue to the error page
An error occurred. http://example.com/update.php?id=2&op=do (no information available).

The error page linked is not much better, very vague:

Fatal error: Exception thrown without a stack frame in Unknown on line 0

So, this broke update.php.

Backing out the patch makes update.php works again.

I tried creating an system_update_7012() for the new fields and indexes for the registry table, but that caused update.php to white screen (probably a PDO error ...)

Anonymous’s picture

just some simple data points of HEAD vs HEAD + this patch.

using ab -n 1000 -c 10 http:///node/1

APC on, page and block caching off, anonymous user, all core modules enabled, one comment on node/1:

HEAD
Requests per second: 27.88 [#/sec] (mean)
Requests per second: 27.74 [#/sec] (mean)
Requests per second: 27.59 [#/sec] (mean)

patched
Requests per second: 30.62 [#/sec] (mean)
Requests per second: 30.50 [#/sec] (mean)
Requests per second: 30.69 [#/sec] (mean)

Dries’s picture

justin: how does that compare to a Drupal 6 site? Would be useful, but not critical, to know.

Dries’s picture

- Overall this looks like a good API clean-up. The only part that smells a bit is the maintenance version of module_implements. The documentation of that falls short: "This is the maintenance version of module_implements." Please add some more documentation description why this function is important/needed, and when it should (or should not) be called. By adding some more comments, we might be able to increase the comfort level of having such a function.

-

+        $suffix = '';
+        if ($type == 'function' && !empty($module)) {
+          $n = strlen($module);
+          if (substr($resource_name, 0, $n) == $module) {
+            $suffix = substr($resource_name, $n + 1);
+          }
+        }

Maybe add a code comment explaining what we do? I understand what we do, but it would saved me 15 seconds if there was a code comment. ;-)

- Typo: It'the name.

Looks good -- it is a matter of adding a bit more documentation so we can make sense of the maintenance version.

kbahey’s picture

@Dries & Justin

I was trying to exactly do that: compare performance of before/after the patch and to Drupal 6

I wanted to keep all factors the same by upgrading the database from D6 to D7.

See my comment in 74.

If we can get over this, I can do more thorough performance analysis.

chx’s picture

Status: Needs work » Needs review

@kbahey, there are issues open for update.php please do not derail this one, i will add more docs and add an underscore to the maintenance version as you should never call it.

kbahey’s picture

@chx

Maybe I was not clear.

Without the patch, I can update a site fine.

With the patch, I cannot update a site at all.

So the patch introduced something that causes failure. So was pointing this out.

I am fine that the update be fixed in a separate issue, just wanted to point this out in case it can be easily fixed.

chx’s picture

FileSize
15.41 KB

Wait a second, I already had an underscore there. Ah well. It needed a reroll anyways and added "for internal use only".

Anonymous’s picture

@Dries, repeated with DRUPAL_6 vs HEAD vs HEAD + this patch

using ab -n 1000 -c 10 http://site/node/1

APC on, page and block caching off, anonymous user, all core modules enabled, one comment on node/1:

DRUPAL_6
Requests per second: 37.50 [#/sec] (mean)
Requests per second: 37.60 [#/sec] (mean)
Requests per second: 37.56 [#/sec] (mean)

HEAD
Requests per second: 27.58 [#/sec] (mean)
Requests per second: 27.63 [#/sec] (mean)
Requests per second: 27.60 [#/sec] (mean)

HEAD + patch
Requests per second: 30.64 [#/sec] (mean)
Requests per second: 30.58 [#/sec] (mean)
Requests per second: 30.66 [#/sec] (mean)

APC off, page and block caching off, anonymous user, all core modules enabled, one comment on node/1:

DRUPAL_6
Requests per second: 9.42 [#/sec] (mean)
Requests per second: 9.44 [#/sec] (mean)
Requests per second: 9.45 [#/sec] (mean)

HEAD
Requests per second: 7.96 [#/sec] (mean)
Requests per second: 7.91 [#/sec] (mean)
Requests per second: 7.93 [#/sec] (mean)

HEAD + patch
Requests per second: 8.34 [#/sec] (mean)
Requests per second: 8.36 [#/sec] (mean)
Requests per second: 8.34 [#/sec] (mean)

note, this is not an upgrade of the 6 install to 7, its two clean installs, enable all modules, create one node and add a comment to the node. clearly HEAD with or without the patch is slower, inline with the results from khalid.

Anonymous’s picture

FileSize
17.07 KB

rerolled to keep up with HEAD, and added a couple of lines of comments to the hook gathering code in the registry rebuild cycle as per Dries comments in #77.

so, no code changes, and all tests still pass, so RTBC.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

ahem, RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

_module_implements_maintenance should explain how it is different from the regular _module_implements and why it is a safe path. Almost there ...

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
15.86 KB

It's safe because it does not use the DB. Explained. In detail.

keith.smith’s picture

Status: Reviewed & tested by the community » Needs work

As Dries pointed out in #77:

+        'description' => t("The part of the name after the module. It'the name of the hook this function implements, if any."),

And, as in #38 and #60,

+ *   By default, modules are ordered by weight and filename, settings this
+ *   option to TRUE, module list will be ordered by module name.

there's something odd with "settings" here.

But, these are both minor issues; I'll roll a patch if someone doesn't beat me to it.

Anonymous’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
17.82 KB

geez, you'd think people around here actually cared about code quality or something :-)

updated patch attached to address typo's in #87.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks guys.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.