Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
#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.
Comment | File | Size | Author |
---|---|---|---|
#28 | system_list.patch | 5.56 KB | Anonymous (not verified) |
#25 | system_list.patch | 5.34 KB | Anonymous (not verified) |
#22 | system_list.patch | 4.85 KB | Anonymous (not verified) |
#18 | screenshot_001.png | 47.9 KB | catch |
#18 | screenshot_002.png | 49.32 KB | catch |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedprobably slightly naive patch attached.
not sure if we want to use one cache rather than two, but lets start here.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedwow, 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.
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commenteddo 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.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedok, lets see what the test bots of this simple patch, just don't blow away the bootstrap module list in _drupal_bootstrap_page_header.
Comment #6
catchYeah 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.
Comment #7
Dries CreditAttribution: Dries commentedIt would be interesting to see some basic performance results.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentednot 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)
Comment #9
catchLooks about right for one query. Let's get this in, then back to active for actual system_list() caching for non-bootstrap stuff.
Comment #10
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks for the patch and the tests!
Comment #11
chx CreditAttribution: chx commentedThis did not add the caching the issue is about.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedwork 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.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedok, test bot likes that much, setting to needs work because thats its real status.
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedmore 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.
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedthanks 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 :-\
Comment #17
catchTook 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()
Comment #18
catchdevel screenshots.
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedyes, 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.
Comment #21
catchJust a shame fixing the installer completely pwned tests, I spent a little bit of time this morning debugging but didn't get very far.
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedlets 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.
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #25
Anonymous (not verified) CreditAttribution: Anonymous commentedmuch better, changed
drupal_static_reset('system_list')
tosystem_list_reset()
inlist_themes()
when$refresh = TRUE
, and the failing tests pass locally for me.lets see what the bot thinks.
Comment #26
catchComment #27
chx CreditAttribution: chx commentedThe // 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.
Comment #28
Anonymous (not verified) CreditAttribution: Anonymous commentedcomments-only update of the patch based on chx's comment and discussion with chx and catch in #drupal.
Comment #29
chx CreditAttribution: chx commentedLovely.
Comment #30
Dries CreditAttribution: Dries commentedLooks good to me. Committed to CVS HEAD.