Reduce {system} database hits

I've been planning to open a separate issue for a 'cache_local' bin - although cache_bootstrap is a better name.

Potential candidates include this one, variables, locale, module_implements - anything needed on every request.

Not sure about 1s expiration though - doesn't that mean a write per-second when using default database storage, that's an unacceptable hit.

I was thinking about putting bootstrap into a variable once this was running, hadn't thought about all modules though, that's a nice idea.

Before seeing moshe's post I had this idea:

Do one query of the system table.

SELECT name, filename, type FROM system WHERE status = 1 ORDER BY weight ASC, filename ASC

Take that array and do a single foreach, which generates the following four arrays - to be used for the following purposes:

1. Bootstrap modules
2. Enabled modules
3. http://api.drupal.org/api/function/list_themes/7
4. http://api.drupal.org/api/function/drupal_get_filename/7

Then have module_list(), list_themes() and drupal_get_filename() all use that one function to get what they need.

That would take us from four system table queries per page down to one (I nearly always see one from drupal_get_filename(), the rest are required on any normal request). Once thats done, we can then look at cache_get()/cache_set() for that array or using it to populate a variable.

Here's a start on #4, it should allow us to do #3 with a bit of tweaking too if we want.

What it does:

# Centralizes all system module queries called on a single page into one function - this is currently module_list() * 2, and list_themes()
# Moves the static priming of module_list()/drupal_get_filename() to that central function to save a second foreach (and because that doesn't make any more sense to have in module_list()) - this is done for themes as well as modules now, not sure if that improves anything yet, but neither can it do any harm.

This should cut out two queries for every request in HEAD. While doing this, I realised a variable for modules would also require us to store their filenames somewhere due to the drupal_get_filename() issue.

Nice, wasn't sure if it would, but this also kills the one stray query from drupal_get_filename() we do every page in HEAD too, so that's 4 queries down to 1 for this group of functions on every request, without adding a new cache apart from the drupal_static().

Applied #607244: Decrease performance impact of contextual links and created a test admin user so I wasn't getting contextual links.

Empty front page, default profile:

HEAD:
Executed 40 queries in 23.09 milliseconds.

Patch:
Executed 37 queries in 21.24 milliseconds.

(query time not at all consistent between page views, just pasted for reference).

While here, I also noticed that 16 of those queries are from cache_get() - so actual required database hits to serve an uncached page is ~20 with this patch applied.

Then I benchmarked the same page as an anonymous user, with page caching disabled.

HEAD:

Concurrency Level:      1
Time taken for tests:   88.515 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      6147000 bytes
HTML transferred:       5660000 bytes
Requests per second:    11.30 [#/sec] (mean)
Time per request:       88.515 [ms] (mean)
Time per request:       88.515 [ms] (mean, across all concurrent requests)
Transfer rate:          67.82 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    79   88   6.8     87     144
Waiting:       79   88   6.7     87     144
Total:         79   88   6.8     87     144

Patch:

Concurrency Level:      1
Time taken for tests:   84.632 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      6147000 bytes
HTML transferred:       5660000 bytes
Requests per second:    11.82 [#/sec] (mean)
Time per request:       84.632 [ms] (mean)
Time per request:       84.632 [ms] (mean, across all concurrent requests)
Transfer rate:          70.93 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    77   85   7.1     82     140
Waiting:       77   84   7.0     82     140
Total:         77   85   7.1     82     140

The drupal_get_filename() and module_list() come out as 0.5-1.5ms each on my system (although there's also some PDO overhead executing queries), so that's as expected.

This one fixes the installer for me.

Renamed to system_list(), added required argument since I can't see any case where you'd indiscriminately want the three arrays - and we have other API functions for getting the full contents of this table, which this doesn't and can't replace. Dropped the default from drupal_static() and changed to isset().

Also ropped two no longer pertinent indexes from {system}, added a new one so that the new query doesn't cause a filesort.

Query with this patch:

 EXPLAIN SELECT * FROM system WHERE status = 1 ORDER BY weight ASC, name ASC;
+----+-------------+--------+------+---------------+-------------+---------+-------+------+-------------+
| id | select_type | table  | type | possible_keys | key         | key_len | ref   | rows | Extra       |
+----+-------------+--------+------+---------------+-------------+---------+-------+------+-------------+
|  1 | SIMPLE      | system | ref  | system_list   | system_list | 4       | const |   30 | Using where | 
+----+-------------+--------+------+---------------+-------------+---------+-------+------+-------------+

Both the bootstrap and modules queries in head also come out at const/ "Using where".

tha_sun noticed a phpdoc typo.

sun asked in irc about the index- whether we additionally need to include 'type'.

I said no, then double checked, then had to ask David Strauss to understand why it's a no.

Here's an EXPLAIN with the index in the current patch:

mysql> EXPLAIN SELECT * FROM system WHERE status = 1 AND type IN ('module', 'theme') ORDER BY weight ASC, name ASC;
+----+-------------+--------+------+-----------------------+-------------+---------+-------+------+-------------+
| id | select_type | table  | type | possible_keys         | key         | key_len | ref   | rows | Extra       |
+----+-------------+--------+------+-----------------------+-------------+---------+-------+------+-------------+
|  1 | SIMPLE      | system | ref  | system_list,type_name | system_list | 4       | const |   31 | Using where | 
+----+-------------+--------+------+-----------------------+-------------+---------+-------+------+-------------+
1 row in set (0.00 sec)

And one with the index on type:

mysql> EXPLAIN SELECT * FROM system WHERE status = 1 AND type IN ('module', 'theme') ORDER BY weight ASC, name ASC;
+----+-------------+--------+------+-------------------------------------+---------------+---------+-------+------+-----------------------------+
| id | select_type | table  | type | possible_keys                       | key           | key_len | ref   | rows | Extra                       |
+----+-------------+--------+------+-------------------------------------+---------------+---------+-------+------+-----------------------------+
|  1 | SIMPLE      | system | ref  | system_list,type_name,system_list_2 | system_list_2 | 4       | const |   24 | Using where; Using filesort | 
+----+-------------+--------+------+-------------------------------------+---------------+---------+-------+------+-----------------------------+
1 row in set (0.00 sec)

And here's a pasted irc conversation from #drupal with David Strauss, apologies for code tags but that's the easiest way not to have handles eaten by the HTML filter:

<catch> davidstrauss: two explains, same query - http://drupalbin.com/12287?nocache=1
<catch> davidstrauss: one has no filesort but more rows, the other a filesort but less rows.
<catch> The one with the filesort has IMO a better index.
<catch> CREATE INDEX system_list_2 ON system (status, type, weight, name);
<catch> vs. CREATE INDEX system_list ON system (status, weight, name);
<davidstrauss> catch: It doesn't really matter because it will end up in the query cache anyway
<catch> davidstrauss: and neither is particularly worse than the other right?
<davidstrauss> catch: pretty much, although system_list_2 is needlessly long for this query
<davidstrauss> catch: mysql is only using status and type
<catch> davidstrauss: that explains the filesort then. Is this one of those where what the optimizer does depends on the size of the table?
<davidstrauss> catch: the second explain is running two queries (one for each type), combining the results, and then sorting
<davidstrauss> catch: the first is getting a sorted list of all enabled items and then walking through it, picking out ones marked as theme or module
<davidstrauss> catch: are there non-theme, non-module items in the system table?
<catch> davidstrauss: profiles and theme engines.
<catch> I think.
<davidstrauss> catch: ah, so that must be the difference between 24 and 31
<davidstrauss> catch: are there 7 enabled, non-theme, non-module items in your system table
<catch> davidstrauss: no...
<davidstrauss> catch: what do you have?
<catch> davidstrauss: 86 records, 31 enabled.
<davidstrauss> catch: yeah, but i asked how many of the enabled ones are neither themes nor modules
<catch> davidstrauss: zero.
<davidstrauss> catch: so why do you only have 24 rows in the second query?
<catch> davidstrauss: trying to figure that out now.
<catch> davidstrauss: the query returns 31 rows, EXPLAIN only mentions 24.
<davidstrauss> catch: i wonder if that's statistics, then
<davidstrauss> catch: run optimize on your tables
<davidstrauss> catch: and then explain again
<davidstrauss> catch: the rows in mysql are often an estimate
<catch> OK now, with system_list_2, 23 rows.
<davidstrauss> well, statistics aren't perfect
<davidstrauss> they're not intended to be
<catch> without, 31.
<davidstrauss> catch: the one with 31 is probably not estimating
<davidstrauss> catch: it's probably counting
<davidstrauss> catch: i recommend excluding the type from indexes, then
<davidstrauss> catch: mysql *thinks* that using the type-based index will reduce its working set enough to justify the filesort
<davidstrauss> catch: i disagree. empirically in your case, the query without the type-based index will be faster
<davidstrauss> catch: but we could really improve this table with some refactoring

I copy and pasted from D6, but the queries are identical in HEAD apart from the throttle column.

I'm not really expecting anyone to use system_lists() - it should really only be used via module_list() or list_themes() anyway. If anyone has a better idea for a name (I originally had drupal_build_system_lists() but that's no less abstract), would be great.

Ahh. Re-rolled with sun's suggestion.

I was under the apparently mistaken impression that theme engines and etc. appear in the system table, but it looks like not, so removed.

+      if (file_exists($record->filename)) {
+        drupal_get_filename($record->type, $record->name, $record->filename);
+      }

This was simply copied from module_list() but I think we can get away without it, so removed that too.

By the way, drupal_get_filename() could take advantage of the shared list too.

It could, and I considered adding that in, but with the optimization here, we don't cause any queries with drupal_get_filename() on most requests anyway, I'd like to look at converting that in a followup maybe, but it's not necessary here yet, and adds additional complexity to the patch.

The drupal_static_reset() should be the job of module_list(TRUE).

It should, but it can't - because the static is reset in both calls to module_load_all() - which would cause it to rebuild the list from the {system} table twice each request, which is precisely what this patch tries to avoid. See #222109: module_list() should be rewritten as 2 functions: module_list() and module_list_bootstrap() and inline comments in drupal_get_schema() as well. I don't want to litter core with multi-line inline comments everywhere we reset this static - there's not one place to do that, so have not added additional documentation, since there's already an outstanding issue, but we could do that if we wanted.

I've opened #626688: Add caching for system_list() and #626690: Use system_list() for drupal_get_filename() for the caching and drupal_get_filename() followups.

Last one looks good.

Also, sorry for breaking HEAD.

Sorry to re-open yet again but this is ridiculous. While I can see the utility in a generic function, making it as generic as this caused a 50% slowdown with normal page caching. At least 25% of that overall cost was in the change to loading all records from the system table as opposed to just enabled - that took the query from 1.4ms to over 2ms according to devel (100 reqs/second is 5ms per request) - there are much fewer disabled themes than modules. I missed the list_themes() requirement in the initial patch that went in, but fixes overcompensated for that whereas we should've taken a different direction altogether when that was realised. HEAD is working now so let's get this fixed up for the last time.

Attached patch doesn't fix everything here, hence CNW, but demonstrates the problem:

ab -c1 -n1000 http://d7.7/
HEAD:

Document Path:          /
Document Length:        5662 bytes
Concurrency Level:      1
Time taken for tests:   21.763 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      6145000 bytes
HTML transferred:       5662000 bytes
Requests per second:    45.95 [#/sec] (mean)
Time per request:       21.763 [ms] (mean)
Time per request:       21.763 [ms] (mean, across all concurrent requests)
Transfer rate:          275.75 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    14   22   3.8     23      50
Waiting:        0   21   3.9     23      50
Total:         14   22   3.8     23      50

Patch:

Document Path:          /
Document Length:        5662 bytes

Concurrency Level:      1
Time taken for tests:   12.271 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      6145000 bytes
HTML transferred:       5662000 bytes
Requests per second:    81.49 [#/sec] (mean)
Time per request:       12.271 [ms] (mean)
Time per request:       12.271 [ms] (mean, across all concurrent requests)
Transfer rate:          489.04 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     8   12   2.5     12      47
Waiting:        8   12   2.5     12      46
Total:          8   12   2.5     13      47

What I suggest we do is:

1. Special case the bootstrap list when serving a cached page which is what I've done here - we need a solid check for "I'm going to serve the page from cache now" - I thought this was drupal_page_get_cache(TRUE) but that takes #/sec down from 80 to 60 so obviously isn't doing what we want here. For now drupal_page_is_cacheable() is good enough.

2. Cache the bootstrap modules separately once #627338: Add a cache_bootstrap bin is in - so cache_get() only has to fetch and unserialize a very small array instead of potentially four arrays 50-100 items long.

3. Don't ever keep an array of all disabled modules in a static variable that's built on every request. We have http://api.drupal.org/api/function/system_get_info/7 for that sort of thing, I have no idea why that was added here.

4. Consider using a separate query for list_themes() to avoid query bloat on uncached pages (so that we don't load the entire contents of the system table into memory then chuck 50% of it away). While that gives us two queries on uncached pages again (still down from four but not ideal), we can then cache that resultant list for uncached pages too, in cache_bootstrap(), so that high performance sites are able to skip it entirely as well.

That ought to give us the quickest possible response for both cached and uncached pages, without one impacting the other too much. The drupal_page_is_cacheable() check will fail if a page is cacheable, but not served from the cache (basically cache misses), but it'd still be optimized compared to HEAD a week or so ago, so of all the places to let a query split through that's probably the least worst - and it'll be cache_get() by the time we're done here anyway.

Alright with this version of the patch:

normal cache 98 #/sec
HEAD: 48 #/sec
D6: 91 #/sec
So actually faster than D6 out of the box and twice as fast as current HEAD.

We have two queries if you serve a cached page - because we don't seem to have any way at this point to know if we're serving from cache or not, so that gets duplicated, but still an improvement for logged-in users there too.

The patch is also almost 10% faster without page caching - as far as I can see due to removing disabled modules from the equation.

Patch:

Concurrency Level:      1
Time taken for tests:   83.710 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      6149000 bytes
HTML transferred:       5662000 bytes
Requests per second:    11.95 [#/sec] (mean)
Time per request:       83.710 [ms] (mean)
Time per request:       83.710 [ms] (mean, across all concurrent requests)
Transfer rate:          71.73 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    75   84   9.9     80     198
Waiting:       75   83   9.9     80     194
Total:         75   84   9.9     80     198

HEAD:

Concurrency Level:      1
Time taken for tests:   91.427 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      6149000 bytes
HTML transferred:       5662000 bytes
Requests per second:    10.94 [#/sec] (mean)
Time per request:       91.427 [ms] (mean)
Time per request:       91.427 [ms] (mean, across all concurrent requests)
Transfer rate:          65.68 [Kbytes/sec] received

Additionally, I profiled a cached page view, and this is the only remaining query required to serve a page with the normal page cache (assuming you disable IP blocking in $conf) - so sites running non database caching backends will now be able to serve pages without touching the database.

Now using the cache_bootstrap bin.