From 3142fe44197fccdcf75839012e5fca13d893560c Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 24 Jan 2012 19:03:30 -0800 Subject: [PATCH] Remove massive "rule applied" query Summary: Herald rules may be marked as "one-time". We track this by writing a row with when we apply a rule. However, the current test for rule application involves loading every pair. We also always write this row even for rules which are not one-time, so if there are 100 rules, we'll load 1,000,000 rows after processing 10,000 objects. Instead, load only the pairs, which are guaranteed to be bounded to at most the number of rules. I'll follow up with a diff that causes us to write rows only for one-time rules, and deletes all historic rows which are not associated with one-time rules. Test Plan: Grepped for callsites to loadAllByContentTypeWithFullData(). Ran rules in test console. Reviewers: nh, btrahan, jungejason Reviewed By: nh CC: aran, epriestley Differential Revision: https://secure.phabricator.com/D1483 --- .../sql/patches/101.heraldruleapplied.sql | 2 + .../test/HeraldTestConsoleController.php | 5 ++- .../herald/engine/engine/HeraldEngine.php | 4 +- .../herald/storage/rule/HeraldRule.php | 38 ++++++++++--------- ...habricatorRepositoryCommitHeraldWorker.php | 5 ++- 5 files changed, 31 insertions(+), 23 deletions(-) create mode 100644 resources/sql/patches/101.heraldruleapplied.sql diff --git a/resources/sql/patches/101.heraldruleapplied.sql b/resources/sql/patches/101.heraldruleapplied.sql new file mode 100644 index 0000000000..30c7a1a8ad --- /dev/null +++ b/resources/sql/patches/101.heraldruleapplied.sql @@ -0,0 +1,2 @@ +ALTER TABLE phabricator_herald.herald_ruleapplied + ADD KEY (phid); diff --git a/src/applications/herald/controller/test/HeraldTestConsoleController.php b/src/applications/herald/controller/test/HeraldTestConsoleController.php index a8eb194e79..da94aa46d9 100644 --- a/src/applications/herald/controller/test/HeraldTestConsoleController.php +++ b/src/applications/herald/controller/test/HeraldTestConsoleController.php @@ -1,7 +1,7 @@ getHeraldTypeName()); + $adapter->getHeraldTypeName(), + $object->getPHID()); $engine = new HeraldEngine(); $effects = $engine->applyRules($rules, $adapter); diff --git a/src/applications/herald/engine/engine/HeraldEngine.php b/src/applications/herald/engine/engine/HeraldEngine.php index 6ef84673a7..9426d8afd1 100644 --- a/src/applications/herald/engine/engine/HeraldEngine.php +++ b/src/applications/herald/engine/engine/HeraldEngine.php @@ -28,7 +28,9 @@ class HeraldEngine { public static function loadAndApplyRules(HeraldObjectAdapter $object) { $content_type = $object->getHeraldTypeName(); - $rules = HeraldRule::loadAllByContentTypeWithFullData($content_type); + $rules = HeraldRule::loadAllByContentTypeWithFullData( + $content_type, + $object->getPHID()); $engine = new HeraldEngine(); $effects = $engine->applyRules($rules, $object); diff --git a/src/applications/herald/storage/rule/HeraldRule.php b/src/applications/herald/storage/rule/HeraldRule.php index bc1702a15f..4938fc318c 100644 --- a/src/applications/herald/storage/rule/HeraldRule.php +++ b/src/applications/herald/storage/rule/HeraldRule.php @@ -32,7 +32,10 @@ class HeraldRule extends HeraldDAO { private $ruleApplied = array(); // phids for which this rule has been applied - public static function loadAllByContentTypeWithFullData($content_type) { + public static function loadAllByContentTypeWithFullData( + $content_type, + $object_phid) { + $rules = id(new HeraldRule())->loadAllWhere( 'contentType = %s', $content_type); @@ -53,17 +56,18 @@ class HeraldRule extends HeraldDAO { $applied = queryfx_all( id(new HeraldRule())->establishConnection('r'), - 'SELECT * FROM %T WHERE ruleID in (%Ld)', - self::TABLE_RULE_APPLIED, $rule_ids - ); + 'SELECT * FROM %T WHERE phid = %s', + self::TABLE_RULE_APPLIED, + $object_phid); + $applied = ipull($applied, null, 'ruleID'); $conditions = mgroup($conditions, 'getRuleID'); $actions = mgroup($actions, 'getRuleID'); $applied = igroup($applied, 'ruleID'); - foreach ($rules as $rule) { - $rule->attachAllRuleApplied(idx($applied, $rule->getID(), array())); + $rule->setRuleApplied($object_phid, isset($applied[$rule->getID()])); + $rule->attachConditions(idx($conditions, $rule->getID(), array())); $rule->attachActions(idx($actions, $rule->getID(), array())); } @@ -72,26 +76,24 @@ class HeraldRule extends HeraldDAO { } public function getRuleApplied($phid) { - // defaults to false because (ruleID, phid) pairs not in the db imply - // a rule that's not been applied before - return idx($this->ruleApplied, $phid, false); + if (idx($this->ruleApplied, $phid) === null) { + throw new Exception("Call setRuleApplied() before getRuleApplied()!"); + } + return $this->ruleApplied[$phid]; } - public function setRuleApplied($phid) { - $this->ruleApplied[$phid] = true; + public function setRuleApplied($phid, $applied) { + $this->ruleApplied[$phid] = $applied; + return $this; } - public function attachAllRuleApplied(array $applied) { - // turn array of array(ruleID, phid) into array of ruleID => true - $this->ruleApplied = array_fill_keys(ipull($applied, 'phid'), true); - } - public static function saveRuleApplied($rule_id, $phid) { queryfx( id(new HeraldRule())->establishConnection('w'), 'INSERT IGNORE INTO %T (phid, ruleID) VALUES (%s, %d)', - self::TABLE_RULE_APPLIED, $phid, $rule_id - ); + self::TABLE_RULE_APPLIED, + $phid, + $rule_id); } public function loadConditions() { diff --git a/src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php b/src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php index 3ba9992f49..acf34dfa68 100644 --- a/src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php +++ b/src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php @@ -1,7 +1,7 @@ getID()); $rules = HeraldRule::loadAllByContentTypeWithFullData( - HeraldContentTypeConfig::CONTENT_TYPE_COMMIT); + HeraldContentTypeConfig::CONTENT_TYPE_COMMIT, + $commit->getPHID()); $adapter = new HeraldCommitAdapter( $repository,