Cache registry lookups

we only call registry_mark_code when code is found in the registry or already loaded, so i don't think we need to worry about filtering the file path cache.

ok, here's the patch, real simple now that #298600: DBTNG: make module_implements work regardless of bootstrap phase is in.

all tests pass.

CNR. i'm such a n00b at remembering to change status...

some small benchmarks, all with APC, all core modules enabled, 1 node with 1 comment.

ab -n 1000 -c 10 http://site/node/1

HEAD, not logged in:
Requests per second: 61.91 [#/sec] (mean)
Requests per second: 61.97 [#/sec] (mean)
Requests per second: 61.84 [#/sec] (mean)

HEAD + patch, not logged in:
Requests per second: 65.77 [#/sec] (mean)
Requests per second: 65.91 [#/sec] (mean)
Requests per second: 65.79 [#/sec] (mean)

ab -n 1000 -C my=cookie -c 10 http://site/node/1

HEAD, logged in:
Requests per second: 57.88 [#/sec] (mean)
Requests per second: 58.05 [#/sec] (mean)
Requests per second: 57.98 [#/sec] (mean)

HEAD + patch, logged in:
Requests per second: 61.48 [#/sec] (mean)
Requests per second: 61.42 [#/sec] (mean)
Requests per second: 61.64 [#/sec] (mean)

comments added, no code changes. that was a bit lazy of me.

@Damien: can you share your benchmark setups and results? good catch on the 'fixing the condition on cache_get', i added the array part of the declaration after the benchmarking, fixed in this patch.

i ran the benchmarks again, against this patch, and i see less queries, and faster results in line with earlier runs. is anyone else able to run some benchmarks? if this only improves drupal on my computer, we need to know :-)

@sun: we run a query that saves us more queries, so its a net gain. also, for a bigger site, where hitting the cache probably means hitting memcached/APC instead of the db, its an even bigger win.

@Dries: i think that D6 -> D7 performance comparisons involve a lot more than the registry. i'd hate to see 'D7 is slower, the registry is to blame' become an accepted truth, because its likely going to narrow the focus too much.

having said that, getting performance iprovements out of the registry will involve a lot of changes outside of the registry code.

for example, _drupal_bootstrap_full, which loads all enabled modules and a huge chunk of includes/*.inc code for every request. making the bootstrap a lot lighter is something we talked about during the development of the registry patch, but we haven't gotten to it yet.

also, the help system, which will load all enabled modules on just about every request, even if we remove module_load_all from the bootstrap. there's an issue for this #299050: Help System - Master Patch (which includes improvements above and beyond making the help system load less code).

@Dries: can we hold off on this? i'm experimenting with a different approach to the problem, will post later today.

this patch caches all registry lookups, not just misses, and removes the path-based caching. all tests pass.

my benchmarks showed it to be quicker, both for default modules enabled, and all modules enabled, for node/1 with one comment. i'll try to get kbahey to run his tests on it for confirmation.

this makes registry caching simpler, and moves us closer to true lazy loading without killing the db. the more i looked at how much code the path-based cache was loading, and how often the cache is destroyed and rewritten (eg, watch the cache get rewritten for the same path for logged in vs anon requests), the less happy i am with this approach.

Hem. So basically you load a big array of all lookup_key => files out there.
....
(1) there is a big memory cost involved (what is the size of this?), and the cost will increase with the number of modules: bad, that sort of O(module) overhead is what the registry is trying to avoid,

do you have any numbers? i gathered data on this. i enabled all modules, created a node of each built-in content type, added comments, created users, some new roles, edited permisssions, visited any link in the admin section that wouldn't rebuild the registry and a few other things i've forgotten.

that is, i went out of my way to make the cache as big as i could. the array reached 372 elements, a large number of which had a value of FALSE. how much memory does a single dimension 372 element array with many empty keys take up? on my desktop, ubuntu intrepid, running php cli, that php reports 50kilobytes.

print memory_get_usage() . "\n";
$cache = array();
for ($i = 0; $i < 372; $i++) {
    if ($i % 2) {
        $cache[$i . 'resource_name'] = FALSE;
    }
    else {
        $cache[$i . 'resource_name'] = 'modules/memory/memory.module';
    }
}
print memory_get_usage() . "\n";

keep in mind, this patch is eliminating the path cache, so the impact is less than that. i'm willing to listen to objections about this, but not ones that come with no evidence.

(2) you lost the concept of "type", so you are out of luck if, for example, a function and a class has the same name

ahem:

  $cache_key = $type{0} . $name;

I suggest we keep the concept of path-based caching, but make it progressive (as in your approach). We could also study how we could break that big array in two part to save space in the case a given file has several points of entry of a file changes: I'm thinking of a (lookup_key, type) => file_id first part and a file_id => filename second part.

Of course, all this will have to be benchmarked (including memory usage).

care to elaborate on these ideas more? i'm willing to completely ditch this implementation (just like i think we should be willing to ditch the path-based caching) if we can find a better way. right now, i think the path taken by the patch at #20 is the best way.

updated patch attached, changes constant names to address chx's comment in #25.

@Arancaytar: thanks for the memory data. i'll try to work up some more data on memory usage to make sure this patch doesn't make things worse memory-wise.

oh, and here's the patch.

@catch: thanks for benchmarks. that's about what i saw as well.

currently, the bulk of the members of the registry lookup cache are misses, and misses for resources that will never exist.

if we can chop some/most of those out 'upstream', then this patch gets even nicer.

first target, in module_rebuild_cache, this code is bloating the cache with bogus lookups:

  // Log the critical hooks implemented by this module.
  $bootstrap = 0;
  foreach (bootstrap_hooks() as $hook) {
    if (module_hook($file->name, $hook)) {
      $bootstrap = 1;
      break;
    }
  }

updated patch makes module_rebuild_cache only look for bootstrap hooks for installed modules.

saves *66* queries on a standard install when module_rebuild_cache is called! oh, and makes the registry cache smaller.

fixed a bug in the write to cache code - make sure we return, even if there's nothing new written to cache. one less query.

this time its sytem_modules that's looking for hook_help in uninstalled modules. fixed that, another boatload of useless queries gone, registry cache gets smaller again.

fresh HEAD, with devel and simpletest enabled, i get:
Executed 303 queries in 29.44 milliseconds. at admin/by-module
Executed 241 queries in 39.39 milliseconds. at admin

with this patch, with devel and simpletest enabled, i get:
Executed 128 queries in 20.12 milliseconds. at admin/by-module
Executed 145 queries in 25.48 milliseconds. at admin

some benchmarks, APC enabled, anonymous user, no page or block cache for all runs.

default modules, no content, front page:
HEAD
Requests per second: 114.93 [#/sec] (mean)
Requests per second: 114.62 [#/sec] (mean)
Requests per second: 114.22 [#/sec] (mean)
HEAD + patch
Requests per second: 125.47 [#/sec] (mean)
Requests per second: 125.57 [#/sec] (mean)
Requests per second: 125.13 [#/sec] (mean)

all core modules, no content, front page:
HEAD
Requests per second: 81.67 [#/sec] (mean)
Requests per second: 81.60 [#/sec] (mean)
Requests per second: 81.47 [#/sec] (mean)
HEAD + patch
Requests per second: 90.04 [#/sec] (mean)
Requests per second: 89.93 [#/sec] (mean)
Requests per second: 89.81 [#/sec] (mean)

all core modules, 50 nodes of each content type with 4 comments each via devel, /node:
HEAD
Requests per second: 44.55 [#/sec] (mean)
Requests per second: 44.64 [#/sec] (mean)
Requests per second: 44.87 [#/sec] (mean)
HEAD + patch
Requests per second: 48.19 [#/sec] (mean)
Requests per second: 48.12 [#/sec] (mean)
Requests per second: 48.17 [#/sec] (mean)

if anyone else can benchmark this to validate those results that would be great.

more better, more smaller registry cache if we make module_hook use module_implements instead of drupal_function_exists.

got to this via trying to figure out why system_get_module_admin_tasks was adding so many misses to the registry cache.

this makes some more of drupal's admin lighter.

Damien is right, patch at #35 is broken.

new patch leaves module_hook alone, and makes system_get_module_admin_tasks use module_implements directly.

same lightening of the registry cache, without the breakage. all tests pass.

i think there might be further opportunities to tune the registry cache, but in the interests of kittens, i think they are best left to follow up patches.

as it is, this patch makes drupal quicker, and run less db lookups, so i think its good to go.

its also an important step to making drupal a lazy loading system, which i intend to follow up on at #332003: cut the fat out of _drupal_bootstrap_full.

changing status.

thanks for the review chx.

=== NULL is just a little slower than isset but still (is_null is a lot slower).
...
$type{0} use [0], PHP guys tried to make people use {} for string indexes, it failed and it'ls deprecated since 5.1 and not supported in PHP6 at all.

fixed.

And then you have $lookup_cache = $name; instead of $lookup_cache = NULL;, whom are you trying to confuse?

reworked this to make it clearer. because it was clearly not obvious, the intent is to 'rewarm' the lookup cache with resources that we've cached previously that haven't changed during a rebuild.

the registry rebuild now takes care of keeping the lookup cache warm, and $lookup_cache is set to NULL in _registry_check_code.

also, added some wrapper functions to convert the lookup cache to a string, after some suggestions from chx in #drupal.

all tests pass, so posting now. will post benchmarks shortly.

argh, status update again.

re-ran benchmarks, results have the same trend as #34 - this patch is measurably quicker than HEAD. again, would love to see some other benchmarks to validate this.

@smk-ka: thanks for the review.

1. Further optimization of _registry_check_code(): $type is either a numeric constant value or it's a string. Strict (===) checks are expensive. By wrapping the two checks for the REGISTRY_* constants in an additional is_numeric() block, we can possibly speed up execution.

maybe you're looking at an older version of the patch? the patch at #40 doesn't use === anymore.

2. This looks like a c'n'p error: +function _registry_set_lookup_cache(array $lookup_cache) {

nope, that should always be an array, and i want to die screaming if its not. as an aside, i think we should do that in more places in drupal (use php 5's type hinting).

on the memory stuff, well, i'd prefer to err on the side of being overcautious about memory consumption. this patch is clearly quicker, and does have to work when we have a larger lookup cache. i think avoiding serialize is the way to go, so i'm setting this back to CNR. can you share your benchmarks or suggest other ways to illustrate that avoiding serialize is not necessary?

rerolled to take out some fuzz, and converted a couple more places where we aren't using the hook implementation code to cut down on useless registry queries.

registry cache, now with even less fat.

updated patch based on IRC review by webchick.

added more comments for + $cache_key = $type[0] . $name;, and more comments about the memory impact of serialize().

benchmarks anyone?

updated patch to keep up with HEAD. all tests that pass on HEAD pass with the patch.