From 96fde137a1dcb028c2f770a1442123f094415f99 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 27 Feb 2019 01:43:02 -0800
Subject: [PATCH] Improve performance of "arc diff" updates for changes with
 large diff text

Summary:
See PHI1104. The older "differential.querydiffs" method includes the entire raw diff text for all the diffs associated with a revision in its response, but we: only care about the most recent diff; and don't care about the text at all.

For reasonably large changes with several updates, this can be significantly slow.

We can get this same information more efficiently from the modern "differential.diff.search", since D19386 (April 2018). The only trick is that we need a "revisionPHID", which we don't have on hand.

For now, just fetch the revision PHID. In the future, we can likely make adjustments so that we have the revision PHID already by the time we get here.

This may slow down the normal case very slightly (since we now do two calls instead of one), but it speeds up the bad cases dramatically.

Test Plan:
Ran `arc diff` to update a change in a local repository. `var_dump()`'d the old and new algorithm results, saw the same outcome.

Used `arc diff --trace` on an update to a change to verify that `differential.diff.search` is called but `differential.querydiffs` is not.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20221
---
 src/workflow/ArcanistDiffWorkflow.php | 71 ++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 6 deletions(-)

diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php
index 873d5542..4fa37fce 100644
--- a/src/workflow/ArcanistDiffWorkflow.php
+++ b/src/workflow/ArcanistDiffWorkflow.php
@@ -2164,8 +2164,8 @@ EOTEXT
     // we always get this 100% right, we're just trying to do something
     // reasonable.
 
-    $local = $this->loadActiveLocalCommitInfo();
-    $hashes = ipull($local, null, 'commit');
+    $hashes = $this->loadActiveDiffLocalCommitHashes();
+    $hashes = array_fuse($hashes);
 
     $usable = array();
     foreach ($commit_messages as $message) {
@@ -2212,8 +2212,8 @@ EOTEXT
       return null;
     }
 
-    $local = $this->loadActiveLocalCommitInfo();
-    $hashes = ipull($local, null, 'commit');
+    $hashes = $this->loadActiveDiffLocalCommitHashes();
+    $hashes = array_fuse($hashes);
 
     $usable = array();
     foreach ($messages as $rev => $message) {
@@ -2261,7 +2261,63 @@ EOTEXT
     return implode('', $default);
   }
 
-  private function loadActiveLocalCommitInfo() {
+  private function loadActiveDiffLocalCommitHashes() {
+    // The older "differential.querydiffs" method includes the full diff text,
+    // which can be very slow for large diffs. If we can, try to use
+    // "differential.diff.search" instead.
+
+    // We expect this to fail if the Phabricator version on the server is
+    // older than April 2018 (D19386), which introduced the "commits"
+    // attachment for "differential.revision.search".
+
+    // TODO: This can be optimized if we're able to learn the "revisionPHID"
+    // before we get here. See PHI1104.
+
+    try {
+      $revisions_raw = $this->getConduit()->callMethodSynchronous(
+        'differential.revision.search',
+        array(
+          'constraints' => array(
+            'ids' => array(
+              $this->revisionID,
+            ),
+          ),
+        ));
+
+      $revisions = $revisions_raw['data'];
+      $revision = head($revisions);
+      if ($revision) {
+        $revision_phid = $revision['phid'];
+
+        $diffs_raw = $this->getConduit()->callMethodSynchronous(
+          'differential.diff.search',
+          array(
+            'constraints' => array(
+              'revisionPHIDs' => array(
+                $revision_phid,
+              ),
+            ),
+            'attachments' => array(
+              'commits' => true,
+            ),
+            'limit' => 1,
+          ));
+
+        $diffs = $diffs_raw['data'];
+        $diff = head($diffs);
+
+        if ($diff) {
+          $commits = idxv($diff, array('attachments', 'commits', 'commits'));
+          if ($commits !== null) {
+            $hashes = ipull($commits, 'identifier');
+            return array_values($hashes);
+          }
+        }
+      }
+    } catch (Exception $ex) {
+      // If any of this fails, fall back to the older method below.
+    }
+
     $current_diff = $this->getConduit()->callMethodSynchronous(
       'differential.querydiffs',
       array(
@@ -2270,7 +2326,10 @@ EOTEXT
     $current_diff = head($current_diff);
 
     $properties = idx($current_diff, 'properties', array());
-    return idx($properties, 'local:commits', array());
+    $local = idx($properties, 'local:commits', array());
+    $hashes = ipull($local, 'commit');
+
+    return array_values($hashes);
   }