From 91a01e57039eaf9e3313307d6e6dc0863563dad9 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Jan 2016 06:41:38 -0800
Subject: [PATCH] Improve Diffusion browse performance for large files

Summary:
When looking at a large file in Diffusion:

  - disable highlighting if it's huge and show a note about why;
  - pick up a few other optimizations.

Test Plan: Locally, this improves the main render of `__phutil_library_map__.php` from 3,200ms to 600ms for me, at the cost of syntax highlighting (we can eventually add view options and let users re-enable it).

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D14959
---
 .../controller/DiffusionBrowseController.php  | 84 ++++++++++++-------
 src/view/form/PHUIInfoView.php                |  1 -
 2 files changed, 54 insertions(+), 31 deletions(-)

diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php
index 033050402b..cb14b62b75 100644
--- a/src/applications/diffusion/controller/DiffusionBrowseController.php
+++ b/src/applications/diffusion/controller/DiffusionBrowseController.php
@@ -585,6 +585,8 @@ final class DiffusionBrowseController extends DiffusionController {
     }
 
     $file_corpus = $file_content->getCorpus();
+    $highlight_limit = DifferentialChangesetParser::HIGHLIGHT_BYTE_LIMIT;
+    $can_highlight = (strlen($file_corpus) <= $highlight_limit);
 
     if (!$show_color) {
       $lines = phutil_split_lines($file_corpus);
@@ -625,12 +627,16 @@ final class DiffusionBrowseController extends DiffusionController {
           implode('', $rows));
       }
     } else {
-      require_celerity_resource('syntax-highlighting-css');
+      if ($can_highlight) {
+        require_celerity_resource('syntax-highlighting-css');
 
-      $highlighted = PhabricatorSyntaxHighlighter::highlightWithFilename(
-        $path,
-        $file_corpus);
-      $lines = phutil_split_lines($highlighted);
+        $highlighted = PhabricatorSyntaxHighlighter::highlightWithFilename(
+          $path,
+          $file_corpus);
+        $lines = phutil_split_lines($highlighted);
+      } else {
+        $lines = phutil_split_lines($file_corpus);
+      }
 
       $rows = $this->buildDisplayRows(
         $lines,
@@ -706,6 +712,18 @@ final class DiffusionBrowseController extends DiffusionController {
       ->appendChild($corpus)
       ->setCollapsed(true);
 
+    if (!$can_highlight) {
+      $message = pht(
+        'This file is larger than %s, so syntax highlighting is disabled '.
+        'by default.',
+        phutil_format_bytes($highlight_limit));
+
+      $corpus->setInfoView(
+        id(new PHUIInfoView())
+          ->setSeverity(PHUIInfoView::SEVERITY_WARNING)
+          ->setErrors(array($message)));
+    }
+
     return $corpus;
   }
 
@@ -851,6 +869,7 @@ final class DiffusionBrowseController extends DiffusionController {
     $show_blame) {
 
     $drequest = $this->getDiffusionRequest();
+    $repository = $drequest->getRepository();
 
     $handles = array();
     if ($blame) {
@@ -989,8 +1008,6 @@ final class DiffusionBrowseController extends DiffusionController {
     }
     $handles = $this->loadViewerHandles($phids);
 
-    Javelin::initBehavior('phabricator-oncopy', array());
-
     $engine = null;
     $inlines = array();
     if ($this->getRequest()->getStr('lint') !== null && $this->lintMessages) {
@@ -1021,13 +1038,21 @@ final class DiffusionBrowseController extends DiffusionController {
       (bool)$this->coverage,
       $engine);
 
+    // NOTE: We're doing this manually because rendering is otherwise
+    // dominated by URI generation for very large files.
+    $line_base = (string)$repository->generateURI(
+      array(
+        'action'  => 'browse',
+        'stable'  => true,
+      ));
+
+    require_celerity_resource('aphront-tooltip-css');
+    Javelin::initBehavior('phabricator-oncopy');
+    Javelin::initBehavior('phabricator-tooltips');
+    Javelin::initBehavior('phabricator-line-linker');
+
     foreach ($display as $line) {
-      $line_href = $drequest->generateURI(
-        array(
-          'action'  => 'browse',
-          'line'    => $line['line'],
-          'stable'  => true,
-        ));
+      $line_href = $line_base.'$'.$line['line'];
 
       $blame = array();
       $style = null;
@@ -1051,9 +1076,6 @@ final class DiffusionBrowseController extends DiffusionController {
             $tooltip = null;
           }
 
-          Javelin::initBehavior('phabricator-tooltips', array());
-          require_celerity_resource('aphront-tooltip-css');
-
           $commit_link = javelin_tag(
             'a',
             array(
@@ -1095,19 +1117,23 @@ final class DiffusionBrowseController extends DiffusionController {
             }
           }
 
-          $uri = $line_href->alter('before', $commit);
-          $before_link = javelin_tag(
-            'a',
-            array(
-              'href'  => $uri->setQueryParam('view', 'blame'),
-              'sigil' => 'has-tooltip',
-              'meta'  => array(
-                'tip'     => pht('Skip Past This Commit'),
-                'align'   => 'E',
-                'size'    => 300,
+          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'),
+                  'align'   => 'E',
+                  'size'    => 300,
+                ),
               ),
-            ),
-            "\xC2\xAB");
+              "\xC2\xAB");
+          }
         }
 
         $blame[] = phutil_tag(
@@ -1149,8 +1175,6 @@ final class DiffusionBrowseController extends DiffusionController {
         ),
         $line_link);
 
-      Javelin::initBehavior('phabricator-line-linker');
-
       if ($line['target']) {
         Javelin::initBehavior(
           'diffusion-jump-to',
diff --git a/src/view/form/PHUIInfoView.php b/src/view/form/PHUIInfoView.php
index 3a8f259f52..0da471574c 100644
--- a/src/view/form/PHUIInfoView.php
+++ b/src/view/form/PHUIInfoView.php
@@ -41,7 +41,6 @@ final class PHUIInfoView extends AphrontView {
   }
 
   public function addButton(PHUIButtonView $button) {
-
     $this->buttons[] = $button;
     return $this;
   }