tmf: Fix rounding and range issues in periodic marker source
authorPatrick Tasse <patrick.tasse@gmail.com>
Wed, 19 Oct 2016 15:21:34 +0000 (11:21 -0400)
committerPatrick Tasse <patrick.tasse@gmail.com>
Wed, 16 Nov 2016 21:25:09 +0000 (16:25 -0500)
When the requested time range and reference are far apart, the floating
point calculations lose precision and can produce marker times that are
not multiples of the period but instead clamped to the nearest multiple
of a power of 2.

The calculations are modified to adjust the reference to a point that is
closer to the request time range, by converting the period to a fraction
representation, if possible, and using exact integer calculations.

Some rounding calculations are corrected to apply to smaller numbers.

When using alternate shading periodic markers, the first marker returned
could overlap with the start time. If this partially visible marker was
selected by the user, the 'previous marker' action would not find any
previous marker. The code is adjusted to make sure that the first marker
returned is the first fully non-visible previous marker.

Change-Id: I1a8ad36441bf44a735621ffe4361e73ee6faaa60
Signed-off-by: Patrick Tasse <patrick.tasse@gmail.com>
Reviewed-on: https://git.eclipse.org/r/83545
Reviewed-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Reviewed-by: Hudson CI
rcp/org.eclipse.tracecompass.rcp/feature.xml
rcp/org.eclipse.tracecompass.rcp/pom.xml
releng/org.eclipse.tracecompass.target/tracecompass-e4.5.target
releng/org.eclipse.tracecompass.target/tracecompass-e4.6.target
releng/org.eclipse.tracecompass.target/tracecompass-eStaging.target
tmf/org.eclipse.tracecompass.tmf.ui.tests/src/org/eclipse/tracecompass/tmf/ui/tests/markers/PeriodicMarkerEventSourceTest.java
tmf/org.eclipse.tracecompass.tmf.ui/META-INF/MANIFEST.MF
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/markers/PeriodicMarkerEventSource.java
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/model/IMarkerEventSource.java

index 2b5278f7e1feb306a7e22e0dd12d5b00b199624d..0b59b40fe10173e418584d492b7be928cefe629d 100644 (file)
          version="0.0.0"
          unpack="false"/>
 
+   <plugin
+         id="org.apache.commons.lang3"
+         download-size="0"
+         install-size="0"
+         version="0.0.0"
+         unpack="false"/>
+
 </feature>
index 5e1c740f50b86ef2c65b40d5cf7157a0c4539d0b..58e2fe6a20c755b6f0c2b22faffebfa0a80a78bc 100644 (file)
@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!--
-   Copyright (C) 2014 Ericsson
+   Copyright (C) 2014, 2016 Ericsson
 
    All rights reserved. This program and the accompanying materials
    are made available under the terms of the Eclipse Public License v1.0
@@ -37,6 +37,7 @@
             <configuration>
               <excludes>
                 <plugin id="org.apache.commons.compress"/>
+                <plugin id="org.apache.commons.lang3"/>
                 <plugin id="org.apache.xerces"/>
                 <plugin id="org.apache.xml.resolver"/>
                 <plugin id="org.apache.xml.serializer"/>
index 0766d028a6095ea2a71b2f63691d51ed16a5ce0f..7e791677dcc8bba6de5e7123c96f1883d1d9feb0 100644 (file)
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8" standalone="no"?>
-<?pde version="3.8"?><target name="tracecompass-e4.5" sequenceNumber="76">
+<?pde version="3.8"?><target name="tracecompass-e4.5" sequenceNumber="77">
 <locations>
 <location includeAllPlatforms="false" includeConfigurePhase="false" includeMode="planner" includeSource="true" type="InstallableUnit">
 <unit id="org.eclipse.cdt.gnu.dsf.feature.group" version="0.0.0"/>
@@ -31,6 +31,7 @@
 <unit id="org.antlr.runtime.source" version="3.2.0.v201101311130"/>
 <unit id="org.apache.commons.compress" version="0.0.0"/>
 <unit id="org.apache.commons.io" version="0.0.0"/>
+<unit id="org.apache.commons.lang3" version="0.0.0"/>
 <unit id="org.swtchart" version="0.0.0"/>
 <unit id="com.google.gson" version="0.0.0"/>
 <unit id="com.google.guava" version="15.0.0.v201403281430"/>
index 2daf7bf220dd9c881f3aa35b4d95a2b9bb63d681..4876a957398088d11d2470194978575affc4a1ef 100644 (file)
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8" standalone="no"?>
-<?pde version="3.8"?><target name="tracecompass-e4.6" sequenceNumber="24">
+<?pde version="3.8"?><target name="tracecompass-e4.6" sequenceNumber="25">
 <locations>
 <location includeAllPlatforms="false" includeConfigurePhase="false" includeMode="planner" includeSource="true" type="InstallableUnit">
 <unit id="org.eclipse.cdt.gnu.dsf.feature.group" version="0.0.0"/>
@@ -34,6 +34,7 @@
 <unit id="org.antlr.runtime.source" version="3.2.0.v201101311130"/>
 <unit id="org.apache.commons.compress" version="0.0.0"/>
 <unit id="org.apache.commons.io" version="0.0.0"/>
+<unit id="org.apache.commons.lang3" version="0.0.0"/>
 <unit id="org.swtchart" version="0.7.0.v201201201914"/>
 <unit id="com.google.guava" version="15.0.0.v201403281430"/>
 <unit id="org.json" version="0.0.0"/>
index a5baf1950bd04dfeb156ad3a907cb5383e690b57..a066081e4e1006380705a0a2d410d6ae91773351 100644 (file)
@@ -34,6 +34,7 @@
 <unit id="org.antlr.runtime.source" version="3.2.0.v201101311130"/>
 <unit id="org.apache.commons.compress" version="0.0.0"/>
 <unit id="org.apache.commons.io" version="0.0.0"/>
+<unit id="org.apache.commons.lang3" version="0.0.0"/>
 <unit id="org.swtchart" version="0.7.0.v201201201914"/>
 <unit id="com.google.guava" version="15.0.0.v201403281430"/>
 <unit id="org.json" version="0.0.0"/>
index ba70a6acf804bc4320d445619fdc36a851503b50..64629da6aaf300986eeb1b045d8441fffde62956 100644 (file)
@@ -76,6 +76,7 @@ public class PeriodicMarkerEventSourceTest {
         IMarkerEventSource source = new PeriodicMarkerEventSource(CATEGORY, Reference.ZERO, 100L, 0, EVEN_COLOR, ODD_COLOR, false);
         assertEquals(Arrays.asList(CATEGORY), source.getMarkerCategories());
         List<IMarkerEvent> expected = Arrays.asList(
+                new MarkerEvent(null, -100L, 100L, CATEGORY, ODD_COLOR, "-1", false),
                 new MarkerEvent(null, 0L, 100L, CATEGORY, EVEN_COLOR, "0", false),
                 new MarkerEvent(null, 100L, 100L, CATEGORY, ODD_COLOR, "1", false),
                 new MarkerEvent(null, 200L, 100L, CATEGORY, EVEN_COLOR, "2", false),
index 6dfaeac829cfa0bda42a06adf1bc5fbf884a820f..b008836d584b8433896362c36504ee2ba371256f 100644 (file)
@@ -25,7 +25,8 @@ Require-Bundle: org.eclipse.core.expressions,
  com.ibm.icu,
  org.eclipse.linuxtools.dataviewers.piechart,
  org.eclipse.tracecompass.segmentstore.core,
- org.apache.commons.compress
+ org.apache.commons.compress,
+ org.apache.commons.lang3
 Export-Package: org.eclipse.tracecompass.internal.tmf.ui;x-friends:="org.eclipse.tracecompass.tmf.ui.tests,org.eclipse.tracecompass.tmf.ctf.ui.tests",
  org.eclipse.tracecompass.internal.tmf.ui.commands;x-internal:=true,
  org.eclipse.tracecompass.internal.tmf.ui.dialogs;x-internal:=true,
index 17909af8df6bb19bf69634ea677b520c90f41abe..15062f95d640a0cf9499ec53393e611e09d9bf70 100644 (file)
@@ -19,6 +19,7 @@ import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 
+import org.apache.commons.lang3.math.Fraction;
 import org.eclipse.core.runtime.IProgressMonitor;
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
@@ -44,7 +45,7 @@ public class PeriodicMarkerEventSource implements IMarkerEventSource {
         public static final Reference ZERO = new Reference(0L, 0);
 
         private final long time;
-        private final int index;
+        private final long index;
 
         /**
          * Constructor
@@ -58,11 +59,32 @@ public class PeriodicMarkerEventSource implements IMarkerEventSource {
             this.time = time;
             this.index = index;
         }
+
+        /**
+         * Constructor
+         *
+         * @param time
+         *            the reference marker time in time units
+         * @param index
+         *            the reference marker index
+         * @since 2.2
+         */
+        public Reference(long time, long index) {
+            this.time = time;
+            this.index = index;
+        }
+
+        @Override
+        public String toString() {
+            return String.format("[%d, %d]", time, index); //$NON-NLS-1$
+        }
     }
 
     private final String fCategory;
     private final Reference fReference;
     private final double fPeriod;
+    private final long fPeriodInteger;
+    private @Nullable Fraction fPeriodFraction;
     private final long fRollover;
     private final RGBA fColor;
     private final @Nullable RGBA fOddColor;
@@ -133,6 +155,13 @@ public class PeriodicMarkerEventSource implements IMarkerEventSource {
         fCategory = category;
         fReference = reference;
         fPeriod = period;
+        fPeriodInteger = (long) period;
+        try {
+            fPeriodFraction = Fraction.getFraction(fPeriod - fPeriodInteger);
+        } catch (ArithmeticException e) {
+            /* can't convert to fraction, use floating-point arithmetic */
+            fPeriodFraction = null;
+        }
         fRollover = rollover;
         fColor = evenColor;
         fOddColor = oddColor;
@@ -150,14 +179,14 @@ public class PeriodicMarkerEventSource implements IMarkerEventSource {
             return Collections.emptyList();
         }
         List<IMarkerEvent> markers = new ArrayList<>();
-        long time = startTime;
-        if (Math.round((Math.round((time - fReference.time) / fPeriod)) * fPeriod + fReference.time) >= time) {
-            /* Subtract one period to ensure previous marker is included */
-            time -= Math.max(fPeriod, resolution);
-        }
+        /* Subtract 1.5 periods to ensure previous marker is included */
+        long time = startTime - Math.max(Math.round(1.5 * fPeriod), resolution);
+        Reference reference = adjustReference(fReference, time);
+        IMarkerEvent markerEvent = null;
         while (true) {
-            long index = Math.round((time - fReference.time) / fPeriod) + fReference.index;
-            time = Math.round((index - fReference.index) * fPeriod) + fReference.time;
+            long index = Math.round((time - reference.time) / fPeriod) + reference.index;
+            time = Math.round((index - reference.index) * fPeriod) + reference.time;
+            long duration = (fOddColor == null) ? 0 : Math.round((index + 1 - reference.index) * fPeriod) + reference.time - time;
             long labelIndex = index;
             if (fRollover != 0) {
                 labelIndex %= fRollover;
@@ -165,22 +194,49 @@ public class PeriodicMarkerEventSource implements IMarkerEventSource {
                     labelIndex += fRollover;
                 }
             }
-            if (fOddColor == null) {
-                markers.add(new MarkerEvent(null, time, 0, fCategory, fColor, getMarkerLabel(labelIndex), fForeground));
-            } else {
-                RGBA color = index % 2 == 0 ? fColor : fOddColor;
-                long duration = Math.round((index + 1 - fReference.index) * fPeriod + fReference.time) - time;
-                markers.add(new MarkerEvent(null, time, duration, fCategory, color, getMarkerLabel(labelIndex), fForeground));
+            /* Add previous marker if current is visible */
+            if ((time >= startTime || time + duration > startTime) && markerEvent != null) {
+                markers.add(markerEvent);
             }
+            RGBA color = (fOddColor == null) ? fColor : (index % 2 == 0) ? fColor : fOddColor;
+            markerEvent = new MarkerEvent(null, time, duration, fCategory, color, getMarkerLabel(labelIndex), fForeground);
             if (time > endTime) {
                 /* The next marker out of range is included */
+                markers.add(markerEvent);
                 break;
             }
-            time += Math.max(fPeriod, resolution);
+            time += Math.max(Math.round(fPeriod), resolution);
         }
         return markers;
     }
 
+    /*
+     * Adjust to a reference that is closer to the start time, to avoid rounding
+     * errors in floating point calculations with large numbers.
+     */
+    private Reference adjustReference(Reference baseReference, long time) {
+        long offsetIndex = (long) ((time - baseReference.time) / fPeriod);
+        long offsetTime = 0;
+        Fraction fraction = fPeriodFraction;
+        if (fraction != null) {
+            /*
+             * If period = int num/den, find an offset index that is an exact
+             * multiple of den and calculate index * period = (index * int) +
+             * (index / den * num), all exact calculations.
+             */
+            offsetIndex = offsetIndex - offsetIndex % fraction.getDenominator();
+            offsetTime = offsetIndex * fPeriodInteger + offsetIndex / fraction.getDenominator() * fraction.getNumerator();
+        } else {
+            /*
+             * Couldn't compute fractional part as fraction, use simple
+             * multiplication but with possible rounding error.
+             */
+            offsetTime = Math.round(offsetIndex * fPeriod);
+        }
+        Reference reference = new Reference(baseReference.time + offsetTime, baseReference.index + offsetIndex);
+        return reference;
+    }
+
     /**
      * Get the marker label for the given marker index.
      * <p>
index 632ae9a8ed70873e86a57b1f55d514c765abf4b0..a46a5ee8493f3ff2edb5fb2c1f18e4a9efe14b79 100644 (file)
@@ -35,9 +35,8 @@ public interface IMarkerEventSource {
      * Gets the list of marker events of a specific category that intersect the
      * given time range (inclusively).
      * <p>
-     * The list should include the nearest previous marker that starts before
-     * the time range (it might already be included if it intersects the time
-     * range), and the nearest next marker that starts after the time range.
+     * The list should also include the nearest previous and next markers that
+     * do not intersect the time range.
      *
      * @param category
      *            The marker category
This page took 0.030716 seconds and 5 git commands to generate.