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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pounard’s picture

Status: Active » Needs review
FileSize
741 bytes
pounard’s picture

I can't edit tags, but this needs the "Performance" tag.

pounard’s picture

Issue tags: +Performance

Please, someone, review this.

pounard’s picture

Patch is still the same, just removing the outdated comment.

pounard’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs backport to D7

I 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).

pounard’s picture

pounard’s picture

Status: Needs review » Reviewed & tested by the community

Switching back to RTBC to get some attention.

catch’s picture

The 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.

pounard’s picture

I 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.

pounard’s picture

Both 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.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review

Yeah I think this is fine as is. Committed/pushed to 8.x, moving to 7.x for backport.

pounard’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
837 bytes

Thanks a lot! D7 patch is the same, and #4 still apply. I'm providing a cleaner patch that does not cause hunk #... messages.

pounard’s picture

Drupal 7 is not dead yet, this patch is ready.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.23 release notes
pounard’s picture

Thank you once again! I'm happy to see Drupal 7 movement again!

Automatically closed -- issue fixed for 2 weeks with no activity.