[Performance] Implement render_cache module for comments

Sure, can I get read-only access to any dev instance plus XHProf to check which code path is used to show comments.

If its just the normal comment_view_multiple the tasks will be:

- Install and enabled entity, render_cache (once new release is out - due today), entity_modified modules
- Also enable render_cache_comments
- Add to a custom module like dorg_performance something like:

/**
 * Implements hook_render_cache_entity_default_cache_info_alter().
 */
function drupalorg_render_cache_entity_default_cache_info_alter(&$cache_info, $context) {
  // Disable entity_view() render caching by default to avoid side effects.
  $cache_info['granularity'] = DRUPAL_NO_CACHE;
}

/**
 * Implements hook_render_cache_entity_cache_info_alter().
 */
function drupalorg_render_cache_entity_cache_info_alter(&$cache_info, $entity, $context) {
  // Allow to disable render caching via variable.
  if (!variable_get('drupalorg_render_cache_enabled', TRUE)) {
    return;
  }

  // Always render_to_markup if possible.
  // Yes, please save rendered data.
  $cache_info['render_cache_render_to_markup'] = TRUE;

  // Check if render caching should be enabled for comments.
  if (variable_get('drupalorg_render_cache_comment_enabled', TRUE)
      && $context['entity_type'] == 'comment'
      && $context['view_mode'] == 'full') {
    $cache_info['granularity'] = DRUPAL_CACHE_PER_ROLE;
    // Store that this needs special comment processing.
    $cache_info['drupalorg_comment_processing'] = TRUE;
  }
}

/**
 * Implements hook_render_cache_entity_hash_alter().
 */
function drupalorg_render_cache_entity_hash_alter(&$hash, $entity, $cache_info, $context) {
  if (!empty($cache_info['drupalorg_comment_processing'])) {
    // Comment is displaying users, so need to add this to the hash, entity is already added.
    $hash['comment_uid'] = $entity->uid;
    $account = user_load($entity->uid);
    if (!empty($account->uid)) {
      $hash['comment_uid_modified'] = entity_modified_last('user', $account);
    }

    $hash['is_viewer'] = $entity->uid == $GLOBALS['user']->uid;

    $node = node_load($entity->nid);
    $hash['is_author'] = $entity->uid == $node->uid;
  }
}

And that should theoretically just work without knowing much about the internals.

I'll probably release a stable version of render_cache first (has been in production since almost a year now) and apply the RTBC patch for render_cache_comments (which I wrote), but this can be used to start playing around with this.

Memcache is not absolutely needed as cache_render bin is created by render_cache module, but DB could get full quite soon as there is no LRU obviously on DB Cache Backend ...

Though a drush cc all fixes that, its probably not a problem.

Also I suggest to keep the variables to disable at run-time and for dev-instances.

Yes, I I can still login to devwww.

The 1.0 version of render_cache and 1.1 version of entity_modified have been released.

I will now make a patch to drupalorg module and then all we need is testing this and this should be good to go ...

Adding an obviously untested patch for drupalorg module.

And a new patch, which also has update hooks and _dependencies() in the .install (if I got this right). Also adds to .info file.

Okay, cool. Thanks for the explanation.

Keep the change to the .info though?

And a new patch with suggested changes that I am gonna try on my dev instance now.

TL;DR: 30% performance improvement on a page with just 44 comments ...

I have to do two changes to entity_modified and drupalorg to skip a rather extensive, but unnecessary user_load(), but besides that this is finished it seems:

Full Page

Just the callback showing the comments

Interdiff

diff --git a/drupalorg/drupalorg.module b/drupalorg/drupalorg.module
index cc78da1..f70c94d 100755
--- a/drupalorg/drupalorg.module
+++ b/drupalorg/drupalorg.module
@@ -2857,9 +2857,8 @@ function drupalorg_render_cache_entity_hash_alter(&$hash, $entity, $cache_info,
   if (!empty($cache_info['drupalorg_comment_processing'])) {
     // Comment is displaying users, so need to add this to the hash, entity is already added.
     $hash['comment_uid'] = $entity->uid;
-    $account = user_load($entity->uid);
-    if (!empty($account->uid)) {
-      $hash['comment_uid_modified'] = entity_modified_last('user', $account);
+    if (!empty($entity->uid)) {
+      $hash['comment_uid_modified'] = entity_modified_last_id('user', $entity->uid);
     }
 
     $hash['is_viewer'] = $entity->uid == $GLOBALS['user']->uid;

Likely to get the new release for entity_modified out today with that little change ...

CNW, I forgot the new_first and new flags that are set within classes and different markup.

Okay, implemented is_new, first_new, in_preview and divs and divs_final properties.

I verified NEW functionality works perfectly now again.

Without whitespace change...

On special request by webchick two screenshots of issues with 220 and 146 comments:

node/15365 - threaded; handbook

74% saved time

node/2280195 - issue

50% saved time

I did some more testing and I still think this is ready. After more investigation I think I'll be able to remove the @todos ...

@drumm: Any chance you'll be able to review that?

I will open a new issue to track the new render_cache-7.x-2.x stuff that should theoretically allow advanced node, block and page caching - similar to Drupal 8 render_cache. This is a huge WIP still, so this will continue to serve as the stable version here, but putting that here, because it means my sandbox could be unstable and is not usable for testing the change once I deploy the experimental features test wise.

#24: Thanks for the review!

- Yes, I will disable comment caching if the preview flag is set.

- I think for nested discussions the divs can change some, but on the other hand it is a valid variation and the speed ups are very nice there, too. I guess we will just need to monitor memcache cache growth to see the whole impact of this feature. I'll add a kill switch for nested divs - just in case.

Yes, this is fixed! Thanks so much!

The preview is never needed to be added to the hash as its displayed via comment_view() instead of entity_view(), which means that it is not run through the caching function.