ss: Bug 475300: Fix inconsistent TreeMapStore due to value comparators
authorPatrick Tasse <patrick.tasse@gmail.com>
Wed, 19 Aug 2015 17:34:09 +0000 (13:34 -0400)
committerPatrick Tasse <patrick.tasse@gmail.com>
Thu, 20 Aug 2015 20:17:36 +0000 (16:17 -0400)
Add Comparable to the ISegment interface for segments to specify
a natural ordering. This ordering will then be used by the tree map
to "break ties" when segments have the same start and end times.

Also removed the getElementAtIndex() method. It was not doing what
the Javadoc said it did, and it does not seem to be useful for the
analysis as it is.

Change-Id: Ic65f50b12f6e94c59d4a60f0e96d9c78ee04b741
Signed-off-by: Patrick Tasse <patrick.tasse@gmail.com>
Signed-off-by: Alexandre Montplaisir <alexmonthy@voxpopuli.im>
Reviewed-on: https://git.eclipse.org/r/54146
Reviewed-by: Hudson CI
statesystem/org.eclipse.tracecompass.segmentstore.core.tests/src/org/eclipse/tracecompass/segmentstore/core/tests/treemap/TreeMapStoreTest.java
statesystem/org.eclipse.tracecompass.segmentstore.core/src/org/eclipse/tracecompass/segmentstore/core/BasicSegment.java
statesystem/org.eclipse.tracecompass.segmentstore.core/src/org/eclipse/tracecompass/segmentstore/core/ISegment.java
statesystem/org.eclipse.tracecompass.segmentstore.core/src/org/eclipse/tracecompass/segmentstore/core/ISegmentStore.java
statesystem/org.eclipse.tracecompass.segmentstore.core/src/org/eclipse/tracecompass/segmentstore/core/SegmentComparators.java [new file with mode: 0644]
statesystem/org.eclipse.tracecompass.segmentstore.core/src/org/eclipse/tracecompass/segmentstore/core/treemap/TreeMapStore.java

index ba8a0e5eede894ffb9286daa6032cc91dfd5c921..f9f112133b3aa60f1e9dfdf4a47b59681932a20a 100644 (file)
@@ -9,11 +9,11 @@
 
 package org.eclipse.tracecompass.segmentstore.core.tests.treemap;
 
+import static org.eclipse.tracecompass.common.core.NonNullUtils.checkNotNull;
 import static org.junit.Assert.assertEquals;
 
 import java.util.List;
 
-import org.eclipse.tracecompass.common.core.NonNullUtils;
 import org.eclipse.tracecompass.segmentstore.core.BasicSegment;
 import org.eclipse.tracecompass.segmentstore.core.ISegment;
 import org.eclipse.tracecompass.segmentstore.core.treemap.TreeMapStore;
@@ -23,6 +23,7 @@ import org.junit.Test;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
 
 /**
  * Unit tests for intersecting elements in a TreeMapStore
@@ -33,11 +34,14 @@ public class TreeMapStoreTest {
 
     private TreeMapStore<ISegment> fSegmentStore;
 
-    private static final ISegment SEGMENT_2_4 = new BasicSegment(2, 4);
+    private static final ISegment SEGMENT_2_6 = new BasicSegment(2, 6);
+    private static final ISegment SEGMENT_4_6 = new BasicSegment(4, 6);
+    private static final ISegment SEGMENT_4_8 = new BasicSegment(4, 8);
     private static final ISegment SEGMENT_6_8 = new BasicSegment(6, 8);
     private static final ISegment SEGMENT_10_14 = new BasicSegment(10, 14);
 
-    private static final List<ISegment> SEGMENTS = ImmutableList.of(SEGMENT_2_4, SEGMENT_6_8, SEGMENT_10_14);
+    private static final List<ISegment> SEGMENTS = ImmutableList.of(SEGMENT_2_6, SEGMENT_4_6, SEGMENT_4_8, SEGMENT_6_8, SEGMENT_10_14);
+    private static final List<ISegment> REVERSE_SEGMENTS = Lists.reverse(SEGMENTS);
 
     /**
      * Initialize data (test vector) that will be tested
@@ -45,8 +49,8 @@ public class TreeMapStoreTest {
     @Before
     public void setup() {
         fSegmentStore = new TreeMapStore<>();
-        for (int i = 0; i < SEGMENTS.size(); i++) {
-            fSegmentStore.addElement(NonNullUtils.checkNotNull(SEGMENTS.get(i)));
+        for (ISegment segment : SEGMENTS) {
+            fSegmentStore.addElement(checkNotNull(segment));
         }
     }
 
@@ -67,15 +71,52 @@ public class TreeMapStoreTest {
     }
 
     /**
-     * Testing method getElementAtIndex
+     * Try adding duplicate elements, they should be ignored
      */
     @Test
-    public void testGetElementAtIndex() {
-        for (int i = 0; i < SEGMENTS.size(); i++) {
-            assertEquals(SEGMENTS.get(i), fSegmentStore.getElementAtIndex(i));
+    public void testNoDuplicateElements() {
+        for (ISegment segment : SEGMENTS) {
+            fSegmentStore.addElement(new BasicSegment(segment.getStart(), segment.getEnd()));
+        }
+        assertEquals(SEGMENTS.size(), fSegmentStore.getNbElements());
+    }
+
+    /**
+     * Test the iteration order of the complete segment store.
+     */
+    @Test
+    public void testIterationOrder() {
+        int i = 0;
+        for (ISegment segment : fSegmentStore) {
+            assertEquals(SEGMENTS.get(i++), segment);
         }
     }
 
+    /**
+     * Test the iteration order when the elements are not inserted in sorted
+     * order.
+     */
+    @Test
+    public void testIterationOrderNonSortedInsertion() {
+        /* Prepare the segment store, we don't use the 'fixture' in this test */
+        TreeMapStore<ISegment> store = new TreeMapStore<>();
+        for (ISegment segment : REVERSE_SEGMENTS) {
+            store.addElement(checkNotNull(segment));
+        }
+
+        /*
+         * Test each element one by one, the iteration order should follow the
+         * start times, not the insertion order.
+         */
+        int i = 0;
+        for (ISegment segment : store) {
+            assertEquals(SEGMENTS.get(i++), segment);
+        }
+
+        /* Manually dispose our own store */
+        store.dispose();
+    }
+
     /**
      * Testing method getIntersectingElements(long start, long end)
      */
@@ -91,50 +132,50 @@ public class TreeMapStoreTest {
         assertEquals(0, Iterables.size(intersectingElements));
 
         /*
-         * Range start time : Before first segment start time Range end time :
-         * After last segment end time
+         * Range start time : Before first segment start time
+         * Range end time : After last segment end time
          */
         intersectingElements = fSegmentStore.getIntersectingElements(1, 15);
-        assertEquals(3, Iterables.size(intersectingElements));
+        assertEquals(5, Iterables.size(intersectingElements));
 
         /*
-         * Range start time : On first segment start time Range end time : On
-         * last segment end time
+         * Range start time : On first segment start time
+         * Range end time : On last segment end time
          */
         intersectingElements = fSegmentStore.getIntersectingElements(2, 14);
-        assertEquals(3, Iterables.size(intersectingElements));
+        assertEquals(5, Iterables.size(intersectingElements));
 
         /*
-         * Range start time : After one segment start time Range end time :
-         * Before one segment end time
+         * Range start time : After one segment start time
+         * Range end time : Before one segment end time
          */
         intersectingElements = fSegmentStore.getIntersectingElements(11, 13);
         assertEquals(1, Iterables.size(intersectingElements));
-        assert (SEGMENT_10_14.equals(intersectingElements));
+        assertEquals(SEGMENT_10_14, Iterables.getOnlyElement(intersectingElements));
 
         /*
-         * Range start time : On one segment start time Range end time : On one
-         * segment end time
+         * Range start time : On one segment start time
+         * Range end time : On one segment end time
          */
-        intersectingElements = fSegmentStore.getIntersectingElements(6, 8);
+        intersectingElements = fSegmentStore.getIntersectingElements(10, 14);
         assertEquals(1, Iterables.size(intersectingElements));
-        assert (SEGMENT_6_8.equals(intersectingElements));
+        assertEquals(SEGMENT_10_14, Iterables.getOnlyElement(intersectingElements));
 
         /*
-         * Range start time : On last segment end time Range end time : After
-         * last segment end time
+         * Range start time : On last segment end time
+         * Range end time : After last segment end time
          */
         intersectingElements = fSegmentStore.getIntersectingElements(14, 18);
         assertEquals(1, Iterables.size(intersectingElements));
-        assert (SEGMENT_10_14.equals(intersectingElements));
+        assertEquals(SEGMENT_10_14, Iterables.getOnlyElement(intersectingElements));
 
         /*
-         * Range start time : Before first segment start time Range end time :
-         * On first segment start time
+         * Range start time : Before first segment start time
+         * Range end time : On first segment start time
          */
         intersectingElements = fSegmentStore.getIntersectingElements(1, 2);
         assertEquals(1, Iterables.size(intersectingElements));
-        assert (SEGMENT_2_4.equals(intersectingElements));
+        assertEquals(SEGMENT_2_6, Iterables.getOnlyElement(intersectingElements));
     }
 
     /**
@@ -150,21 +191,33 @@ public class TreeMapStoreTest {
          */
         intersectingElements = fSegmentStore.getIntersectingElements(3);
         assertEquals(1, Iterables.size(intersectingElements));
-        assert (SEGMENT_2_4.equals(intersectingElements));
+        assertEquals(SEGMENT_2_6, Iterables.getOnlyElement(intersectingElements));
 
         /*
          * Time on segment start time
          */
         intersectingElements = fSegmentStore.getIntersectingElements(2);
         assertEquals(1, Iterables.size(intersectingElements));
-        assert (SEGMENT_2_4.equals(intersectingElements));
+        assertEquals(SEGMENT_2_6, Iterables.getOnlyElement(intersectingElements));
 
         /*
          * Time on segment end time
          */
-        intersectingElements = fSegmentStore.getIntersectingElements(4);
+        intersectingElements = fSegmentStore.getIntersectingElements(14);
         assertEquals(1, Iterables.size(intersectingElements));
-        assert (SEGMENT_2_4.equals(intersectingElements));
+        assertEquals(SEGMENT_10_14, Iterables.getOnlyElement(intersectingElements));
+
+        /*
+         * Time overlapping many segments
+         */
+        intersectingElements = fSegmentStore.getIntersectingElements(6);
+        assertEquals(4, Iterables.size(intersectingElements));
+
+        /*
+         * Time between segments
+         */
+        intersectingElements = fSegmentStore.getIntersectingElements(9);
+        assertEquals(0, Iterables.size(intersectingElements));
 
         /*
          * Time before all segment start time
@@ -185,7 +238,7 @@ public class TreeMapStoreTest {
     @Test
     public void testDispose() {
         TreeMapStore<ISegment> store = new TreeMapStore<>();
-        store.addElement(NonNullUtils.checkNotNull(SEGMENT_2_4));
+        store.addElement(checkNotNull(SEGMENT_2_6));
         store.dispose();
         assertEquals(0, store.getNbElements());
     }
index 8abd688c63cbb9e24bb4bc2b0960396ec8bf4f48..252b76eaef5c6a91e4018dc176c0007b69fe77a4 100644 (file)
@@ -9,6 +9,14 @@
 
 package org.eclipse.tracecompass.segmentstore.core;
 
+import static org.eclipse.tracecompass.common.core.NonNullUtils.checkNotNull;
+
+import java.util.Comparator;
+
+import org.eclipse.jdt.annotation.Nullable;
+
+import com.google.common.collect.Ordering;
+
 /**
  * Basic implementation of {@link ISegment}.
  *
@@ -18,6 +26,10 @@ public class BasicSegment implements ISegment {
 
     private static final long serialVersionUID = -3257452887960883177L;
 
+    private static final Comparator<ISegment> COMPARATOR = checkNotNull(Ordering
+            .from(SegmentComparators.INTERVAL_START_COMPARATOR)
+            .compound(SegmentComparators.INTERVAL_END_COMPARATOR));
+
     private final long fStart;
     private final long fEnd;
 
@@ -54,9 +66,16 @@ public class BasicSegment implements ISegment {
         return (fEnd - fStart);
     }
 
+    @Override
+    public int compareTo(@Nullable ISegment o) {
+        if (o == null) {
+            throw new IllegalArgumentException();
+        }
+        return COMPARATOR.compare(this, o);
+    }
+
     @Override
     public String toString() {
         return new String('[' + String.valueOf(fStart) + ", " + String.valueOf(fEnd) + ']'); //$NON-NLS-1$
     }
-
 }
index 0b6d6b615422ecb379b0b4aa8322019120ccff3b..8b21c4c14fc7c0da0d87d4e1d6c90ab0f2f11de8 100644 (file)
@@ -20,7 +20,7 @@ import java.io.Serializable;
  *
  * @author Alexandre Montplaisir
  */
-public interface ISegment extends Serializable {
+public interface ISegment extends Serializable, Comparable<ISegment> {
 
     /**
      * The start position/time of the segment.
index 8d1931ab5fc8e7f2c8f9cb41a6c75ec1c8b81531..41af51d1442467ab303c59af30d518a7f48bc3e0 100644 (file)
@@ -37,19 +37,6 @@ public interface ISegmentStore<T extends ISegment> extends Iterable<T> {
      */
     long getNbElements();
 
-    /**
-     * To seek rapidly among all elements, the elements should be indexed by
-     * their ascending order of start times.
-     *
-     * This method returns an individual element, given a position in this
-     * index.
-     *
-     * @param index
-     *            Retrieve the element at this index
-     * @return The element at this index
-     */
-    T getElementAtIndex(long index);
-
     /**
      * Retrieve all elements that inclusively cross the given position.
      *
diff --git a/statesystem/org.eclipse.tracecompass.segmentstore.core/src/org/eclipse/tracecompass/segmentstore/core/SegmentComparators.java b/statesystem/org.eclipse.tracecompass.segmentstore.core/src/org/eclipse/tracecompass/segmentstore/core/SegmentComparators.java
new file mode 100644 (file)
index 0000000..2335bc5
--- /dev/null
@@ -0,0 +1,76 @@
+/*******************************************************************************
+ * Copyright (c) 2015 EfficiOS Inc., Alexandre Montplaisir
+ *
+ * All rights reserved. This program and the accompanying materials are
+ * made available under the terms of the Eclipse Public License v1.0 which
+ * accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *******************************************************************************/
+
+package org.eclipse.tracecompass.segmentstore.core;
+
+import java.util.Comparator;
+
+import org.eclipse.jdt.annotation.Nullable;
+
+/**
+ * Segments comparators. These do not allow for null arguments.
+ *
+ * @author Alexandre Montplaisir
+ * @noimplement This interface only contains static definitions.
+ */
+public interface SegmentComparators {
+
+    /**
+     * Basic long comparator
+     */
+    Comparator<Long> LONG_COMPARATOR = new Comparator<Long>() {
+        @Override
+        public int compare(@Nullable Long o1, @Nullable Long o2) {
+            if (o1 == null || o2 == null) {
+                throw new IllegalArgumentException();
+            }
+            return o1.compareTo(o2);
+        }
+    };
+
+    /**
+     * Start time comparator
+     */
+    Comparator<ISegment> INTERVAL_START_COMPARATOR = new Comparator<ISegment>() {
+        @Override
+        public int compare(@Nullable ISegment o1, @Nullable ISegment o2) {
+            if (o1 == null || o2 == null) {
+                throw new IllegalArgumentException();
+            }
+            return Long.compare(o1.getStart(), o2.getStart());
+        }
+    };
+
+    /**
+     * End time comparator
+     */
+    Comparator<ISegment> INTERVAL_END_COMPARATOR = new Comparator<ISegment>() {
+        @Override
+        public int compare(@Nullable ISegment o1, @Nullable ISegment o2) {
+            if (o1 == null || o2 == null) {
+                throw new IllegalArgumentException();
+            }
+            return Long.compare(o1.getEnd(), o2.getEnd());
+        }
+    };
+
+    /**
+     * Length comparator
+     */
+    Comparator<ISegment> INTERVAL_LENGTH_COMPARATOR = new Comparator<ISegment>() {
+        @Override
+        public int compare(@Nullable ISegment o1, @Nullable ISegment o2) {
+            if (o1 == null || o2 == null) {
+                throw new IllegalArgumentException();
+            }
+            return Long.compare(o1.getLength(), o2.getLength());
+        }
+    };
+
+}
index a013aa960b2f3371504c4e3cea68bfd2c25f480d..e6a0f93897db8d06a059b8600fc16fce68c2b254 100644 (file)
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2015 EfficiOS Inc., Alexandre Montplaisir
+ * Copyright (c) 2015 EfficiOS Inc., Alexandre Montplaisir and others.
  *
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
@@ -14,17 +14,15 @@ package org.eclipse.tracecompass.segmentstore.core.treemap;
 
 import static org.eclipse.tracecompass.common.core.NonNullUtils.checkNotNull;
 
-import java.util.Comparator;
-import java.util.HashMap;
 import java.util.Iterator;
-import java.util.Map;
 import java.util.TreeMap;
 
-import org.eclipse.jdt.annotation.Nullable;
 import org.eclipse.tracecompass.segmentstore.core.ISegment;
 import org.eclipse.tracecompass.segmentstore.core.ISegmentStore;
+import org.eclipse.tracecompass.segmentstore.core.SegmentComparators;
 
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Ordering;
 import com.google.common.collect.Sets;
 import com.google.common.collect.TreeMultimap;
 
@@ -33,8 +31,17 @@ import com.google.common.collect.TreeMultimap;
  * This relatively simple implementation holds everything in memory, and as such
  * cannot contain too much data.
  *
+ * The TreeMapStore itself is Iterable, and its iteration order will be by
+ * ascending order of start times. For segments with identical start times, the
+ * secondary comparator will be the end time. If even those are equal, it will
+ * defer to the segments' natural ordering ({@link ISegment#compareTo}).
+ *
+ * The store's tree maps will not accept duplicate key-value pairs, which means
+ * that if you want several segments with the same start and end times, make
+ * sure their compareTo() differentiates them.
+ *
  * @param <T>
- *            The time of time range held
+ *            The type of segment held in this store
  *
  * @author Alexandre Montplaisir
  */
@@ -43,17 +50,32 @@ public class TreeMapStore<T extends ISegment> implements ISegmentStore<T> {
     private final TreeMultimap<Long, T> fStartTimesIndex;
     private final TreeMultimap<Long, T> fEndTimesIndex;
 
-    private final Map<Long, T> fPositionMap;
-
     private volatile long fSize;
 
     /**
-     *Constructor
+     * Constructor
      */
     public TreeMapStore() {
-        fStartTimesIndex = checkNotNull(TreeMultimap.<Long, T> create(LONG_COMPARATOR, INTERVAL_START_COMPARATOR));
-        fEndTimesIndex = checkNotNull(TreeMultimap.<Long, T> create(LONG_COMPARATOR, INTERVAL_END_COMPARATOR));
-        fPositionMap = new HashMap<>();
+        /*
+         * For the start times index, the "key comparator" will compare the
+         * start times as longs directly. This is the primary comparator for its
+         * tree map.
+         *
+         * The secondary "value" comparator will check the end times first, and
+         * in the event of a tie, defer to the ISegment's Comparable
+         * implementation, a.k.a. its natural ordering.
+         *
+         * The same is done for the end times index, but swapping the first two
+         * comparators instead.
+         */
+        fStartTimesIndex = checkNotNull(TreeMultimap.<Long, T> create(
+                SegmentComparators.LONG_COMPARATOR,
+                Ordering.from(SegmentComparators.INTERVAL_END_COMPARATOR).compound(Ordering.natural())));
+
+        fEndTimesIndex = checkNotNull(TreeMultimap.<Long, T> create(
+                SegmentComparators.LONG_COMPARATOR,
+                Ordering.from(SegmentComparators.INTERVAL_START_COMPARATOR).compound(Ordering.natural())));
+
         fSize = 0;
     }
 
@@ -64,10 +86,10 @@ public class TreeMapStore<T extends ISegment> implements ISegmentStore<T> {
 
     @Override
     public synchronized void addElement(T val) {
-        fStartTimesIndex.put(Long.valueOf(val.getStart()), val);
-        fEndTimesIndex.put(Long.valueOf(val.getEnd()), val);
-        fPositionMap.put(fSize, val);
-        fSize++;
+        if (fStartTimesIndex.put(Long.valueOf(val.getStart()), val)) {
+            fEndTimesIndex.put(Long.valueOf(val.getEnd()), val);
+            fSize++;
+        }
     }
 
     @Override
@@ -75,11 +97,6 @@ public class TreeMapStore<T extends ISegment> implements ISegmentStore<T> {
         return fSize;
     }
 
-    @Override
-    public T getElementAtIndex(long index) {
-        return checkNotNull(fPositionMap.get(Long.valueOf(index)));
-    }
-
     @Override
     public Iterable<T> getIntersectingElements(long position) {
         /*
@@ -104,43 +121,6 @@ public class TreeMapStore<T extends ISegment> implements ISegmentStore<T> {
     public synchronized void dispose() {
         fStartTimesIndex.clear();
         fEndTimesIndex.clear();
-        fPositionMap.clear();
         fSize = 0;
     }
-
-    // ------------------------------------------------------------------------
-    // Comparators, used for the tree maps
-    // ------------------------------------------------------------------------
-
-    private static final Comparator<Long> LONG_COMPARATOR = new Comparator<Long>() {
-        @Override
-        public int compare(@Nullable Long o1, @Nullable Long o2) {
-            if (o1 == null || o2 == null) {
-                throw new IllegalArgumentException();
-            }
-            return o1.compareTo(o2);
-        }
-    };
-
-    private static final Comparator<ISegment> INTERVAL_START_COMPARATOR = new Comparator<ISegment>() {
-        @Override
-        public int compare(@Nullable ISegment o1, @Nullable ISegment o2) {
-            if (o1 == null || o2 == null) {
-                throw new IllegalArgumentException();
-            }
-            return Long.compare(o1.getStart(), o2.getStart());
-        }
-    };
-
-    private static final Comparator<ISegment> INTERVAL_END_COMPARATOR = new Comparator<ISegment>() {
-        @Override
-        public int compare(@Nullable ISegment o1, @Nullable ISegment o2) {
-            if (o1 == null || o2 == null) {
-                throw new IllegalArgumentException();
-            }
-            return Long.compare(o1.getEnd(), o2.getEnd());
-        }
-    };
-
-
 }
This page took 0.033736 seconds and 5 git commands to generate.