Optimize the field cache

I wanted the results indexed by cid though, although a typo in the foreach didn't make it clear why that was useful. I'm not sure how expensive fetchAllAssoc is though - if it's doing a lot of work, then we should probably key the results in that same foreach loop.

New patch, changes the return to return array('cached' => $cache, 'misses' => $cids);

So in the node multiple load implementation it'd look something like this:

$result = cache_get_multiple($nids, 'cache_node');
$nodes += $result['cached'];
$nids = $result['misses'];
// grab what's left of $nids with db_select()

This would save doing array_diff_key(array_flip($cids), $cache); whenever you care about the stuff that wasn't fetched from cache, which is presumably all the time.

Removed the fetchAllAssoc() and created a new array rather than unsetting stuff from the result set. Also moved garbage collection into a shared function to remove the remaining code duplication.

re-roll for the test bot.

I've rolled this into #111127: Cache node_load - but don't want to close it out as duplicate yet since there's potentially other use cases (although this patch is wrong, fixes are in the patch for the other issue). So marking postponed pending what happens with node_load().

Here's current work from the cache node_load() patch broken out, but this would all happen a lot quicker if more people could review that other patch instead of subscribing to this postponed one, I'm hoping we can mark as a duplicate when node load goes in.

Postponed for almost six months months is long enough.

Here's a refreshed version of the patch from node_load() which tidies up the way the return value from cache_get_multiple is handled and adds cache clearing with an array of cids. Both cache_get_multiple() and cache_clear_all($array_of_cids) have unit tests.

I also modified field_attach_load() to use cache_get_multiple(). Since we don't actually have any multiple loaded objects with fields attached to them in core yet, this'll take a bit of setup to benchmark.

Seems I hadn't actually saved field.attach.inc and that bit doesn't work yet. Have to go out, but here's a snapshot with that bit so far.

Patch works now.

Benchmarked HEAD with http://drupal.org/files/issues/body-as-field-372743-120.patch, 10 nodes on front page.
7.40 reqs/second

Same again with this patch applied:

7.58-7.62 reqs/second

Saves 9 queries and about 3%.

Added a check before we add an item to the $cids array and consolidated the $cid variable in the foreach. Also made the tests pass.

I'm also not sure if it's the slicing or just the new variable which was the problem, I'm hoping just the new variable - since the slicing is awesome (I can say this because it was Justin's update, not mine, which introduced it).

Re-rolled, removed the variable.

For what it's worth, I did some additional benchmarks / devel logging with this over at http://drupal.org/node/369011#comment-1536282 to see how much we can squeeze out of it.

Thanks for the review, attached patch addresses the feedback, slightly improves the doc for $table (we're getting data, not storing it, in cache_get) and also removes an unnecessary line break before a return elsewhere. Didn't change the $exists check because we have an entire cache test using this style and I kept it consistent -not planning to make the patch twice as big changing the rest of the test.

max_allowed_packet defaults to 1MB on most systems I think - MySQL docs say it's 16MB now. 1,000 ought to be conservative even with long cache IDs.

test bot meltdown.

gotcha, a straight delete from with 1000 cache ids isn't going to hit 1mb - much more likely to hit it with a straight cache_set on a large object with lots of text in it.

Me neither. But according to http://www.unitconversion.org/data-storage/characters-to-kilobytes-conve... it comes to 259k.

Rerolled for getInfo() static method changes.

Changes in field_attach_load() made this no longer apply. Re-rolled but was getting a lot of exceptions / fails - however fields loaded via the CCK UI work fine here, so not sure why. Uploading to see how bad it really is.

And it is :)

Patch does exactly the same thing, but there's enough changes to make it work with the new field_attach_load() that it could do with another look over before it's back to rtbc.

Those changes look good. Test bot is happy.

Bumping to add a summary - I wrote up loads of summaries on node load caching but there's no proper explanation of what the patch does here.

At the moment the field cache does a cache_get() for every object loaded on a page. With database caching this means a query per object - which makes it the exception in terms of how our fieldable objects are loaded.

The field and filter caches are currently the only places in HEAD where we have per-object queries in listings. When combined with #369011: Performance: do check_markup() in text_field_load() we should have exactly the same number of database queries on a page regardless of how many objects are loaded.

So the patch itself adds cache_get_multiple() - which takes an array of cids, and gives you an array of cache records back (and your $cids array reduced to the items you still need for convenience). With node or profile listings, this reduces the field cache lookups from 10 (or 30, or 300) to 1.

With the body as fields patch, this saves us 9 queries and gives us an extra 3% requests per second on the default front page with 1 node (see #16). If we ever make taxonomy terms fieldable, it'll save us hundreds of lookups to the field cache on node listings - because we load full term objects there which would call field_attach_load() - and issue a cache_get() for every term on the page.

Swappable cache systems should only have to do:

foreach ($cids as $cid) {
  $cache[$cid] = cache_get($cid);
}

And not a lot else if there's no benefit to re-implementing the multiple getting. There might be situations where a pluggable system supports something like this and could use it though - to reduce impact of network latency for example.

The patch also adds the facility to clear cache ids using an array of ids in addition to the current cid/wildcard/everything options - this should allow for more intelligent cache clearing when that kind of information is available. This was something necessary for the node_load() caching, but might be considered kitten killing as part of this patch. I think it'll be useful in core when combined with things like nodereference #392494: Fields API needs to allow searching of scalars though so leaving it in until I'm told otherwise. That bit also comes with tests.

Now body as field is back to CNR, it's possible to benchmark this usefully.

Front page, 30 nodes. Devel output and ab -c1 -n500

HEAD:
Executed 101 queries in 72.98 milliseconds
3.42 [#/sec]

Body as field:
Executed 73 queries in 71.71ms
2.86 [#/sec]

Body as field + latest patch on this issue:
Executed 43 queries in 58.79 milliseconds.
3.03 [#req/sec]

tagging.

With #413192: Make taxonomy terms fieldable the gains are even more obvious - this is with a completely empty field cache. Once we have thousands of records in there the individual queries will take a bit longer. Combined with body as field, half the front page queries will be from the field cache.

Devel, 30 nodes on front page, each with 5 terms attached, works out as 50-ish unique terms on the page:

HEAD + fieldable terms.
Executed 115 queries in 55.51 milliseconds.

Patch:
_get_multiple() + fieldable terms
Executed 72 queries in 50.18 milliseconds.

fieldable terms and body as field are in HEAD, so new benchmarks. Can't upgrade a D6 site to get large amounts of content because that's broken at the moment, so one node with 20-odd freetagging terms created manually. Since we're now getting dozens of database hits from cache_get() on node listings with lots of unique taxonomy terms, bumping to critical.

One node, 20 odd terms.
HEAD:
Executed 62 queries in 44.97 milliseconds.
8.31 [#/sec]

Patch:
Executed 45 queries in 32.31 milliseconds.
8.60 [#/sec]

chx also pointed me to http://us3.php.net/manual/en/function.memcache-get.php which takes an array of cache keys - so it looks like this would help some alternative drivers rather than make things harder at least in some cases

Re-rolled after #482622: Make cache friendlier for alternative implementations.

Not sure about _garbageCollection() and _prepareItem() but it all works.

Thanks chx. Re-rolled with those changes.

Spoke to chx and bjaspan in irc (and had discussed briefly with yched before) - here's a slight extension of the patch.

We now create a cache table for each fieldable entity - i.e. cache_field_node, cache_field_user etc. This means table size (and hence overhead, key sizes etc.) will be a lot smaller, you'd be able to set up seperate cache bins in memcache per entity - good for when you have millions of records , and also backends like memcache which don't handle wildcard cache clearing very well. It also means we can use object id as the cache key, which should also make lookups a tiny bit faster than using composite strings like field:$obj_type:$id

Couple of very small kittens - hook_modules_installed() isn't called in install.inc, and hook_modules_uninstalled() is currently called after a module and all it's tables have been removed - not very useful if you need to check things like hook_fieldable_info() for the module being uninstalled.

yched - if we do two separate patches, then they'll break each other, I'm fed up re-rolling this one every couple of weeks and don't want to compound that by maintaining two conflicting patches in tandem.

Updated field_cache_clear(), thought I'd already done that, but apparently not...

Doh. Right patch this time.

Modules loaded in install is sufficiently broken that the table-per-entity part of this is going to end up postponed on four or five other patches.

So splitting that out to #496344: Create a cache table per entity.

This patch just makes the field_attach_load() changes and adds cache_get_multiple() and yes chx, that array_diff works fine. Note to self, listen to yched more.

nudging test bot.