Problem/Motivation

Everytime the frontpage is changed on our site, a new revision is created. As a "living node" you can imagine that overtime, the revision history for this node is going to get large. Its already at 175 revisions and diff module is currently bloating out page loads by 300+ seconds.

Take a look at this callgraph slice
XHPROF Callgraph slice

As DiffEntityComparison::summary recurses the number of function calls from it goes exponential. The more entity revision diff possibilities it seems creates huge amounts of overhead.

At first glance, it would seem that the Diff module should have some gates or strategies in place to deal with diff summarisation at scale.

CommentFileSizeAuthor
#10 interdiff-8-10.txt2.96 KBmarcoscano
#10 2880936-10.patch6.29 KBmarcoscano
#8 interdiff-6-8.txt609 bytesmarcoscano
#8 2880936-8.patch4.67 KBmarcoscano
#6 2880936-6.patch4.57 KBmarcoscano
callgraph_php__6079×6996_.png196.62 KBJosh Waihi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Josh Waihi created an issue. See original summary.

Josh Waihi’s picture

Josh Waihi CreditAttribution: Josh Waihi at Acquia commented
Issue summary: View changes
johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
CreditAttribution: johnchque commented
Version: 8.x-1.0-rc1 » 8.x-1.x-dev

Hmm seems like a big problem, can you help us providing some tests? (Bug reports should always be committed with tests) so we can fix it. :)
Bug reports go always against dev.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented

See also the related issue:

Field names was a theoretical idea that should help with understanding a diff.
However, in reality it just tells "Body" over and over again - or cryptic Paragraphs items "Content13 > Text"

So yeah, the first step is really completely remove the autogenerated summary before we can then discuss how a better autogenerated summary should look and generate it for storage on revision save.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Title: Diff overhead on revisions gets exponential » Remove runtime generated summary: Diff overhead on revisions gets exponential
Priority: Normal » Critical

After the removal, rest in follow-ups.
Critical because the revision tab gets dysfunctional on some sites.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
CreditAttribution: marcoscano at MD Systems GmbH commented
Status: Active » Needs review
FileSize
2880936-6.patch4.57 KB

Quick removal-only patch :)

Didn't run tests locally, let's see what the testbot has to say.

Status: Needs review » Needs work

The last submitted patch, 6: 2880936-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
CreditAttribution: marcoscano at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
2880936-8.patch4.67 KB
interdiff-6-8.txt609 bytes
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work 
  1. +++ b/src/Tests/DiffRevisionContentModerationTest.php
@@ -91,10 +91,10 @@ class DiffRevisionContentModerationTest extends DiffRevisionTest {
     // Verify proper moderation states are displayed.
     $diff_rows = $this->xpath('//tbody/tr/td[1]/p');
-    $this->assertEqual('Changes on: Title (Draft)', (string) $diff_rows[0]);
-    $this->assertEqual('No changes. (Published)', (string) $diff_rows[1]);
-    $this->assertEqual('Changes on: Title (Draft)', (string) $diff_rows[2]);
-    $this->assertEqual('Initial revision. (Draft)', (string) $diff_rows[3]);
+    $this->assertEqual(' (Draft)', (string) $diff_rows[0]);
+    $this->assertEqual(' (Published)', (string) $diff_rows[1]);
+    $this->assertEqual(' (Draft)', (string) $diff_rows[2]);
+    $this->assertEqual(' (Draft)', (string) $diff_rows[3]);
   }

    this looks a bit strange. Maybe we could manually enter revision log messages into those revisions, so they're a bit easier to identify? 

  2. +++ b/src/Tests/DiffRevisionTest.php
@@ -246,7 +245,6 @@ class DiffRevisionTest extends DiffTestBase {
     // Assert the revision summary.
-    $this->assertNoUniqueText('Changes on: Title, Body');
     $this->assertText('Copy of the revision from');

    wondering what this was testing exactly with the no unique, possibyl that the revision log message is copied or that there is more than one such revision.. could we restore that coverage with a manual revision log message as well?

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
CreditAttribution: marcoscano at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
2880936-10.patch6.29 KB
interdiff-8-10.txt2.96 KB

Thanks for reviewing!

Let's see what the testbot says about this.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Yeah, lets do this, and then add something back later that performs better.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented
Status: Reviewed & tested by the community » Fixed

Committed, with an extra @todo comment.

At least it's fast again and there's room now for a new and better approach.

Created follow-up to readd the summary.
#2972177: generate a diff summary

Status: Fixed » Closed (fixed)

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