Updated: Comment #88

Problem/Motivation

With system_list(), memory usage increases significantly when large numbers of modules are enabled.

Issue #887870: Make system_list() return full module records triggered this memory spike, since full module records are returned, rather than just module names. Since #328357: Allow module .info files to add CSS/JS allowed module .info files to pull in CSS/JS, this can lead to very high memory usage.

Examples:

  • 31 modules enabled: Roughly 400kb of memory used
  • 90 modules enabled: Roughly 676kb of memory used

Proposed resolution

A lot of information in the system table isn't needed with system_list(). Instead, we can just store the array directly with the relevant info and improve how the cache is handled.

Here is an example of the performance improvements:

Before Patch

  • Cache clear: 38206ms, 188.5MB RAM
  • Cached page load: 315ms, 15.25MB RAM

After Patch

  • Cache clear: 35875ms, 186.5MB RAM
  • Cached page load: 308ms, 14.5MB RAM

Please note that this patch requires update.php to be run after it is applied.

Remaining tasks

Currently this patch is queued for re-testing.

User interface changes

None

API changes

Rather than loading full module records into the system list, only the requested module information is returned.

Original report by [catch]

system_list() with 31 modules enabled uses around 400kb of memory. On a D7 site with 90 modules installed this increases to 676kb - 13% of total memory usage on that site.

This is due to #887870: Make system_list() return full module records - we did benchmarks there but no-one including me actually checked the memory implications.
We have to keep info there, which is the biggest issue, due to css/js additions in system_init(), I didn't like that change but it's too late to change now at least for D7. Although with a small API change we could strip some of the other info keys, but that needs to happen in another issue. edit: that's out of the question for D7 too, we're using things like configuration_path via system_get_info() too.

However there's a lot of crap in system table that we don't need, so let's try to cut that down.

Here's how block module looks in HEAD:


            [block] => stdClass Object
                (
                    [filename] => modules/block/block.module
                    [name] => block
                    [type] => module
                    [owner] =>
                    [status] => 1
                    [bootstrap] => 0
                    [schema_version] => 7007
                    [weight] => -5
                    [info] => Array
                        (
                            [name] => Block
                            [description] => Controls the visual building blocks a page is constructed with. Blocks are boxes of content rendered into an area, or region, of a web page.
                            [package] => Core
                            [version] => 7.0-dev
                            [core] => 7.x
                            [files] => Array
                                (
                                    [0] => block.test
                                )

                            [configure] => admin/structure/block
                            [dependencies] => Array
                                (
                                )

                            [php] => 5.2.4
                            [bootstrap] => 0
                        )

                )

Instead of this we can just store the array directly.

These numbers include the entire memory usage from system_info() - including bootstrap, themes and filenames, but the saving is only against modules, still very measurable though:

With 30 modules:
before patch: 415,016 / peak: 495,464

After patch: 330,240 / peak: 394,712

With 90 modules:

Before patch: 691,620 / peak: 829,164

After patch: 569,200 / 684,872

So that's > 100kb off inclusive peak memory usage, and between 85kb to 120kb off inclusive memory usage. With 150 or 200 contrib modules installed we could be looking at more like 200kb of savings.

This is going to be a bit tricky with upgrades due to the array being used in full bootstrap, you can get more modest memory savings from keeping it as an object with just the info property, that's about 15kb more expensive with 30 modules but less of a change. Uploading both patches for now.

Files: 
CommentFileSizeAuthor
#103 system-list-memory-usage-1061924-103.patch6.41 KBBerdir
#81 system-list-memory-usage-1061924-81.patch6.3 KBgeerlingguy
PASSED: [[SimpleTest]]: [MySQL] 41,127 pass(es). View
#59 drupal-1061924-59.patch7.17 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 37,247 pass(es). View
#59 drupal-1061924-59-combined-with-1503224.patch11.57 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 37,102 pass(es). View
#58 drupal-1061924-combined_with_1503224-58.patch11.6 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 37,287 pass(es). View
#53 drupal8.system-info.53.patch7.17 KBsun
FAILED: [[SimpleTest]]: [MySQL] 37,294 pass(es), 0 fail(s), and 24 exception(s). View
#53 interdiff.txt2.08 KBsun
#49 drupal-1061924-49.patch6.34 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 37,028 pass(es). View
#49 interdiff.txt2.68 KBtim.plunkett
#46 system_info.patch5.91 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 36,854 pass(es). View
#44 system_info.patch5.91 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#42 system_info.patch5.74 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#38 system_info.patch5.73 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,168 pass(es). View
#33 system_info.patch5.73 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 34,164 pass(es). View
#31 system_get_module_info.patch5.7 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 33,634 pass(es). View
#29 system_list_15-16_interdiff.txt1.73 KBbfroehle
#29 system_list_16.patch5.55 KBbfroehle
PASSED: [[SimpleTest]]: [MySQL] 32,922 pass(es). View
#27 system_list.patch5.47 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 32,901 pass(es). View
#24 system_info.patch3.24 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 32,859 pass(es). View
#23 system_get_info_1.patch9.46 KBStefan Freudenberg
PASSED: [[SimpleTest]]: [MySQL] 33,679 pass(es). View
#21 Desk 1_084.png95.7 KBcatch
#21 Desk 1_086.png108.61 KBcatch
#21 Desk 1_085.png106.21 KBcatch
#21 system_get_info.patch9.48 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 32,773 pass(es). View
#19 system_get_info.patch9.48 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 32,188 pass(es), 2 fail(s), and 0 exception(es). View
#15 system_list.patch4.84 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 29,841 pass(es). View
#12 system_list_1061924.patch3.73 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 29,433 pass(es). View
#5 head.png78.46 KBcatch
#5 system_get_info.png54.14 KBcatch
#5 system_list.png73.5 KBcatch
#5 system_list.patch3.54 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 31,527 pass(es). View
#4 system_list.patch1.05 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 31,571 pass(es). View
#1 system_list.patch1.04 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 31,565 pass(es). View
#1 system_list_safer.patch614 bytescatch
FAILED: [[SimpleTest]]: [MySQL] 31,474 pass(es), 73 fail(s), and 1,331 exception(es). View
system_list_safer.patch614 bytescatch
FAILED: [[SimpleTest]]: [MySQL] 31,484 pass(es), 73 fail(s), and 1,331 exception(es). View

Comments

catch’s picture

Status: Active » Needs review
FileSize
614 bytes
FAILED: [[SimpleTest]]: [MySQL] 31,474 pass(es), 73 fail(s), and 1,331 exception(es). View
1.04 KB
PASSED: [[SimpleTest]]: [MySQL] 31,565 pass(es). View

Status: Needs review » Needs work

The last submitted patch, system_list_safer.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
+++ includes/module.inc
@@ -165,7 +165,7 @@ function system_list($type) {
         // Build a list of all enabled modules.
         if ($record->type == 'module') {
-          $lists['module_enabled'][$record->name] = $record;
+          $lists['module_enabled'][$record->name] = $record->info;
         }

I seriously wonder why this patch passed. $record->name is not used anywhere?

Powered by Dreditor.

catch’s picture

FileSize
1.05 KB
PASSED: [[SimpleTest]]: [MySQL] 31,571 pass(es). View

Here's the 'safe' patch that should pass at least the help tests.

catch’s picture

FileSize
3.54 KB
PASSED: [[SimpleTest]]: [MySQL] 31,527 pass(es). View
73.5 KB
54.14 KB
78.46 KB

So #4 is a simple change that should be pretty uncontroversial.

Here's a different approach, requires an API addition to system_get_info(), but one that is completely backwards compatible, this has much, much bigger memory savings. This is revisiting Frando's suggestion from http://drupal.org/node/887870#comment-3358654 except it keeps system_get_info() handling the caching and tries to optimize that for the most common cases.

system_get_info() now accepts an additional, optional $keys argument - an array of info keys that are needed. This allows system_add_module_assets() to only request stylesheets and scripts for example.

Using xhprof on a site with 32 modules enabled.

With HEAD, system_list() takes 400k/6% (500k/6.9% peak) memory.

With the patch system_list() uses 190k/2.8% (245k/3.5% peak) memory.

system_get_info() with the patch is using it's own cache, that takes 13k / 0.2% (18k/0.3% peak) memory.

So combining the two, we're looking at around 50% memory savings or around 200k of memory. Screenshots attached for that.

I then enabled all core modules + ctools, media, media gallery and some others to get a total of 65 modules enabled.

HEAD:
system_list: 700k / 7.4% (778,776 / 8.3% peak)

Patch:
system_list: 250k 2.8% (310k / 3.3% peak)
system_get_info() 22k / 0.2% (43k / 0.5% peak).

So with 63 modules there's a 60% reduction in memory usage, and getting close to half a megabyte of memory saved.

Also note that in HEAD the percentage change from memory usage jumps from 6% to 7.4%. However with the patch we're static at 2.8% - this means we're at a point where the memory usage from this at least isn't growing faster than everything else when modules are added.

If we doubled the number of modules again (126 is nowhere near the most I've seen enabled on a site), then we're looking at a full mb or more of memory saved here. At that point we might also see measurable performance changes as well, since the quantity of data to fetch and unserialize is going to be quite large by this point.

One issue here is the old system_list cache entry is incompatible with the new one, so had to change the cid slightly, better suggestions than adding ':' to the end welcome.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

I read over the code and it looks like a safe (i.e. backwards compatible) and highly desireable memory improvement.

catch’s picture

Also note that this is a straight regression from Drupal 6, and a recent regression in Drupal 7 - original patch was committed September 2010 which was either 9 months or a year after code freeze depending on when you count from.

Overall we have in the region of 2mb memory regression from Drupal 6 when using an opcode cache - see the numbers at http://ca.tchpole.net/node/2.

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +needs backport to D7
Dries’s picture

Status: Reviewed & tested by the community » Needs work
+++ includes/module.inc
@@ -197,6 +197,7 @@ function system_list_reset() {
   cache_clear_all('bootstrap_modules', 'cache_bootstrap');
   cache_clear_all('system_list', 'cache_bootstrap');
+  cache_clear_all('system_info:', 'cache', TRUE);

Would be cleaner if we'd get rid of the ':'. I think we can.

+++ modules/system/system.module
@@ -2291,6 +2291,9 @@ function system_update_files_database(&$files, $type) {
+ * @param $keys
+ *   (optional) An array of info object keys. If passed, only these keys will
+ *   be present in the info array. Otherwise all info will be returned.
  *

I had to read this twice and look at the source code. It wasn't clear what 'info object keys' are. Also, the parameter is called $options instead of $keys.

Also,I'd mention why this is useful to specify.

+++ modules/system/system.module
@@ -2300,16 +2303,27 @@ function system_update_files_database(&$files, $type) {
+  $cid = 'system_info' . ':' . $type . implode('|', $options);

Any reason we don't implode with ':'? Looks like it would be safe to.

sun’s picture

I disagree with the removal of the ':' suffix, as this cache clear is using a wildcard. Thus, removing the ':' suffix may unintentionally kill other caches, and it's going to be very hard to notice and detect this bug when it happens.

catch’s picture

Assigned: Unassigned » catch

Reminding myself I need to refresh this. I agree with #10 - the namespacing is important to me.

The rest of #9 looks fine - will fix those in a re-roll.

catch’s picture

Status: Needs work » Needs review
FileSize
3.73 KB
PASSED: [[SimpleTest]]: [MySQL] 29,433 pass(es). View

This should cover everything except the ':' suffix on the wildcard clear.

catch’s picture

Priority: Normal » Major

Bumping this to major since 500kb of memory with just 63 modules enabled is significant. I have seen more than one Drupal 6 site with over 200 modules enabled, we're looking at a couple of megabytes of memory wasted on sites like that.

sun’s picture

Status: Needs review » Needs work
+++ b/includes/module.inc
@@ -167,7 +167,7 @@ function system_list($type) {
         $record->info = unserialize($record->info);

The unserialize() is only required for themes then.

+++ b/includes/module.inc
@@ -167,7 +167,7 @@ function system_list($type) {
-          $lists['module_enabled'][$record->name] = $record;
+          $lists['module_enabled'][] = $record->name;

We should still key the module list by module name (i.e., key + value being identical). Themes are keyed by their name, too.

+++ b/includes/module.inc
@@ -199,6 +199,7 @@ function system_list_reset() {
   cache_clear_all('bootstrap_modules', 'cache_bootstrap');
   cache_clear_all('system_list', 'cache_bootstrap');
+  cache_clear_all('system_info:', 'cache', TRUE);

Shouldn't the items live in cache_bootstrap as well?

+++ b/modules/system/system.module
@@ -1908,7 +1908,7 @@ function system_init() {
-  foreach (system_get_info('module') as $module => $info) {
+  foreach (system_get_info('module', NULL, array('stylesheets', 'scripts')) as $module => $info) {

This reminds me a lot of #624848: Allow to retrieve a list of modules defining a certain .info file property

However, that was purely meant for .info file data retrieval that doesn't happen on every page load.

From a performance perspective, in this patch I could imagine that we'll quickly see duplicate caches and needless lookups. For example, scripts + stylesheets are retrieved here, while another module may only retrieve stylesheets, and yet another only scripts, etc.

It might be more sensible to only allow for a single .info file key ($option) instead of multiple.

+++ b/modules/system/system.module
@@ -2290,6 +2290,12 @@ function system_update_files_database(&$files, $type) {
+ *   (optional) An array of keys to get info for. For example array('styles')

"A list of .info file properties to retrieve."

+++ b/modules/system/system.module
@@ -2299,16 +2305,27 @@ function system_update_files_database(&$files, $type) {
+  $cid = 'system_info' . ':' . $type . implode('|', $options);

1) We're missing a delimiter after $type.

2) 'system_info' and ':' don't need to be concatenated.

3) Replace '|' with ':' in the implode?

4) If we're going to keep multiple $options, then we should at least sort() them.

Powered by Dreditor.

catch’s picture

Status: Needs work » Needs review
FileSize
4.84 KB
PASSED: [[SimpleTest]]: [MySQL] 29,841 pass(es). View
+++ b/includes/module.inc
@@ -167,7 +167,7 @@ function system_list($type) {
         $record->info = unserialize($record->info);

The unserialize() is only required for themes then.

Fixed.

+++ b/includes/module.inc
@@ -167,7 +167,7 @@ function system_list($type) {
-          $lists['module_enabled'][$record->name] = $record;
+          $lists['module_enabled'][] = $record->name;

We should still key the module list by module name (i.e., key + value being identical). Themes are keyed by their name, too.

Fixed.

+++ b/includes/module.inc
@@ -199,6 +199,7 @@ function system_list_reset() {
   cache_clear_all('bootstrap_modules', 'cache_bootstrap');
   cache_clear_all('system_list', 'cache_bootstrap');
+  cache_clear_all('system_info:', 'cache', TRUE);

Shouldn't the items live in cache_bootstrap as well?

No, we should never, ever have been loading data from .info files in bootstrap, and this is just a bandaid because 7.x came out with this regression. I have another issue open to move the css/js code out of system_init() into hook_page_alter() or similar, so when that's done, this will never be loaded during bootstrap again.

+++ b/modules/system/system.module
@@ -1908,7 +1908,7 @@ function system_init() {
-  foreach (system_get_info('module') as $module => $info) {
+  foreach (system_get_info('module', NULL, array('stylesheets', 'scripts')) as $module => $info) {

This reminds me a lot of #624848: Allow to retrieve a list of modules defining a certain .info file property

However, that was purely meant for .info file data retrieval that doesn't happen on every page load.

I wish someone had brought that up on the issue where this regression was introduced.

From a performance perspective, in this patch I could imagine that we'll quickly see duplicate caches and needless lookups.

Bettter than loading the entire thing on every request.

For example, scripts + stylesheets are retrieved here, while another module may only retrieve stylesheets, and yet another only scripts, etc.

Those are contrib modules, this is core. If you install a stupid contrib module, that's a shame. Let's not add i/o and cpu usage to core just in case.

It might be more sensible to only allow for a single .info file key ($option) instead of multiple.

No because then instead of this patch adding one extra network request, it would add two extra network requests, for data we know system_init() is going to be requesting every bootstrap on every Drupal 7 site everywhere.

+++ b/modules/system/system.module
@@ -2290,6 +2290,12 @@ function system_update_files_database(&$files, $type) {
+ *   (optional) An array of keys to get info for. For example array('styles')

"A list of .info file properties to retrieve."

Fixed.

+++ b/modules/system/system.module
@@ -2299,16 +2305,27 @@ function system_update_files_database(&$files, $type) {
+  $cid = 'system_info' . ':' . $type . implode('|', $options);
1) We're missing a delimiter after $type.

2) 'system_info' and ':' don't need to be concatenated.

3) Replace '|' with ':' in the implode?

4) If we're going to keep multiple $options, then we should at least sort() them.

All fixed.

aspilicious’s picture

Status: Needs review » Needs work
+++ b/modules/system/system.moduleundefined
@@ -2290,6 +2290,12 @@ function system_update_files_database(&$files, $type) {
+ *   all modules can be a very large amount of data. ¶

Remove trailing whitespace in next iteration :)

Powered by Dreditor.

sun’s picture

+++ b/modules/system/system.module
@@ -2299,16 +2305,28 @@ function system_update_files_database(&$files, $type) {
+  $cid = 'system_info:' . $type . ':' . implode(':', $options);
+  if ($cached = cache_get($cid)) {
+    $info = $cached->data;
   }
...
+  else {
+    $result = db_query('SELECT * FROM {system} WHERE type = :type AND status = 1', array(':type' => $type));
+    foreach($result as $record) {
+      $record->info = unserialize($record->info);
+      if (!empty($options)) {
+        sort($options);
+        $record->info = array_intersect_key($record->info, array_flip($options));
+        if (!empty($record->info)) {
+          $info[$record->name] = $record->info;
+        }

mmm, the sort() was supposed to be at the beginning of the function; i.e., for the initial implode().

Powered by Dreditor.

catch’s picture

fwiw this is probably something that can use CacheArrayObject from the theme registry issue, this would also allow us to have just a single $key parameter and share a single cache entry without a lot of extra code, so I may re-roll using that rather than continuing with the current version of the patch (assuming the other has a decent chance to get in).

catch’s picture

Status: Needs work » Needs review
FileSize
9.48 KB
FAILED: [[SimpleTest]]: [MySQL] 32,188 pass(es), 2 fail(s), and 0 exception(es). View

Here's a patch using that, the includes/bootstrap.inc hunk is just copied from #402896: Introduce DrupalCacheArray and use it for drupal_get_schema(), putting it here so tests will run.

Status: Needs review » Needs work

The last submitted patch, system_get_info.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
9.48 KB
PASSED: [[SimpleTest]]: [MySQL] 32,773 pass(es). View
106.21 KB
108.61 KB
95.7 KB

Fixed test failures.

Uploading xhprof screenshots.

Memory usage for system_list() before the patch on a vanilla install is 400kb.

Memory usage for system_list() + system_get_module_info() combined on the same install is 226kb (this is with stylesheets and scripts cached).

xjm’s picture

Tagging issues not yet using summary template.

Stefan Freudenberg’s picture

FileSize
9.46 KB
PASSED: [[SimpleTest]]: [MySQL] 33,679 pass(es). View

Updated the patch to current 8.x and fixed whitespace and indentation issues. I assume that at least system_get_info() is covered by tests, not so sure about system_add_module_assets(). Does Simpletest provide metrics about test coverage?

catch’s picture

FileSize
3.24 KB
PASSED: [[SimpleTest]]: [MySQL] 32,859 pass(es). View

New patch now that DrupalCacheArray is in (woot!) based off #23.

@Stefan Freudenberg we don't have code coverage on qa.drupal.org, there's http://drupal.org/project/code_coverage but that hasn't been run for a while afaik.

However system_add_module_assets() is run on every request from hook_init(), so it's executed a lot by the core simpletests, not sure if it has dedicated core test coverage though in terms of asserting the behaviour.

Stefan Freudenberg’s picture

Status: Needs review » Reviewed & tested by the community

I say it's good then.

catch’s picture

Status: Reviewed & tested by the community » Needs work

There's something wrong here, looks like I didn't make the changes to system_list() itself.

catch’s picture

Status: Needs work » Needs review
FileSize
5.47 KB
PASSED: [[SimpleTest]]: [MySQL] 32,901 pass(es). View

Re-rolled with the module.inc hunks this time.

bfroehle’s picture

Status: Needs review » Needs work
+++ b/includes/module.incundefined
@@ -112,9 +110,9 @@ function module_list($refresh = FALSE, $bootstrap_refresh = FALSE, $sort = FALSE
+ *   'bootstrap' and 'module_enabled', the array values equal the keys.
+ *   For For $type 'theme', the array values are objects representing the

Minor typo here (For For).

bfroehle’s picture

Status: Needs work » Needs review
FileSize
5.55 KB
PASSED: [[SimpleTest]]: [MySQL] 32,922 pass(es). View
1.73 KB

I cleaned up the For For typo and simplified the control flow of system_list() slightly. See the attached interdiff.

bfroehle’s picture

+++ b/modules/system/system.moduleundefined
@@ -2325,6 +2328,52 @@ function system_get_info($type, $name = NULL) {
+/**
+ * Extends DrupalCacheArray to lazy load .info properties for modules.

I think it'd be nice to have some more documentation here. In particular, reading just the docblock I'd have no idea what the array offsets into a ModuleInfo object would correspond to. At a minimum there should probably be a note saying that users should instead call system_get_module_info().

catch’s picture

FileSize
5.7 KB
PASSED: [[SimpleTest]]: [MySQL] 33,634 pass(es). View

Patch no longer applied due to cache API changes.

I've re-rolled and added that note, looks better?

catch’s picture

#31: system_get_module_info.patch queued for re-testing.

catch’s picture

FileSize
5.73 KB
PASSED: [[SimpleTest]]: [MySQL] 34,164 pass(es). View

Re-rolled for /core.

sun’s picture

+++ b/core/modules/system/system.module
@@ -2325,6 +2328,56 @@ function system_get_info($type, $name = NULL) {
+function system_get_module_info($property) {
+  static $info;
+  if (!isset($info)) {
+    $info = new ModuleInfo('system_info', 'cache');
+  }

Doesn't ModuleInfo or DrupalCacheArray cache the info statically already? I thought that was part of the primary purpose of DrupalCacheArray...?

29 days to next Drupal core point release.

catch’s picture

DrupalCacheArray doesn't static cache by itself, instead you static cache the class in a factory function (or in this case wrapper since it's not returning the instance of the class).

catch’s picture

#33: system_info.patch queued for re-testing.

swentel’s picture

Just did some (basic) memory profiling with devel at "admin/reports/dblog path" as user 1 on a Plain Drupal install with a couple of additional core modules enabled.

Before patch: Memory used at: devel_boot()=1 MB, devel_shutdown()=3.39 MB, PHP peak=3.75 MB.
After patch: Memory used at: devel_boot()=1 MB, devel_shutdown()=3.25 MB, PHP peak=3.5 MB.

So it saves memory, good! I'm not setting this RTBC, maybe some else could do a quick profile as well?

sun’s picture

FileSize
5.73 KB
PASSED: [[SimpleTest]]: [MySQL] 34,168 pass(es). View

Re-uploading #33, as the testbot got stuck on it.

sun’s picture

Issue tags: +API change
  1. I'm confused why this only targets modules and not any other $type like themes? I.e., why isn't it SystemInfo instead of ModuleInfo?
  2. I don't think the ModuleInfo and singleton should live in system.module - why aren't they in module.inc?
catch’s picture

Issue tags: -API change +API addition

@sun. Themes have had .info available for several releases, there is no need to change that in Drupal 7.

However modules had .info (and the rest of the contents of the system table) loaded into memory only after #328357: Allow module .info files to add CSS/JS which was a feature added to Drupal 7 way after code freeze that introduced this major performance regression.

This patch just attempts to fix that performance regression in Drupal 7 without breaking backwards compatibility.

The entire module system depends on system module, as does system_get_info(), that is not introduced here.

Niklas Fiekas’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/system.moduleundefined
@@ -2325,6 +2328,56 @@ function system_get_info($type, $name = NULL) {
+ * @see ModuleInfo()

If it's a class, should we have the ()? Also, there should be an empty line before the @see.

+++ b/core/modules/system/system.moduleundefined
@@ -2325,6 +2328,56 @@ function system_get_info($type, $name = NULL) {
+ * @see system_get_module_info()

There should be an empty line before this @see.

catch’s picture

Status: Needs work » Needs review
FileSize
5.74 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Added the blank line before the @see.

Status: Needs review » Needs work

The last submitted patch, system_info.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
5.91 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Patch needed updating for DrupalCacheArray moving to namespaces.

Status: Needs review » Needs work

The last submitted patch, system_info.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
5.91 KB
PASSED: [[SimpleTest]]: [MySQL] 36,854 pass(es). View
moshe weitzman’s picture

#46: system_info.patch queued for re-testing.

moshe weitzman’s picture

Status: Needs review » Needs work

Still green, but the new class needs PSR-0 shitification.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.68 KB
6.34 KB
PASSED: [[SimpleTest]]: [MySQL] 37,028 pass(es). View

Now PSR-ified.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Thanks Tim.

sun’s picture

#49: drupal-1061924-49.patch queued for re-testing.

catch’s picture

Assigned: catch » Unassigned

Unassigning me now this is RTBC since any of the three of us can commit it.

sun’s picture

Assigned: Unassigned » sun
FileSize
2.08 KB
7.17 KB
FAILED: [[SimpleTest]]: [MySQL] 37,294 pass(es), 0 fail(s), and 24 exception(s). View

Re-rolled for #1503224: Cleanup module_list()

...which allows to simplify the module_list() code, and also converted an instance in search.admin.inc.

sun’s picture

Assigned: sun » Unassigned
Status: Reviewed & tested by the community » Needs review

Status: Needs review » Needs work
Issue tags: -Performance, -Needs issue summary update, -memory, -API addition, -needs backport to D7

The last submitted patch, drupal8.system-info.53.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#53: drupal8.system-info.53.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Performance, +Needs issue summary update, +memory, +API addition, +needs backport to D7

The last submitted patch, drupal8.system-info.53.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
11.6 KB
PASSED: [[SimpleTest]]: [MySQL] 37,287 pass(es). View

Ah! These exceptions are fixed by #1503224-52: Cleanup module_list(). Here's a combined patch to prove it, once that's in #53 should pass just fine.

tim.plunkett’s picture

FileSize
11.57 KB
PASSED: [[SimpleTest]]: [MySQL] 37,102 pass(es). View
7.17 KB
PASSED: [[SimpleTest]]: [MySQL] 37,247 pass(es). View
tim.plunkett’s picture

#59: drupal-1061924-59.patch queued for re-testing.

sun’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +API clean-up

#1493098: Convert cron settings to configuration system landed, so this is ready to be committed again.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks for helping to push this one home. Committed/pushed to 8.x. Moving to 7.x for backport.

Berdir’s picture

Status: Patch (to be ported) » Needs review
FileSize
6.03 KB
PASSED: [[SimpleTest]]: [MySQL] 39,397 pass(es). View

Backport for 7.x based on #46, I believe everything that followed was 8.x specific, correct me if I'm wrong.

Seems to work, had to run update.php, drush seems to be broken, gives me a fatal "DrupalCacheArray not found", looks like bootstrap.inc isn't included or something like that?

Berdir’s picture

Applied the patch on a big site, memory usage of system_list() went down from 3,4MB to 600KB.

tim.plunkett’s picture

Cameron Tod’s picture

Cameron Tod’s picture

I had a quick debug of drush with this patch, and yes, as berdir suspects in #63 bootstrap.inc is not being included by _drush_bootstrap_drupal_root(). I will have another look later, as I think it would be great if we could get this in soon, without having to patch drush.

moshe weitzman’s picture

I'm happy to answer Drush questions. Not sure that the error is drush's fault though. See me in #drush on IRC.

Berdir’s picture

Actually, it's quite straight forward. I did add the .info.inc to files[] but I didn't actually move the class definition to that file :)

This should be good, works fine with drush, let's see what the tests have to say.

Berdir’s picture

FileSize
6.33 KB
PASSED: [[SimpleTest]]: [MySQL] 39,407 pass(es). View

Erm.

Cameron Tod’s picture

Status: Needs review » Needs work

With drush 5.7:

$ drush -vd pml
Bootstrap to phase 0. [0.02 sec, 4.58 MB]                            [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drush() [0.03 sec, 4.84 MB] [bootstrap]
Cache HIT cid: 5.7-commandfiles-0-72ee8fb941020eb637983d60ed1d6b95 [0.05 sec, 4.85 MB]                                                                                                                                                                            [debug]
Bootstrap to phase 0. [0.17 sec, 10.02 MB]                                                                                                                                                                                                                    [bootstrap]
Bootstrap to phase 6. [0.26 sec, 10.03 MB]                                                                                                                                                                                                                    [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drupal_root() [0.28 sec, 10.03 MB]                                                                                                                                                                                   [bootstrap]
Initialized Drupal 7.16-dev root directory at /Users/cam8001/Sites/drupal [0.31 sec, 13.72 MB]                                                                                                                                                                   [notice]
Drush bootstrap phase : _drush_bootstrap_drupal_site() [0.32 sec, 13.73 MB]                                                                                                                                                                                   [bootstrap]
Initialized Drupal site default at sites/default [0.32 sec, 13.73 MB]                                                                                                                                                                                            [notice]
Cache HIT cid: 5.7-commandfiles-2-c32acd775c18a90c5b41d0fc9accf5ab [0.33 sec, 13.74 MB]                                                                                                                                                                           [debug]
Drush bootstrap phase : _drush_bootstrap_drupal_configuration() [0.35 sec, 13.73 MB]                                                                                                                                                                          [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drupal_database() [0.36 sec, 13.75 MB]                                                                                                                                                                               [bootstrap]
Successfully connected to the Drupal database. [0.36 sec, 13.75 MB]                                                                                                                                                                                           [bootstrap]
Drush bootstrap phase : _drush_bootstrap_drupal_full() [0.38 sec, 14.61 MB]                                                                                                                                                                                   [bootstrap]
Drush command terminated abnormally due to an unrecoverable error.                                                                                                                                                                                            [error]
Error: Class 'SystemModuleInfo' not found in /Users/cam8001/Sites/drupal/modules/system/system.module, line 2360 [0.47 sec, 27.17 MB]

Fatal error: Class 'SystemModuleInfo' not found in /Users/cam8001/Sites/drupal/modules/system/system.module on line 2360

Call Stack:
    0.0020     700168   1. {main}() /Users/cam8001/Documents/sources/drupal/drush/drush.php:0
    0.0246    4795376   2. drush_main() /Users/cam8001/Documents/sources/drupal/drush/drush.php:14
    0.1850   10506456   3. _drush_bootstrap_and_dispatch() /Users/cam8001/Documents/sources/drupal/drush/drush.php:59
    0.2841   10511800   4. drush_bootstrap_to_phase() /Users/cam8001/Documents/sources/drupal/drush/drush.php:79
    0.3977   15319424   5. drush_bootstrap() /Users/cam8001/Documents/sources/drupal/drush/includes/bootstrap.inc:308
    0.4044   15320584   6. _drush_bootstrap_drupal_full() /Users/cam8001/Documents/sources/drupal/drush/includes/bootstrap.inc:185
    0.4045   15361656   7. drupal_bootstrap() /Users/cam8001/Documents/sources/drupal/drush/includes/bootstrap.inc:922
    0.4192   16976864   8. _drupal_bootstrap_full() /Users/cam8001/Sites/drupal/includes/bootstrap.inc:2182
    0.4893   28476656   9. module_invoke_all() /Users/cam8001/Sites/drupal/includes/common.inc:5100
    0.4895   28477984  10. call_user_func_array() /Users/cam8001/Sites/drupal/includes/module.inc:850
    0.4895   28478232  11. system_init() /Users/cam8001/Sites/drupal/includes/module.inc:0
    0.4907   28490384  12. system_add_module_assets() /Users/cam8001/Sites/drupal/modules/system/system.module:1918
    0.4907   28490464  13. system_get_module_info() /Users/cam8001/Sites/drupal/modules/system/system.module:1925


Variables in local scope (#13):
  $info = NULL
  $property = 'stylesheets'
Berdir’s picture

Did you run update.php/drush rr? Works fine for me after drush rr but I haven't tested if update.php works too.

Cameron Tod’s picture

So after a rebuild of the site, the .info.inc file is picked up ok. If I use an existing install, then apply the patch, Drupal can't boot from either the web interface or drush. If I rebuild the registry with rfay's script, everything comes together, the class file is included, and Drupal boots.

What's a better approach, manually including system.info.inc on demand, or moving SystemModuleInfo into, say, system.module?

edit: or implementing an update hook or something?

Berdir’s picture

It was in system.module, that's why it broke drush :) As I said, have you tried running update.php?

podarok’s picture

#70

+++ b/modules/system/system.info.incundefined
@@ -0,0 +1,34 @@
+}
\ No newline at end of file

+}
\ No newline at end of file

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
726 bytes
6.3 KB
PASSED: [[SimpleTest]]: [MySQL] 39,387 pass(es). View

Yep, works fine after running update.php. Is it ok to commit a patch that will kill sites until they run update? Is there a way we could fail more gracefully?

Here's a patch with a couple of small code style fixes.

Berdir’s picture

I'm not sure. Not sure if we've done that with the other classes that we added.

Also, note that the Return/Returns change is wrong in the 8.x patch that got commited too, so that will need to be forward-ported.

podarok’s picture

#76 looks good for me
lets wait for a testbot

bojanz’s picture

Yep, works fine after running update.php. Is it ok to commit a patch that will kill sites until they run update? Is there a way we could fail more gracefully?

You could never just replace the files and expect the site to work without running update.php. That was never claimed to be supported, no?

So the current behavior is something I'd expect.

geerlingguy’s picture

I've always expected update.php is expected immediately after Drupal point upgrades, and, while I don't have specific references, I know that other changes have disabled sites until update.php is run. That's why we have the instruction to put the site in maintenance mode while updating :)

geerlingguy’s picture

FileSize
6.3 KB
PASSED: [[SimpleTest]]: [MySQL] 41,127 pass(es). View

Here are some testing results on a MacBook Air with a hefty Drupal 7 site. I ran each test three times, averaging the results.

Before Patch

Cache clear: 38206ms, 188.5MB RAM
Cached page load: 315ms, 15.25MB RAM

After Patch

Cache clear: 35875ms, 186.5MB RAM
Cached page load: 308ms, 14.5MB RAM

Also, I found one more whitespace error in the patch in #76 that should be fixed in this patch.

pounard’s picture

Module info should never be accessed on normal pages, it should be used for administrative purpose only: if we consider that as true and discourage module info read at normal runtime, it should never end up cached and we shouldn't need the SystemModuleInfo class.

catch’s picture

@pounard: that used to be true until #328357: Allow module .info files to add CSS/JS landed (without any profiling). I'd love to see that patch reverted, but it definitely won't be reverted from Drupal 7 so we need to fix the performance issue there. There's no issue currently open to revert the .info css/js adding but if you open one I'll follow it. Note there's also #1029460: Find a better hook than hook_init() for drupal_add_js() and drupal_add_css() open which would at least mean this doesn't happen on non-themed requests - right now the logic runs for autocomplete callbacks and all kinds of other irrelevant things.

pounard’s picture

Ok I see. So we are fixing in a complex fashion this problem because another peice of code is doing it wrong, that's the kind of almost necessary patch I don't like to see happening.

catch’s picture

Yeah me neither, but unfortunately I only did serious profiling of Drupal 7 for memory/cache size in December 2011 2010 (and no-one else had previously, or if they did they kept it a secret), about a week before it was released, so it was too late to fight the original patch at that point.

sun’s picture

catch’s picture

blueminds’s picture

mdrummond’s picture

Issue summary: View changes

Working on issue summary template

mdrummond’s picture

Issue summary: View changes

Completed formatting of issue summary

Anybody’s picture

This issue seems to be sleeping for quite a while. So let me ask, what's the current status on this? Is there still someone working on it?

mgifford’s picture

The patch from #81 still applies. Seems to nicely break out the code. @geerlingguy's profiling from 2 years ago look like it will speed up page loads & reduce memory demands.

Why isn't there movement on bringing this into Core?

podarok’s picture

Status: Needs review » Reviewed & tested by the community

#81 speedup a lot massive cache rebuilds on large site.
Looks good for me.
Let's push it
+1 RTBC

podarok’s picture

Status: Reviewed & tested by the community » Needs work

I have tested it on another large site

after drush cc all

Error: Call to undefined function user_access() in
modules/system/system.module, line 1958

looks like this needs work

Berdir’s picture

Updating from this issue is a bit brittle. Did you try to run update.php first?

Status: Needs work » Needs review
mgifford’s picture

After looking at the code, I'm a bit baffled at how it would affect user_access(). If we can't replicate this, it's probably worth resetting to RTBC.

@podarok - how large a site? 1 million nodes? 200 modules?

How does it work in one site and not another? Could there be a module conflict with it?

swentel’s picture

Status: Needs review » Needs work

This patch has problems with rules, going to admin/config/workflow/rules errors in

FacesExtendableException: There is no method process for this instance of the class RulesAction. in FacesExtendable->__call() (line 133 of //sites/all/modules/contrib/rules/includes/faces.inc

Getting notices also like this at runtime

Notice: Trying to get property of non-object in EntityDefaultI18nStringController->hook_string_info() (line 118 of /sites/all/modules/contrib/entity/entity.i18n.inc)

  • catch committed 20ba61f on 8.3.x
    Issue #1061924 by catch, tim.plunkett, sun, Stefan Freudenberg, bfroehle...

  • catch committed 20ba61f on 8.3.x
    Issue #1061924 by catch, tim.plunkett, sun, Stefan Freudenberg, bfroehle...

  • catch committed 20ba61f on 8.4.x
    Issue #1061924 by catch, tim.plunkett, sun, Stefan Freudenberg, bfroehle...

  • catch committed 20ba61f on 8.4.x
    Issue #1061924 by catch, tim.plunkett, sun, Stefan Freudenberg, bfroehle...
Berdir’s picture

joseph.olstad’s picture

Issue tags: +Drupal 7.60 target

I just triggered all other php 5.x and 7.x versions to be tested.