#623992: Reduce {system} database hits just got committed. Since there are limited places where those lists get rebuilt, we could also consider a cache_get() / cache_set() for it too. This would be a no-op when using database caching, but allow sites using alternate caching backends with Drupal's page cache to skip a database query on cached pages. I'm not sure how many we do for cached pages, but one less is always good.

CommentFileSizeAuthor
#28 system_list.patch5.56 KBAnonymous (not verified)
#25 system_list.patch5.34 KBAnonymous (not verified)
#22 system_list.patch4.85 KBAnonymous (not verified)
#18 screenshot_001.png47.9 KBcatch
#18 screenshot_002.png49.32 KBcatch
#17 bootstrap.patch4.24 KBcatch
#14 system_list.patch5.69 KBAnonymous (not verified)
#12 system_list.patch3.97 KBAnonymous (not verified)
#5 no.killing.bootstrap.module.cache_.patch958 bytesAnonymous (not verified)
#1 system_list.cache_.patch3.76 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Assigned: Unassigned »
Status: Active » Needs review
FileSize
3.76 KB

probably slightly naive patch attached.

not sure if we want to use one cache rather than two, but lets start here.

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

wow, the lifecycle of $bootstrap_list and $lists inside system_list is, umm, not how it feels it should be.

hitting node/1, it goes something like:

1. drupal_bootstrap --> _drupal_bootstrap_page_cache --> drupal_bootstrap --> _drupal_bootstrap_variables --> module_load_all --> module_list -> drupal_static_reset('system_list')

2. drupal_bootstrap --> _drupal_bootstrap_page_cache --> drupal_bootstrap --> _drupal_bootstrap_variables --> module_load_all --> module_list --> system_list('bootstrap') --> fetches from cache, sets $lists['bootstrap'] --> returns it

3. drupal_bootstrap --> _drupal_bootstrap_page_header --> bootstrap_invoke_all --> module_list -> drupal_static_reset('system_list') --> who needed that bootstrap list anyway?

4. drupal_bootstrap --> _drupal_bootstrap_page_header --> bootstrap_invoke_all --> module_list --> system_list('bootstrap') --> fetches from cache, sets $lists['bootstrap'] --> woah, deja vu, returns it

5. drupal_bootstrap --> _drupal_bootstrap_full --> module_load_all --> module_list --> drupal_static_reset('system_list') --> who needed that bootstrap list anyway?

seems to behave as it should after that, the full list is built, on the next hit, and subsequent hits just return built bits without hitting the db.

Anonymous’s picture

do we need to worry about $list['bootstrap'] changing between _drupal_bootstrap_page_cache() and _drupal_bootstrap_page_header()? if not, then there's a db cache call (in the common case) we can get rid of we don't blow away the bootstrap cache in between those stages.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
958 bytes

ok, lets see what the test bots of this simple patch, just don't blow away the bootstrap module list in _drupal_bootstrap_page_header.

catch’s picture

Yeah module_list() is so, so horrible :( I moved everything to a separate function to try to avoid a refactoring, but still can't escape it. That change looks fine to me and no fails at all.

On the overall caching - I think we unfortunately need two caches, one for bootstrap, one for everything else - because we only have c.5ms for cached page requests, and it's possible that loading four big arrays from cache and unserializing them at once takes a ms or two. See also #626690: Use system_list() for drupal_get_filename() which would add an extra array here.

Dries’s picture

It would be interesting to see some basic performance results.

Anonymous’s picture

not much of a gain, but its something.

ab -n200 -c2, APC on, hitting the front page, no nodes.

HEAD:
Requests per second: 10.61 [#/sec] (mean)
Requests per second: 10.71 [#/sec] (mean)
Requests per second: 10.63 [#/sec] (mean)
Requests per second: 10.63 [#/sec] (mean)

Patch:
Requests per second: 10.84 [#/sec] (mean)
Requests per second: 10.82 [#/sec] (mean)
Requests per second: 10.81 [#/sec] (mean)
Requests per second: 10.84 [#/sec] (mean)

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks about right for one query. Let's get this in, then back to active for actual system_list() caching for non-bootstrap stuff.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks for the patch and the tests!

chx’s picture

Status: Fixed » Active

This did not add the caching the issue is about.

Anonymous’s picture

Status: Active » Needs review
FileSize
3.97 KB

work in progress, haven't integrated it with drupal_get_filename enough to get rid of the query there yet, so no need for careful review yet.

lets see what the bot thinks.

Anonymous’s picture

Status: Needs review » Needs work

ok, test bot likes that much, setting to needs work because thats its real status.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
5.69 KB

more work in progress, definitely not working completely right, but i can't find tests that are broken, so lets get the bot to do it.

starting to make drupal_get_filename depend on system_list, stopping the 'prime drupal_get_filename' stuff.

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

thanks test bot. not good news though. i think BootstrapGetFilenameTestCase() is failing because its out of date: #642468: BootstrapGetFilenameTestCase is out of date.

assuming that is fixed, everything else passes, but when i check a fresh install with this patch, its wonky - admin theme is garland, and there are no blocks enabled at all. so, the patch passes all tests but is broken :-\

catch’s picture

Status: Needs work » Needs review
FileSize
4.24 KB

Took a look at this. Reverted the changes to drupal_get_filename() 'cos the database down / maintenance stuff in there gives me the willies, and instead went back to priming the static cache as we've had before. The difference here is that we cache an array of enabled modules and themes with their filenames, then loop over it whether loading from cache or not.

This fixes the installer, and I tested blocks / admin theme and had a bit of a click around, will let the test bot do the heavy work.

This should have two performance/scaling implications:

1. The system_list() query was taking anywhere from 1.5-2ms (looking in devel). Since we cache a lot less data than we get from the uncached query, subsequent database cache lookups are approximately 0.6ms on my localhost. Meaning at the very, very worst this is a no-op if you're not running memcache.

2. You can now bootstrap all the way to DRUPAL_BOOTSTRAP_FULL if you're using a non-database caching backend without hitting the database - the first direct query when viewing a page is from menu_execute_active_handler() / menu_get_item()

catch’s picture

FileSize
49.32 KB
47.9 KB

devel screenshots.

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

yes, i'd support that. drupal_get_filename() is just scary, and i think we get all the benefits anyway. i guess we'll have to leave to leave it as a steaming pile of .... along with module_list etc until D8.

catch’s picture

Just a shame fixing the installer completely pwned tests, I spent a little bit of time this morning debugging but didn't get very far.

Anonymous’s picture

FileSize
4.85 KB

lets see what the bot thinks of this. i've got tests running again and passing, but i haven't run the whole suite.

adds a system_list_reset() to _drupal_install_module(), which is a bit 'oh, that feels wrong but works', but anyway.

Anonymous’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

FileSize
5.34 KB

much better, changed drupal_static_reset('system_list') to system_list_reset() in list_themes() when $refresh = TRUE, and the failing tests pass locally for me.

lets see what the bot thinks.

catch’s picture

Status: Needs work » Needs review
chx’s picture

Status: Needs review » Needs work

The // On every request, not just cache misses, prime the drupal_get_filename() static cache. comment is weird. Let's work together on a better one.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
5.56 KB

comments-only update of the patch based on chx's comment and discussion with chx and catch in #drupal.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Lovely.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Looks good to me. Committed to CVS HEAD.

Status: Fixed » Closed (fixed)
Issue tags: -Performance

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