From e6ddd6d0e95dfa3a124ebe0bbfae97c690662302 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 5 Dec 2016 20:22:46 -0800 Subject: [PATCH] Cache Almanac URIs for repositories Summary: Ref T11954. This is kind of complex and I'm not sure I want to actually land it, but it gives us a fairly good improvement for clustered repositories so I'm leaning toward moving forward. When we make (or receive) clustered repository requests, we must first load a bunch of stuff out of Almanac to figure out where to send the request (or if we can handle the request ourselves). This involves several round trip queries into Almanac (service, device, interfaces, bindings, properties) and generally is fairly slow/expensive. The actual data we get out of it is just a list of URIs. Caching this would be very easy, except that invalidating the cache is difficult, since editing any binding, property, interface, or device may invalidate the cache for indirectly connected services and repositories. To address this, introduce `PhabricatorCacheEngine`, which is an extensible engine like `PhabricatorDestructionEngine` for propagating cache updates. It has two modes: - Discover linked objects (that is: find related objects which may need to have caches invalidated). - Invalidate caches (that is: nuke any caches which need to be nuked). Both modes are extensible, so third-party code can build repository-dependent caches or whatever. This may be overkill but even if Almanac is the only thing we use it for it feels like a fairly clean solution to the problem. With `CacheEngine`, make any edit to Almanac stuff propagate up to the Service, and then from the Service to any linked Repositories. Once we hit repositories, invalidate their caches when Almanac changes. Test Plan: - Observed a 20-30ms performance improvement with `ab -n 100`. - (The main page making Conduit calls also gets a performance improvement, although that's a little trickier to measure directly.) - Added debugging code to the cache engine stuff to observe the linking and invalidation phases. - Made invalidation throw; verified that editing properties, bindings, etc, properly invalidates the cache of any indirectly linked repositories. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11954 Differential Revision: https://secure.phabricator.com/D17000 --- src/__phutil_library_map__.php | 8 ++ .../AlmanacCacheEngineExtension.php | 53 +++++++++++ .../DiffusionCacheEngineExtension.php | 52 ++++++++++ .../query/PhabricatorRepositoryQuery.php | 13 +++ .../storage/PhabricatorRepository.php | 86 ++++++++++++----- .../system/engine/PhabricatorCacheEngine.php | 94 +++++++++++++++++++ .../PhabricatorCacheEngineExtension.php | 42 +++++++++ ...habricatorApplicationTransactionEditor.php | 4 + 8 files changed, 328 insertions(+), 24 deletions(-) create mode 100644 src/applications/almanac/engineextension/AlmanacCacheEngineExtension.php create mode 100644 src/applications/diffusion/engineextension/DiffusionCacheEngineExtension.php create mode 100644 src/applications/system/engine/PhabricatorCacheEngine.php create mode 100644 src/applications/system/engine/PhabricatorCacheEngineExtension.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4e48f12ce3..1eed2611ff 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -22,6 +22,7 @@ phutil_register_library_map(array( 'AlmanacBindingTransactionQuery' => 'applications/almanac/query/AlmanacBindingTransactionQuery.php', 'AlmanacBindingViewController' => 'applications/almanac/controller/AlmanacBindingViewController.php', 'AlmanacBindingsSearchEngineAttachment' => 'applications/almanac/engineextension/AlmanacBindingsSearchEngineAttachment.php', + 'AlmanacCacheEngineExtension' => 'applications/almanac/engineextension/AlmanacCacheEngineExtension.php', 'AlmanacClusterDatabaseServiceType' => 'applications/almanac/servicetype/AlmanacClusterDatabaseServiceType.php', 'AlmanacClusterRepositoryServiceType' => 'applications/almanac/servicetype/AlmanacClusterRepositoryServiceType.php', 'AlmanacClusterServiceType' => 'applications/almanac/servicetype/AlmanacClusterServiceType.php', @@ -584,6 +585,7 @@ phutil_register_library_map(array( 'DiffusionBrowseQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php', 'DiffusionBrowseResultSet' => 'applications/diffusion/data/DiffusionBrowseResultSet.php', 'DiffusionBrowseTableView' => 'applications/diffusion/view/DiffusionBrowseTableView.php', + 'DiffusionCacheEngineExtension' => 'applications/diffusion/engineextension/DiffusionCacheEngineExtension.php', 'DiffusionCachedResolveRefsQuery' => 'applications/diffusion/query/DiffusionCachedResolveRefsQuery.php', 'DiffusionChangeController' => 'applications/diffusion/controller/DiffusionChangeController.php', 'DiffusionChangeHeraldFieldGroup' => 'applications/diffusion/herald/DiffusionChangeHeraldFieldGroup.php', @@ -2023,6 +2025,8 @@ phutil_register_library_map(array( 'PhabricatorBulkContentSource' => 'infrastructure/daemon/contentsource/PhabricatorBulkContentSource.php', 'PhabricatorBusyUIExample' => 'applications/uiexample/examples/PhabricatorBusyUIExample.php', 'PhabricatorCacheDAO' => 'applications/cache/storage/PhabricatorCacheDAO.php', + 'PhabricatorCacheEngine' => 'applications/system/engine/PhabricatorCacheEngine.php', + 'PhabricatorCacheEngineExtension' => 'applications/system/engine/PhabricatorCacheEngineExtension.php', 'PhabricatorCacheGeneralGarbageCollector' => 'applications/cache/garbagecollector/PhabricatorCacheGeneralGarbageCollector.php', 'PhabricatorCacheManagementPurgeWorkflow' => 'applications/cache/management/PhabricatorCacheManagementPurgeWorkflow.php', 'PhabricatorCacheManagementWorkflow' => 'applications/cache/management/PhabricatorCacheManagementWorkflow.php', @@ -4571,6 +4575,7 @@ phutil_register_library_map(array( 'AlmanacBindingTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'AlmanacBindingViewController' => 'AlmanacServiceController', 'AlmanacBindingsSearchEngineAttachment' => 'AlmanacSearchEngineAttachment', + 'AlmanacCacheEngineExtension' => 'PhabricatorCacheEngineExtension', 'AlmanacClusterDatabaseServiceType' => 'AlmanacClusterServiceType', 'AlmanacClusterRepositoryServiceType' => 'AlmanacClusterServiceType', 'AlmanacClusterServiceType' => 'AlmanacServiceType', @@ -5225,6 +5230,7 @@ phutil_register_library_map(array( 'DiffusionBrowseQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionBrowseResultSet' => 'Phobject', 'DiffusionBrowseTableView' => 'DiffusionView', + 'DiffusionCacheEngineExtension' => 'PhabricatorCacheEngineExtension', 'DiffusionCachedResolveRefsQuery' => 'DiffusionLowLevelQuery', 'DiffusionChangeController' => 'DiffusionController', 'DiffusionChangeHeraldFieldGroup' => 'HeraldFieldGroup', @@ -6883,6 +6889,8 @@ phutil_register_library_map(array( 'PhabricatorBulkContentSource' => 'PhabricatorContentSource', 'PhabricatorBusyUIExample' => 'PhabricatorUIExample', 'PhabricatorCacheDAO' => 'PhabricatorLiskDAO', + 'PhabricatorCacheEngine' => 'Phobject', + 'PhabricatorCacheEngineExtension' => 'Phobject', 'PhabricatorCacheGeneralGarbageCollector' => 'PhabricatorGarbageCollector', 'PhabricatorCacheManagementPurgeWorkflow' => 'PhabricatorCacheManagementWorkflow', 'PhabricatorCacheManagementWorkflow' => 'PhabricatorManagementWorkflow', diff --git a/src/applications/almanac/engineextension/AlmanacCacheEngineExtension.php b/src/applications/almanac/engineextension/AlmanacCacheEngineExtension.php new file mode 100644 index 0000000000..20c6bbcd71 --- /dev/null +++ b/src/applications/almanac/engineextension/AlmanacCacheEngineExtension.php @@ -0,0 +1,53 @@ +getViewer(); + + $results = array(); + foreach ($this->selectObjects($objects, 'AlmanacBinding') as $object) { + $results[] = $object->getServicePHID(); + $results[] = $object->getDevicePHID(); + $results[] = $object->getInterfacePHID(); + } + + $devices = $this->selectObjects($objects, 'AlmanacDevice'); + if ($devices) { + $interfaces = id(new AlmanacInterfaceQuery()) + ->setViewer($viewer) + ->withDevicePHIDs(mpull($devices, 'getPHID')) + ->execute(); + foreach ($interfaces as $interface) { + $results[] = $interface; + } + } + + foreach ($this->selectObjects($objects, 'AlmanacInterface') as $iface) { + $results[] = $iface->getDevicePHID(); + $results[] = $iface->getNetworkPHID(); + } + + foreach ($this->selectObjects($objects, 'AlmanacProperty') as $object) { + $results[] = $object->getObjectPHID(); + } + + return $results; + } + + public function deleteCaches( + PhabricatorCacheEngine $engine, + array $objects) { + return; + } + +} diff --git a/src/applications/diffusion/engineextension/DiffusionCacheEngineExtension.php b/src/applications/diffusion/engineextension/DiffusionCacheEngineExtension.php new file mode 100644 index 0000000000..8a862643f8 --- /dev/null +++ b/src/applications/diffusion/engineextension/DiffusionCacheEngineExtension.php @@ -0,0 +1,52 @@ +getViewer(); + $results = array(); + + // When an Almanac Service changes, update linked repositories. + + $services = $this->selectObjects($objects, 'AlmanacService'); + if ($services) { + $repositories = id(new PhabricatorRepositoryQuery()) + ->setViewer($viewer) + ->withAlmanacServicePHIDs(mpull($services, 'getPHID')) + ->execute(); + foreach ($repositories as $repository) { + $results[] = $repository; + } + } + + return $results; + } + + public function deleteCaches( + PhabricatorCacheEngine $engine, + array $objects) { + + $keys = array(); + $repositories = $this->selectObjects($objects, 'PhabricatorRepository'); + foreach ($repositories as $repository) { + $keys[] = $repository->getAlmanacServiceCacheKey(); + } + + $keys = array_filter($keys); + + if ($keys) { + $cache = PhabricatorCaches::getMutableStructureCache(); + $cache->deleteKeys($keys); + } + } + +} diff --git a/src/applications/repository/query/PhabricatorRepositoryQuery.php b/src/applications/repository/query/PhabricatorRepositoryQuery.php index b67b3c5610..c7cfb4df5c 100644 --- a/src/applications/repository/query/PhabricatorRepositoryQuery.php +++ b/src/applications/repository/query/PhabricatorRepositoryQuery.php @@ -12,6 +12,7 @@ final class PhabricatorRepositoryQuery private $uris; private $datasourceQuery; private $slugs; + private $almanacServicePHIDs; private $numericIdentifiers; private $callsignIdentifiers; @@ -134,6 +135,11 @@ final class PhabricatorRepositoryQuery return $this; } + public function withAlmanacServicePHIDs(array $phids) { + $this->almanacServicePHIDs = $phids; + return $this; + } + public function needCommitCounts($need_counts) { $this->needCommitCounts = $need_counts; return $this; @@ -659,6 +665,13 @@ final class PhabricatorRepositoryQuery $try_uris); } + if ($this->almanacServicePHIDs !== null) { + $where[] = qsprintf( + $conn, + 'r.almanacServicePHID IN (%Ls)', + $this->almanacServicePHIDs); + } + return $where; } diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index cc1942d8d2..1a42eb8bd2 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1865,21 +1865,25 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $never_proxy, array $protocols) { - $service = $this->loadAlmanacService(); - if (!$service) { + $cache_key = $this->getAlmanacServiceCacheKey(); + if (!$cache_key) { return null; } - $bindings = $service->getActiveBindings(); - if (!$bindings) { - throw new Exception( - pht( - 'The Almanac service for this repository is not bound to any '. - 'interfaces.')); + $cache = PhabricatorCaches::getMutableStructureCache(); + $uris = $cache->getKey($cache_key, false); + + // If we haven't built the cache yet, build it now. + if ($uris === false) { + $uris = $this->buildAlmanacServiceURIs(); + $cache->setKey($cache_key, $uris); + } + + if ($uris === null) { + return null; } $local_device = AlmanacKeys::getDeviceID(); - if ($never_proxy && !$local_device) { throw new Exception( pht( @@ -1890,10 +1894,8 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $protocol_map = array_fuse($protocols); - $uris = array(); - foreach ($bindings as $binding) { - $iface = $binding->getInterface(); - + $results = array(); + foreach ($uris as $uri) { // If we're never proxying this and it's locally satisfiable, return // `null` to tell the caller to handle it locally. If we're allowed to // proxy, we skip this check and may proxy the request to ourselves. @@ -1901,22 +1903,17 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO // return `null`, and then the request will actually run.) if ($local_device && $never_proxy) { - if ($iface->getDevice()->getName() == $local_device) { + if ($uri['device'] == $local_device) { return null; } } - $uri = $this->getClusterRepositoryURIFromBinding($binding); - - $protocol = $uri->getProtocol(); - if (empty($protocol_map[$protocol])) { - continue; + if (isset($protocol_map[$uri['protocol']])) { + $results[] = new PhutilURI($uri['uri']); } - - $uris[] = $uri; } - if (!$uris) { + if (!$results) { throw new Exception( pht( 'The Almanac service for this repository is not bound to any '. @@ -1931,10 +1928,51 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO 'Cluster hosts must correctly route their intracluster requests.')); } - shuffle($uris); - return head($uris); + shuffle($results); + return head($results); } + public function getAlmanacServiceCacheKey() { + $service_phid = $this->getAlmanacServicePHID(); + if (!$service_phid) { + return null; + } + + $repository_phid = $this->getPHID(); + return "diffusion.repository({$repository_phid}).service({$service_phid})"; + } + + private function buildAlmanacServiceURIs() { + $service = $this->loadAlmanacService(); + if (!$service) { + return null; + } + + $bindings = $service->getActiveBindings(); + if (!$bindings) { + throw new Exception( + pht( + 'The Almanac service for this repository is not bound to any '. + 'interfaces.')); + } + + $uris = array(); + foreach ($bindings as $binding) { + $iface = $binding->getInterface(); + + $uri = $this->getClusterRepositoryURIFromBinding($binding); + $protocol = $uri->getProtocol(); + $device_name = $iface->getDevice()->getName(); + + $uris[] = array( + 'protocol' => $protocol, + 'uri' => (string)$uri, + 'device' => $device_name, + ); + } + + return $uris; + } /** * Build a new Conduit client in order to make a service call to this diff --git a/src/applications/system/engine/PhabricatorCacheEngine.php b/src/applications/system/engine/PhabricatorCacheEngine.php new file mode 100644 index 0000000000..f89fe26b4b --- /dev/null +++ b/src/applications/system/engine/PhabricatorCacheEngine.php @@ -0,0 +1,94 @@ +getPHID() => $object, + ); + + $new_objects = $objects; + while (true) { + $discovered_objects = array(); + $load = array(); + + $extensions = PhabricatorCacheEngineExtension::getAllExtensions(); + foreach ($extensions as $key => $extension) { + $discoveries = $extension->discoverLinkedObjects($this, $new_objects); + if (!is_array($discoveries)) { + throw new Exception( + pht( + 'Cache engine extension "%s" did not return a list of linked '. + 'objects.', + get_class($extension))); + } + + foreach ($discoveries as $discovery) { + if ($discovery === null) { + // This is allowed because it makes writing extensions a lot + // easier if they don't have to check that related PHIDs are + // actually set to something. + continue; + } + + $is_phid = is_string($discovery); + if ($is_phid) { + $phid = $discovery; + } else { + $phid = $discovery->getPHID(); + if (!$phid) { + throw new Exception( + pht( + 'Cache engine extension "%s" returned object (of class '. + '"%s") with no PHID.', + get_class($extension), + get_class($discovery))); + } + } + + if (isset($objects[$phid])) { + continue; + } + + if ($is_phid) { + $load[$phid] = $phid; + } else { + $objects[$phid] = $discovery; + $discovered_objects[$phid] = $discovery; + + // If another extension only knew about the PHID of this object, + // we don't need to load it any more. + unset($load[$phid]); + } + } + } + + if ($load) { + $load_objects = id(new PhabricatorObjectQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($load) + ->execute(); + foreach ($load_objects as $phid => $loaded_object) { + $objects[$phid] = $loaded_object; + $discovered_objects[$phid] = $loaded_object; + } + } + + // If we didn't find anything new to update, we're all set. + if (!$discovered_objects) { + break; + } + + $new_objects = $discovered_objects; + } + + foreach ($extensions as $extension) { + $extension->deleteCaches($this, $objects); + } + } + +} diff --git a/src/applications/system/engine/PhabricatorCacheEngineExtension.php b/src/applications/system/engine/PhabricatorCacheEngineExtension.php new file mode 100644 index 0000000000..3759f1057e --- /dev/null +++ b/src/applications/system/engine/PhabricatorCacheEngineExtension.php @@ -0,0 +1,42 @@ +getPhobjectClassConstant('EXTENSIONKEY'); + } + + abstract public function getExtensionName(); + + public function discoverLinkedObjects( + PhabricatorCacheEngine $engine, + array $objects) { + return array(); + } + + public function deleteCaches( + PhabricatorCacheEngine $engine, + array $objects) { + return null; + } + + final public static function getAllExtensions() { + return id(new PhutilClassMapQuery()) + ->setAncestorClass(__CLASS__) + ->setUniqueMethod('getExtensionKey') + ->execute(); + } + + final public function selectObjects(array $objects, $class_name) { + $results = array(); + + foreach ($objects as $phid => $object) { + if ($object instanceof $class_name) { + $results[$phid] = $object; + } + } + + return $results; + } + +} diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 575972104b..5851e0186c 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -987,6 +987,10 @@ abstract class PhabricatorApplicationTransactionEditor throw $ex; } + // If we need to perform cache engine updates, execute them now. + id(new PhabricatorCacheEngine()) + ->updateObject($object); + // Now that we've completely applied the core transaction set, try to apply // Herald rules. Herald rules are allowed to either take direct actions on // the database (like writing flags), or take indirect actions (like saving