| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | entity system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | API clean-up, Performance |
Issue Summary
This is a long delayed followup to http://drupal.org/node/111127#comment-1494790
Reasons I'm posting this issue:
1. Despite converting lots of things to fields, caching at the node level rather than the field level gives a 10% performance improvement on a single node view with only the default profile and database caching. So the field cache does not 'do the same job' as an entity level cache as was suggested by Dries and bjaspan in that issue - it can't remove the cost of node_load() itself, the query building, the field_attach_load() calls etc.
2. I posted a proof-of-concept module at http://drupal.org/project/entitycache which implements node, taxonomy term and comment caching from contrib. This works, when hacked into the default profile, all but a couple of tests pass, and the entire .module is 14k - some of which is dead code for handling users which should probably be abandoned, some of which could probably be tidied up (pretty much all the NodeController / TaxonomyTermController overrides don't need to be there if we can call back to those classes directly), so potentially < 10k.
So, I'm proposing that we add this module to core, enabled in the default install profile. By having it as a separate module, we're able to maintain all the cache clearing code in one central place, so it doesn't have the messiness of putting node cache clears in taxonomy module, or taxonomy cache clears in image module or whatever. However we'll get real performance benefits in core rather than having to schlep off to contrib to get them.
We have real performance issues in Drupal 7, and this is one possible way to ameliorate those without changing any APIs (it adds a total of one hook). I'm not posting a patch here, since the code is in CVS, and this is more of an architecture/policy decision at this stage. Abusing status though to get it in the real issue queue.
Comments
#1
You say that this only works at Drupal installation time - why is that?
#2
To clarify, the only way I know of to enable a contrib module throughout all core tests is to hack it into default.profile - that way all tests get run with the caching enabled and you can see what breaks. entitycache itself doesn't care when it's installed.
#3
Entity caching belongs in core.
#4
Benchmarked node/1 with Drupal 6 vs. Drupal 7 vs. Drupal 7 plus entitycache module. Numbers were pretty consistent between multiple runs of ab.
6: 17.48 #/sec
7: 11.49 #/sec
7 with entity caching: 14.18 #/sec
I make that a 20% performance gain - this using database caching and very few modules acting on hook_node_load(). It also clears just under ~40% of the gap between D7 and D6 on single node views.
Drupal 6:
Concurrency Level: 1
Time taken for tests: 57.223 seconds
Complete requests: 1000
Failed requests: 0
Write errors: 0
Total transferred: 7954000 bytes
HTML transferred: 7388000 bytes
Requests per second: 17.48 [#/sec] (mean)
Time per request: 57.223 [ms] (mean)
Time per request: 57.223 [ms] (mean, across all concurrent requests)
Transfer rate: 135.74 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 40 57 12.9 56 246
Waiting: 40 57 12.7 56 230
Total: 40 57 12.9 56 246
Drupal 7:
Server Hostname: d7.7
Server Port: 80
Document Path: /node/1
Document Length: 6313 bytes
Concurrency Level: 1
Time taken for tests: 86.995 seconds
Complete requests: 1000
Failed requests: 0
Write errors: 0
Total transferred: 6800000 bytes
HTML transferred: 6313000 bytes
Requests per second: 11.49 [#/sec] (mean)
Time per request: 86.995 [ms] (mean)
Time per request: 86.995 [ms] (mean, across all concurrent requests)
Transfer rate: 76.33 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 1
Processing: 75 87 12.6 82 144
Waiting: 74 87 12.5 82 144
Total: 75 87 12.6 82 144
Drupal 7 + entity caching:
Document Path: /node/1
Document Length: 6313 bytes
Concurrency Level: 1
Time taken for tests: 70.537 seconds
Complete requests: 1000
Failed requests: 0
Write errors: 0
Total transferred: 6800000 bytes
HTML transferred: 6313000 bytes
Requests per second: 14.18 [#/sec] (mean)
Time per request: 70.537 [ms] (mean)
Time per request: 70.537 [ms] (mean, across all concurrent requests)
Transfer rate: 94.14 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 60 70 9.9 67 109
Waiting: 60 70 9.9 67 109
Total: 60 70 9.9 67 109
#5
Huge +1 to entity caching in core. Let's make use of what we gained through the new entity API. On an actual production site with many contrib modules, gains through entity caching will even be much bigger. No reason at all to not do this in core, IMO. And gets us quite a bit closer to solving our perfomance problems in HEAD, as the benchmarks in #4 show.
#6
Alright, let's do a patch then.
This is a straight cp of the contrib module to core directory. What to do about poll/book which do things we don't want to cache inside hook_node_load() needs discussion since the approach taken in the module is a bit of a sledgehammer. There's existing code for that in #111127: Cache node_load though.
The only place where I found core test failures which might require some changes to something in core outside the new module due to this, was in translation module - but that's likely due to a lack of hook invocations in the right places.
Going to let the test bot have a crack at it (by adding to default profile we get all tests run with entity caching for free) - last time I tried to run tests locally, apart from finding unreported fatal errors in HEAD, things went surprisingly well. That was a couple weeks ago though.
#7
We just need a second hook here, that's also the result of #111127: Cache node_load. Only the individual modules can decide whether their data is cacheable at the entity level or not. Note that this is not necessarily an API change but merely the addition of a new hook. So I would say we just need to decide on naming.
I'd vote for
hook_ENTITY_TYPE_load()for everything that's cacheable andhook_ENTITY_TYPE_load_uncached()orhook_ENTITY_TYPE_postload(), as it was suggested in the original node_load cache issue. Which one I don't care, with proper documentation both are fine.#8
The last submitted patch failed testing.
#9
If we do that, let's rename hook_field_info()'s 'cacheable' property into 'field cache'.
It should be done nonetheless as a WTF fix, but this change would make the WTF even worse.
#10
... or better: keep 'cacheable' (or 'cache'), but turn it into 'TRUE = yes, I have entity caching' instead of currently 'TRUE = yes, use the field cache because I have no entity caching' (meaning field API should take it backwards) ?
#11
Agreed on the 'cacheable' => 'field cache' change. We also have a 'static cache' property which is already used by the entity API.
Attached patch just removes bogus hook_user() implementations which are causing those exceptions. Should be down to genuine bugs / fails with this one. Help identifying/fixing those welcome of course - as is code review (my OOP is extremely basic, so there may well be better/more concise ways to do what I'm doing in EntityCacheBaseController). Leaving CNR for the bot.
#12
Hmmm... not sure - somehow I preferred the more fine-grained options at first sight (i.e. entity cache, field cache, static cache). I think that would allow for some very granular performance tuning on sites that need it? Just my $0.02
#13
Wrong patch sorry.
#14
The last submitted patch failed testing.
#15
re #11-12: 'entity cache, field cache, static cache'.
It's just that entity cache + field cache is useless, the two should be mutually exclusive.
#16
Subscribing. I'll try to have a look at the latest patch from an OO perspective soon.
#17
Cache clearing sounds like fun :-/
Field API will need an API function to clear the cache for a selected set of entities - like nodes 12, 13, 14, users 5, 7, 9...
Use case: clear the cached check_markup() for text fields when a format definition changes.
More fun: for some entity types, that data will be in the field cache, for others it will be in the entity-level cache...
#18
It looks like this is the text module issue for check_plain() - #556022: When a text format is deleted, some modules (like text.module) don't update their data, even though we say they do. Just posted in there, but I think for that kind of update, you're looking at a general cache clear anyway - it's a rare enough operation, and expensive enough to try to figure it out granularly.
For general reference fields like terms / nodereference / userreference, we already have #493314: Add multiple hook for formatters / #606114: taxonomy_field_formatter_load() should use taxonomy_term_load_multiple() which bypasses the infinite recursion and/or stale data issues we'd get from trying to cache other entities within the field or entity caches. The advantage here, is that taxonomy_field_formatter() load only ends up a cache hit in most cases. If a term has actually been deleted, but the tid is still in the field cache, then we handle missing terms already - since the neither the term field nor the field cache has garbage collection for missing references anyway.
#19
catch: agreed on those cases.
I'm a little worried that we have no API way to clear the entity cache. Field API has field_cache_clear() or knows how to clear it's own cache for a specific object when it needs. If data can be cached either in the field cache or 'somewhere else', it becomes more problematic.
Er, I might be just rehashing #111127: Cache node_load, which I don't remember that well :-).
in comment #207 over there, I wrote: "Field API needs to know the name of the cache table (or assume 'cache_[obj_type]' ?) and the structure of the cids (or assume [id] ?)." - or get an API function from entity API ?
#20
We could very easily add a drupal_get_entity_loader('entity')->flushCache() style thing, similar to how the static flushing happens now. That seems like a good idea in general. fwiw all the tables are cache_$obj_type and all the cids are $id - so it can also just guess, and it'd be right ;)
#21
This should fix most of the fails, and get us down to a couple of exceptions - which are down to the hack in entitycache_entitycache_node_load().
However fixing that hack means deciding on whether to keep this as a distinct module or merge it back into the entity loader itself, and how we deal with those uncachable hook implementations in either case. So I'll stop here for a bit until that discussion's been had.
#22
Down to translation locally, CNR for the bot.
#23
The last submitted patch failed testing.
#24
"deciding on whether to keep this as a distinct module or merge it back into the entity loader itself"
Possibly related: isn't it a bit limitative that another module (e.g contrib) willing to extend entityController for whatever reason will basicaly forbid the use of the entity-level cache for lack of multiple inheritance ?
#25
I think you could do this:
hook_entity_info_alter() same as entitycache does.
Require entitycache module.
Your class extends EntityCacheController instead of the default.
Or just copy the code and then work off that.
But if everyone does that, then there's no point separating them, no.
#26
subscribe
#27
>>"deciding on whether to keep this as a distinct module or merge it back into the entity loader itself"
>Possibly related: isn't it a bit limitative that another module (e.g contrib) willing to extend entityController for whatever reason will basicaly forbid the use of the entity-level cache for lack of multiple inheritance ?
I've extended it already, see this. So requiring entitycache module would be the way out for me there. However I'd prefer to see that baken in the DefaultEntityController class. So why do we need a separate module at all?
>By having it as a separate module, we're able to maintain all the cache clearing code in one central place, so it doesn't have the messiness of putting node cache clears in taxonomy module, or taxonomy cache clears in image module or whatever.
Having the comment cache clear in comment_save makes a lot of sense. Node cache clears in taxonomy module won't fly anyway, because entitycache can't support entity X introduced in contrib. But taxonomy module knows it provides a field type needing a cache refresh, so it should look up the created fields and clear the caches as appropriate.
#28
The reason there's a separate module is because the node_load() caching patch sat at RTBC for six months then was postponed by Dries indefinitely, to be revisited when we'd converted some stuff to fields and compare the field cache. We've done that, I've compared them, this is still a very significant improvement, so this is the same thing revamped. However I have no intention of spending time rolling a proper patch and keeping it up to date until there's some indication that it'll even be considered given what happened last time, new modules don't get CVS conflicts. Once that indication is given either way (or if someone else wants to step in), then we should definitely roll it into DrupalDefaultEntityController.
#29
I'll do my best to revisit this patch/approach tomorrow, and to provide some direction.
#30
subscribe
#31
Right now I'm playing with an interesting bug in the attached patch. If the poll module is enabled, then all nodes that are loaded from the cache are passed through poll_load. Since most nodes are, in fact, not polls, this seems less than ideal behaviour. In the following code, it $this->entityType = 'node' AND the poll module is enabled...
protected function attachAfterLoad($new_entities) {foreach (module_implements('entitycache_' . $this->entityType . '_load') as $module) {
$function = $module . '_entitycache_' . $this->entityType . '_load';
$function($new_entities);
}
}
then poll_entitycache_node_load is called, which in turn calls poll_load. The same happens for books as well. I'm trying to find a good way to fix it AND clean up some of the testbot failures.
#32
Probably the easiest way to fix it would be for poll_entitycache_node_load() to check for node type before executing poll_load() on each node it gets.
However for a proper core patch, I would split the user-specific stuff in poll_load() into poll_node_view() or similar - see the original patch in #111127: Cache node_load (and we'd probably need to do a similar thing for translation.module as was done for node_load() caching too).
@Dries - did you get a chance to review this issue yet?
#33
I wonder if this might be done more cleanly. Just off the top of my head, what if implementations of hook_load were free to add '#cachable' => FALSE or '#cachable' => TRUE (defaulting to TRUE)? This would allow most everything to decide whether or not to cache, removing the problem that hook_entitycache_node_load appears to be addressing.
If we take this one (logical) step further and add '#cached' => TRUE to all nodes loaded from the cache, then we could use hook_nodeapi easily to update anything that needs to be changed. We could use poll_load as per usual and then only use poll_nodeapi to update the data that should not be cached. For larger nodes where most of the data will remain the same and only a very little will need to be updated, I think that this might be more efficient (since several queries could potentially be saved.)
I believe this might be a little easier / more versatile. (Or I could be completely and totally wrong. I've done that before too.)
#34
I'm reluctant to do that, because I actually think poll_load() shouldn't be cramming user-specific stuff about which links get displayed etc. into node_load() anyway. Doing it in poll_node_view() (is there even a hook_view()? - could happen there) or wherever solves the problem without adding extra complexity for well-behaved modules - a single foreach() to filter out non-poll node types isn't expensive at all.
The trickier one here is book_node_load() - where it's relatively inexpensive, and we don't want to cache it, but it's much more arguably canonical data about the node than poll permissions is. But even then, either re-using an existing hook that's not cached, or adding new hook which isn't persistently cached as the node_load() caching patch did seems safer to me than trying to conditionally call some hook implementations on some nodes some of the time depending on various flags.
#35
I reviewed the patch, and I like the idea of entity caching. I'd certainly want to explore this more as I agree that this is one of the most important performance patches in the queue. Sorry it took me so long to get to this issue.
1. I'm not a big fan of making this a module. Why aren't we integrating this better into the default implementation? Worst case, I'd prefer to make this an include file. A module called entitycaching is too low-level for my liking -- especially for core.
2. Does this allow us to remove some other caching code?
3.
+++ modules/entitycache/entitycache.module 28 Oct 2009 15:48:01 -0000@@ -0,0 +1,289 @@
+ $entity_info['node']['cacheable'] = FALSE;
Reading the patch top to buttom, it is unclear what this cacheable variable does. Could use a code comment, because I don't understand why it matters. Given that this is a caching module, I'd expect those to be set to TRUE.
4.
+++ modules/entitycache/entitycache.module 28 Oct 2009 15:48:01 -0000@@ -0,0 +1,289 @@
+ // Load any remaining entities from the database. This is the case if $ids
+ // is set to FALSE (so we load all entities), if there are any ids left to
+ // load, if loading a revision, or if $conditions was passed without $ids.
+ if ($this->ids === FALSE || $this->ids || $this->revisionId || ($this->conditions && !$passed_ids)) {
I don't like how ids can be FALSE. It seems like we're overloading the variable ids to mean a number of different things.
5.
+++ modules/entitycache/entitycache.module 28 Oct 2009 15:48:01 -0000@@ -0,0 +1,289 @@
+function entitycache_comment_helper($comment) {
+ cache_clear_all($comment->cid, 'cache_entity_comment');
+ cache_clear_all($comment->nid, 'cache_entity_node');
+}
_comment_helper() sounds like a really poor function name for what it does. Also, the function is small enough that I wouldn't bother with the helper function to begin with.
6. I'd be interested to see actual benchmark results. You mention 10%, but is that documented somewhere?
#36
re Dries
That's the variable the controls *field* level caching. Was originally part of hook_fieldable_info(), and thus takes things from the field end. 'cacheable' = FALSE means 'I have an entity-level cache of some kind, don't bother caching at field level.
It should obviously be renamed to 'field_cache' now (API change...). Been mentioned several times in the past, but it we never got actually done :-/.
#37
Yeah we need cacheable / field_cache, cache / static_cache and add entity_cache to make this work and sensible. That plus one new hook should be the only API change though I hope.
I don't have a bunch of time this week, but I'll try to have something to look at in the next week-ish.
#38
subscribing
#39
me too.
#40
subscribe
#41
initial patch to move this to entity.inc and make it more of a proper API.
Done:
Makes all the necessary changes to entity load, hook_entity_info() and adds cache tables for nodes, files, taxonomy and comments. i.e. enough for benchmarking and a look at the API additions / changes - both of those welcome.
Not done (hint, these are in order for anyone who wants to help):
cache clearing in node_save() and similar places.
book_node_load() -> book_node_load_uncached()
half of poll_load() -> poll_node_load_uncached()
fixing tests (there's a method name change for resetCache())
#42
Slight cleanup in entity.inc, not got to the TODOs yet.
Comparing node/n, article, default profile, anonymous user (no page caching), only user and comment modules implementing hook_node_load().
Short version, HEAD 10.4 #/sec, Patch: 12.75 #/sec -). That's an 18% performance improvement on single node views.
As usual, devel numbers given only for sanity checking and query counts, I tried to get the most representative of a few requests, but they're all over the place for just one request.
HEAD:
devel says: Executed 49 queries in 45.64 milliseconds. Page execution time was 175.59 ms.
Concurrency Level: 1
Time taken for tests: 96.150 seconds
Complete requests: 1000
Failed requests: 0
Write errors: 0
Total transferred: 7986000 bytes
HTML transferred: 7434000 bytes
Requests per second: 10.40 [#/sec] (mean)
Time per request: 96.150 [ms] (mean)
Time per request: 96.150 [ms] (mean, across all concurrent requests)
Transfer rate: 81.11 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 82 96 11.3 94 289
Waiting: 82 96 11.2 94 282
Total: 82 96 11.3 94 289
Patch:
Devel says: Executed 45 queries in 33.78 milliseconds. Page execution time was 160.92 ms.
Document Path: /node/209
Document Length: 7434 bytes
Concurrency Level: 1
Time taken for tests: 78.422 seconds
Complete requests: 1000
Failed requests: 0
Write errors: 0
Total transferred: 7986000 bytes
HTML transferred: 7434000 bytes
Requests per second: 12.75 [#/sec] (mean)
Time per request: 78.422 [ms] (mean)
Time per request: 78.422 [ms] (mean, across all concurrent requests)
Transfer rate: 99.45 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 68 78 8.9 76 137
Waiting: 68 78 8.9 76 136
Total: 68 78 8.9 76 137
I'd expect the results to be even better with a non-database caching backend.
It'd be good to get benchmarks with taxonomy terms and comments attached to nodes - since these would also be cached, to see if we continue to see 15-20% gains or it starts flattening out. Would also be good to see what the change is on a node listing - I'm pretty sure it drops to 5-10% for those since we're already efficient in terms of SQL using multiple load.
#43
catch: bravo, this is very cool and exciting stuff.
#44
Side note, I've opened #665618: Add entity caching for users - having tried to do this briefly in entitycache.module, it was tough enough that I don't think it's a good idea to tackle here. However if we can keep this issue focused, it'd be worth trying to get that working in separate patch, then maybe working it in here later if it's not too hellish. Various issues with doing user_load() all over the place at #636992: Entity loading needs protection from infinite recursion, #638070: Router loaders causing a lot of database hits for access checks and #490108: user_load() should add session properties when loading the currently logged-in user.
In answer to Dries' #2. Not really. Both the field cache and entity-level caching should allow us to get rid of the filter cache in Drupal 8, assuming most diy formatted textareas get converted to text fields. If we're able to successfully apply entity caching for users, then that points towards possibly removing the field cache since we'd have no use case in core, and core entities cover a lot of use cases - but again I think that's a D8 thing.
#45
OK there's still a bunch of test failures to fix, but all the 'core' tests like comment, taxonomy, node, user, file are passing now - so the rest is cleanup, plus poll/book/translation which need special treatment. However I'm going to mark this as needs review, since all the API changes are now in the patch, along with all the changes to entity.inc, and it needs a wider variety of benchmarks run on it.
Side note, I noticed that we clean the field cache when a taxonomy term is deleted, however our taxonomy field formatter handles missing terms. Also we don't cache any term information, only the reference to the term, in either the field or entity cache. So that's a lot of unnecessary cache clearing. This should be removed in a separate issue probably, but I've rolled it in here for now.
#46
And a summary of the changes:
This patch now adds a cache_get_multiple() and cache_set() stage to the default entity controller, this is in between fetching from static cache and from the database. Both persistent and static caching remain optional.
The 'cacheable' key in hook_entity_info() has been changed to 'field cache', this remains the default.
Two new keys are added: 'entity cache', and 'entity cache bin'. Both are required if you want to use entity caching, at the moment all core entities create their own cache bins, with taxonomy sharing one bin between vocabularies and terms. It would be feasible to do this with dynamic schema at some point, but the module install system is still pretty fragile at the moment, and there's various opportunities for race conditions (esp user_load() if we're able to do that), enough to make this not worth automating.
Modules are responsible for clearing the entity cache in their update and delete operations - when/if we eventually have an entity API which provides save / update / delete as well as load, that'll be handled centrally.
Field API modules don't need to know anything about the cache. Modules using _load() / _save() / _delete() hooks to interact with entities may need to, but only if they're doing something user / language specific in those hooks, or if they're altering data which goes into _load() outside of the entities own API functions - i.e. Poll and comment.
There are a couple of API changes we could avoid making if we really wanted to, but I've tried to keep variable names etc. as self-explanatory as possible rather than keeping them exactly as is (given changes like that still seem to be acceptable for new APIs).
I'm going to keep working on tests, and hopefully some new benchmarks over the next day or so. But reviews in the meantime are welcome.
#47
This looks very promising, but haven't looked at the latest patch yet. Thanks for running with this, catch.
#48
nitpicks below.
<?php+ /**
+ * Clear the persistent cache for an entity.
+ *
+ * @param $ids
+ * A single entity ID or array of IDs, if present clear the entity cache for
+ * these specific entities. If no argument is given, the entire bin
+ * specified in hook_entity_info() will be flushed.
+ */
+ public function resetCache($id = NULL);
?>
should be $ids no $id.
<?php+ // Call hook_TYPE_load(). The first argument for hook_TYPE_load() are
?>
should be 'is' not 'are'.
<?php- if (isset($form['identity']['type'])) {
+ if (isset($form['type'])) {
?>
is that hunk from another patch?
possibly bikeshedding, but i think resetCache should be clearCache, as that seems to be more inline with what its does and the terminology we use elsewhere?
#49
Sorry for poluting the issue, but catch, can you point me to your benchmark/profiling doc again? I can't find it anymore :/
Or is it enough to bench with ab?
#50
Fixed field, poll, translation, book and comment tests, should be close to 100% pass but I'll leave the entire test suite to the bot when it's back up and running.
Remaining TODOs:
1. Benchmark single node with taxonomy terms / comments etc.
2. Benchmark node listing
3. API documentation for hook_ENTITY_load_uncached() - although we have book and poll examples for this already.
#51
And the patch.
#52
Crossposted with Justin and scroogie:
Justin: resetCache/clearCache - yeah that seems like a reasonable change. It also means that if anyone is already calling resetCache() with no arguments, they'll get a proper error instead of unknowingly clearing the entire cache bin for that entity. Have made that change and fixed the typos.
@scroogie - ab is fine.
Another thing we need to figure out. When modules are installed or disabled, if they include any hook_ENTITY_load() implementations, those won't get picked up until the cache is flushed.
I think we can do two things here potentially - 1. since hook_entity_info() has the name of the hooks (hook_node_load(), hook_comment_load() etc., we could only do this when modules implement one of those hooks. 2. We can only flush the cache tables for the entity types which are affected. I'm not sure if we'll have all the information at the right time to do this, but seems worth a try. If not we may just have to flush all entity caches everywhere on module disable / enable.
#53
cut and paste of #drupal discussion about hook_ENTITY_load between catch and i:
(18:25:15) beejeebus: catch: cool. on the hook_ENTITY_load question, creating new entities doesn't clear the cache right?
(18:25:49) beejeebus: catch: so we could get some entities with the new load hook called on them, and some not, until the cache is cleared? that feels messy
(18:26:17) catch: beejeebus: creating new entities?
(18:26:42) beejeebus: catch: if we have a bunch of foo entities cached
(18:27:05) beejeebus: catch: and we enable a module that implements the hook_foo_load
(18:27:28) beejeebus: catch: new foos will pass through that, but existing cached foos wont?
(18:27:36) catch: beejeebus: OK yeah, in the current patch, that is broke.
(18:27:59) catch: beejeebus: but I'm wondering if we can only clear the cache if the module implements a hook_foo_load(), and only for foo. Not just clear everything.
(18:28:10) beejeebus: catch: that seems reasonable
(18:28:45) beejeebus: catch: perhaps that's should be an implementation of hook_modules_enabled ?
(18:29:00) catch: beejeebus: ooh that would be a nice place to put it yeah.
(18:29:08) catch: It'll have to be in system.module but that's OK.
#54
Sorry, I'm new to this. I didn't know what configuration I should test this with, so I just created a node with some comments, tags and terms and got these results.
Configuration is Windows XP, Nginx 0.8.30, PHP-CGI 5.3.1, APC enabled.
node/1 without patch
===============
Finished 300 requests
Server Software: nginx/0.8.30
Server Hostname: d7.localhost
Server Port: 80
Document Path: /node/1
Document Length: 105085 bytes
Concurrency Level: 1
Time taken for tests: 9.391 seconds
Complete requests: 300
Failed requests: 0
Write errors: 0
Total transferred: 31665900 bytes
HTML transferred: 31525500 bytes
Requests per second: 31.95 [#/sec] (mean)
Time per request: 31.302 [ms] (mean)
Time per request: 31.302 [ms] (mean, across all concurrent requests)
Transfer rate: 3293.04 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 16 31 2.4 31 47
Waiting: 16 30 4.4 31 47
Total: 16 31 2.4 31 47
Percentage of the requests served within a certain time (ms)
50% 31
66% 31
75% 31
80% 31
90% 31
95% 31
98% 31
99% 47
100% 47 (longest request)
node/1 with patch:
=============
Finished 300 requests
Server Software: nginx/0.8.30
Server Hostname: d7.localhost
Server Port: 80
Document Path: /node/1
Document Length: 105085 bytes
Concurrency Level: 1
Time taken for tests: 9.281 seconds
Complete requests: 300
Failed requests: 0
Write errors: 0
Total transferred: 31665900 bytes
HTML transferred: 31525500 bytes
Requests per second: 32.32 [#/sec] (mean)
Time per request: 30.938 [ms] (mean)
Time per request: 30.938 [ms] (mean, across all concurrent requests)
Transfer rate: 3331.85 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 16 31 2.2 31 31
Waiting: 16 30 4.8 31 31
Total: 16 31 2.2 31 31
Percentage of the requests served within a certain time (ms)
50% 31
66% 31
75% 31
80% 31
90% 31
95% 31
98% 31
99% 31
100% 31 (longest request)
#55
The last submitted patch, , failed testing.
#56
My bad.
#57
The following notices are introduced by this patch (and happen dozens of times with each page load on my test website):
Notice: Undefined index: field cache in field_attach_load() (line 543 of /var/www/html/drupal-7.x-dev-entitycache/modules/field/field.attach.inc).Notice: Undefined index: field cache in entity_extract_ids() (line 6358 of /var/www/html/drupal-7.x-dev-entitycache/includes/common.inc).
#58
@Jeremy, you'll need to clear the {cache} table before testing.
@scroogie - thanks, those results are still an improvement, but a much smaller relative improvement than mine (and 30 requests a second seems a lot, maybe nginx really is that fast though). Couple of things - could you check that anonymous users have been given access to view comments, that's off by default. Also please run with -c1 -n1000 to give things time to warm up. And if benchmarking with a lot of comments, it might be worth applying #636992: Entity loading needs protection from infinite recursion to both the before and after version of HEAD, since that's such a massive slowdown at the moment it'll skew results..
Benchmarked a node with a comment and some taxonomy terms this time. This is probably the most favourable example towards caching, since we don't get any advantages of multiple load apart from on the terms. Will try some more unfavourable examples next.
Just a note I got almost exactly the same result the first time I ran the benchmarks, then cleared caches and re-ran the benchmarks both times, then consistently got these, this also happened once before. Not sure if it's just me benchmarking the wrong thing first time or if there's a weird side-effect when applying or un-applying the patch in either direction.
HEAD:
Executed 68 queries in 64.01 milliseconds. Page execution time was 190.02 ms.
Concurrency Level: 1
Time taken for tests: 103.548 seconds
Complete requests: 1000
Failed requests: 0
Write errors: 0
Total transferred: 9455000 bytes
HTML transferred: 8929000 bytes
Requests per second: 9.66 [#/sec] (mean)
Time per request: 103.548 [ms] (mean)
Time per request: 103.548 [ms] (mean, across all concurrent requests)
Transfer rate: 89.17 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 89 103 15.0 98 159
Waiting: 82 96 14.2 90 150
Total: 89 103 15.0 98 159
Patch:
Executed 60 queries in 53.31 milliseconds. Page execution time was 148.35 ms.
Concurrency Level: 1
Time taken for tests: 82.455 seconds
Complete requests: 1000
Failed requests: 0
Write errors: 0
Total transferred: 9455000 bytes
HTML transferred: 8929000 bytes
Requests per second: 12.13 [#/sec] (mean)
Time per request: 82.455 [ms] (mean)
Time per request: 82.455 [ms] (mean, across all concurrent requests)
Transfer rate: 111.98 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 71 82 10.7 78 129
Waiting: 65 75 10.1 71 119
Total: 71 82 10.7 79 129
#60
One node, 50 comments. Results are not what I expected - ~12% improvement which is too high for one query and a bit of PHP shortcutting.
There seems to be two reasons for this:
1. In devel query log, I'm seeing a 17ms query caused by field_sql_storage_schema() - this should never be called on a regular page load in normal circumstances, needs it's own bug report. Opened #667098: drupal_get_bootstrap_phase() is broken.
2. The regular comment_load_multiple() query takes 25.09ms on my machine, whereas the cache_get_multiple() is > 1ms. This seems to be simply due to the joins it does, there's no filesort or anything. We might be able to optimize this in the uncached case, but it's a genuine improvement as things stand. Opened #667100: comment_load_multiple() query joins on two tables and is slow.
Concurrency Level: 1
Time taken for tests: 214.880 seconds
Complete requests: 1000
Failed requests: 0
Write errors: 0
Total transferred: 124642000 bytes
HTML transferred: 124114000 bytes
Requests per second: 4.65 [#/sec] (mean)
Time per request: 214.880 [ms] (mean)
Time per request: 214.880 [ms] (mean, across all concurrent requests)
Transfer rate: 566.46 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 195 215 15.4 210 299
Waiting: 187 206 15.1 202 290
Total: 195 215 15.4 210 299
Patch:
Executed 110 queries in 42.58 milliseconds. Page execution time was 262.16 ms.
Concurrency Level: 1
Time taken for tests: 189.897 seconds
Complete requests: 1000
Failed requests: 0
Write errors: 0
Total transferred: 124642000 bytes
HTML transferred: 124114000 bytes
Requests per second: 5.27 [#/sec] (mean)
Time per request: 189.897 [ms] (mean)
Time per request: 189.897 [ms] (mean, across all concurrent requests)
Transfer rate: 640.98 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 175 190 13.8 186 304
Waiting: 166 181 13.4 177 295
Total: 175 190 13.8 186 304
I can't get devel_generate to generate 10 nodes with proper node bodies yet, might need an update for recent changes in HEAD, but if you ignore the fact that caching appears to allow us to gloss over some existing performance bugs in HEAD, then this looks like consistently between 10-20% with database caching in various scenarios. Would be very happy if we could get one or two more indpendent benchmarks though.
#61
I'm seeing an overall slowdown with this patch. My fastest page load time is a little faster, and my slowest page load time is a little faster, but the average page load time is slower. It's possible I didn't let the tests run long enough to see full gains from this patch, as after it had run with entity caching enabled I only had 141 cached nodes and 169 cached comments. Or perhaps I'll need to set a minimum cache lifetime to see any gains?
I'm using a large database for testing: ~50,000 nodes, 10,000 users, and ~500,000 comments (all data was generated with the devel module -- I was unable to get taxonomy generation working.) Before each test, I dropped / reloaded the database from a backup, restarted mysqld and apache, and flushed the cache or restarted memcache as applicable. Page caching, block caching, and JS and CSS aggregation were all enabled during all tests. I manually loaded the front page 1 time before each test.
The actual testing was done with jMeter (all tests were developed thanks to the Examiner/NowPublic). I simultaneously ran 6 different tests at the same time to try and generate more realistic traffic: 1) anonymous spiders of the entire website, 2) anonymous visitors favoring the front page and spidering links from the front page, 3) logged in visitors favoring the front page and spidering links from the front page, 4) logged in user posting nodes, 5) logged in user posting comments, and 6) anonymous user creating user accounts. Test 6 is currently adding comments to the same node repeatedly. All tests are run together for 5 minutes, and then all are automatically stopped and the data is collected.
(I also tested the same website with Plain Drupal 7 + memcache to confirm that I saw a significant speedup.)
--
Test 1: Plain Drupal 7
Test 2: Drupal 7 + entity caching patch
#62
With that schema bug patched, here's more realistic results for HEAD in the 50 comments case. That's more like a 2% improvement which is closer to what I was expecting.
Concurrency Level: 1
Time taken for tests: 194.905 seconds
Complete requests: 1000
Failed requests: 0
Write errors: 0
Total transferred: 124642000 bytes
HTML transferred: 124114000 bytes
Requests per second: 5.13 [#/sec] (mean)
Time per request: 194.905 [ms] (mean)
Time per request: 194.905 [ms] (mean, across all concurrent requests)
Transfer rate: 624.51 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 178 195 13.4 191 285
Waiting: 170 186 13.0 182 272
Total: 178 195 13.4 191 285
Patch:
Concurrency Level: 1
Time taken for tests: 192.491 seconds
Complete requests: 1000
Failed requests: 0
Write errors: 0
Total transferred: 124642000 bytes
HTML transferred: 124114000 bytes
Requests per second: 5.20 [#/sec] (mean)
Time per request: 192.491 [ms] (mean)
Time per request: 192.491 [ms] (mean, across all concurrent requests)
Transfer rate: 632.34 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 179 192 12.2 189 311
Waiting: 170 183 11.7 180 301
Total: 179 192 12.2 189 311
Even more fun, it turns out that bug is largely responsible for the speedup with just a single node as well - basically, if something stupid is happening during node_load(), and you cache node_load(), then you ameliorate the effects of the stupidity. But at least with database caching, the actual caching itself isn't buying us all that much in itself. This might well change with an alternate caching backend, or with more going on in hook_node_load(), but not as exciting :(
#63
Crossposted with Jeremy. Yeah this patch allowed us to mask #667098: drupal_get_bootstrap_phase() is broken - which happens on pretty much any situation after cache clear, except for visiting the front page, and was causing at least a 15ms slowdown (easily enough to account for 20% of the page request). Results seemed a bit too good to be true - node caching was more like 10% a few months back, but took a few long looks at the devel query log to figure out what was going on. Bumping this down to normal and CNW :(
#64
Re-test of from comment #2403194 was requested by @user.
#65
catch: Do I need to do anything else besides enabling Page Caching to activate the entitity cache? Or shouldn't I activate page caching? I'm still a bit confused. :)
#66
The last submitted patch, , failed testing.
#67
Entity caching will only make any difference with the page cache disabled - the page cache bypasses just about everything. Generally when benchmarking core, unless you're making a change to very low-level bootstrap or page caching itself, you should never enable page caching.
Also until the bootstrap issue is solved, you'll need to visit the front page before benchmarking to allow caches to rebuild properly.
#68
catch: want me to work on #53? don't want to duplicate work if you're already on it.
#70
Can this be assumed to be D8 work now?
#71
Yeah we don't get real gains with out of the box Drupal, so that leaves sites with lots of hook_node_load() implementation, or using non-SQL caching, which is contrib for D7 in http://drupal.org/project/entitycache, and we can see how that goes for inclusion for D8.
#72
I'm really sorry to move this back down a day before alpha, but I just realised that entitycache has no clean way to override the base class. I previously thought this was my lack of OOP-fu, but a conversation with Damien indicated it's actually not possible to do nicely in PHP. A quick patch to make entity.inc swappable is a no-go since we have registry breakage if you try to do that as far as I know. So if we want to properly support caching in contrib, core has to support caching even if it doesn't use it.
This patch is exactly the same as the previous patches, except it doesn't enable caching for any core modules. The only thing entity cache would then do from contrib is hook_entity_info_alter() and create cache bins, plus probably a couple of hook implementations to clear translation caches since those changes are specific to having node caching enabled.
#73
Fixed one missed find/replace and what looks to be an uncaught test failure in node.test - in that it shouldn't be passing in HEAD either.
#74
The last submitted patch, entity_cache_api_changes_only.patch, failed testing.
#75
HEAD moving fast today.
#76
The last submitted patch, entity_cache_api_changes_only.patch, failed testing.
#77
Not that fast, uploaded the same patch :(
#78
The last submitted patch, entity_cache_api_changes_only.patch, failed testing.
#79
#80
Looking at all this, apart from #611752: taxonomy_field_validate calls taxonomy_allowed_values and #684202: Entity insert/delete/update hooks are missing, we can make do for now, back to D8.
#81
mostly looks good - but I'd like some minor clarification of the variable/key names.
E.g. 'field cache' doesn't suggest to me that it's a Boolean unlike the 'cacheable' it replaced. Maybe 'field caching' ?
#82
cross-post
#83
D8 is open for business. Folks think we should move this to core?
#84
#85
fwiw:
- http://drupal.org/project/entitycache has 183 users at the moment (not loads, but it's also increasing each week).
- Most core entity tests pass with it enabled, in the project there's a script to generate the tests which also documents each one that fails and related core issues.
- Last time we benchmarked this, it didn't make much difference, at least with database caching. That might be worth revisiting though.
#86
also fwiw:
I use the Pressflow entity cache patch (https://code.launchpad.net/~catch-drupal/pressflow/load_cache/+merge/39241) on a fairly large (75k users, 100k nodes) D6 site, with great results. Probably more dramatic in D6 since D6 lacks static caching.
The real benefit is for complex custom sites, with expensive hook_user_load/hook_node_load() implementations which call many queries. For them, entity caching is a huge bonus. It seems like having full support in core for this is better in the long run than having half-core half-contrib like we do now :)
#87