ctf: Cleanup StructDeclaration
authorAlexandre Montplaisir <alexmonthy@voxpopuli.im>
Wed, 25 Jun 2014 22:06:02 +0000 (18:06 -0400)
committerAlexandre Montplaisir <alexmonthy@voxpopuli.im>
Thu, 26 Jun 2014 21:26:18 +0000 (17:26 -0400)
No need to keep separate lists for the fields and values of structs.
A LinkedHashMap preserves the insertion order, and Iterable defines
a specific iteration order.

No apparent performance impact, both versions hover around 975-980 ns/ev
on my machine.

Change-Id: I487b2c756d5827a7489a2ef65e550fb3d018cb72
Signed-off-by: Alexandre Montplaisir <alexmonthy@voxpopuli.im>
Reviewed-on: https://git.eclipse.org/r/29009
Reviewed-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Tested-by: Hudson CI
org.eclipse.linuxtools.ctf.core.tests/src/org/eclipse/linuxtools/ctf/core/tests/types/StructDeclarationTest.java
org.eclipse.linuxtools.ctf.core/src/org/eclipse/linuxtools/ctf/core/event/types/StructDeclaration.java
org.eclipse.linuxtools.ctf.core/src/org/eclipse/linuxtools/ctf/core/event/types/StructDefinition.java
org.eclipse.linuxtools.ctf.core/src/org/eclipse/linuxtools/ctf/core/trace/CTFStreamInputPacketReader.java
org.eclipse.linuxtools.ctf.core/src/org/eclipse/linuxtools/internal/ctf/core/event/metadata/IOStructGen.java

index 3b7bd9d99faa15704bf1f3fa298c7b7f82e32229..62588755e520c80c9cb80a1cfebc739e5587e754 100644 (file)
@@ -13,10 +13,10 @@ package org.eclipse.linuxtools.ctf.core.tests.types;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
 import java.nio.ByteBuffer;
-import java.util.Map;
 
 import org.eclipse.linuxtools.ctf.core.event.io.BitBuffer;
 import org.eclipse.linuxtools.ctf.core.event.types.IDeclaration;
@@ -88,14 +88,13 @@ public class StructDeclarationTest {
     }
 
     /**
-     * Run the HashMap<String, Declaration> getFields() method test.
+     * Run the Declaration getField(String) method test.
      */
     @Test
-    public void testGetFields() {
-        Map<String, IDeclaration> result = fixture.getFields();
+    public void testGetField() {
+        IDeclaration result = fixture.getField("test");
 
-        assertNotNull(result);
-        assertEquals(0, result.size());
+        assertNull(result);
     }
 
     /**
index 03a682936a6959984034b63d5f3ba8377caf0b6a..fa18581ddb9e1053d1df1bf479c9dcaa74868194 100644 (file)
 
 package org.eclipse.linuxtools.ctf.core.event.types;
 
+import java.util.Iterator;
 import java.util.LinkedHashMap;
-import java.util.List;
 import java.util.Map;
 
 import org.eclipse.jdt.annotation.NonNull;
+import org.eclipse.jdt.annotation.Nullable;
 import org.eclipse.linuxtools.ctf.core.event.io.BitBuffer;
 import org.eclipse.linuxtools.ctf.core.event.scope.IDefinitionScope;
 import org.eclipse.linuxtools.ctf.core.trace.CTFReaderException;
 
-import com.google.common.collect.ImmutableList;
-
 /**
  * A CTF structure declaration.
  *
@@ -41,13 +40,7 @@ public class StructDeclaration extends Declaration {
     // ------------------------------------------------------------------------
 
     /** linked list of field names. So fieldName->fieldValue */
-    private final Map<String, IDeclaration> fFieldMap = new LinkedHashMap<>();
-
-    /** List of strings for acceleration */
-    @NonNull
-    private ImmutableList<String> fFieldNames;
-    /** array declaration for acceleration */
-    private List<IDeclaration> fFieldDeclarations;
+    private final @NonNull Map<String, IDeclaration> fFieldMap = new LinkedHashMap<>();
 
     /** maximum bit alignment */
     private long fMaxAlign;
@@ -64,11 +57,8 @@ public class StructDeclaration extends Declaration {
      *            aligned and has a 32 bit aligned field, the struct becomes 32
      *            bit aligned.
      */
-    @SuppressWarnings("null")
-    // ImmutableList.of()
     public StructDeclaration(long align) {
         fMaxAlign = Math.max(align, 1);
-        fFieldNames = ImmutableList.of();
     }
 
     /**
@@ -80,11 +70,9 @@ public class StructDeclaration extends Declaration {
      *            all the fields
      * @since 3.0
      */
-    @SuppressWarnings("null")
-    // ImmutableList.of()
     public StructDeclaration(String[] names, Declaration[] declarations) {
         fMaxAlign = 1;
-        fFieldNames = ImmutableList.of();
+
         for (int i = 0; i < names.length; i++) {
             addField(names[i], declarations[i]);
         }
@@ -115,15 +103,28 @@ public class StructDeclaration extends Declaration {
     }
 
     /**
-     * get the fields of the struct in a map. Faster access time than a list.
+     * Get the fields of the struct as a map.
      *
-     * @return a HashMap of the fields (key is the name)
+     * @return a Map of the fields (key is the name)
      * @since 2.0
      */
     public Map<String, IDeclaration> getFields() {
         return fFieldMap;
     }
 
+    /**
+     * Get the field declaration corresponding to a field name.
+     *
+     * @param fieldName
+     *            The field name
+     * @return The declaration of the field, or null if there is no such field.
+     * @since 3.1
+     */
+    @Nullable
+    public IDeclaration getField(String fieldName) {
+        return fFieldMap.get(fieldName);
+    }
+
     /**
      * Gets the field list. Very important since the map of fields does not
      * retain the order of the fields.
@@ -146,10 +147,8 @@ public class StructDeclaration extends Declaration {
     @Override
     public int getMaximumSize() {
         int maxSize = 0;
-        if (fFieldDeclarations != null) {
-            for (IDeclaration field : fFieldDeclarations) {
-                maxSize += field.getMaximumSize();
-            }
+        for (IDeclaration field : fFieldMap.values()) {
+            maxSize += field.getMaximumSize();
         }
         return Math.min(maxSize, Integer.MAX_VALUE);
     }
@@ -161,16 +160,21 @@ public class StructDeclaration extends Declaration {
     /**
      * @since 3.0
      */
-    @SuppressWarnings("null")
-    // immutablelist
     @Override
     public StructDefinition createDefinition(IDefinitionScope definitionScope,
             String fieldName, BitBuffer input) throws CTFReaderException {
         alignRead(input);
-        final Definition[] myFields = new Definition[fFieldNames.size()];
-        StructDefinition structDefinition = new StructDefinition(this, definitionScope, fieldName, fFieldNames, myFields);
-        for (int i = 0; i < fFieldNames.size(); i++) {
-            myFields[i] = fFieldDeclarations.get(i).createDefinition(structDefinition, fFieldNames.get(i), input);
+        final Definition[] myFields = new Definition[fFieldMap.size()];
+        StructDefinition structDefinition = new StructDefinition(this, definitionScope, fieldName, fFieldMap.keySet(), myFields);
+
+        Iterator<Map.Entry<String, IDeclaration>> iter = fFieldMap.entrySet().iterator();
+        for (int i = 0; i < fFieldMap.size(); i++) {
+            Map.Entry<String, IDeclaration> entry = iter.next();
+            String name = entry.getKey();
+            if (name == null) {
+                throw new IllegalStateException();
+            }
+            myFields[i] = entry.getValue().createDefinition(structDefinition, name, input);
         }
         return structDefinition;
     }
@@ -183,13 +187,9 @@ public class StructDeclaration extends Declaration {
      * @param declaration
      *            the declaration of the field
      */
-    @SuppressWarnings("null")
-    // Immutable list copyof cannot return null
     public void addField(String name, IDeclaration declaration) {
         fFieldMap.put(name, declaration);
         fMaxAlign = Math.max(fMaxAlign, declaration.getAlignment());
-        fFieldNames = ImmutableList.copyOf(fFieldMap.keySet());
-        fFieldDeclarations = ImmutableList.<IDeclaration>copyOf(fFieldMap.values());
     }
 
     @Override
index 9bd62046e658006f92d972bdc05f41dcc020d471..664087654fa148962617c693cc1648b4bf98ff7a 100644 (file)
@@ -50,6 +50,33 @@ public final class StructDefinition extends ScopedDefinition {
     // Constructors
     // ------------------------------------------------------------------------
 
+    /**
+     * *DEPRECATED* TODO: To remove once we break the API...
+     *
+     * Not marked with the annotation to not annoy callers using a List, which
+     * is still as valid with the new constructor. But the compiler gives an
+     * error even though a Iterable is a List too...
+     *
+     * @param declaration
+     *            the parent declaration
+     * @param definitionScope
+     *            the parent scope
+     * @param structFieldName
+     *            the field name
+     * @param fieldNames
+     *            the list of fields
+     * @param definitions
+     *            the definitions
+     * @since 3.1
+     */
+    public StructDefinition(@NonNull StructDeclaration declaration,
+            IDefinitionScope definitionScope,
+            @NonNull String structFieldName,
+            List<String> fieldNames,
+            Definition[] definitions) {
+        this(declaration, definitionScope, structFieldName, (Iterable<String>) fieldNames, definitions);
+    }
+
     /**
      * Constructor
      *
@@ -63,10 +90,13 @@ public final class StructDefinition extends ScopedDefinition {
      *            the list of fields
      * @param definitions
      *            the definitions
-     * @since 3.0
+     * @since 3.1
      */
     public StructDefinition(@NonNull StructDeclaration declaration,
-            IDefinitionScope definitionScope, @NonNull String structFieldName, List<String> fieldNames, Definition[] definitions) {
+            IDefinitionScope definitionScope,
+            @NonNull String structFieldName,
+            Iterable<String> fieldNames,
+            Definition[] definitions) {
         super(declaration, definitionScope, structFieldName);
         fFieldNames = ImmutableList.copyOf(fieldNames);
         fDefinitions = definitions;
@@ -89,19 +119,17 @@ public final class StructDefinition extends ScopedDefinition {
      */
     public Definition getDefinition(String fieldName) {
         if (fDefinitionsMap == null) {
-            buildFieldsMap();
-        }
-        return fDefinitionsMap.get(fieldName);
-    }
-
-    private void buildFieldsMap() {
-        Builder<String, Definition> mapBuilder = new ImmutableMap.Builder<>();
-        for (int i = 0; i < fFieldNames.size(); i++) {
-            if (fDefinitions[i] != null) {
-                mapBuilder.put(fFieldNames.get(i), fDefinitions[i]);
+            /* Build the definitions map */
+            Builder<String, Definition> mapBuilder = new ImmutableMap.Builder<>();
+            for (int i = 0; i < fFieldNames.size(); i++) {
+                if (fDefinitions[i] != null) {
+                    mapBuilder.put(fFieldNames.get(i), fDefinitions[i]);
+                }
             }
+            fDefinitionsMap = mapBuilder.build();
         }
-        fDefinitionsMap = mapBuilder.build();
+
+        return fDefinitionsMap.get(fieldName);
     }
 
     /**
index d3b7fd6187b89367bcae9e7c378d750804d5d6d0..3a88ac31a40913818f50923aac40176d765c8f46 100644 (file)
@@ -312,12 +312,12 @@ public class CTFStreamInputPacketReader implements IDefinitionScope, AutoCloseab
             EventDeclaration lostEventDeclaration = EventDeclaration.getLostEventDeclaration();
             StructDeclaration lostFields = lostEventDeclaration.getFields();
             // this is a hard coded map, we know it's not null
-            IntegerDeclaration lostFieldsDecl = (IntegerDeclaration) lostFields.getFields().get(CTFStrings.LOST_EVENTS_FIELD);
+            IntegerDeclaration lostFieldsDecl = (IntegerDeclaration) lostFields.getField(CTFStrings.LOST_EVENTS_FIELD);
             if (lostFieldsDecl == null)
             {
                 throw new IllegalStateException("Lost events count not declared!"); //$NON-NLS-1$
             }
-            IntegerDeclaration lostEventsDurationDecl = (IntegerDeclaration) lostFields.getFields().get(CTFStrings.LOST_EVENTS_DURATION);
+            IntegerDeclaration lostEventsDurationDecl = (IntegerDeclaration) lostFields.getField(CTFStrings.LOST_EVENTS_DURATION);
             if (lostEventsDurationDecl == null) {
                 throw new IllegalStateException("Lost events duration not declared!"); //$NON-NLS-1$
             }
index c4c517f7bc7e5ff48cc90777d5cbddcb65a66733..935507d38d0a8d18ac7254d1fafb78a4340c73f9 100644 (file)
@@ -576,14 +576,13 @@ public class IOStructGen {
             ByteOrder byteOrder) throws ParseException {
 
         for (String s : sd.getFieldsList()) {
-            IDeclaration d = sd.getFields().get(s);
+            IDeclaration d = sd.getField(s);
 
             if (d instanceof StructDeclaration) {
                 setAlign(parentScope, (StructDeclaration) d, byteOrder);
 
             } else if (d instanceof VariantDeclaration) {
                 setAlign(parentScope, (VariantDeclaration) d, byteOrder);
-
             } else if (d instanceof IntegerDeclaration) {
                 IntegerDeclaration decl = (IntegerDeclaration) d;
                 if (decl.getByteOrder() != byteOrder) {
This page took 0.034681 seconds and 5 git commands to generate.