From: Julien HENRY Date: Mon, 25 Aug 2014 16:03:54 +0000 (+0200) Subject: SONAR-5528 Improve performance of syntax highlighting API X-Git-Tag: 4.5-RC1~98 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=1cf7fa356ee647d94db8ed92996d3a7acf3625ae;p=sonarqube.git SONAR-5528 Improve performance of syntax highlighting API --- diff --git a/sonar-batch/src/main/java/org/sonar/batch/highlighting/SyntaxHighlightingData.java b/sonar-batch/src/main/java/org/sonar/batch/highlighting/SyntaxHighlightingData.java index 56c9b795ac9..8e4f5314ec6 100644 --- a/sonar-batch/src/main/java/org/sonar/batch/highlighting/SyntaxHighlightingData.java +++ b/sonar-batch/src/main/java/org/sonar/batch/highlighting/SyntaxHighlightingData.java @@ -21,29 +21,28 @@ package org.sonar.batch.highlighting; import org.sonar.batch.index.Data; -import java.util.List; +import java.util.ArrayList; +import java.util.Collection; public class SyntaxHighlightingData implements Data { private static final String FIELD_SEPARATOR = ","; private static final String RULE_SEPARATOR = ";"; - private List syntaxHighlightingRuleSet; + private Collection syntaxHighlightingRuleSet; - public SyntaxHighlightingData(List syntaxHighlightingRuleSet) { - this.syntaxHighlightingRuleSet = syntaxHighlightingRuleSet; + public SyntaxHighlightingData(Collection syntaxHighlightingRuleSet) { + this.syntaxHighlightingRuleSet = new ArrayList(syntaxHighlightingRuleSet); } - public List syntaxHighlightingRuleSet() { + public Collection syntaxHighlightingRuleSet() { return syntaxHighlightingRuleSet; } @Override public String writeString() { StringBuilder sb = new StringBuilder(); - List orderedRules = syntaxHighlightingRuleSet; - - for (SyntaxHighlightingRule highlightingRule : orderedRules) { + for (SyntaxHighlightingRule highlightingRule : syntaxHighlightingRuleSet) { sb.append(highlightingRule.getStartPosition()) .append(FIELD_SEPARATOR) .append(highlightingRule.getEndPosition()) diff --git a/sonar-batch/src/main/java/org/sonar/batch/highlighting/SyntaxHighlightingDataBuilder.java b/sonar-batch/src/main/java/org/sonar/batch/highlighting/SyntaxHighlightingDataBuilder.java index 3a74dbf4884..792cfb2eb3a 100644 --- a/sonar-batch/src/main/java/org/sonar/batch/highlighting/SyntaxHighlightingDataBuilder.java +++ b/sonar-batch/src/main/java/org/sonar/batch/highlighting/SyntaxHighlightingDataBuilder.java @@ -20,33 +20,38 @@ package org.sonar.batch.highlighting; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Predicate; -import com.google.common.collect.Collections2; import com.google.common.collect.Ordering; -import org.slf4j.LoggerFactory; +import org.elasticsearch.common.collect.Sets; import javax.annotation.Nullable; -import java.util.Collection; -import java.util.List; - -import static com.google.common.collect.Lists.newArrayList; +import java.util.Iterator; +import java.util.Set; public class SyntaxHighlightingDataBuilder { - private List syntaxHighlightingRuleSet; + private Set syntaxHighlightingRuleSet; public SyntaxHighlightingDataBuilder() { - syntaxHighlightingRuleSet = newArrayList(); + syntaxHighlightingRuleSet = Sets.newTreeSet(new Ordering() { + @Override + public int compare(@Nullable SyntaxHighlightingRule left, + @Nullable SyntaxHighlightingRule right) { + int result = left.getStartPosition() - right.getStartPosition(); + if (result == 0) { + result = left.getEndPosition() - right.getEndPosition(); + } + return result; + } + }); + } + + @VisibleForTesting + public Set getSyntaxHighlightingRuleSet() { + return syntaxHighlightingRuleSet; } public SyntaxHighlightingDataBuilder registerHighlightingRule(int startOffset, int endOffset, String typeOfText) { - if (ruleConflictsWithExistingRules(startOffset, endOffset)) { - String errorMsg = String.format("Cannot register highlighting rule for characters from %s to %s as it " + - "overlaps at least one existing rule", startOffset, endOffset); - LoggerFactory.getLogger(SyntaxHighlightingDataBuilder.class).error(errorMsg); - throw new UnsupportedOperationException(errorMsg); - } SyntaxHighlightingRule syntaxHighlightingRule = SyntaxHighlightingRule.create(startOffset, endOffset, typeOfText); this.syntaxHighlightingRuleSet.add(syntaxHighlightingRule); @@ -54,50 +59,24 @@ public class SyntaxHighlightingDataBuilder { } public SyntaxHighlightingData build() { - return new SyntaxHighlightingData(getSortedRules()); + checkOverlappingBoudaries(); + return new SyntaxHighlightingData(syntaxHighlightingRuleSet); } - private boolean ruleConflictsWithExistingRules(final int startOffset, final int endOffset) { - Collection conflictingRules = Collections2 - .filter(syntaxHighlightingRuleSet, new Predicate() { - @Override - public boolean apply(@Nullable SyntaxHighlightingRule syntaxHighlightingRule) { - - if (syntaxHighlightingRule != null) { - boolean overlapsStartBoundary = startOffset < syntaxHighlightingRule.getStartPosition() - && endOffset >= syntaxHighlightingRule.getStartPosition() + 1 - && endOffset < syntaxHighlightingRule.getEndPosition(); - - boolean overlapsEndBoundary = startOffset > syntaxHighlightingRule.getStartPosition() - && startOffset < syntaxHighlightingRule.getEndPosition() - && endOffset > syntaxHighlightingRule.getEndPosition(); - - return overlapsStartBoundary || overlapsEndBoundary; - } - return false; + private void checkOverlappingBoudaries() { + if (syntaxHighlightingRuleSet.size() > 1) { + Iterator it = syntaxHighlightingRuleSet.iterator(); + SyntaxHighlightingRule previous = it.next(); + while (it.hasNext()) { + SyntaxHighlightingRule current = it.next(); + if (previous.getEndPosition() > current.getStartPosition() && !(previous.getEndPosition() >= current.getEndPosition())) { + String errorMsg = String.format("Cannot register highlighting rule for characters from %s to %s as it " + + "overlaps at least one existing rule", current.getStartPosition(), current.getEndPosition()); + throw new IllegalStateException(errorMsg); } - }); - return !conflictingRules.isEmpty(); - } - - @VisibleForTesting - public List getSortedRules() { - Ordering ruleOrdering = new Ordering() { - @Override - public int compare(@Nullable SyntaxHighlightingRule left, - @Nullable SyntaxHighlightingRule right) { - int result; - if (left != null && right != null) { - result = left.getStartPosition() - right.getStartPosition(); - if (result == 0) { - result = left.getEndPosition() - right.getEndPosition(); - } - return result; - } - return left != null ? 1 : -1; + previous = current; } - }; - - return ruleOrdering.sortedCopy(syntaxHighlightingRuleSet); + } } + } diff --git a/sonar-batch/src/test/java/org/sonar/batch/highlighting/SyntaxHighlightingDataBuilderTest.java b/sonar-batch/src/test/java/org/sonar/batch/highlighting/SyntaxHighlightingDataBuilderTest.java index 6fad264c831..e511bc388de 100644 --- a/sonar-batch/src/test/java/org/sonar/batch/highlighting/SyntaxHighlightingDataBuilderTest.java +++ b/sonar-batch/src/test/java/org/sonar/batch/highlighting/SyntaxHighlightingDataBuilderTest.java @@ -19,20 +19,18 @@ */ package org.sonar.batch.highlighting; - -import org.sonar.batch.highlighting.SyntaxHighlightingDataBuilder; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; -import java.util.List; +import java.util.Collection; import static org.fest.assertions.Assertions.assertThat; public class SyntaxHighlightingDataBuilderTest { - private List highlightingRules; + private Collection highlightingRules; @Rule public ExpectedException throwable = ExpectedException.none(); @@ -48,7 +46,7 @@ public class SyntaxHighlightingDataBuilderTest { highlightingDataBuilder.registerHighlightingRule(24, 65, "cppd"); highlightingDataBuilder.registerHighlightingRule(12, 20, "cd"); - highlightingRules = highlightingDataBuilder.getSortedRules(); + highlightingRules = highlightingDataBuilder.getSyntaxHighlightingRuleSet(); } @Test @@ -58,17 +56,27 @@ public class SyntaxHighlightingDataBuilderTest { @Test public void should_order_by_start_then_end_offset() throws Exception { - assertThat(highlightingRules).onProperty("startPosition").containsExactly(0, 10, 12, 24, 24, 42); - assertThat(highlightingRules).onProperty("endPosition").containsExactly(10, 12, 20, 38, 65, 50); - assertThat(highlightingRules).onProperty("textType").containsExactly("cd", "k", "cd", "k", "cppd", "k"); + assertThat(highlightingRules).onProperty("startPosition").containsOnly(0, 10, 12, 24, 24, 42); + assertThat(highlightingRules).onProperty("endPosition").containsOnly(10, 12, 20, 38, 65, 50); + assertThat(highlightingRules).onProperty("textType").containsOnly("cd", "k", "cd", "k", "cppd", "k"); + } + + @Test + public void should_suport_overlapping() throws Exception { + SyntaxHighlightingDataBuilder builder = new SyntaxHighlightingDataBuilder(); + builder.registerHighlightingRule(0, 15, "k"); + builder.registerHighlightingRule(8, 12, "cppd"); + builder.build(); } @Test - public void should_prevent_rules_overlapping() throws Exception { - throwable.expect(UnsupportedOperationException.class); + public void should_prevent_boudaries_overlapping() throws Exception { + throwable.expect(IllegalStateException.class); + throwable.expectMessage("Cannot register highlighting rule for characters from 8 to 15 as it overlaps at least one existing rule"); SyntaxHighlightingDataBuilder builder = new SyntaxHighlightingDataBuilder(); builder.registerHighlightingRule(0, 10, "k"); builder.registerHighlightingRule(8, 15, "k"); + builder.build(); } } diff --git a/sonar-batch/src/test/java/org/sonar/batch/mediumtest/highlighting/HighlightingMediumTest.java b/sonar-batch/src/test/java/org/sonar/batch/mediumtest/highlighting/HighlightingMediumTest.java index b9fab7ba450..6b0501ec9fc 100644 --- a/sonar-batch/src/test/java/org/sonar/batch/mediumtest/highlighting/HighlightingMediumTest.java +++ b/sonar-batch/src/test/java/org/sonar/batch/mediumtest/highlighting/HighlightingMediumTest.java @@ -23,8 +23,10 @@ import com.google.common.collect.ImmutableMap; import org.apache.commons.io.FileUtils; import org.junit.After; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; +import org.junit.rules.TestName; import org.sonar.api.batch.fs.InputFile; import org.sonar.api.batch.sensor.highlighting.HighlightingBuilder; import org.sonar.batch.mediumtest.BatchMediumTester; @@ -38,9 +40,12 @@ import static org.fest.assertions.Assertions.assertThat; public class HighlightingMediumTest { - @org.junit.Rule + @Rule public TemporaryFolder temp = new TemporaryFolder(); + @Rule + public TestName testName = new TestName(); + public BatchMediumTester tester = BatchMediumTester.builder() .registerPlugin("xoo", new XooPlugin()) .addDefaultQProfile("xoo", "Sonar Way") @@ -89,4 +94,46 @@ public class HighlightingMediumTest { } + @Test + public void computeSyntaxHighlightingOnBigFile() throws IOException { + + File baseDir = temp.newFolder(); + File srcDir = new File(baseDir, "src"); + srcDir.mkdir(); + + File xooFile = new File(srcDir, "sample.xoo"); + File xoohighlightingFile = new File(srcDir, "sample.xoo.highlighting"); + FileUtils.write(xooFile, "Sample xoo\ncontent"); + int chunkSize = 100000; + StringBuilder sb = new StringBuilder(16 * chunkSize); + for (int i = 0; i < chunkSize; i++) { + sb.append(i).append(":").append(i + 1).append(":s\n"); + } + FileUtils.write(xoohighlightingFile, sb.toString()); + + long start = System.currentTimeMillis(); + TaskResult result = tester.newTask() + .properties(ImmutableMap.builder() + .put("sonar.task", "scan") + .put("sonar.projectBaseDir", baseDir.getAbsolutePath()) + .put("sonar.projectKey", "com.foo.project") + .put("sonar.projectName", "Foo Project") + .put("sonar.projectVersion", "1.0-SNAPSHOT") + .put("sonar.projectDescription", "Description of Foo Project") + .put("sonar.sources", "src") + .build()) + .start(); + long duration = System.currentTimeMillis() - start; + assertDurationLessThan(duration, 20000L); + + InputFile file = result.inputFiles().get(0); + assertThat(result.highlightingTypeFor(file, 0)).isEqualTo(HighlightingBuilder.TypeOfText.STRING); + + } + + void assertDurationLessThan(long duration, long maxDuration) { + assertThat(duration).as(String.format("Expected less than %d ms, got %d ms", maxDuration, duration)).isLessThanOrEqualTo(maxDuration); + System.out.printf("Test %s : %d ms (max allowed is %d)\n", testName.getMethodName(), duration, maxDuration); + } + } diff --git a/sonar-batch/src/test/java/org/sonar/batch/source/DefaultHighlightableTest.java b/sonar-batch/src/test/java/org/sonar/batch/source/DefaultHighlightableTest.java index 2f98e07cc79..efa8024c6a9 100644 --- a/sonar-batch/src/test/java/org/sonar/batch/source/DefaultHighlightableTest.java +++ b/sonar-batch/src/test/java/org/sonar/batch/source/DefaultHighlightableTest.java @@ -44,7 +44,7 @@ public class DefaultHighlightableTest { DefaultHighlightable highlightablePerspective = new DefaultHighlightable(mock(Component.class), null); highlightablePerspective.newHighlighting().highlight(0, 10, "k").highlight(20, 30, "cppd"); - assertThat(highlightablePerspective.getHighlightingRules().getSortedRules()).hasSize(2); + assertThat(highlightablePerspective.getHighlightingRules().getSyntaxHighlightingRuleSet()).hasSize(2); } @Test diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/batch/sensor/highlighting/HighlightingBuilder.java b/sonar-plugin-api/src/main/java/org/sonar/api/batch/sensor/highlighting/HighlightingBuilder.java index db8d5c46e93..0acc7848e22 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/batch/sensor/highlighting/HighlightingBuilder.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/batch/sensor/highlighting/HighlightingBuilder.java @@ -19,7 +19,6 @@ */ package org.sonar.api.batch.sensor.highlighting; - /** * This builder is used to define syntax highlighting (aka code coloration) on files. * @since 4.5 @@ -73,6 +72,7 @@ public interface HighlightingBuilder { /** * Call this method only once when your are done with defining highlighting of the file. + * @throws IllegalStateException if you have defined overlapping highlighting */ void done(); }