From: Julien Lancelot Date: Fri, 14 Mar 2014 11:01:30 +0000 (+0100) Subject: SONAR-5141 Improve performance of technical debt measures decorator X-Git-Tag: 4.3~443 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=6fad9a863175e5c44ad9f1452fb8af13a70af4e5;p=sonarqube.git SONAR-5141 Improve performance of technical debt measures decorator --- diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/technicaldebt/TechnicalDebtDecorator.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/technicaldebt/TechnicalDebtDecorator.java index 0827cb4cb0c..633a5f26b22 100644 --- a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/technicaldebt/TechnicalDebtDecorator.java +++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/technicaldebt/TechnicalDebtDecorator.java @@ -21,9 +21,7 @@ package org.sonar.plugins.core.technicaldebt; import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ListMultimap; import org.sonar.api.CoreProperties; import org.sonar.api.PropertyType; import org.sonar.api.batch.*; @@ -43,12 +41,13 @@ import org.sonar.api.rules.RuleFinder; import org.sonar.api.technicaldebt.batch.Characteristic; import org.sonar.api.technicaldebt.batch.TechnicalDebtModel; +import javax.annotation.CheckForNull; import javax.annotation.Nullable; import java.util.Arrays; -import java.util.Collection; import java.util.List; import java.util.Map; +import java.util.Set; import static com.google.common.collect.Lists.newArrayList; import static com.google.common.collect.Maps.newHashMap; @@ -93,39 +92,68 @@ public final class TechnicalDebtDecorator implements Decorator { } private void saveMeasures(DecoratorContext context, List issues) { - // group issues by rule keys - ListMultimap issuesByRule = issuesByRule(issues); - - double total = 0.0; - Map characteristicCosts = newHashMap(); - Map ruleDebtCosts = newHashMap(); - - for (Rule newRule : rules.findWithDebt()) { - String characteristicKey = newRule.characteristic(); - if (characteristicKey != null) { - org.sonar.api.rules.Rule rule = ruleFinder.findByKey(newRule.key()); - double value = computeTechnicalDebt(CoreMetrics.TECHNICAL_DEBT, context, rule, issuesByRule.get(newRule.key())); - ruleDebtCosts.put(rule, value); - total += value; - Characteristic characteristic = model.characteristicByKey(characteristicKey); - propagateTechnicalDebtInParents(characteristic, value, characteristicCosts); + Long total = 0L; + SumMap ruleDebts = new SumMap(); + SumMap characteristicDebts = new SumMap(); + + // Aggregate rules debt from current issues (and populate current characteristic debt) + for (Issue issue : issues) { + Long debt = ((DefaultIssue) issue).debtInMinutes(); + if (computeDebt(debt, issue.ruleKey(), ruleDebts, characteristicDebts)) { + total += debt; + } + } + + // Aggregate rules debt from children (and populate children characteristics debt) + for (Measure measure : context.getChildrenMeasures(MeasuresFilters.rules(CoreMetrics.TECHNICAL_DEBT))) { + Long debt = measure.getValue().longValue(); + RuleMeasure ruleMeasure = (RuleMeasure) measure; + if (computeDebt(debt, ruleMeasure.getRule().ruleKey(), ruleDebts, characteristicDebts)) { + total += debt; } } - context.saveMeasure(CoreMetrics.TECHNICAL_DEBT, total); - saveOnCharacteristic(context, characteristicCosts); - saveOnRule(context, ruleDebtCosts); + context.saveMeasure(CoreMetrics.TECHNICAL_DEBT, total.doubleValue()); + saveOnRule(context, ruleDebts); + + for (Characteristic characteristic : model.characteristics()) { + Long debt = characteristicDebts.get(characteristic); + saveTechnicalDebt(context, characteristic, debt != null ? debt.doubleValue() : 0d, false); + } + } + + private boolean computeDebt(@Nullable Long debt, RuleKey ruleKey, SumMap ruleDebts, SumMap characteristicDebts) { + if (debt != null) { + Rule rule = rules.find(ruleKey); + if (rule != null) { + String characteristicKey = rule.characteristic(); + if (characteristicKey != null) { + Characteristic characteristic = model.characteristicByKey(characteristicKey); + if (characteristic != null) { + ruleDebts.add(ruleKey, debt); + characteristicDebts.add(characteristic, debt); + propagateTechnicalDebtInParents(characteristic.parent(), debt, characteristicDebts); + return true; + } + } + } + } + return false; } - private void saveOnCharacteristic(DecoratorContext context, Map characteristicCosts) { - for (Map.Entry entry : characteristicCosts.entrySet()) { - saveTechnicalDebt(context, entry.getKey(), entry.getValue(), false); + private void propagateTechnicalDebtInParents(@Nullable Characteristic characteristic, long value, SumMap characteristicDebts) { + if (characteristic != null) { + characteristicDebts.add(characteristic, value); + propagateTechnicalDebtInParents(characteristic.parent(), value, characteristicDebts); } } - private void saveOnRule(DecoratorContext context, Map requirementCosts) { - for (Map.Entry entry : requirementCosts.entrySet()) { - saveTechnicalDebt(context, entry.getKey(), entry.getValue(), ResourceUtils.isEntity(context.getResource())); + private void saveOnRule(DecoratorContext context, SumMap ruleDebts) { + for (Map.Entry entry : ruleDebts.entrySet()) { + org.sonar.api.rules.Rule oldRule = ruleFinder.findByKey(entry.getKey()); + if (oldRule != null) { + saveTechnicalDebt(context, oldRule, entry.getValue().doubleValue(), ResourceUtils.isEntity(context.getResource())); + } } } @@ -158,48 +186,6 @@ public final class TechnicalDebtDecorator implements Decorator { context.saveMeasure(measure); } - @VisibleForTesting - ListMultimap issuesByRule(List issues) { - ListMultimap result = ArrayListMultimap.create(); - for (Issue issue : issues) { - result.put(issue.ruleKey(), issue); - } - return result; - } - - private double computeTechnicalDebt(Metric metric, DecoratorContext context, org.sonar.api.rules.Rule rule, Collection issues) { - long debt = 0L; - if (issues != null) { - for (Issue issue : issues) { - Long currentDebt = ((DefaultIssue) issue).debtInMinutes(); - if (currentDebt != null) { - debt += currentDebt; - } - } - } - - for (Measure measure : context.getChildrenMeasures(MeasuresFilters.rule(metric, rule))) { - // Comparison on rule is only used for unit test, otherwise no need to do this check - RuleMeasure ruleMeasure = (RuleMeasure) measure; - if (measure != null && ruleMeasure.getRule().equals(rule) && measure.getValue() != null) { - debt += measure.getValue(); - } - } - return debt; - } - - private void propagateTechnicalDebtInParents(@Nullable Characteristic characteristic, double value, Map characteristicCosts) { - if (characteristic != null) { - Double parentCost = characteristicCosts.get(characteristic); - if (parentCost == null) { - characteristicCosts.put(characteristic, value); - } else { - characteristicCosts.put(characteristic, value + parentCost); - } - propagateTechnicalDebtInParents(characteristic.parent(), value, characteristicCosts); - } - } - private boolean shouldSaveMeasure(DecoratorContext context) { return context.getMeasure(CoreMetrics.TECHNICAL_DEBT) == null; } @@ -216,4 +202,27 @@ public final class TechnicalDebtDecorator implements Decorator { ); } + private static class SumMap { + private Map sumByKeys; + + public SumMap() { + sumByKeys = newHashMap(); + } + + public void add(@Nullable E key, Long value) { + if (key != null) { + Long currentValue = sumByKeys.get(key); + sumByKeys.put(key, currentValue != null ? currentValue + value : value); + } + } + + @CheckForNull + public Long get(E key) { + return sumByKeys.get(key); + } + + public Set> entrySet() { + return sumByKeys.entrySet(); + } + } } diff --git a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/technicaldebt/TechnicalDebtDecoratorTest.java b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/technicaldebt/TechnicalDebtDecoratorTest.java index e24fb32d492..7e38616d66f 100644 --- a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/technicaldebt/TechnicalDebtDecoratorTest.java +++ b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/technicaldebt/TechnicalDebtDecoratorTest.java @@ -20,7 +20,8 @@ package org.sonar.plugins.core.technicaldebt; -import com.google.common.collect.ListMultimap; +import com.google.common.collect.Lists; +import edu.emory.mathcs.backport.java.util.Collections; import org.apache.commons.lang.ObjectUtils; import org.apache.commons.lang.builder.ToStringBuilder; import org.apache.commons.lang.builder.ToStringStyle; @@ -50,8 +51,6 @@ import org.sonar.api.technicaldebt.batch.internal.DefaultCharacteristic; import org.sonar.api.test.IsMeasure; import org.sonar.api.utils.Duration; -import java.util.List; - import static com.google.common.collect.Lists.newArrayList; import static org.fest.assertions.Assertions.assertThat; import static org.mockito.Matchers.any; @@ -71,7 +70,7 @@ public class TechnicalDebtDecoratorTest { Resource resource; @Mock - TechnicalDebtModel defaultTechnicalDebtModel; + TechnicalDebtModel debtModel; @Mock Issuable issuable; @@ -86,6 +85,12 @@ public class TechnicalDebtDecoratorTest { RuleKey ruleKey2 = RuleKey.of("repo2", "rule2"); Rules rules; + DefaultCharacteristic efficiency = new DefaultCharacteristic().setKey("EFFICIENCY"); + DefaultCharacteristic memoryEfficiency = new DefaultCharacteristic().setKey("MEMORY_EFFICIENCY").setParent(efficiency); + + DefaultCharacteristic reusability = new DefaultCharacteristic().setKey("REUSABILITY"); + DefaultCharacteristic modularity = new DefaultCharacteristic().setKey("MODULARITY").setParent(reusability); + TechnicalDebtDecorator decorator; @Before @@ -99,7 +104,13 @@ public class TechnicalDebtDecoratorTest { when(ruleFinder.findByKey(ruleKey1)).thenReturn(org.sonar.api.rules.Rule.create(ruleKey1.repository(), ruleKey1.rule())); when(ruleFinder.findByKey(ruleKey2)).thenReturn(org.sonar.api.rules.Rule.create(ruleKey2.repository(), ruleKey2.rule())); - decorator = new TechnicalDebtDecorator(perspectives, defaultTechnicalDebtModel, rules, ruleFinder); + when(debtModel.characteristics()).thenReturn(newArrayList(efficiency, memoryEfficiency, reusability, modularity)); + when(debtModel.characteristicByKey("EFFICIENCY")).thenReturn(efficiency); + when(debtModel.characteristicByKey("MEMORY_EFFICIENCY")).thenReturn(memoryEfficiency); + when(debtModel.characteristicByKey("REUSABILITY")).thenReturn(reusability); + when(debtModel.characteristicByKey("MODULARITY")).thenReturn(modularity); + + decorator = new TechnicalDebtDecorator(perspectives, debtModel, rules, ruleFinder); } @Test @@ -121,21 +132,6 @@ public class TechnicalDebtDecoratorTest { verify(context, never()).saveMeasure(argThat(new IsMeasure(CoreMetrics.TECHNICAL_DEBT))); } - @Test - public void group_issues_by_requirement() throws Exception { - Issue issue1 = createIssue("rule1", "repo1"); - Issue issue2 = createIssue("rule1", "repo1"); - Issue issue3 = createIssue("rule2", "repo2"); - - List issues = newArrayList(issue1, issue2, issue3); - - ListMultimap result = decorator.issuesByRule(issues); - - assertThat(result.keySet().size()).isEqualTo(2); - assertThat(result.get(ruleKey1)).containsExactly(issue1, issue2); - assertThat(result.get(ruleKey2)).containsExactly(issue3); - } - @Test public void add_technical_debt_from_one_issue_and_no_parent() throws Exception { Issue issue = createIssue("rule1", "repo1").setDebt(Duration.create(ONE_DAY_IN_MINUTES)); @@ -162,16 +158,12 @@ public class TechnicalDebtDecoratorTest { Issue issue = createIssue("rule1", "repo1").setDebt(Duration.create(ONE_DAY_IN_MINUTES)); when(issuable.issues()).thenReturn(newArrayList(issue)); - DefaultCharacteristic parentCharacteristic = new DefaultCharacteristic().setKey("EFFICIENCY"); - DefaultCharacteristic characteristic = new DefaultCharacteristic().setKey("MEMORY_EFFICIENCY").setParent(parentCharacteristic); - when(defaultTechnicalDebtModel.characteristicByKey("MEMORY_EFFICIENCY")).thenReturn(characteristic); - decorator.decorate(resource, context); verify(context).saveMeasure(CoreMetrics.TECHNICAL_DEBT, ONE_DAY_IN_MINUTES.doubleValue()); - verify(context).saveMeasure(argThat(new IsCharacteristicMeasure(CoreMetrics.TECHNICAL_DEBT, parentCharacteristic, ONE_DAY_IN_MINUTES.doubleValue()))); - verify(context).saveMeasure(argThat(new IsCharacteristicMeasure(CoreMetrics.TECHNICAL_DEBT, characteristic, ONE_DAY_IN_MINUTES.doubleValue()))); verify(context).saveMeasure(argThat(new IsRuleMeasure(CoreMetrics.TECHNICAL_DEBT, ruleKey1, ONE_DAY_IN_MINUTES.doubleValue()))); + verify(context).saveMeasure(argThat(new IsCharacteristicMeasure(CoreMetrics.TECHNICAL_DEBT, efficiency, ONE_DAY_IN_MINUTES.doubleValue()))); + verify(context).saveMeasure(argThat(new IsCharacteristicMeasure(CoreMetrics.TECHNICAL_DEBT, memoryEfficiency, ONE_DAY_IN_MINUTES.doubleValue()))); } @Test @@ -185,15 +177,6 @@ public class TechnicalDebtDecoratorTest { Issue issue4 = createIssue("rule2", "repo2").setDebt(Duration.create(technicalDebt2)); when(issuable.issues()).thenReturn(newArrayList(issue1, issue2, issue3, issue4)); - when(defaultTechnicalDebtModel.characteristicByKey("MEMORY_EFFICIENCY")).thenReturn( - new DefaultCharacteristic().setKey("MEMORY_EFFICIENCY").setParent( - new DefaultCharacteristic().setKey("EFFICIENCY") - ), - new DefaultCharacteristic().setKey("MODULARITY").setParent( - new DefaultCharacteristic().setKey("REUSABILITY") - ) - ); - decorator.decorate(resource, context); verify(context).saveMeasure(CoreMetrics.TECHNICAL_DEBT, 6d * ONE_DAY_IN_MINUTES); @@ -202,25 +185,46 @@ public class TechnicalDebtDecoratorTest { } @Test - public void add_technical_debt_from_children_measures() throws Exception { + public void add_technical_debt_from_current_and_children_measures() throws Exception { Issue issue1 = createIssue("rule1", "repo1").setDebt(Duration.create(ONE_DAY_IN_MINUTES)); Issue issue2 = createIssue("rule1", "repo1").setDebt(Duration.create(ONE_DAY_IN_MINUTES)); when(issuable.issues()).thenReturn(newArrayList(issue1, issue2)); - when(defaultTechnicalDebtModel.characteristicByKey("MEMORY_EFFICIENCY")).thenReturn( - new DefaultCharacteristic().setKey("MEMORY_EFFICIENCY").setParent( - new DefaultCharacteristic().setKey("EFFICIENCY") - ) - ); - - org.sonar.api.rules.Rule oldRule = org.sonar.api.rules.Rule.create(ruleKey1.repository(), ruleKey1.rule()); - Measure measure = new RuleMeasure(CoreMetrics.TECHNICAL_DEBT, oldRule, null, null).setValue(5d * ONE_DAY_IN_MINUTES); - when(context.getChildrenMeasures(any(MeasuresFilter.class))).thenReturn(newArrayList(measure)); - + when(context.getChildrenMeasures(any(MeasuresFilter.class))).thenReturn(Lists.newArrayList( + new RuleMeasure(CoreMetrics.TECHNICAL_DEBT, + org.sonar.api.rules.Rule.create(ruleKey1.repository(), ruleKey1.rule()), null, null) + .setValue(5d * ONE_DAY_IN_MINUTES) + )); decorator.decorate(resource, context); verify(context).saveMeasure(CoreMetrics.TECHNICAL_DEBT, 7d * ONE_DAY_IN_MINUTES); verify(context).saveMeasure(argThat(new IsRuleMeasure(CoreMetrics.TECHNICAL_DEBT, ruleKey1, 7d * ONE_DAY_IN_MINUTES))); + verify(context).saveMeasure(argThat(new IsCharacteristicMeasure(CoreMetrics.TECHNICAL_DEBT, memoryEfficiency, 7d * ONE_DAY_IN_MINUTES))); + verify(context).saveMeasure(argThat(new IsCharacteristicMeasure(CoreMetrics.TECHNICAL_DEBT, efficiency, 7d * ONE_DAY_IN_MINUTES))); + } + + @Test + public void add_technical_debt_only_from_children_measures() throws Exception { + when(issuable.issues()).thenReturn(Collections.emptyList()); + + when(context.getChildrenMeasures(any(MeasuresFilter.class))).thenReturn(Lists.newArrayList( + new RuleMeasure(CoreMetrics.TECHNICAL_DEBT, + org.sonar.api.rules.Rule.create(ruleKey1.repository(), ruleKey1.rule()) + , null, null).setValue(5d * ONE_DAY_IN_MINUTES), + + new RuleMeasure(CoreMetrics.TECHNICAL_DEBT, + org.sonar.api.rules.Rule.create(ruleKey2.repository(), ruleKey2.rule()) + , null, null).setValue(10d * ONE_DAY_IN_MINUTES) + )); + decorator.decorate(resource, context); + + verify(context).saveMeasure(CoreMetrics.TECHNICAL_DEBT, 15d * ONE_DAY_IN_MINUTES); + verify(context).saveMeasure(argThat(new IsRuleMeasure(CoreMetrics.TECHNICAL_DEBT, ruleKey1, 5d * ONE_DAY_IN_MINUTES))); + verify(context).saveMeasure(argThat(new IsRuleMeasure(CoreMetrics.TECHNICAL_DEBT, ruleKey2, 10d * ONE_DAY_IN_MINUTES))); + verify(context).saveMeasure(argThat(new IsCharacteristicMeasure(CoreMetrics.TECHNICAL_DEBT, memoryEfficiency, 5d * ONE_DAY_IN_MINUTES))); + verify(context).saveMeasure(argThat(new IsCharacteristicMeasure(CoreMetrics.TECHNICAL_DEBT, efficiency, 5d * ONE_DAY_IN_MINUTES))); + verify(context).saveMeasure(argThat(new IsCharacteristicMeasure(CoreMetrics.TECHNICAL_DEBT, reusability, 10d * ONE_DAY_IN_MINUTES))); + verify(context).saveMeasure(argThat(new IsCharacteristicMeasure(CoreMetrics.TECHNICAL_DEBT, modularity, 10d * ONE_DAY_IN_MINUTES))); } @Test @@ -242,6 +246,7 @@ public class TechnicalDebtDecoratorTest { public void always_save_technical_debt_for_project_if_top_characteristic() throws Exception { DecoratorContext context = mock(DecoratorContext.class); when(context.getResource()).thenReturn(new Project("foo")); + // this is a top characteristic DefaultCharacteristic rootCharacteristic = new DefaultCharacteristic().setKey("root"); @@ -306,7 +311,11 @@ public class TechnicalDebtDecoratorTest { @Override public void describeTo(Description description) { - description.appendText(ToStringBuilder.reflectionToString(this, ToStringStyle.SHORT_PREFIX_STYLE)); + description.appendText(new StringBuilder() + .append("value=").append(value).append(",") + .append("characteristic=").append(characteristic.key()).append(",") + .append("metric=").append(metric.getKey()).toString()) + ; } } diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/batch/rule/Rules.java b/sonar-plugin-api/src/main/java/org/sonar/api/batch/rule/Rules.java index 2a2ff644f32..14fb6cb7ac3 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/batch/rule/Rules.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/batch/rule/Rules.java @@ -39,6 +39,4 @@ public interface Rules { Collection findByRepository(String repository); - Collection findWithDebt(); - } diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/batch/rule/internal/DefaultRules.java b/sonar-plugin-api/src/main/java/org/sonar/api/batch/rule/internal/DefaultRules.java index 20de45fd89f..3c14387a31c 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/batch/rule/internal/DefaultRules.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/batch/rule/internal/DefaultRules.java @@ -31,16 +31,12 @@ import javax.annotation.concurrent.Immutable; import java.util.Collection; import java.util.List; -import static com.google.common.collect.Lists.newArrayList; - @Immutable class DefaultRules implements Rules { // TODO use disk-backed cache (persistit) instead of full in-memory cache ? private final ListMultimap rulesByRepository; - private final Collection rulesWithDebt; - DefaultRules(Collection newRules) { ImmutableListMultimap.Builder builder = ImmutableListMultimap.builder(); for (NewRule newRule : newRules) { @@ -48,13 +44,6 @@ class DefaultRules implements Rules { builder.put(r.key().repository(), r); } rulesByRepository = builder.build(); - - rulesWithDebt = newArrayList(); - for (Rule rule : rulesByRepository.values()) { - if (rule.characteristic() != null) { - rulesWithDebt.add(rule); - } - } } @Override @@ -77,9 +66,4 @@ class DefaultRules implements Rules { public Collection findByRepository(String repository) { return rulesByRepository.get(repository); } - - @Override - public Collection findWithDebt() { - return rulesWithDebt; - } } diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/technicaldebt/batch/TechnicalDebtModel.java b/sonar-plugin-api/src/main/java/org/sonar/api/technicaldebt/batch/TechnicalDebtModel.java index cf1b5bc1dd5..4cca21cd19f 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/technicaldebt/batch/TechnicalDebtModel.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/technicaldebt/batch/TechnicalDebtModel.java @@ -21,6 +21,7 @@ package org.sonar.api.technicaldebt.batch; import org.sonar.api.rule.RuleKey; +import org.sonar.api.technicaldebt.batch.internal.DefaultCharacteristic; import javax.annotation.CheckForNull; @@ -57,4 +58,9 @@ public interface TechnicalDebtModel { @Deprecated List requirements(); + /** + * @since 4.3 + */ + List characteristics(); + }