This is an automated email from the ASF dual-hosted git repository.

dschneider pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/develop by this push:
     new 447897ce5f GEODE-10185: fix MemoryOverheadIntegrationTest for jdk17 
(#7591)
447897ce5f is described below

commit 447897ce5f01478aa966df25af84f21feabab94c
Author: Darrel Schneider <dar...@vmware.com>
AuthorDate: Thu Apr 14 20:46:03 2022 -0700

    GEODE-10185: fix MemoryOverheadIntegrationTest for jdk17 (#7591)
    
    * If ObjectTraverser has been told to skip static fields
    then it will no longer attempt to make static fields accessible.
    * cleaned up ObjectTraverser class
    * added unit test that verifies static fields will not be cached if they 
are not visited
    removed statics from ObjectTraverser to aid unit testing
    cleaned up warnings in ObjectGraphSizer and ObjectTraverser
---
 .../geode/internal/size/ObjectGraphSizer.java      |  33 ++---
 .../geode/internal/size/ObjectTraverser.java       | 136 ++++++++++-----------
 .../internal/size/ObjectTraverserJUnitTest.java    |  35 +++---
 3 files changed, 105 insertions(+), 99 deletions(-)

diff --git 
a/geode-core/src/main/java/org/apache/geode/internal/size/ObjectGraphSizer.java 
b/geode-core/src/main/java/org/apache/geode/internal/size/ObjectGraphSizer.java
index cfc4a955fe..c0d919cde3 100644
--- 
a/geode-core/src/main/java/org/apache/geode/internal/size/ObjectGraphSizer.java
+++ 
b/geode-core/src/main/java/org/apache/geode/internal/size/ObjectGraphSizer.java
@@ -23,6 +23,7 @@ import java.util.Set;
 import java.util.TreeSet;
 
 import org.apache.geode.annotations.Immutable;
+import org.apache.geode.annotations.internal.MakeNotStatic;
 import org.apache.geode.internal.classloader.ClassPathLoader;
 import org.apache.geode.internal.size.ObjectTraverser.Visitor;
 import org.apache.geode.util.internal.GeodeGlossary;
@@ -36,9 +37,11 @@ public class ObjectGraphSizer {
   static final SingleObjectSizer SIZE_OF_UTIL;
   @Immutable
   private static final ObjectFilter NULL_FILTER = (parent, object) -> true;
+  @MakeNotStatic
+  private static final ObjectTraverser objectTraverser = new ObjectTraverser();
 
   static {
-    Class sizeOfClass;
+    Class<?> sizeOfClass;
     try {
       sizeOfClass = ClassPathLoader.getLatest().forName(SIZE_OF_CLASS_NAME);
       SIZE_OF_UTIL = new CachingSingleObjectSizer((SingleObjectSizer) 
sizeOfClass.newInstance());
@@ -84,7 +87,7 @@ public class ObjectGraphSizer {
    *
    * @param root the object to size
    * @param filter that can exclude objects from being counted in the results. 
If an object is not
-   *        accepted, it's size will not be included and it's children will 
not be visited unless
+   *        accepted, it's size will not be included, and it's children will 
not be visited unless
    *        they are reachable by some other path.
    * @param includeStatics if set to true, static members of a class will be 
traversed the first
    *        time that a class is encountered.
@@ -93,7 +96,7 @@ public class ObjectGraphSizer {
   public static long size(Object root, ObjectFilter filter, boolean 
includeStatics)
       throws IllegalArgumentException, IllegalAccessException {
     SizeVisitor visitor = new SizeVisitor(filter);
-    ObjectTraverser.breadthFirstSearch(root, visitor, includeStatics);
+    objectTraverser.breadthFirstSearch(root, visitor, includeStatics);
 
     return visitor.getTotalSize();
   }
@@ -105,19 +108,19 @@ public class ObjectGraphSizer {
 
   public static String histogram(Object root, ObjectFilter filter, boolean 
includeStatics)
       throws IllegalArgumentException, IllegalAccessException {
-    HistogramVistor visitor = new HistogramVistor(filter);
-    ObjectTraverser.breadthFirstSearch(root, visitor, includeStatics);
+    HistogramVisitor visitor = new HistogramVisitor(filter);
+    objectTraverser.breadthFirstSearch(root, visitor, includeStatics);
 
     return visitor.dump();
 
   }
 
-  private static class HistogramVistor implements ObjectTraverser.Visitor {
-    private final Map<Class, Integer> countHisto = new HashMap<>();
-    private final Map<Class, Long> sizeHisto = new HashMap<>();
+  private static class HistogramVisitor implements ObjectTraverser.Visitor {
+    private final Map<Class<?>, Integer> countHisto = new HashMap<>();
+    private final Map<Class<?>, Long> sizeHisto = new HashMap<>();
     private final ObjectFilter filter;
 
-    public HistogramVistor(ObjectFilter filter) {
+    public HistogramVisitor(ObjectFilter filter) {
       this.filter = filter;
     }
 
@@ -158,18 +161,18 @@ public class ObjectGraphSizer {
       result.append("clazz\tsize\tcount\n");
       Set<HistogramEntry> orderedSize = getOrderedSet();
       for (HistogramEntry entry : orderedSize) {
-        Class clazz = entry.clazz;
+        Class<?> clazz = entry.clazz;
         Integer count = entry.count;
         Long size = entry.size;
-        result.append(clazz + "\t" + size + "\t" + count + "\n");
+        
result.append(clazz).append("\t").append(size).append("\t").append(count).append("\n");
       }
       return result.toString();
     }
 
     public Set<HistogramEntry> getOrderedSet() {
       TreeSet<HistogramEntry> result = new TreeSet<>();
-      for (Map.Entry<Class, Long> entry : sizeHisto.entrySet()) {
-        Class clazz = entry.getKey();
+      for (Map.Entry<Class<?>, Long> entry : sizeHisto.entrySet()) {
+        Class<?> clazz = entry.getKey();
         Long size = entry.getValue();
         Integer count = countHisto.get(clazz);
         result.add(new HistogramEntry(clazz, count, size));
@@ -178,11 +181,11 @@ public class ObjectGraphSizer {
     }
 
     private static class HistogramEntry implements Comparable<HistogramEntry> {
-      private final Class clazz;
+      private final Class<?> clazz;
       private final Integer count;
       private final Long size;
 
-      public HistogramEntry(Class clazz, Integer count, Long size) {
+      public HistogramEntry(Class<?> clazz, Integer count, Long size) {
         this.size = size;
         this.clazz = clazz;
         this.count = count;
diff --git 
a/geode-core/src/main/java/org/apache/geode/internal/size/ObjectTraverser.java 
b/geode-core/src/main/java/org/apache/geode/internal/size/ObjectTraverser.java
index 276a713aed..03128eef5a 100644
--- 
a/geode-core/src/main/java/org/apache/geode/internal/size/ObjectTraverser.java
+++ 
b/geode-core/src/main/java/org/apache/geode/internal/size/ObjectTraverser.java
@@ -24,16 +24,18 @@ import java.util.Map;
 import it.unimi.dsi.fastutil.objects.ReferenceOpenHashSet;
 
 import org.apache.geode.annotations.Immutable;
-import org.apache.geode.annotations.internal.MakeNotStatic;
 import org.apache.geode.internal.util.concurrent.CopyOnWriteWeakHashMap;
 
 
 public class ObjectTraverser {
-  @MakeNotStatic
-  private static final Map<Class, FieldSet> FIELD_CACHE =
+  private final Map<Class<?>, Field[]> FIELD_CACHE =
       new CopyOnWriteWeakHashMap<>();
+  private final Map<Class<?>, Field[]> STATIC_FIELD_CACHE =
+      new CopyOnWriteWeakHashMap<>();
+  @Immutable
+  private static final Field[] NON_PRIMITIVE_ARRAY = new Field[0];
   @Immutable
-  private static final FieldSet NON_PRIMATIVE_ARRAY = new FieldSet(null, null);
+  private static final Field[] PRIMITIVE_ARRAY = new Field[0];
 
   /**
    * Visit all objects reachable from a given root object, using a breadth 
first search. Using this
@@ -41,10 +43,10 @@ public class ObjectTraverser {
    *
    * @param root object to traverse from
    * @param visitor a visitor to visit each node
-   * @param includeStatics if true, the first time we see a new object type, 
we will visit all of
-   *        the static fields.
+   * @param includeStatics if true, then first time we see a new object type, 
all of its static
+   *        fields will be visited.
    */
-  public static void breadthFirstSearch(Object root, Visitor visitor, boolean 
includeStatics)
+  public void breadthFirstSearch(Object root, Visitor visitor, boolean 
includeStatics)
       throws IllegalArgumentException, IllegalAccessException {
     VisitStack stack = new VisitStack(visitor, includeStatics);
 
@@ -56,17 +58,13 @@ public class ObjectTraverser {
 
   }
 
-  private static void doSearch(Object root, VisitStack stack)
+  private void doSearch(Object root, VisitStack stack)
       throws IllegalArgumentException, IllegalAccessException {
-    Class clazz = root.getClass();
+    Class<?> clazz = root.getClass();
     boolean includeStatics = stack.shouldIncludeStatics(clazz);
-    FieldSet set = FIELD_CACHE.get(clazz);
-    if (set == null) {
-      set = cacheFieldSet(clazz);
-    }
+    Field[] nonPrimitiveFields = getNonPrimitiveFields(clazz, includeStatics);
 
-    if (set == NON_PRIMATIVE_ARRAY) {
-      Class componentType = clazz.getComponentType();
+    if (nonPrimitiveFields == NON_PRIMITIVE_ARRAY) {
       int length = Array.getLength(root);
       for (int i = 0; i < length; i++) {
         Object value = Array.get(root, i);
@@ -76,57 +74,81 @@ public class ObjectTraverser {
     }
 
     if (includeStatics) {
-      for (Field field : set.getStaticFields()) {
+      for (Field field : getStaticFields(clazz)) {
         Object value = field.get(root);
         stack.add(root, value);
       }
     }
 
-    for (Field field : set.getNonPrimativeFields()) {
+    for (Field field : nonPrimitiveFields) {
       Object value = field.get(root);
       stack.add(root, value);
     }
   }
 
-  private static FieldSet cacheFieldSet(Class clazz) {
-    FieldSet set = buildFieldSet(clazz);
-    FIELD_CACHE.put(clazz, set);
-    return set;
+  private Field[] getNonPrimitiveFields(Class<?> clazz, boolean 
includeStatics) {
+    Field[] result = FIELD_CACHE.get(clazz);
+    if (result == null) {
+      cacheFields(clazz, includeStatics);
+      result = FIELD_CACHE.get(clazz);
+    }
+    return result;
   }
 
-  private static FieldSet buildFieldSet(Class clazz) {
-    ArrayList<Field> staticFields = new ArrayList<>();
-    ArrayList<Field> nonPrimativeFields = new ArrayList<>();
-
-    while (clazz != null) {
-      if (clazz.isArray()) {
-        Class componentType = clazz.getComponentType();
-        if (!componentType.isPrimitive()) {
-          return NON_PRIMATIVE_ARRAY;
-        } else {
-          return new FieldSet(new Field[0], new Field[0]);
-        }
+  private Field[] getStaticFields(Class<?> clazz) {
+    Field[] result = STATIC_FIELD_CACHE.get(clazz);
+    if (result == null) {
+      cacheFields(clazz, true);
+      result = STATIC_FIELD_CACHE.get(clazz);
+    }
+    return result;
+  }
+
+  private void cacheFields(final Class<?> clazz, boolean includeStatics) {
+    if (clazz != null && clazz.isArray()) {
+      Class<?> componentType = clazz.getComponentType();
+      if (componentType.isPrimitive()) {
+        FIELD_CACHE.put(clazz, PRIMITIVE_ARRAY);
+        STATIC_FIELD_CACHE.put(clazz, PRIMITIVE_ARRAY);
+      } else {
+        FIELD_CACHE.put(clazz, NON_PRIMITIVE_ARRAY);
+        STATIC_FIELD_CACHE.put(clazz, NON_PRIMITIVE_ARRAY);
       }
+      return;
+    }
 
-      Field[] fields = clazz.getDeclaredFields();
+    ArrayList<Field> staticFields = new ArrayList<>();
+    ArrayList<Field> nonPrimitiveFields = new ArrayList<>();
+
+    Class<?> currentClass = clazz;
+    while (currentClass != null) {
+      Field[] fields = currentClass.getDeclaredFields();
       for (Field field : fields) {
-        Class fieldType = field.getType();
-        // skip static fields if we've already counted them once
+        Class<?> fieldType = field.getType();
         if (!fieldType.isPrimitive()) {
-          field.setAccessible(true);
           if (Modifier.isStatic(field.getModifiers())) {
-            staticFields.add(field);
+            if (includeStatics) {
+              field.setAccessible(true);
+              staticFields.add(field);
+            }
           } else {
-            nonPrimativeFields.add(field);
+            field.setAccessible(true);
+            nonPrimitiveFields.add(field);
           }
         }
       }
 
-      clazz = clazz.getSuperclass();
+      currentClass = currentClass.getSuperclass();
+    }
+
+    FIELD_CACHE.put(clazz, nonPrimitiveFields.toArray(new Field[0]));
+    if (includeStatics) {
+      STATIC_FIELD_CACHE.put(clazz, staticFields.toArray(new Field[0]));
     }
+  }
 
-    return new FieldSet(staticFields.toArray(new Field[0]),
-        nonPrimativeFields.toArray(new Field[0]));
+  Map<Class<?>, Field[]> getStaticFieldCache() {
+    return STATIC_FIELD_CACHE;
   }
 
   public interface Visitor {
@@ -143,8 +165,8 @@ public class ObjectTraverser {
 
 
   private static class VisitStack {
-    private final ReferenceOpenHashSet seen = new ReferenceOpenHashSet();
-    private final LinkedList stack = new LinkedList();
+    private final ReferenceOpenHashSet<Object> seen = new 
ReferenceOpenHashSet<>();
+    private final LinkedList<Object> stack = new LinkedList<>();
     private final Visitor visitor;
     private final boolean includeStatics;
 
@@ -175,7 +197,7 @@ public class ObjectTraverser {
       return stack.isEmpty();
     }
 
-    public boolean shouldIncludeStatics(Class clazz) {
+    public boolean shouldIncludeStatics(Class<?> clazz) {
       if (!includeStatics) {
         return false;
       }
@@ -184,28 +206,4 @@ public class ObjectTraverser {
       return !keyExists;
     }
   }
-
-  private ObjectTraverser() {
-
-  }
-
-  private static class FieldSet {
-    private final Field[] staticFields;
-    private final Field[] nonPrimativeFields;
-
-    public FieldSet(Field[] staticFields, Field[] nonPrimativeFields) {
-      this.staticFields = staticFields;
-      this.nonPrimativeFields = nonPrimativeFields;
-    }
-
-    public Field[] getStaticFields() {
-      return staticFields;
-    }
-
-    public Field[] getNonPrimativeFields() {
-      return nonPrimativeFields;
-    }
-
-
-  }
 }
diff --git 
a/geode-core/src/test/java/org/apache/geode/internal/size/ObjectTraverserJUnitTest.java
 
b/geode-core/src/test/java/org/apache/geode/internal/size/ObjectTraverserJUnitTest.java
index 5183d59de6..d3bc401018 100644
--- 
a/geode-core/src/test/java/org/apache/geode/internal/size/ObjectTraverserJUnitTest.java
+++ 
b/geode-core/src/test/java/org/apache/geode/internal/size/ObjectTraverserJUnitTest.java
@@ -14,6 +14,7 @@
  */
 package org.apache.geode.internal.size;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 
@@ -32,17 +33,17 @@ public class ObjectTraverserJUnitTest {
 
   @Test
   public void testBasic() throws Exception {
-    Set testData = new HashSet();
+    Set<Object> testData = new HashSet<>();
     Object one = new Object();
     testData.add(one);
     Object[] two = new Object[2];
     testData.add(two);
-    ArrayList three = new ArrayList();
+    ArrayList<Object> three = new ArrayList<>();
     two[0] = three;
     three.add(testData);
 
     TestVisitor visitor = new TestVisitor();
-    ObjectTraverser.breadthFirstSearch(testData, visitor, false);
+    new ObjectTraverser().breadthFirstSearch(testData, visitor, false);
 
     assertNotNull(visitor.visited.remove(testData));
     assertNotNull(visitor.visited.remove(one));
@@ -50,6 +51,7 @@ public class ObjectTraverserJUnitTest {
     assertNotNull(visitor.visited.remove(three));
   }
 
+  @SuppressWarnings("InstantiationOfUtilityClass")
   @Test
   public void testStatics() throws Exception {
     final Object staticObject = new Object();
@@ -57,24 +59,27 @@ public class ObjectTraverserJUnitTest {
     TestObject1 test1 = new TestObject1();
 
     TestVisitor visitor = new TestVisitor();
-    ObjectTraverser.breadthFirstSearch(test1, visitor, false);
-    assertNull(visitor.visited.get(staticObject));
+    ObjectTraverser nonStaticTraverser = new ObjectTraverser();
+    nonStaticTraverser.breadthFirstSearch(test1, visitor, false);
+    assertThat(visitor.visited.get(staticObject)).isNull();
+    
assertThat(nonStaticTraverser.getStaticFieldCache().get(test1.getClass())).isNull();
 
     visitor = new TestVisitor();
-    ObjectTraverser.breadthFirstSearch(test1, visitor, true);
-    assertNotNull(visitor.visited.get(staticObject));
+    ObjectTraverser staticTraverser = new ObjectTraverser();
+    staticTraverser.breadthFirstSearch(test1, visitor, true);
+    assertThat(visitor.visited.get(staticObject)).isNotNull();
+    
assertThat(staticTraverser.getStaticFieldCache().get(test1.getClass())).isNotNull();
   }
 
   @Test
   public void testStop() throws Exception {
-    Set set1 = new HashSet();
-    final Set set2 = new HashSet();
+    Set<Object> set1 = new HashSet<>();
+    final Set<Object> set2 = new HashSet<>();
     Object object3 = new Object();
     set1.add(set2);
     set2.add(object3);
 
-    TestVisitor visitor = new TestVisitor();
-    visitor = new TestVisitor() {
+    TestVisitor visitor = new TestVisitor() {
       @Override
       public boolean visit(Object parent, Object object) {
         super.visit(parent, object);
@@ -82,7 +87,7 @@ public class ObjectTraverserJUnitTest {
       }
     };
 
-    ObjectTraverser.breadthFirstSearch(set1, visitor, true);
+    new ObjectTraverser().breadthFirstSearch(set1, visitor, true);
 
     assertNotNull(visitor.visited.get(set1));
     assertNotNull(visitor.visited.get(set2));
@@ -93,8 +98,8 @@ public class ObjectTraverserJUnitTest {
   @Ignore("commented out because it needs to be verified manually")
   @Test
   public void testHistogram() throws Exception {
-    Set set1 = new HashSet();
-    final Set set2 = new HashSet();
+    Set<Object> set1 = new HashSet<>();
+    final Set<Object> set2 = new HashSet<>();
     Object object3 = new Object();
     set1.add(set2);
     set2.add(object3);
@@ -107,7 +112,7 @@ public class ObjectTraverserJUnitTest {
   private static class TestVisitor implements ObjectTraverser.Visitor {
     private static final Object VALUE = new Object();
 
-    public Map visited = new IdentityHashMap();
+    public Map<Object, Object> visited = new IdentityHashMap<>();
 
     @Override
     public boolean visit(Object parent, Object object) {

Reply via email to