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).
Comment | File | Size | Author |
---|---|---|---|
#23 | 794936-1-D6.patch | 2.51 KB | c960657 |
#17 | 794936_system_rebuild_module_data.patch | 2.86 KB | amateescu |
#10 | system_rebuild_module_data-5.patch | 4.3 KB | c960657 |
#9 | system_rebuild_module_data-4.patch | 6.84 KB | c960657 |
#7 | system_rebuild_module_data-3.patch | 4.33 KB | c960657 |
Comments
Comment #1
Dries CreditAttribution: Dries commentedNo patch attached. :)
Comment #2
Dries CreditAttribution: Dries commentedWhat's up with the Issue tag behavior in #1?
Comment #3
c960657 CreditAttribution: c960657 commentedHere's the patch.
Comment #5
c960657 CreditAttribution: c960657 commentedWow, that's a lot of test failures for such a small change :-)
Let's try using drupal_static() instead.
Comment #7
c960657 CreditAttribution: c960657 commentedThis 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.
Comment #8
c960657 CreditAttribution: c960657 commentedComment #9
c960657 CreditAttribution: c960657 commentedReroll.
Comment #10
c960657 CreditAttribution: c960657 commentedIgnore the last patch.
Comment #11
dwwConfirmed 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.
Comment #12
carlos8f CreditAttribution: carlos8f commentedPatch looks interesting. Will review.
Comment #13
dwwx-post. :/
Comment #14
carlos8f CreditAttribution: carlos8f commented#218066: Prevent cross posting from reverting metadata fields -- sorry :)
Comment #15
tom_o_t CreditAttribution: tom_o_t commented#10: system_rebuild_module_data-5.patch queued for re-testing.
Comment #16
webchickNo longer applies.
Comment #17
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #18
amateescu CreditAttribution: amateescu commentedBack to RTBC per #13.
Comment #19
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #20
carlos8f CreditAttribution: carlos8f commentedPatch looks reasonable. Nice work.
Comment #22
c960657 CreditAttribution: c960657 commentedD6 backport.
Comment #23
c960657 CreditAttribution: c960657 commented