Updated: Comment #7

Problem/Motivation

When using threaded comments, template_preprocess_comment() in comment.module loads the parent comment when rendering every comment in the thread.This duplicates a lot of the preparation of the comment variables that's already been done when rendering the actual parent comment, assuming it's done on the same page. That's likely to be a noticeable regression when you have a lot of threaded comments on a page, for example on groups.drupal.org. We should look at finding way to re-use the variables created for the parent comment I think.

Proposed resolution

  • create a repeatable test case (generating a number of threaded comments)
  • provide an initial xhprof profiling run for benchmarking
  • look at finding way to re-use the variables created for rendering the parent comment
  • review the variables which get created in this preprocess and remove any which are not used in the template

Remaining tasks

Everything.

Original report by catch

Follow-up from [#1272870:79]

+  if ($comment->pid > 0) {
+    // Fetch and store the parent comment information for use in templates.
+    $comment_parent = comment_load($comment->pid);
+    $variables['parent_comment'] = $comment_parent;
+    $variables['parent_author'] = theme('username', array('account' => $comment_parent));
+    $variables['parent_created'] = format_date($comment_parent->created);
+    $variables['parent_changed'] = format_date($comment_parent->changed);
+    $uri_parent = $comment_parent->uri();
+    $uri_parent['options'] += array('attributes' => array('class' => 'permalink', 'rel' => 'bookmark'));
+    $variables['parent_title'] = l($comment_parent->subject, $uri_parent['path'], $uri_parent['options']);
+    $variables['parent_permalink'] = l(t('Parent permalink'), $uri_parent['path'], $uri_parent['options']);
+    $variables['parent'] = t('In reply to !parent_title by !parent_username',
+     

This means that if you're using threaded comments, then we're going to duplicate a lot of the preparation of the comment variables (format_date() twice, theme_username() for example), that's already been done when rendering the actual parent comment, assuming it's done on the same page. That's likely to be a noticeable regression when you have a lot of threaded comments on a page, for example on groups.drupal.org. We should look at finding way to re-use the variables created for the parent comment I think.

Also there are variables created here that never get used? Can we just remove those please?

Comments

fago’s picture

We are not only duplicating the parent template variables, we are loading the parents twice also. If the parent is rendered on the same page, it is still loaded again when rendering its children as comments have static entity load caches turned off.

mgifford’s picture

This can be fixed after feature freeze I assume.

Berdir’s picture

The loading problem is being fixed in #1844146: comment_load() call in template_preprocess_comment() re-loads comment parent entities because of disabled static cache.

This is different. What @catch means is that for example if you have a parent comment with 100 sub-comments, then we are calling "format_date($comment_parent->created)" over and over again for the parent comment for every comment below it.

dcam’s picture

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

Gábor Hojtsy’s picture

How is this a major performance problem? (vs. a normal one).

catch’s picture

Priority: Major » Normal

It was major with the double loading of comments (actually more like critical), but I think it's probably 'normal' now. There's situations with threaded comments where it'll still be a significant regression but that requires threaded comments with lots of replies to the same comment.

tsphethean’s picture

Updated issue summary, needs profiling.

tsphethean’s picture

Issue summary: View changes

Updated issue summary.

tsphethean’s picture

Issue summary: View changes

Updated issue summary.

tsphethean’s picture

I was taking a look at this and was thinking we could solve it with a static variable containing all the parent entities loaded in the page request, and checking for it in the static before loading it - does that sound a sensible way forward or do we need to be looking at something higher up the call stack?

tsphethean’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

andypost’s picture

Status: Active » Needs review
Issue tags: +accessibility
FileSize
3.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,917 pass(es). View

1) Comment::hasParentComment() should not load parent entity.
2) Suppose removing "parent" is not possible without breaking following "accessibility":

    {#
      Indicate the semantic relationship between parent and child comments
      for accessibility. The list is difficult to navigate in a screen
      reader without this information.
    #}
    {% if parent %}
      <p class="parent visually-hidden">{{ parent }}</p>
    {% endif %}

If we get rid of this line then parent entity could not be loaded,
but the same time when thread of comments is rendering parent comment already loaded so fine.

At least better to minimize a defaults for "parent"

mgifford’s picture

Is there another way to do this? What if the link is just to the parent, without actually defining the parent title?

It would be important to have some semantic relationship, but maybe we could do a simpler one so that comment template variables aren't built twice...

mgifford’s picture

Status: Needs review » Needs work

doesn't apply.

mgifford’s picture

Status: Needs work » Needs review
FileSize
3.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,036 pass(es). View
andypost’s picture

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -316,7 +316,8 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
   public function hasParentComment() {
-    $parent = $this->get('pid')->entity;
+    // Do not load parent comment entity.
+    $parent = $this->get('pid')->target_id;
     return !empty($parent);

At least this part should be commited

mgifford’s picture

hasParentComment() looks fairly straight forward, but then so do the changes to comment.html.twig.

Would it help if I rolled a patch just for everything but the changes to comment.module?

andypost’s picture

Sure!
I just think that better to limit variables related to parent comment.
If contrib will need more vars it's easy to add them and anyway will need to change template

andypost’s picture

That means:
1) there's comment_parent: Full parent comment entity (if any). variable already
2) a12y needs "parent" (last patch forgets to translate this
3) templates can access to parent comment properties via comment_parent.subject so no reason to prepare so much useless variables.

mgifford’s picture

FileSize
1.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,611 pass(es). View
76.35 KB

I don't think it needs more variables, but I'm pretty certain that the inline docs in comment.html.twig were wrong.

No new variables have been added in this patch. Thanks for pointing out the missing internationalization.

I didn't touch on anything by comment_parent.subject, but right now it seems to be working.

Screenshot of child comment.

andypost’s picture

FileSize
2.72 KB
3.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,596 pass(es). View

Mike, great! now we should remove the same string from preprocess.
Also s/entity/commented_entity - fix wrong name in template

mgifford’s picture

Status: Needs review » Needs work

I don't know why this seems to be happening, but I'm not seeing <p class="comment-parent visually-hidden">...</p> in #19 but do in #18.

I'm not sure why this would be but I spun up to instances to compare and they both had the same issue. Why would parent_comment be true in one and not the other?

EDIT: Example (for the next 3 hrs) - http://sa5dd4e9d2ca61c1.s2.simplytest.me/node/1

andypost’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
6.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,240 pass(es). View

@mgifford You was right! I missed to change bartik's template

dawehner’s picture

Great fixes here!

+++ b/core/modules/comment/templates/comment.html.twig
@@ -90,9 +85,12 @@
+      <p class="parent visually-hidden">{{ "In reply to !parent_title by !parent_author"|t({ '!parent_title': parent_title, '!parent_author': parent_author }) }}</p>

+++ b/core/themes/bartik/templates/comment.html.twig
@@ -79,13 +74,16 @@
+          {{ "In reply to !parent_title by !parent_author"|t({ '!parent_title': parent_title, '!parent_author': parent_author }) }}

We afaik use single quotes also for strings in templates

andypost’s picture

FileSize
1.35 KB
38.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,238 pass(es). View

sure, now that needs profiling, suppose the main difference would be caused by

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -324,7 +324,8 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
   public function hasParentComment() {
-    $parent = $this->get('pid')->entity;
+    // Do not load parent comment entity.
+    $parent = $this->get('pid')->target_id;
     return !empty($parent);

that code

Berdir’s picture

I actually just opened #2328629: Comment::hasParentComment() tries to load comment with ID 0 today, with a similar change for that method.

andypost’s picture

FileSize
560 bytes
6.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,241 pass(es). View

properly merged head and incorporate #24

andypost’s picture

FileSize
31.66 KB

Profiling of node with 50 threaded comments

andypost’s picture

FileSize
6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1857946-comment-parent-27.patch. Unable to apply patch. See the log in the details link for more information. View

re-roll

andypost’s picture

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Seems to work fine in my testing on SimplyTest.me

I created environment, added a node, then proceeded to add comments and child comments.

The code comments look like good additions too. Think this is ready for Core.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

I'm not sure that the solution fully addresses @catch's original concerns about calling format_date and drupal_render($username) multiple times on the same parent comment.

andypost’s picture

the best we can do is to remove 'parent_author' from prepare, but this causes an a11y regression

andypost’s picture

duplicated comment

mgifford’s picture

Could we just add a static variable so that format_date and drupal_render($username) are stored rather than re-generated for comments?

catch’s picture

What about removing all the variables that aren't used from the preprocess?

andypost’s picture

@catch they are all used, see #16-17

andypost’s picture

+++ b/core/modules/comment/templates/comment.html.twig
@@ -43,21 +43,16 @@
- * - comment_parent: Full parent comment entity (if any).
+ * - parent_comment: Full parent comment entity (if any).

Has separate issue #2010196: Rename comment parent theme variable

Status: Needs review » Needs work

The last submitted patch, 27: 1857946-comment-parent-27.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 27: 1857946-comment-parent-27.patch, failed testing.

mgifford’s picture

Issue tags: +Needs reroll
rpayanm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,697 pass(es), 6 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 42: 1857946-42.patch, failed testing.

ashwinikumar’s picture

Assigned: Unassigned » ashwinikumar
ashwinikumar’s picture

Assigned: ashwinikumar » Unassigned
Status: Needs work » Needs review
FileSize
4.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1857946-comment-parent-45_0.patch. Unable to apply patch. See the log in the details link for more information. View

Rerolled patch with correction of comment indentation.

Status: Needs review » Needs work

The last submitted patch, 45: 1857946-comment-parent-45.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 45: 1857946-comment-parent-45.patch, failed testing.

gauravjeet’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2015
FileSize
4.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,779 pass(es), 6 fail(s), and 0 exception(s). View

I removed a major chunk of code from the patch, and found the comments to be working fine for me. There is proper indentation for child comments and the parent comment seems to be fine. No need for this code in the comment.module file.
Please reply, if I'm on the wrong track

Status: Needs review » Needs work

The last submitted patch, 49: comment_parent_template-1857946-49.patch.patch, failed testing.

ashwinikumar’s picture

FileSize
5.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,790 pass(es). View
ashwinikumar’s picture

Status: Needs work » Needs review
andypost’s picture

Status: Needs review » Reviewed & tested by the community

Suppose it ready

rahul.shinde’s picture

Issue tags: +#drupalgoa2015
Cottser’s picture

Status: Reviewed & tested by the community » Needs work

On my phone but I am pretty sure this misses updating Classy's comment template.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
7.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,786 pass(es). View
2.16 KB
andypost’s picture

Status: Needs review » Needs work
Related issues: +#2031883: Markup for: comment.html.twig

@Cottser please take a look at related #2031883: Markup for: comment.html.twig

+++ b/core/themes/classy/templates/content/comment.html.twig
@@ -98,12 +93,14 @@
+      <p class="parent visually-hidden">{{ 'In reply to !parent_title by
+		!parent_author'|t({ '!parent_title': parent_title, '!parent_author':
+        parent_author }) }}</p>

indent problem (seems tab)

rpayanm’s picture

Status: Needs work » Needs review
FileSize
608 bytes
7.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1857946-58.patch. Unable to apply patch. See the log in the details link for more information. View
andypost’s picture

Status: Needs review » Reviewed & tested by the community
Related issues: +#2318579: Remove comment_prepare_thread()

I'm going to organize all this render/performance issues in one meta.
Comments are improved in #2031883-34: Markup for: comment.html.twig
Also bigger performance gain in #2318579-22: Remove comment_prepare_thread()

RTBC, because this is a most independent issue

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 58: 1857946-58.patch, failed testing.

andypost’s picture

Issue tags: +Needs reroll
kerby70’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
7.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,150 pass(es), 1 fail(s), and 0 exception(s). View

Reroll attached, just 1 fuzz and 2 line shift, no real changes.

Status: Needs review » Needs work

The last submitted patch, 62: comment_parent_template-1857946-62.patch, failed testing.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
7.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,166 pass(es). View
andypost’s picture

Status: Needs review » Reviewed & tested by the community

great!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

So the patch and the issue summary and title seem completely at odds - what am I missing?

andypost’s picture

Issue tags: +Twig, +frontend

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.