Merge pull request #5738 from Toflar/reduce-hashing-calls

Reduce calls on Rule::getHash()

src/Composer/DependencyResolver/RuleSet.php

@@ -58,6 +58,18 @@ class RuleSet implements \IteratorAggregate, \Countable
             throw new \OutOfBoundsException('Unknown rule type: ' . $type);
         }
 
+        $hash = $rule->getHash();
+
+        // Do not add if rule already exists
+        if (isset($this->rulesByHash[$hash])) {
+            $potentialDuplicates = $this->rulesByHash[$hash];
+            foreach ($potentialDuplicates as $potentialDuplicate) {
+                if ($rule->equals($potentialDuplicate)) {
+                    return;
+                }
+            }
+        }
+
         if (!isset($this->rules[$type])) {
             $this->rules[$type] = array();
         }
@@ -68,7 +80,6 @@ class RuleSet implements \IteratorAggregate, \Countable
 
         $this->nextRuleId++;
 
-        $hash = $rule->getHash();
         if (!isset($this->rulesByHash[$hash])) {
             $this->rulesByHash[$hash] = array($rule);
         } else {
@@ -135,20 +146,6 @@ class RuleSet implements \IteratorAggregate, \Countable
         return array_keys($types);
     }
 
-    public function containsEqual($rule)
-    {
-        if (isset($this->rulesByHash[$rule->getHash()])) {
-            $potentialDuplicates = $this->rulesByHash[$rule->getHash()];
-            foreach ($potentialDuplicates as $potentialDuplicate) {
-                if ($rule->equals($potentialDuplicate)) {
-                    return true;
-                }
-            }
-        }
-
-        return false;
-    }
-
     public function getPrettyString(Pool $pool = null)
     {
         $string = "\n";

src/Composer/DependencyResolver/RuleSetGenerator.php

@@ -137,7 +137,7 @@ class RuleSetGenerator
      */
     private function addRule($type, Rule $newRule = null)
     {
-        if (!$newRule || $this->rules->containsEqual($newRule)) {
+        if (!$newRule) {
             return;
         }
 

tests/Composer/Test/DependencyResolver/RuleSetTest.php

@@ -32,8 +32,8 @@ class RuleSetTest extends TestCase
         $rules = array(
             RuleSet::TYPE_PACKAGE => array(),
             RuleSet::TYPE_JOB => array(
-                new Rule(array(), Rule::RULE_JOB_INSTALL, null),
+                new Rule(array(1), Rule::RULE_JOB_INSTALL, null),
-                new Rule(array(), Rule::RULE_JOB_INSTALL, null),
+                new Rule(array(2), Rule::RULE_JOB_INSTALL, null),
             ),
             RuleSet::TYPE_LEARNED => array(
                 new Rule(array(), Rule::RULE_INTERNAL_ALLOW_UPDATE, null),
@@ -49,6 +49,25 @@ class RuleSetTest extends TestCase
         $this->assertEquals($rules, $ruleSet->getRules());
     }
 
+    public function testAddIgnoresDuplicates()
+    {
+        $rules = array(
+            RuleSet::TYPE_JOB => array(
+                new Rule(array(), Rule::RULE_JOB_INSTALL, null),
+                new Rule(array(), Rule::RULE_JOB_INSTALL, null),
+                new Rule(array(), Rule::RULE_JOB_INSTALL, null),
+            )
+        );
+
+        $ruleSet = new RuleSet;
+
+        $ruleSet->add($rules[RuleSet::TYPE_JOB][0], RuleSet::TYPE_JOB);
+        $ruleSet->add($rules[RuleSet::TYPE_JOB][1], RuleSet::TYPE_JOB);
+        $ruleSet->add($rules[RuleSet::TYPE_JOB][2], RuleSet::TYPE_JOB);
+
+        $this->assertCount(1, $ruleSet->getIteratorFor(array(RuleSet::TYPE_JOB)));
+    }
+
     /**
      * @expectedException \OutOfBoundsException
      */
@@ -63,8 +82,8 @@ class RuleSetTest extends TestCase
     {
         $ruleSet = new RuleSet;
 
-        $ruleSet->add(new Rule(array(), Rule::RULE_JOB_INSTALL, null), RuleSet::TYPE_JOB);
+        $ruleSet->add(new Rule(array(1), Rule::RULE_JOB_INSTALL, null), RuleSet::TYPE_JOB);
-        $ruleSet->add(new Rule(array(), Rule::RULE_JOB_INSTALL, null), RuleSet::TYPE_JOB);
+        $ruleSet->add(new Rule(array(2), Rule::RULE_JOB_INSTALL, null), RuleSet::TYPE_JOB);
 
         $this->assertEquals(2, $ruleSet->count());
     }
@@ -83,8 +102,8 @@ class RuleSetTest extends TestCase
     {
         $ruleSet = new RuleSet;
 
-        $rule1 = new Rule(array(), Rule::RULE_JOB_INSTALL, null);
+        $rule1 = new Rule(array(1), Rule::RULE_JOB_INSTALL, null);
-        $rule2 = new Rule(array(), Rule::RULE_JOB_INSTALL, null);
+        $rule2 = new Rule(array(2), Rule::RULE_JOB_INSTALL, null);
         $ruleSet->add($rule1, RuleSet::TYPE_JOB);
         $ruleSet->add($rule2, RuleSet::TYPE_LEARNED);
 
@@ -98,8 +117,8 @@ class RuleSetTest extends TestCase
     public function testGetIteratorFor()
     {
         $ruleSet = new RuleSet;
-        $rule1 = new Rule(array(), Rule::RULE_JOB_INSTALL, null);
+        $rule1 = new Rule(array(1), Rule::RULE_JOB_INSTALL, null);
-        $rule2 = new Rule(array(), Rule::RULE_JOB_INSTALL, null);
+        $rule2 = new Rule(array(2), Rule::RULE_JOB_INSTALL, null);
 
         $ruleSet->add($rule1, RuleSet::TYPE_JOB);
         $ruleSet->add($rule2, RuleSet::TYPE_LEARNED);
@@ -112,8 +131,8 @@ class RuleSetTest extends TestCase
     public function testGetIteratorWithout()
     {
         $ruleSet = new RuleSet;
-        $rule1 = new Rule(array(), Rule::RULE_JOB_INSTALL, null);
+        $rule1 = new Rule(array(1), Rule::RULE_JOB_INSTALL, null);
-        $rule2 = new Rule(array(), Rule::RULE_JOB_INSTALL, null);
+        $rule2 = new Rule(array(2), Rule::RULE_JOB_INSTALL, null);
 
         $ruleSet->add($rule1, RuleSet::TYPE_JOB);
         $ruleSet->add($rule2, RuleSet::TYPE_LEARNED);
@@ -123,38 +142,6 @@ class RuleSetTest extends TestCase
         $this->assertSame($rule2, $iterator->current());
     }
 
-    public function testContainsEqual()
-    {
-        $ruleSet = new RuleSet;
-
-        $rule = $this->getRuleMock();
-        $rule->expects($this->any())
-            ->method('getHash')
-            ->will($this->returnValue('rule_1_hash'));
-        $rule->expects($this->any())
-            ->method('equals')
-            ->will($this->returnValue(true));
-
-        $rule2 = $this->getRuleMock();
-        $rule2->expects($this->any())
-            ->method('getHash')
-            ->will($this->returnValue('rule_2_hash'));
-
-        $rule3 = $this->getRuleMock();
-        $rule3->expects($this->any())
-            ->method('getHash')
-            ->will($this->returnValue('rule_1_hash'));
-        $rule3->expects($this->any())
-            ->method('equals')
-            ->will($this->returnValue(false));
-
-        $ruleSet->add($rule, RuleSet::TYPE_LEARNED);
-
-        $this->assertTrue($ruleSet->containsEqual($rule));
-        $this->assertFalse($ruleSet->containsEqual($rule2));
-        $this->assertFalse($ruleSet->containsEqual($rule3));
-    }
-
     public function testPrettyString()
     {
         $repo = new ArrayRepository;