On a quite big Commerce based project, a team was doing massive stress test using different connection scenarios, a bit of randomisation, hundreds of concurrent connections on a production-equivalent environment. During those measures, we all noticed that POST requests where doing quite bad.
Doing some unit page profiling, I figured out that module_implements_write_cache()
is silent for every other request that HEAD or GET.
We can see this in this piece of code:
function module_implements_write_cache() {
$implementations = &drupal_static('module_implements');
// Check whether we need to write the cache. We do not want to cache hooks
// which are only invoked on HTTP POST requests since these do not need to be
// optimized as tightly, and not doing so keeps the cache entry smaller.
if (isset($implementations['#write_cache']) && ($_SERVER['REQUEST_METHOD'] == 'GET' || $_SERVER['REQUEST_METHOD'] == 'HEAD')) {
unset($implementations['#write_cache']);
cache_set('module_implements', $implementations, 'cache_bootstrap');
}
}
I removed the $_SERVER
test and I actually noticed an improvement, especially with Rules and Commerce, because the later relies on the former, and both are consuming really A LOT of hooks. This especially during POST requests.
If we trash the module implements cache on every request, the overhead is noticable on the result and in my case, the site was less performant all scenarios included.
The comment
// Check whether we need to write the cache. We do not want to cache hooks
// which are only invoked on HTTP POST requests since these do not need to be
// optimized as tightly, and not doing so keeps the cache entry smaller.
Is definitely untrue, in my use case at least. Drupal being a CMS, its main use case is splitting out cached pages, and when doing this, this cache is not involved (or has an unsignificant impact even if loaded fully) while during POST requests this cache is mandatory especially in high traffic scenarios. Without POST are slower, not much, but enough to bring down the maximum site capacity. All other use case are authenticated user browse, but I think those use case have way more problems than just loading a few K more from this cache entry (not even sure this cache will grow that much in the end).
Is there any other reason why this was done? Can anyone find the reason back in the issue queues or can remember why? I did some research and found #557542: Cache module_implements() but I must have missed if there is any real reason for not caching this.
Comment | File | Size | Author |
---|---|---|---|
#13 | 1860876-13-module_implements-cache_in_post.patch | 837 bytes | pounard |
#7 | 1860876-7-d8-module_implements-cache_in_post.patch | 1.05 KB | pounard |
#4 | 1860876-4-module_implements-cache_in_post.patch | 837 bytes | pounard |
#1 | 1860876-1-module_implements-cache_in_post.patch | 741 bytes | pounard |
Comments
Comment #1
pounardComment #2
pounardI can't edit tags, but this needs the "Performance" tag.
Comment #3
pounardPlease, someone, review this.
Comment #4
pounardPatch is still the same, just removing the outdated comment.
Comment #5
pounardComment #6
David_Rothstein CreditAttribution: David_Rothstein commentedI agree this looks suspicious. It seems that the main reason it does this is that the code in #557542: Cache module_implements() originally called drupal_page_is_cacheable() here, but then they realized that was wrong and removed it but left behind some of the checks that are in drupal_page_is_cacheable() instead... but it's not clear why.
However, this definitely needs other reviewers, and the code is in Drupal 8 too (see https://api.drupal.org/api/drupal/core!lib!Drupal!Core!EventSubscriber!RequestCloseSubscriber.php/function/RequestCloseSubscriber::onTerminate/8).
Comment #7
pounardPatch for D8.
Comment #8
pounardSwitching back to RTBC to get some attention.
Comment #9
catchThe schema cache caches based on request method, the idea being that lots of tables use drupal_write_record() but less drupal_schema_fields_sql(). That's changing the cache key though rather than not caching at all. Wondering a bit if that approach might make sense here since the hooks will likely be quite different, but would also be fine with the patch as is.
Comment #10
pounardI see your point, but in this case I'm not sure the cache entry size would grow as much as the schema cache. I'm not opposed to such caching policy, but I don't have time to write this kind of patch right now. I'm also fine with the current patch.
I'd like to get it in even if we choose to move on on the per page/method caching: this patch will be longer to achieve, harder to review, may trigger some bikeshed, while the current patch is very simple, straight-forward, and will provide a performance boost for all sites that have a lot of posts. I'm not opposed RTBC'ing this patch then discuss a more complex idea in a follow-up issue.
Comment #11
pounardBoth D8 and D7 patches are working and can be commited. I don't care myself about D8 which is too much a moving target and may break again and again this, but Drupal 7 patch seems important and is pending since a long time now.
Comment #12
catchYeah I think this is fine as is. Committed/pushed to 8.x, moving to 7.x for backport.
Comment #13
pounardThanks a lot! D7 patch is the same, and #4 still apply. I'm providing a cleaner patch that does not cause hunk #... messages.
Comment #14
pounardDrupal 7 is not dead yet, this patch is ready.
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/cadd940
Comment #16
pounardThank you once again! I'm happy to see Drupal 7 movement again!