Cache node types

Subscribing

Subscribing, have wanted to do this for a while.

Needs benchmarks to justify complexity of a new (static) cache

I'm sick at the moment so not going to do benchmarks (also try typing benchmarks? in irc), but here's my justification for patches like this.

On ex2 we have a load testing framework with jmeter which attempts to simulate realistic site traffic. There are up to 400 threads, hitting a combination of various different paths as anonymous and/or authenticated users, with a very widespread distribution of paths (I'd estimate 90% of the paths hit are unique to each load test).

I'm attaching mysqlsla output from a load test where most MySQL queries had already been removed (via mongodb, entitycache, pathcache, various tweaks in performance_hacks.module). The slowest queries in aggregate are cache misses for entitycache, pathcache and menu caching - due to a wide spread of page views these have a very low cache hit rate on the load test (i.e. if 270k unique paths are visited out of 300k, then the path alias lookup, menu router lookup, nodes and taxonomy terms displayed etc. are nearly all misses, this is more or less a worst case cold-start situation and more extreme the more content you have).

Apart from that, it's user permissions and node types which are up at the top - with 386k queries and 291k queries respectively on this load test run - these are exactly the same query regardless of which page they show up on. The total queries logged during that load test was 4.1m, combine the caching of those two and you can shave 650k off that, just the node types query is 7% of the time spent in MySQL.

This is a configuration level query which needs to be up-to-date, there's no option to pass this off to a read-only MySQL slave due to replication lag, so in any situation where MySQL is the bottleneck (which is nearly any setup, from a single server to multiple servers) passing this off to memcache is about the only option.

While I know Moshe and Dries don't particularly like this pattern, we don't have any nice way to do things like this from contrib (apart from hacking core).

For D8 we're going to need some kind of pluggable storage for configuration (and entity base tables) as well as fields, so you can use different storage outright like mongo, or mysql + memcache, and more unified cache invalidation so that modules implementing caching from contrib don't need to figure out every hook (and for node types likely form submissions that don't always fire hooks) just to clear the caches correctly. We can't do any of that for D7 though, so for quick wins like this I think this is (much) better than not doing it at all.

For the default SQL caching backend, a patch like this is basically a no-op - it's going to have an extremely high hit rate and get cleared very rarely, some other caches are a bit more borderline but it shoudnl't do any harm here. And yeah we get to skip some other processing here which is usually worth doing.

edited to add: patch looks fine to me as well.

user_role_permissions() issue is here by the way #684612: Add caching for user_role_permission().

Moving this back to 7. On aggregate this is 7% of time spent in MySQL, there's no way to remove it without a core hack.

Agreed this is important.

But... this breaks the translatability of the node type titles and descriptions. We must add the current interface language to the cache key here.

The cache_clear_all() needs to account for the multiple keys - either wildcard, or clear for each language.

Well... if I didn't mess up my patch, which is quite possible (still learning...), this should fix the 1,008 exceptions.

This looks like a straight-forward performance improvement. But the last time I committed one of those, Moshe bit my face off. ;)

And I see that although this is tagged "needs benchmarks", there aren't any in the issue. Could we take care of that, please?

http://drupal.org/node/898360#comment-3472754

WOW. I need to learn how to read. Benchmarks are in #14, duh.

In that case, I think this is fine, but I'll leave it for 24 hours to give Moshe/Dries a chance to respond.

restoring status.

Those aren't benchmarks, but it's a slow query log from load tests generated by jmeter, which I think explains the motivation for this patch a lot better than benchmarks (which are going to be unexciting, because as Moshe points out regularly, on one page it's just one well indexed query).

The short version is that in aggregate on a well optimized site, these queries run so frequently that they end up taking anywhere between 5-10% of all time taken in MySQL overall. From that query log you can see that nearly all the other queries (that don't have similar issues elsewhere in the queue) are entitycache or menu system cache misses. The ratio of queries which vary on every request and are on tables where the MySQL cache gets invalidated frequently (node or comment posting) is such a high percentage (in the load tests, don't know about live traffic), that Narayan disabled the query cache on ex2 since it was more overhead than it was saving - which means the queries that actually are frequent don't get used for it either.

On a 'normal' site with db-caching, this will be a no-op, or very close to a no-op, it might not do harm to see if there's any measurable change either way, but I doubt it's noticeable. But it's going to take a lot of work off MySQL once they're using memcache as a caching backend. I won't have any time to do anything beyond typing this until at least Tuesday.

I don't like these patches too much, but I'm no longer actively trying to block them. I trust that everyone else can make the right call. I got tired of being an obstructionist.

Catch makes a pretty compelling argument for this patch on more advanced sites with memcache. And it sounds like moshe is willing to stand down on opposition to it.

I agree with sun that of all the "insert a cache into X thing" patches, this one is the most straight-forward. And better performance for D7 is always welcome.

Committed to HEAD.

If this is in fact an API change, should it be announced? If so, please summarize the implications.

Thanks!

@webchick: This wasn't actually committed. In Commit #453544 by webchick at 21:33 #963656: node_access_view_all_nodes() is never invoked was instead committed.

I'm setting back to the previous RTBC status.

D'oh. :P

REALLY committed this time. ;)

I'm still not seeing the actual commit for this.

Dear universe: please stop conspiring against me. thx.