Make mundane performance improvements to Diffusion browse views

Summary:
Ref T2450. This reorganizes code to improve performance.

Mostly, there are a lot of things which are unique per commit (author name, links, short name, etc), but we were rendering them for every line.

This often meant we'd render the same author's name thousands of times. This is slower than rendering it only once.

In 99% of interfaces this doesn't matter, but blame is weird and it's significant on big files.

Test Plan:
Locally, `__phutil_library_map__.php` now has costs of roughly:

  - 550ms for main content (from 650ms before the patch).
  - 1,500ms for blame content (frrom 1,800ms before the patch).

So this isn't huge, is a decent ~20%-ish performance gain for shuffling some stuff around.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2450

Differential Revision: https://secure.phabricator.com/D14963

src/applications/diffusion/controller/DiffusionBrowseController.php

@@ -592,15 +592,12 @@ final class DiffusionBrowseController extends DiffusionController {
 
     $viewer = $this->getViewer();
 
-    $blame_handles = array();
     if ($needs_blame) {
       $blame = $this->loadBlame($path, $drequest->getCommit());
-      if ($blame) {
+      list($blame_list, $blame_commits) = $blame;
-        $author_phids = mpull($blame, 'getAuthorPHID');
-        $blame_handles = $viewer->loadHandles($author_phids);
-      }
     } else {
-      $blame = array();
+      $blame_list = array();
+      $blame_commits = array();
     }
 
     $file_corpus = $file_content->getCorpus();
@@ -608,43 +605,11 @@ final class DiffusionBrowseController extends DiffusionController {
     $can_highlight = (strlen($file_corpus) <= $highlight_limit);
 
     if (!$show_color) {
-      $lines = phutil_split_lines($file_corpus);
+      $corpus = $this->renderPlaintextCorpus(
-
+        $file_corpus,
-      $style =
+        $blame_list,
-        'border: none; width: 100%; height: 80em; font-family: monospace';
+        $blame_commits,
-      if (!$show_blame) {
+        $show_blame);
-        $corpus = phutil_tag(
-          'textarea',
-          array(
-            'style' => $style,
-          ),
-          $file_corpus);
-      } else {
-        $rows = array();
-        foreach ($lines as $line_number => $line) {
-          $commit = idx($blame, $line_number);
-          if ($commit) {
-            $author = $commit->renderAuthorShortName($blame_handles);
-            $commit_name = $commit->getShortName();
-          } else {
-            $author = null;
-            $commit_name = null;
-          }
-
-          $rows[] = sprintf(
-            '%-10s %-20s %s',
-            $commit_name,
-            $author,
-            $line);
-        }
-
-        $corpus = phutil_tag(
-          'textarea',
-          array(
-            'style' => $style,
-          ),
-          implode('', $rows));
-      }
     } else {
       if ($can_highlight) {
         require_celerity_resource('syntax-highlighting-css');
@@ -659,7 +624,8 @@ final class DiffusionBrowseController extends DiffusionController {
 
       $rows = $this->buildDisplayRows(
         $lines,
-        $blame,
+        $blame_list,
+        $blame_commits,
         $show_blame,
         $show_color);
 
@@ -883,25 +849,116 @@ final class DiffusionBrowseController extends DiffusionController {
 
   private function buildDisplayRows(
     array $lines,
-    array $blame,
+    array $blame_list,
+    array $blame_commits,
     $show_color,
     $show_blame) {
 
+    $request = $this->getRequest();
+    $viewer = $this->getViewer();
     $drequest = $this->getDiffusionRequest();
     $repository = $drequest->getRepository();
 
-    $handles = array();
+    $revision_map = array();
-    if ($blame) {
+    $revisions = array();
-      $epoch_list = mpull($blame, 'getEpoch', 'getID');
+    if ($blame_commits) {
-      $epoch_list = array_filter($epoch_list);
+      $commit_map = mpull($blame_commits, 'getCommitIdentifier', 'getPHID');
+
+      $revision_ids = id(new DifferentialRevision())
+        ->loadIDsByCommitPHIDs(array_keys($commit_map));
+      if ($revision_ids) {
+        $revisions = id(new DifferentialRevisionQuery())
+          ->setViewer($viewer)
+          ->withIDs($revision_ids)
+          ->execute();
+        $revisions = mpull($revisions, null, 'getID');
+      }
+
+      foreach ($revision_ids as $commit_phid => $revision_id) {
+        $revision_map[$commit_map[$commit_phid]] = $revision_id;
+      }
+    }
+
+    $phids = array();
+    foreach ($blame_commits as $commit) {
+      $author_phid = $commit->getAuthorPHID();
+      if ($author_phid === null) {
+        continue;
+      }
+      $phids[$author_phid] = $author_phid;
+    }
+
+    foreach ($revisions as $revision) {
+      $author_phid = $revision->getAuthorPHID();
+      if ($author_phid === null) {
+        continue;
+      }
+      $phids[$author_phid] = $author_phid;
+    }
+
+    $handles = $viewer->loadHandles($phids);
+
+    $colors = array();
+    if ($blame_commits) {
+      $epochs = array();
+
+      foreach ($blame_commits as $identifier => $commit) {
+        $epochs[$identifier] = $commit->getEpoch();
+      }
+
+      $epoch_list = array_filter($epochs);
       $epoch_list = array_unique($epoch_list);
       $epoch_list = array_values($epoch_list);
 
       $epoch_min   = min($epoch_list);
       $epoch_max   = max($epoch_list);
       $epoch_range = ($epoch_max - $epoch_min) + 1;
+
+      foreach ($blame_commits as $identifier => $commit) {
+        $epoch = $epochs[$identifier];
+        if (!$epoch) {
+          $color = '#ffffdd'; // Warning color, missing data.
+        } else {
+          $color_ratio = ($epoch - $epoch_min) / $epoch_range;
+          $color_value = 0xE6 * (1.0 - $color_ratio);
+          $color = sprintf(
+            '#%02x%02x%02x',
+            $color_value,
+            0xF6,
+            $color_value);
         }
 
+        $colors[$identifier] = $color;
+      }
+    }
+
+    $display = array();
+    $last_identifier = null;
+    $last_color = null;
+    foreach ($lines as $line_index => $line) {
+      $color = '#f6f6f6';
+      $duplicate = false;
+      if (isset($blame_list[$line_index])) {
+        $identifier = $blame_list[$line_index];
+        if (isset($colors[$identifier])) {
+          $color = $colors[$identifier];
+        }
+
+        if ($identifier === $last_identifier) {
+          $duplicate = true;
+        } else {
+          $last_identifier = $identifier;
+        }
+      }
+
+      $display[$line_index] = array(
+        'data' => $line,
+        'target' => false,
+        'highlighted' => false,
+        'color' => $color,
+        'duplicate' => $duplicate,
+      );
+    }
 
     $line_arr = array();
     $line_str = $drequest->getLine();
@@ -921,112 +978,23 @@ final class DiffusionBrowseController extends DiffusionController {
       }
     }
 
-    $display = array();
+    // Mark the first highlighted line as the target line.
-
-    $line_number = 1;
-    $last_commit = null;
-    $color = null;
-    foreach ($lines as $line_index => $line) {
-      $display_line = array(
-        'epoch'       => null,
-        'commit'      => null,
-        'author'      => null,
-        'target'      => null,
-        'highlighted' => null,
-        'line'        => $line_number,
-        'data'        => $line,
-      );
-
-      if ($show_blame) {
-        // If the line's rev is same as the line above, show empty content
-        // with same color; otherwise generate blame info. The newer a change
-        // is, the more saturated the color.
-
-        $commit = idx($blame, $line_index, $last_commit);
-
-        if ($commit && $last_commit &&
-            ($last_commit->getID() == $commit->getID())) {
-          $display_line['color'] = $color;
-        } else {
-          if ($commit) {
-            $epoch = $commit->getEpoch();
-          } else {
-            $epoch = null;
-          }
-
-          if (!$epoch) {
-            if (!$blame) {
-              $color = '#f6f6f6';
-            } else {
-              $color = '#ffd'; // Render as warning.
-            }
-          } else {
-            $color_ratio = ($epoch - $epoch_min) / $epoch_range;
-            $color_value = 0xE6 * (1.0 - $color_ratio);
-            $color = sprintf(
-              '#%02x%02x%02x',
-              $color_value,
-              0xF6,
-              $color_value);
-          }
-
-          $display_line['epoch'] = $epoch;
-          $display_line['color'] = $color;
-
-          if ($commit) {
-            $display_line['commit'] = $commit;
-          } else {
-            $display_line['commit'] = null;
-          }
-
-          $last_commit = $commit;
-        }
-      }
-
     if ($line_arr) {
-        if ($line_number == $line_arr[0]['min']) {
+      $target_line = $line_arr[0]['min'];
-          $display_line['target'] = true;
+      if (isset($display[$target_line - 1])) {
+        $display[$target_line - 1]['target'] = true;
       }
+    }
+
+    // Mark all other highlighted lines as highlighted.
     foreach ($line_arr as $range) {
-          if ($line_number >= $range['min'] &&
+      for ($ii = $range['min']; $ii <= $range['max']; $ii++) {
-              $line_number <= $range['max']) {
+        if (isset($display[$ii - 1])) {
-            $display_line['highlighted'] = true;
+          $display[$ii - 1]['highlighted'] = true;
         }
       }
     }
 
-      $display[] = $display_line;
-      ++$line_number;
-    }
-
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
-
-    $commits = mpull($blame, null, 'getCommitIdentifier');
-
-    $revision_ids = id(new DifferentialRevision())
-      ->loadIDsByCommitPHIDs(mpull($commits, 'getPHID'));
-    $revisions = array();
-    if ($revision_ids) {
-      $revisions = id(new DifferentialRevisionQuery())
-        ->setViewer($viewer)
-        ->withIDs($revision_ids)
-        ->execute();
-    }
-
-    $phids = array();
-    foreach ($commits as $blame_commit) {
-      if ($blame_commit->getAuthorPHID()) {
-        $phids[] = $blame_commit->getAuthorPHID();
-      }
-    }
-    foreach ($revisions as $revision) {
-      if ($revision->getAuthorPHID()) {
-        $phids[] = $revision->getAuthorPHID();
-      }
-    }
-    $handles = $this->loadViewerHandles($phids);
-
     $engine = null;
     $inlines = array();
     if ($this->getRequest()->getStr('lint') !== null && $this->lintMessages) {
@@ -1059,7 +1027,7 @@ final class DiffusionBrowseController extends DiffusionController {
 
     // NOTE: We're doing this manually because rendering is otherwise
     // dominated by URI generation for very large files.
-    $line_base = (string)$repository->generateURI(
+    $line_base = (string)$drequest->generateURI(
       array(
         'action'  => 'browse',
         'stable'  => true,
@@ -1070,92 +1038,58 @@ final class DiffusionBrowseController extends DiffusionController {
     Javelin::initBehavior('phabricator-tooltips');
     Javelin::initBehavior('phabricator-line-linker');
 
-    foreach ($display as $line) {
+    // Render these once, since they tend to get repeated many times in large
-      $line_href = $line_base.'$'.$line['line'];
+    // blame outputs.
+    $commit_links = $this->renderCommitLinks($blame_commits, $handles);
+    $revision_links = $this->renderRevisionLinks($revisions, $handles);
 
-      $blame = array();
+    $skip_text = pht('Skip Past This Commit');
-      $style = null;
+    foreach ($display as $line_index => $line) {
-      if (array_key_exists('color', $line)) {
+      $row = array();
-        if ($line['color']) {
-          $style = 'background: '.$line['color'].';';
-        }
 
-        $before_link = null;
+      $line_number = $line_index + 1;
-        $commit_link = null;
+      $line_href = $line_base.'$'.$line_number;
-        $revision_link = null;
-        if (idx($line, 'commit')) {
-          $commit = $line['commit'];
 
-          if ($commit) {
+      if (isset($blame_list[$line_index])) {
-            $tooltip = $this->renderCommitTooltip(
+        $identifier = $blame_list[$line_index];
-              $commit,
-              $handles,
-              $commit->renderAuthorLink($handles));
       } else {
-            $tooltip = null;
+        $identifier = null;
       }
 
-          $commit_link = javelin_tag(
+      $revision_link = null;
-            'a',
+      $commit_link = null;
-            array(
+      $before_link = null;
-              'href' => $drequest->generateURI(
+      $style = null;
-                array(
+      if ($identifier && !$line['duplicate']) {
-                  'action' => 'commit',
+        $style = 'background: '.$line['color'].';';
-                  'commit' => $commit->getCommitIdentifier(),
-                )),
-              'sigil' => 'has-tooltip',
-              'meta'  => array(
-                'tip'   => $tooltip,
-                'align' => 'E',
-                'size'  => 600,
-              ),
-            ),
-            $commit->getShortName());
 
-          $revision_id = null;
+        if (isset($commit_links[$identifier])) {
-          if ($commit) {
+          $commit_link = $commit_links[$identifier];
-            $revision_id = idx($revision_ids, $commit->getPHID());
         }
 
-          if ($revision_id) {
+        if (isset($revision_map[$identifier])) {
-            $revision = idx($revisions, $revision_id);
+          $revision_id = $revision_map[$identifier];
-            if ($revision) {
+          if (isset($revision_links[$revision_id])) {
-              $tooltip = $this->renderRevisionTooltip($revision, $handles);
+            $revision_link = $revision_links[$revision_id];
-              $revision_link = javelin_tag(
-                'a',
-                array(
-                  'href' => '/D'.$revision->getID(),
-                  'sigil' => 'has-tooltip',
-                  'meta'  => array(
-                    'tip'   => $tooltip,
-                    'align' => 'E',
-                    'size'  => 600,
-                  ),
-                ),
-                'D'.$revision->getID());
           }
         }
 
-          if ($commit) {
-            $identifier = $commit->getCommitIdentifier();
         $skip_href = $line_href.'?before='.$identifier.'&view=blame';
-
         $before_link = javelin_tag(
           'a',
           array(
             'href'  => $skip_href,
             'sigil' => 'has-tooltip',
             'meta'  => array(
-                  'tip'     => pht('Skip Past This Commit'),
+              'tip'     => $skip_text,
               'align'   => 'E',
               'size'    => 300,
             ),
           ),
           "\xC2\xAB");
       }
-        }
 
-        $blame[] = phutil_tag(
+      $row[] = phutil_tag(
         'th',
         array(
           'class' => 'diffusion-blame-link',
@@ -1169,13 +1103,12 @@ final class DiffusionBrowseController extends DiffusionController {
         $object_links[] = $revision_link;
       }
 
-        $blame[] = phutil_tag(
+      $row[] = phutil_tag(
         'th',
         array(
           'class' => 'diffusion-rev-link',
         ),
         $object_links);
-      }
 
       $line_link = phutil_tag(
         'a',
@@ -1183,9 +1116,9 @@ final class DiffusionBrowseController extends DiffusionController {
           'href' => $line_href,
           'style' => $style,
         ),
-        $line['line']);
+        $line_number);
 
-      $blame[] = javelin_tag(
+      $row[] = javelin_tag(
         'th',
         array(
           'class' => 'diffusion-line-link',
@@ -1210,7 +1143,7 @@ final class DiffusionBrowseController extends DiffusionController {
         $anchor_text = null;
       }
 
-      $blame[] = phutil_tag(
+      $row[] = phutil_tag(
         'td',
         array(
         ),
@@ -1234,7 +1167,7 @@ final class DiffusionBrowseController extends DiffusionController {
           $cov_class = 'N';
         }
 
-        $blame[] = phutil_tag(
+        $row[] = phutil_tag(
           'td',
           array(
             'class' => 'cov cov-'.$cov_class,
@@ -1249,10 +1182,10 @@ final class DiffusionBrowseController extends DiffusionController {
                       'phabricator-source-highlight' :
                       null),
         ),
-        $blame);
+        $row);
 
       $cur_inlines = $this->renderInlines(
-        idx($inlines, $line['line'], array()),
+        idx($inlines, $line_number, array()),
         $show_blame,
         $this->coverage,
         $engine);
@@ -1520,7 +1453,6 @@ final class DiffusionBrowseController extends DiffusionController {
 
   private function renderCommitTooltip(
     PhabricatorRepositoryCommit $commit,
-    array $handles,
     $author) {
 
     $viewer = $this->getRequest()->getUser();
@@ -1528,10 +1460,6 @@ final class DiffusionBrowseController extends DiffusionController {
     $date = phabricator_date($commit->getEpoch(), $viewer);
     $summary = trim($commit->getSummary());
 
-    if ($commit->getAuthorPHID()) {
-      $author = $handles[$commit->getAuthorPHID()]->getName();
-    }
-
     return "{$summary}\n{$date} \xC2\xB7 {$author}";
   }
 
@@ -1809,16 +1737,133 @@ final class DiffusionBrowseController extends DiffusionController {
       $commits = array();
     }
 
-    foreach ($identifiers as $key => $identifier) {
+    return array($identifiers, $commits);
-      $commit = idx($commits, $identifier);
+  }
-      if ($commit) {
+
-        $identifiers[$key] = $commit;
+  private function renderCommitLinks(array $commits, $handles) {
+    $links = array();
+    foreach ($commits as $identifier => $commit) {
+      $tooltip = $this->renderCommitTooltip(
+        $commit,
+        $commit->renderAuthorShortName($handles));
+
+      $commit_link = javelin_tag(
+        'a',
+        array(
+          'href' => $commit->getURI(),
+          'sigil' => 'has-tooltip',
+          'meta'  => array(
+            'tip'   => $tooltip,
+            'align' => 'E',
+            'size'  => 600,
+          ),
+        ),
+        $commit->getShortName());
+
+      $links[$identifier] = $commit_link;
+    }
+
+    return $links;
+  }
+
+  private function renderRevisionLinks(array $revisions, $handles) {
+    $links = array();
+
+    foreach ($revisions as $revision) {
+      $revision_id = $revision->getID();
+
+      $tooltip = $this->renderRevisionTooltip($revision, $handles);
+
+      $revision_link = javelin_tag(
+        'a',
+        array(
+          'href' => $revision->getURI(),
+          'sigil' => 'has-tooltip',
+          'meta'  => array(
+            'tip'   => $tooltip,
+            'align' => 'E',
+            'size'  => 600,
+          ),
+        ),
+        $revision->getMonogram());
+
+      $links[$revision_id] = $revision_link;
+    }
+
+    return $links;
+  }
+
+  private function renderPlaintextCorpus(
+    $file_corpus,
+    array $blame_list,
+    array $blame_commits,
+    $show_blame) {
+
+    $viewer = $this->getViewer();
+
+    if (!$show_blame) {
+      $corpus = $file_corpus;
     } else {
-        $identifiers[$key] = null;
+      $author_phids = array();
+      foreach ($blame_commits as $commit) {
+        $author_phid = $commit->getAuthorPHID();
+        if ($author_phid === null) {
+          continue;
+        }
+        $author_phids[$author_phid] = $author_phid;
+      }
+
+      if ($author_phids) {
+        $handles = $viewer->loadHandles($author_phids);
+      } else {
+        $handles = array();
+      }
+
+      $authors = array();
+      $names = array();
+      foreach ($blame_commits as $identifier => $commit) {
+        $author = $commit->renderAuthorShortName($handles);
+        $name = $commit->getShortName();
+
+        $authors[$identifier] = $author;
+        $names[$identifier] = $name;
+      }
+
+      $lines = phutil_split_lines($file_corpus);
+
+      $rows = array();
+      foreach ($lines as $line_number => $line) {
+        $commit_name = null;
+        $author = null;
+
+        if (isset($blame_list[$line_number])) {
+          $identifier = $blame_list[$line_number];
+
+          if (isset($names[$identifier])) {
+            $commit_name = $names[$identifier];
+          }
+
+          if (isset($authors[$identifier])) {
+            $author = $authors[$identifier];
           }
         }
 
-    return $identifiers;
+        $rows[] = sprintf(
+          '%-10s %-20s %s',
+          $commit_name,
+          $author,
+          $line);
+      }
+      $corpus = implode('', $rows);
+    }
+
+    return phutil_tag(
+      'textarea',
+      array(
+        'style' => 'border: none; width: 100%; height: 80em; '.
+                   'font-family: monospace',
+      ),
+      $corpus);
   }
 
 }