From b772a2b92af5fb2e6d032c19578f521771b417c8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 5 Sep 2014 17:30:26 -0700 Subject: [PATCH] Reduce the cost of loading large numbers of macros Summary: Ref T6013. I accidentally made this cost explosviely huge when fixing macros for logged out users in D10411. Specifically, we'd load all the macros, which would load all the files, which would load all the macros (to do policy checks), which would fill out of cache I think (but maybe only some of the time?). Anyway, bad news. Instead, only load the files if we need them. Test Plan: Viewed macro main page, macro detail, used a macro, used a meme, edited a macro, edited audio. Reviewers: btrahan, csilvers Reviewed By: csilvers Subscribers: epriestley, spicyj Maniphest Tasks: T6013 Differential Revision: https://secure.phabricator.com/D10428 --- .../conduit/MacroQueryConduitAPIMethod.php | 5 +-- .../PhabricatorMacroEditController.php | 1 + .../PhabricatorMacroMemeController.php | 1 + .../PhabricatorMacroViewController.php | 1 + .../macro/query/PhabricatorMacroQuery.php | 35 ++++++++++++------- .../query/PhabricatorMacroSearchEngine.php | 1 + 6 files changed, 29 insertions(+), 15 deletions(-) diff --git a/src/applications/macro/conduit/MacroQueryConduitAPIMethod.php b/src/applications/macro/conduit/MacroQueryConduitAPIMethod.php index 7f680ebdbd..29702c8dc1 100644 --- a/src/applications/macro/conduit/MacroQueryConduitAPIMethod.php +++ b/src/applications/macro/conduit/MacroQueryConduitAPIMethod.php @@ -30,8 +30,9 @@ final class MacroQueryConduitAPIMethod extends MacroConduitAPIMethod { } protected function execute(ConduitAPIRequest $request) { - $query = new PhabricatorMacroQuery(); - $query->setViewer($request->getUser()); + $query = id(new PhabricatorMacroQuery()) + ->setViewer($request->getUser()) + ->needFiles(true); $author_phids = $request->getValue('authorPHIDs'); $phids = $request->getValue('phids'); diff --git a/src/applications/macro/controller/PhabricatorMacroEditController.php b/src/applications/macro/controller/PhabricatorMacroEditController.php index 5442754e42..074b7e5a7f 100644 --- a/src/applications/macro/controller/PhabricatorMacroEditController.php +++ b/src/applications/macro/controller/PhabricatorMacroEditController.php @@ -19,6 +19,7 @@ final class PhabricatorMacroEditController extends PhabricatorMacroController { $macro = id(new PhabricatorMacroQuery()) ->setViewer($user) ->withIDs(array($this->id)) + ->needFiles(true) ->executeOne(); if (!$macro) { return new Aphront404Response(); diff --git a/src/applications/macro/controller/PhabricatorMacroMemeController.php b/src/applications/macro/controller/PhabricatorMacroMemeController.php index d8b9d5c441..badb1759ad 100644 --- a/src/applications/macro/controller/PhabricatorMacroMemeController.php +++ b/src/applications/macro/controller/PhabricatorMacroMemeController.php @@ -29,6 +29,7 @@ final class PhabricatorMacroMemeController $macro = id(new PhabricatorMacroQuery()) ->setViewer($user) ->withNames(array($macro_name)) + ->needFiles(true) ->executeOne(); if (!$macro) { return false; diff --git a/src/applications/macro/controller/PhabricatorMacroViewController.php b/src/applications/macro/controller/PhabricatorMacroViewController.php index 01a3e9a926..da9bc83260 100644 --- a/src/applications/macro/controller/PhabricatorMacroViewController.php +++ b/src/applications/macro/controller/PhabricatorMacroViewController.php @@ -20,6 +20,7 @@ final class PhabricatorMacroViewController $macro = id(new PhabricatorMacroQuery()) ->setViewer($user) ->withIDs(array($this->id)) + ->needFiles(true) ->executeOne(); if (!$macro) { return new Aphront404Response(); diff --git a/src/applications/macro/query/PhabricatorMacroQuery.php b/src/applications/macro/query/PhabricatorMacroQuery.php index f07f308585..c7f9534499 100644 --- a/src/applications/macro/query/PhabricatorMacroQuery.php +++ b/src/applications/macro/query/PhabricatorMacroQuery.php @@ -12,6 +12,8 @@ final class PhabricatorMacroQuery private $dateCreatedBefore; private $flagColor; + private $needFiles; + private $status = 'status-any'; const STATUS_ANY = 'status-any'; const STATUS_ACTIVE = 'status-active'; @@ -83,6 +85,11 @@ final class PhabricatorMacroQuery return $this; } + public function needFiles($need_files) { + $this->needFiles = $need_files; + return $this; + } + protected function loadPage() { $macro_table = new PhabricatorFileImageMacro(); $conn = $macro_table->establishConnection('r'); @@ -196,21 +203,23 @@ final class PhabricatorMacroQuery } protected function didFilterPage(array $macros) { - $file_phids = mpull($macros, 'getFilePHID'); - $files = id(new PhabricatorFileQuery()) - ->setViewer($this->getViewer()) - ->setParentQuery($this) - ->withPHIDs($file_phids) - ->execute(); - $files = mpull($files, null, 'getPHID'); + if ($this->needFiles) { + $file_phids = mpull($macros, 'getFilePHID'); + $files = id(new PhabricatorFileQuery()) + ->setViewer($this->getViewer()) + ->setParentQuery($this) + ->withPHIDs($file_phids) + ->execute(); + $files = mpull($files, null, 'getPHID'); - foreach ($macros as $key => $macro) { - $file = idx($files, $macro->getFilePHID()); - if (!$file) { - unset($macros[$key]); - continue; + foreach ($macros as $key => $macro) { + $file = idx($files, $macro->getFilePHID()); + if (!$file) { + unset($macros[$key]); + continue; + } + $macro->attachFile($file); } - $macro->attachFile($file); } return $macros; diff --git a/src/applications/macro/query/PhabricatorMacroSearchEngine.php b/src/applications/macro/query/PhabricatorMacroSearchEngine.php index 48d2a2c5b7..45390b13b1 100644 --- a/src/applications/macro/query/PhabricatorMacroSearchEngine.php +++ b/src/applications/macro/query/PhabricatorMacroSearchEngine.php @@ -29,6 +29,7 @@ final class PhabricatorMacroSearchEngine public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) { $query = id(new PhabricatorMacroQuery()) + ->needFiles(true) ->withIDs($saved->getParameter('ids', array())) ->withPHIDs($saved->getParameter('phids', array())) ->withAuthorPHIDs($saved->getParameter('authorPHIDs', array()));