From f45a13cff40603cb7c252f1ac17b25d89d53bd39 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 6 Dec 2016 05:02:12 -0800 Subject: [PATCH] Improve settings caches on fast paths like Conduit Summary: Ref T11954. This reduces how much work we need to do to load settings, particularly for Conduit (which currently can not benefit directly from the user cache, because it loads the user indirectly via a token). Specifically: - Cache builtin defaults in the runtime cache. This means Phabricator may need to be restarted if you change a global setting default, but this is exceptionally rare. - Cache global defaults in the mutable cache. This means we do less work to load them. - Avoid loading settings classes if we don't have to. - If we missed the user cache for settings, try to read it from the cache table before we actually go regenerate it (we miss on Conduit pathways). Test Plan: Used `ab -n100 ...` to observe a ~6-10ms performance improvement for `user.whoami`. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11954 Differential Revision: https://secure.phabricator.com/D16998 --- .../people/storage/PhabricatorUser.php | 78 +++++++++++-------- .../people/storage/PhabricatorUserCache.php | 20 +++++ .../PhabricatorUserPreferencesEditor.php | 3 + .../PhabricatorPinnedApplicationsSetting.php | 7 +- 4 files changed, 69 insertions(+), 39 deletions(-) diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index bc102b7275..b30f3cac13 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -490,38 +490,28 @@ final class PhabricatorUser $settings = $this->loadGlobalSettings(); } - // NOTE: To slightly improve performance, we're using all settings here, - // not just settings that are enabled for the current viewer. It's fine to - // get the value of a setting that we wouldn't let the user edit in the UI. - $defaults = PhabricatorSetting::getAllSettings(); - if (array_key_exists($key, $settings)) { $value = $settings[$key]; - - // Make sure the value is valid before we return it. This makes things - // more robust when options are changed or removed. - if (isset($defaults[$key])) { - try { - id(clone $defaults[$key]) - ->setViewer($this) - ->assertValidValue($value); - - return $this->writeUserSettingCache($key, $value); - } catch (Exception $ex) { - // Fall through below and return the default value. - } - } else { - // This is an ad-hoc setting with no controlling object. - return $this->writeUserSettingCache($key, $value); - } + return $this->writeUserSettingCache($key, $value); } - if (isset($defaults[$key])) { - $value = id(clone $defaults[$key]) - ->setViewer($this) - ->getSettingDefaultValue(); + $cache = PhabricatorCaches::getRuntimeCache(); + $cache_key = "settings.defaults({$key})"; + $cache_map = $cache->getKeys(array($cache_key)); + + if ($cache_map) { + $value = $cache_map[$cache_key]; } else { - $value = null; + $defaults = PhabricatorSetting::getAllSettings(); + if (isset($defaults[$key])) { + $value = id(clone $defaults[$key]) + ->setViewer($this) + ->getSettingDefaultValue(); + } else { + $value = null; + } + + $cache->setKey($cache_key, $value); } return $this->writeUserSettingCache($key, $value); @@ -555,12 +545,16 @@ final class PhabricatorUser return $this->getUserSetting(PhabricatorTimezoneSetting::SETTINGKEY); } - private function loadGlobalSettings() { - $cache_key = 'user.settings.global'; - $cache = PhabricatorCaches::getRequestCache(); - $settings = $cache->getKey($cache_key); + public static function getGlobalSettingsCacheKey() { + return 'user.settings.globals.v1'; + } - if ($settings === null) { + private function loadGlobalSettings() { + $cache_key = self::getGlobalSettingsCacheKey(); + $cache = PhabricatorCaches::getMutableStructureCache(); + + $settings = $cache->getKey($cache_key); + if (!$settings) { $preferences = PhabricatorUserPreferences::loadGlobalPreferences($this); $settings = $preferences->getPreferences(); $cache->setKey($cache_key, $settings); @@ -1495,9 +1489,27 @@ final class PhabricatorUser throw new PhabricatorDataNotAttachedException($this); } + $user_phid = $this->getPHID(); + + // Try to read the actual cache before we generate a new value. We can + // end up here via Conduit, which does not use normal sessions and can + // not pick up a free cache load during session identification. + if ($user_phid) { + $raw_data = PhabricatorUserCache::readCaches( + $type, + $key, + array($user_phid)); + if (array_key_exists($user_phid, $raw_data)) { + $raw_value = $raw_data[$user_phid]; + $usable_value = $type->getValueFromStorage($raw_value); + $this->rawCacheData[$key] = $raw_value; + $this->usableCacheData[$key] = $usable_value; + return $usable_value; + } + } + $usable_value = $type->getDefaultValue(); - $user_phid = $this->getPHID(); if ($user_phid) { $map = $type->newValueForUsers($key, array($this)); if (array_key_exists($user_phid, $map)) { diff --git a/src/applications/people/storage/PhabricatorUserCache.php b/src/applications/people/storage/PhabricatorUserCache.php index c74b583923..2afb06a437 100644 --- a/src/applications/people/storage/PhabricatorUserCache.php +++ b/src/applications/people/storage/PhabricatorUserCache.php @@ -96,6 +96,26 @@ final class PhabricatorUserCache extends PhabricatorUserDAO { unset($unguarded); } + public static function readCaches( + PhabricatorUserCacheType $type, + $key, + array $user_phids) { + + $table = new self(); + $conn = $table->establishConnection('r'); + + $rows = queryfx_all( + $conn, + 'SELECT userPHID, cacheData FROM %T WHERE userPHID IN (%Ls) + AND cacheType = %s AND cacheIndex = %s', + $table->getTableName(), + $user_phids, + $type->getUserCacheType(), + PhabricatorHash::digestForIndex($key)); + + return ipull($rows, 'cacheData', 'userPHID'); + } + public static function clearCache($key, $user_phid) { return self::clearCaches($key, array($user_phid)); } diff --git a/src/applications/settings/editor/PhabricatorUserPreferencesEditor.php b/src/applications/settings/editor/PhabricatorUserPreferencesEditor.php index cfb7fe105f..c2ec0a7094 100644 --- a/src/applications/settings/editor/PhabricatorUserPreferencesEditor.php +++ b/src/applications/settings/editor/PhabricatorUserPreferencesEditor.php @@ -158,6 +158,9 @@ final class PhabricatorUserPreferencesEditor PhabricatorUserPreferencesCacheType::KEY_PREFERENCES, $user_phid); } else { + $cache = PhabricatorCaches::getMutableStructureCache(); + $cache->deleteKey(PhabricatorUserPreferences::getGlobalCacheKey()); + PhabricatorUserCache::clearCacheForAllUsers( PhabricatorUserPreferencesCacheType::KEY_PREFERENCES); } diff --git a/src/applications/settings/setting/PhabricatorPinnedApplicationsSetting.php b/src/applications/settings/setting/PhabricatorPinnedApplicationsSetting.php index 0609dd9ee8..2e2ab6dac0 100644 --- a/src/applications/settings/setting/PhabricatorPinnedApplicationsSetting.php +++ b/src/applications/settings/setting/PhabricatorPinnedApplicationsSetting.php @@ -10,12 +10,7 @@ final class PhabricatorPinnedApplicationsSetting } public function getSettingDefaultValue() { - $viewer = $this->getViewer(); - - // If we're editing a template, just show every available application. - if (!$viewer) { - $viewer = PhabricatorUser::getOmnipotentUser(); - } + $viewer = PhabricatorUser::getOmnipotentUser(); $applications = id(new PhabricatorApplicationQuery()) ->setViewer($viewer)