On admin/build/modules, system_rebuild_module_data() is called twice when Update module is enabled: Once from update_get_projects(), and once from system_modules(). If you have a lot of modules, each call may take several seconds. The information can be cached, because the module files do not change during a request (and if they do, e.g. during development or deployment, the changes are picked up in the next request).

This patch simply adds a static cache to system_rebuild_module_data(). This seems easier than making update_get_projects() and system_modules() somehow "coordinate" their efforts and only call the function once.

On a development site with 230 directories in the modules folder, this cuts down the response time of admin/build/modules from ~10 seconds to ~6 seconds (using PHP 5.2.11 on Windows XP).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dries’s picture

No patch attached. :)

Dries’s picture

What's up with the Issue tag behavior in #1?

c960657’s picture

Here's the patch.

Status: Needs review » Needs work

The last submitted patch, system_rebuild_module_data-1.patch, failed testing.

c960657’s picture

Status: Needs work » Needs review
FileSize
1 KB

Wow, that's a lot of test failures for such a small change :-)

Let's try using drupal_static() instead.

Status: Needs review » Needs work

The last submitted patch, system_rebuild_module_data-2.patch, failed testing.

c960657’s picture

This was a bit tricky. I missed that we should reset the cache when a module is enabled or disabled. So I added a drupal_static_reset() in system_list_reset(). I'm not sure that this actually make any of the failing tests pass, but it seems like the right thing to do anyway.

Then I got some very confusing results. $modules in system_rebuild_module_data() was suddenly empty, when it was passed to _module_build_dependencies(). It turned out that this is because the variable is reset by reference from system_list_reset() that is called from system_update_files_database(), so the module list was actually being reset while in the middle of being built. So we need two variables in that function.

Finally I realized that the tests were failing because module.test manipulates the module dependencies using a variable, so we need to explicitly reset the static variable there.

Let's see how the test bot likes this.

c960657’s picture

Status: Needs work » Needs review
c960657’s picture

Reroll.

c960657’s picture

Ignore the last patch.

dww’s picture

Status: Needs review » Reviewed & tested by the community

Confirmed that even with #651086: Cache clearing is an ineffective mess in module_enable() and system_modules_submit() #10 is still needed (and that update status still works fine with it), as expected.

carlos8f’s picture

Status: Reviewed & tested by the community » Needs review

Patch looks interesting. Will review.

dww’s picture

Status: Needs review » Reviewed & tested by the community

x-post. :/

carlos8f’s picture

tom_o_t’s picture

#10: system_rebuild_module_data-5.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

No longer applies.

amateescu’s picture

Status: Needs work » Needs review
FileSize
2.86 KB

Rerolled.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC per #13.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

carlos8f’s picture

Patch looks reasonable. Nice work.

Status: Fixed » Closed (fixed)

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

c960657’s picture

Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Needs review

D6 backport.

c960657’s picture

FileSize
2.51 KB

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.