Make system_list() return full module records

The last submitted patch, drupal.system-list-get-info.0.patch, failed testing.

Odd. The enabled module list can be empty? Very odd. Anyway, let's just cope with that.

I've been looking at this patch a bunch today (actually the first patch - I was trying to figure out how $list could every be empty also but I got nuthin) and everything looks good ... I'd give it a RTBC, but we should probably get another look or two

Are you going to do #3 (from the original post) in a followup?

I like this overall.

1. I think this patch changes the behavior of system_get_info('theme') from returning only enabled themes to returning all themes? Not sure what to do about that.
2. I am not a fan of adding the static cache here. As far as I can tell, the system_list() static cache is a bit of a mess and needs to be cleared lots of places, and this one would too...

   Any reason we can't just do the unserialize() inside system_list(), right when the data is loaded?

3. +        $list = (!empty($list) ? array_combine($list, $list) : array());

   Minor - don't think we should bother with the outer parentheses there.

4. + * @param $name
+ *   (optional) The name of a $type to return. If omitted, all records are
+ *   returned.

   I had to read this a couple times (and then confirm by looking at the code) before I got it :) How about something like "The name of a module or theme whose information will be returned. If omitted, all records for the provided $type will be returned."

Hm. This whole "module_enabled" thing does not make sense.

Let's simplify:

- system_list() retrieves all of $type + statically caches that.

- module_list() only takes over enabled items.

- system_get_info() uses system_list(), and is able to return $only_enabled.

=> Zero API change. Well, almost - system_list()'s "module_enabled" changes into "module".

1. -        if ($record->type == 'module' && $record->status) {
-          // Build a list of all enabled modules.
-          $lists['module_enabled'][$record->name] = $record->name;
+        // Build a list of modules.
+        if ($record->type == 'module') {
+          $lists['module'][$record->name] = $record;

   This makes a lot of sense in terms of consistency, but the side effect is that we are now storing (in the cache) all data for all modules in the filesystem, including e.g. the 40-50 hidden modules used by Simpletest. Because it's cached, that's not too bad, but it's still nonzero work to cache_get() a very large record on each page request, plus store the resultant giant array in the static cache where it takes up memory. Is it definitely worth it?

2. The problem is that normal requests do not need to unserialize that data at all. Thus, the unserialize() would be needless for most requests. However, I'd also be happy to simply remove the static cache. Unserializing this data more than once might be needless, but then again, it's not the end of the world, performance-wise.

   If the unserialize() moved to system_list(), it would not happen on every request, only very rare ones - since system_list() uses a database cache. Whereas right now, for themes it is kind of silly that system_list() caches the info in a "still-serialized" state, but then http://api.drupal.org/api/function/list_themes/7 unserializes that on every request. Your patch adds more of the same to system_get_info(), so indeed not a performance disaster, just perpetuating some odd behavior in a new place, that would be lovely to fix instead :)

3. -function system_get_info($type) {
+function system_get_info($type, $only_enabled = TRUE, $name = NULL) {

   Maybe the order of the last two parameters should be switched? (I would think getting info about a particular module would be a lot more common than wanting to set $only_enabled to FALSE.)

4.   * This function returns the information from the {system} table corresponding
  * to the cached contents of the .info file for each active module or theme.

   With this patch, the "active" in that docblock is no longer quite correct.

Last time someone tried to make system_list() more generic, I won't name names but they're the author of this issue, page caching took a more than 50% performance hit. I can't find that issue but this is more or less exactly the same thing.

The $name parameter confused me. The name module_list() really suggests that we return more than one record.

Do we really need it? It feels like it is cramped into the API, unless I misunderstood what $name does.

Why wouldn't you return everything, and then use the big array to get the details for one record?

Not sure why this was demoted. We still read issues before changing anything, right?

The idea is that, when doing a full bootstrap, functions like being introduced in #769226: Optimize JS/CSS aggregation for front-end performance and DX will need access to full module records anyway. So the point of not querying and caching it because we don't need it, is bogus.

What's debatable and might be removed is the caching of all modules (instead of just enabled).

The idea is that this information is useful for more than CSS/JS. I know of at least two contrib modules that have to retrieve and eventually cache this information manually/separately.

I think I was guilty of using imprecise language in some of my reviews above. To clarify, this patch shouldn't have any effect whatsoever on cached (i.e., anonymous) page requests.

It may have an effect on uncached (i.e., authenticated) pages due to the size of the system_list() cache, as discussed by @effulgentsia and I above. To deal with that, we can limit this to enabled modules only (I think the use case for querying info about disabled modules is relatively small). Frankly, if we do that and then couple this with removing the silly unserialize() call from inside list_themes() we may wind up with a performance improvement: We'd effectively go from "double-unserializing" (once in the cache_get, once outside) all enabled+disabled theme data on every authenticated page view, which is just silly, to "single-unseralizing" both the (enabled) module and (enabled+disabled) theme data. Themes tend to have much bigger info files than modules anyway so who knows how it would all add up. It could be benchmarked.

[updated: these benchmarks may not be accurate - see later comment]

ab shows the patch in #22 reduces performance by 90%.

Without patch: typical 30 req/sec
With patch: typical 3 req/sec

I re-ran these tests a number of times and saw similar results each time.

Without patch

With patch

+        $list = (!empty($list) ? array_combine($list, $list) : array());

use drupal_map_assoc().

@manarth - do you believe your own benchmark? For starters, there should be no change for anonymous cached pages. Are you passing a cookie in order to be logged in? If not, is page caching enabled?

IMO this is critical since it blocks the css/js preprocess issue but not getting fussy over that.

Benchmarks with patch state less than half of the delivered HTML size, so you were likely benchmarking a PHP fatal error or something. Additionally comparing total size to HTML size, it looks like page caching was enabled.

This issue is only major, but there is a critical issue (#328357: Allow module .info files to add CSS/JS) we're working on that allows modules to add css/js in their info files.

If we want that work do be done here instead, doesn't that make this issue critical?

#22 works beautifully.

Of course, you need to flush all caches, so module and theme info is rebuilt. Without doing so, pages contained lots of PHP warnings and notices.

Unrelated to this patch: After flushing all caches, all blocks in Bartik were disabled. Most likely, because Bartik defines custom regions, and no theme info was available during... although, hm, the proper theme info should have been available again, before blocks were rehashed. Anyway, separate issue.

Attempting to Benchmark the patch at #22
Drush Devel generated content.
Without Patch

Applied Patch.
Used Drush CC all to clear caches.
Interestingly, got this warning [multiple times] on first run of drush cc all.

With Patch
Ran ab

This seems to return a performance improvement, so something somewhere might not be right [the patch, my system or my benchmarking].

• Core install with devel + devel-generate enabled
• devel-generate 50 nodes with comments.
• Benchmarked the homepage (which is showing /node)
• Using anonymous-user to benchmark
• No page-cacheing

Here are the results:

• n=250, c=10, t=5:
  • Unpatched: 6.49 req/sec
  • Patched: 8.76 req/sec
• n=1000, c=15, t=5:
  • Unpatched: 7.89 req/sec
  • Patched: 8.84 req/sec
• n=1000, c=12, t=5:
  • Unpatched: 8.78 req/sec
  • Patched: 9.35 req/sec
• Longer running - n=250, c=5, t=60:
  • Unpatched: 10.29 req/sec
  • Patched: 11.24 req/sec

I think it's reasonable to conclude that the patch improves performance.

250 requests, 10 concurrent users (timeout after 5secs)

Without patch

With patch

1000 requests, 15 concurrent users (timeout after 5secs)

Without patch

With patch

1000 requests, 12 concurrent users (timeout after 5secs)

I re-tested with fewer concurrent users, because ab reported unreliable tests for c=15:

Without patch

With patch

Longer running:

Without patch

With patch

NB: the Document lengths is larger for the patched version than the unpatched version by 182 bytes, because the URLs are different on my setup (foo.core vs foo.corepatched). The difference is less than 1% of the total document length and can safely be ignored.

Correct function argument would've helped... system_list('module_enabled');

100k iterations this time.

HEAD: 22.606045007706 seconds

Patch: system_list: 22.733495950699 seconds

Looks like no measurable difference, which is great. Not sure how manarth got consistently faster results though. Did the same test with 'bootstrap' instead of 'module_enabled' and no change there either.

XHprof is happy to report improvements with the patch:

I also just did a quick micro-benchmark of list_themes() - since this patch affected that too. I used @catch's code but with:

With 1000 iterations I got this:

I expected some performance improvement due to removing the unserialize(), but not sure I expected that much, so maybe I did something wrong...

In any case, it sounds like overall the conclusion from everyone is that this patch doesn't hurt performance and may actually help it :)

it's pretty much impossible that list_themes() was taking 20ms, I'd run it once with a timer to compare before and after the patch to see what might be happening.

I'd be really happy if I could see the xhprof report just for system_list() instead of the whole request, but this seems fine to me, thanks to people for actually doing the benchmarks.

Went ahead with option b), replacing 'bootstrap' with 'system_list' index.

Committed to CVS HEAD. Hopefully we can make progress with that critical now. Thanks!