Dramatically increase cache hit rate for feed

Summary:
Ref T8631. The query plan for feed stories is really bad right now, because we miss caches we should be hitting:

  - The workspace cache is stored at each query, so adjacent queries can't benefit from the cache (only subqueries). Feed has primarily sibling queries.
    - There is no technical reason to do this. Store the workspace cache on the root query, so sibling queries can hit it.
  - In `ObjectQuery`, we check the workspace once, then load all the PHIDs. When the PHIDs are a mixture of transactions and objects, we always miss the workspace and load the objects twice.
    - Instead, check the workspace after loading each type of object.
  - `HandleQuery` does not set itself as the parent query for `ObjectQuery`, so handles never hit the workspace cache.
    - Pass it, so they can hit the workspace cache.
  - Feed's weird `PhabricatorFeedStory::loadAllFromRows()` method does not specify a parent query on its object/handle queries.
    - Just declare the object query to be the "root" query until this eventually gets cleaned up.

Test Plan: Saw queries for each object drop from 4-6x to 1x in `/feed/`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8631

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

src/applications/feed/story/PhabricatorFeedStory.php

@@ -78,10 +78,11 @@ abstract class PhabricatorFeedStory
       $object_phids += $phids;
     }
 
-    $objects = id(new PhabricatorObjectQuery())
+    $object_query = id(new PhabricatorObjectQuery())
       ->setViewer($viewer)
-      ->withPHIDs(array_keys($object_phids))
+      ->withPHIDs(array_keys($object_phids));
-      ->execute();
+
+    $objects = $object_query->execute();
 
     foreach ($key_phids as $key => $phids) {
       if (!$phids) {
@@ -142,8 +143,13 @@ abstract class PhabricatorFeedStory
       $handle_phids += $key_phids[$key];
     }
 
+    // NOTE: This setParentQuery() is a little sketchy. Ideally, this whole
+    // method should be inside FeedQuery and it should be the parent query of
+    // both subqueries. We're just trying to share the workspace cache.
+
     $handles = id(new PhabricatorHandleQuery())
       ->setViewer($viewer)
+      ->setParentQuery($object_query)
       ->withPHIDs(array_keys($handle_phids))
       ->execute();
 

src/applications/people/storage/PhabricatorUser.php

@@ -1073,6 +1073,29 @@ final class PhabricatorUser
   }
 
 
+  /**
+   * Get a scalar string identifying this user.
+   *
+   * This is similar to using the PHID, but distinguishes between ominpotent
+   * and public users explicitly. This allows safe construction of cache keys
+   * or cache buckets which do not conflate public and omnipotent users.
+   *
+   * @return string Scalar identifier.
+   */
+  public function getCacheFragment() {
+    if ($this->isOmnipotent()) {
+      return 'u.omnipotent';
+    }
+
+    $phid = $this->getPHID();
+    if ($phid) {
+      return 'u.'.$phid;
+    }
+
+    return 'u.public';
+  }
+
+
 /* -(  Managing Handles  )--------------------------------------------------- */
 
 

src/applications/phid/query/PhabricatorHandleQuery.php

@@ -33,6 +33,7 @@ final class PhabricatorHandleQuery
 
     $object_query = id(new PhabricatorObjectQuery())
       ->withPHIDs($phids)
+      ->setParentQuery($this)
       ->requireCapabilities($this->getRequiredObjectCapabilities())
       ->setViewer($this->getViewer());
 

src/applications/phid/query/PhabricatorObjectQuery.php

@@ -106,19 +106,6 @@ final class PhabricatorObjectQuery
   private function loadObjectsByPHID(array $types, array $phids) {
     $results = array();
 
-    $workspace = $this->getObjectsFromWorkspace($phids);
-
-    foreach ($phids as $key => $phid) {
-      if (isset($workspace[$phid])) {
-        $results[$phid] = $workspace[$phid];
-        unset($phids[$key]);
-      }
-    }
-
-    if (!$phids) {
-      return $results;
-    }
-
     $groups = array();
     foreach ($phids as $phid) {
       $type = phid_get_type($phid);
@@ -127,6 +114,21 @@ final class PhabricatorObjectQuery
 
     $in_flight = $this->getPHIDsInFlight();
     foreach ($groups as $type => $group) {
+      // We check the workspace for each group, because some groups may trigger
+      // other groups to load (for example, transactions load their objects).
+      $workspace = $this->getObjectsFromWorkspace($group);
+
+      foreach ($group as $key => $phid) {
+        if (isset($workspace[$phid])) {
+          $results[$phid] = $workspace[$phid];
+          unset($group[$key]);
+        }
+      }
+
+      if (!$group) {
+        continue;
+      }
+
       // Don't try to load PHIDs which are already "in flight"; this prevents
       // us from recursing indefinitely if policy checks or edges form a loop.
       // We will decline to load the corresponding objects.
@@ -139,7 +141,10 @@ final class PhabricatorObjectQuery
       if ($group && isset($types[$type])) {
         $this->putPHIDsInFlight($group);
         $objects = $types[$type]->loadObjects($this, $group);
-        $results += mpull($objects, null, 'getPHID');
+
+        $map = mpull($objects, null, 'getPHID');
+        $this->putObjectsInWorkspace($map);
+        $results += $map;
       }
     }
 

src/applications/spaces/query/PhabricatorSpacesNamespaceQuery.php

@@ -143,7 +143,7 @@ final class PhabricatorSpacesNamespaceQuery
 
   public static function getViewerSpaces(PhabricatorUser $viewer) {
     $cache = PhabricatorCaches::getRequestCache();
-    $cache_key = self::KEY_VIEWER.'('.$viewer->getPHID().')';
+    $cache_key = self::KEY_VIEWER.'('.$viewer->getCacheFragment().')';
 
     $result = $cache->getKey($cache_key);
     if ($result === null) {

src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php

@@ -406,8 +406,8 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery {
    *
    * **Fully enrich objects pulled from the workspace.** After pulling objects
    * from the workspace, you still need to load and attach any additional
-   * content the query requests. Otherwise, a query might return objects without
+   * content the query requests. Otherwise, a query might return objects
-   * requested content.
+   * without requested content.
    *
    * Generally, you do not need to update the workspace yourself: it is
    * automatically populated as a side effect of objects surviving policy
@@ -419,16 +419,22 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery {
    * @task workspace
    */
   public function putObjectsInWorkspace(array $objects) {
-    assert_instances_of($objects, 'PhabricatorPolicyInterface');
+    $parent = $this->getParentQuery();
-
+    if ($parent) {
-    $viewer_phid = $this->getViewer()->getPHID();
+      $parent->putObjectsInWorkspace($objects);
-
+      return $this;
-    // The workspace is scoped per viewer to prevent accidental contamination.
-    if (empty($this->workspace[$viewer_phid])) {
-      $this->workspace[$viewer_phid] = array();
     }
 
-    $this->workspace[$viewer_phid] += $objects;
+    assert_instances_of($objects, 'PhabricatorPolicyInterface');
+
+    $viewer_fragment = $this->getViewer()->getCacheFragment();
+
+    // The workspace is scoped per viewer to prevent accidental contamination.
+    if (empty($this->workspace[$viewer_fragment])) {
+      $this->workspace[$viewer_fragment] = array();
+    }
+
+    $this->workspace[$viewer_fragment] += $objects;
 
     return $this;
   }
@@ -445,20 +451,21 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery {
    * @task workspace
    */
   public function getObjectsFromWorkspace(array $phids) {
-    $viewer_phid = $this->getViewer()->getPHID();
+    $parent = $this->getParentQuery();
+    if ($parent) {
+      return $parent->getObjectsFromWorkspace($phids);
+    }
+
+    $viewer_fragment = $this->getViewer()->getCacheFragment();
 
     $results = array();
     foreach ($phids as $key => $phid) {
-      if (isset($this->workspace[$viewer_phid][$phid])) {
+      if (isset($this->workspace[$viewer_fragment][$phid])) {
-        $results[$phid] = $this->workspace[$viewer_phid][$phid];
+        $results[$phid] = $this->workspace[$viewer_fragment][$phid];
         unset($phids[$key]);
       }
     }
 
-    if ($phids && $this->getParentQuery()) {
-      $results += $this->getParentQuery()->getObjectsFromWorkspace($phids);
-    }
-
     return $results;
   }