From 4adaf53bf0cba8e798cffcf86432ca96ad44cef3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 30 Jun 2015 11:19:41 -0700 Subject: [PATCH] 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 --- .../feed/story/PhabricatorFeedStory.php | 12 ++++-- .../people/storage/PhabricatorUser.php | 23 +++++++++++ .../phid/query/PhabricatorHandleQuery.php | 1 + .../phid/query/PhabricatorObjectQuery.php | 33 ++++++++------- .../query/PhabricatorSpacesNamespaceQuery.php | 2 +- .../policy/PhabricatorPolicyAwareQuery.php | 41 +++++++++++-------- 6 files changed, 77 insertions(+), 35 deletions(-) diff --git a/src/applications/feed/story/PhabricatorFeedStory.php b/src/applications/feed/story/PhabricatorFeedStory.php index 03b55df5db..9e61c30251 100644 --- a/src/applications/feed/story/PhabricatorFeedStory.php +++ b/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)) - ->execute(); + ->withPHIDs(array_keys($object_phids)); + + $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(); diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index b4985bfaa5..4f2b9b5820 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/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 )--------------------------------------------------- */ diff --git a/src/applications/phid/query/PhabricatorHandleQuery.php b/src/applications/phid/query/PhabricatorHandleQuery.php index 2635b84298..1b72edb2e2 100644 --- a/src/applications/phid/query/PhabricatorHandleQuery.php +++ b/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()); diff --git a/src/applications/phid/query/PhabricatorObjectQuery.php b/src/applications/phid/query/PhabricatorObjectQuery.php index 26d0668cc2..d4bcc9edf3 100644 --- a/src/applications/phid/query/PhabricatorObjectQuery.php +++ b/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; } } diff --git a/src/applications/spaces/query/PhabricatorSpacesNamespaceQuery.php b/src/applications/spaces/query/PhabricatorSpacesNamespaceQuery.php index 96125b6aef..45d7bce1a6 100644 --- a/src/applications/spaces/query/PhabricatorSpacesNamespaceQuery.php +++ b/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) { diff --git a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php index 30b7b8dd0e..b3e9c08237 100644 --- a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php +++ b/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 - * requested content. + * content the query requests. Otherwise, a query might return objects + * 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'); - - $viewer_phid = $this->getViewer()->getPHID(); - - // The workspace is scoped per viewer to prevent accidental contamination. - if (empty($this->workspace[$viewer_phid])) { - $this->workspace[$viewer_phid] = array(); + $parent = $this->getParentQuery(); + if ($parent) { + $parent->putObjectsInWorkspace($objects); + return $this; } - $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])) { - $results[$phid] = $this->workspace[$viewer_phid][$phid]; + if (isset($this->workspace[$viewer_fragment][$phid])) { + $results[$phid] = $this->workspace[$viewer_fragment][$phid]; unset($phids[$key]); } } - if ($phids && $this->getParentQuery()) { - $results += $this->getParentQuery()->getObjectsFromWorkspace($phids); - } - return $results; }