Merge pull request #2665 from ebean-orm/feature/NodeUsageCollector-Cleaner

NodeUsageCollector migrate from using finalize() to Cleaner

ebean-api/src/main/java/io/ebean/bean/EntityBeanIntercept.java

@@ -874,9 +874,6 @@ public final class EntityBeanIntercept implements Serializable {
     }
     if (lazyLoadProperty == -1) {
       lazyLoadProperty = loadProperty;
-      if (nodeUsageCollector != null) {
-        nodeUsageCollector.setLoadProperty(getProperty(lazyLoadProperty));
-      }
       loader.loadBean(this);
       if (lazyLoadFailure) {
         // failed when lazy loading this bean

ebean-api/src/main/java/io/ebean/bean/NodeUsageCollector.java

@@ -1,6 +1,6 @@
 package io.ebean.bean;
 
-import java.lang.ref.WeakReference;
+import java.lang.ref.Cleaner;
 import java.util.LinkedHashSet;
 import java.util.Set;
 
@@ -8,120 +8,102 @@ import java.util.Set;
  * Collects profile information for a bean (or reference/proxy bean) at a given node.
  * <p>
  * The node identifies the location of the bean in the object graph.
- * </p>
- * <p>
- * It has to use a weak reference so as to ensure that it does not stop the
- * associated bean from being garbage collected.
- * </p>
  */
 public final class NodeUsageCollector {
 
-  /**
-   * The point in the object graph for a specific query and call stack point.
-   */
-  private final ObjectGraphNode node;
+  private final static Cleaner cleaner = Cleaner.create();
+
+  public static final class State implements Runnable {
+
+    private final NodeUsageListener listener;
+    /**
+     * The properties used at this profile point.
+     */
+    private final Set<String> used = new LinkedHashSet<>();
+    /**
+     * The point in the object graph for a specific query and call stack point.
+     */
+    private final ObjectGraphNode node;
+    /**
+     * set to true if the bean is modified (setter called)
+     */
+    private boolean modified;
+
+    private State(ObjectGraphNode node, NodeUsageListener listener) {
+      this.node = node;
+      this.listener = listener;
+    }
+
+    @Override
+    public String toString() {
+      return node + " read:" + used + " modified:" + modified;
+    }
+
+    @Override
+    public void run() {
+      listener.collectNodeUsage(this);
+    }
+
+    /**
+     * Return true if no properties where used.
+     */
+    public boolean isEmpty() {
+      return used.isEmpty();
+    }
+
+    /**
+     * Return the associated node which identifies the location in the object
+     * graph of the bean/reference.
+     */
+    public ObjectGraphNode node() {
+      return node;
+    }
+
+    /**
+     * Return the set of used properties.
+     */
+    public Set<String> used() {
+      return used;
+    }
+
+    /**
+     * Return true if the bean was modified by a setter.
+     */
+    public boolean isModified() {
+      return modified;
+    }
+  }
+
+  private final State state;
+
+  public NodeUsageCollector(ObjectGraphNode node, NodeUsageListener listener) {
+    this.state = new State(node, listener);
+    cleaner.register(this, state);
+  }
 
   /**
-   * Weak to allow garbage collection.
+   * Return the underlying state.
    */
-  private final WeakReference<NodeUsageListener> managerRef;
-
-  /**
-   * The properties used at this profile point.
-   */
-  private final Set<String> used = new LinkedHashSet<>();
-
-  /**
-   * set to true if the bean is modified (setter called)
-   */
-  private boolean modified;
-
-  /**
-   * The property that cause a reference to lazy load.
-   */
-  private String loadProperty;
-
-  public NodeUsageCollector(ObjectGraphNode node, WeakReference<NodeUsageListener> managerRef) {
-    this.node = node;
-    // weak to allow garbage collection.
-    this.managerRef = managerRef;
+  public State state() {
+    return state;
   }
 
   /**
    * The bean has been modified by a setter method.
    */
   public void setModified() {
-    modified = true;
+    state.modified = true;
   }
 
   /**
    * Add the name of a property that has been used.
    */
   public void addUsed(String property) {
-    used.add(property);
-  }
-
-  /**
-   * The property that invoked a lazy load.
-   */
-  public void setLoadProperty(String loadProperty) {
-    this.loadProperty = loadProperty;
-  }
-
-  /**
-   * Publish the usage info to the manager.
-   */
-  private void publishUsageInfo() {
-    NodeUsageListener manager = managerRef.get();
-    if (manager != null) {
-      manager.collectNodeUsage(this);
-    }
-  }
-
-  /**
-   * publish the collected usage information when garbage collection occurs.
-   */
-  @Override
-  protected void finalize() throws Throwable {
-    publishUsageInfo();
-    super.finalize();
-  }
-
-  /**
-   * Return the associated node which identifies the location in the object
-   * graph of the bean/reference.
-   */
-  public ObjectGraphNode getNode() {
-    return node;
-  }
-
-  /**
-   * Return true if no properties where used.
-   */
-  public boolean isEmpty() {
-    return used.isEmpty();
-  }
-
-  /**
-   * Return the set of used properties.
-   */
-  public Set<String> getUsed() {
-    return used;
-  }
-
-  /**
-   * Return true if the bean was modified by a setter.
-   */
-  public boolean isModified() {
-    return modified;
-  }
-
-  public String getLoadProperty() {
-    return loadProperty;
+    state.used.add(property);
   }
 
   @Override
   public String toString() {
-    return node + " read:" + used + " modified:" + modified;
+    return state.toString();
   }
 }

ebean-api/src/main/java/io/ebean/bean/NodeUsageListener.java

@@ -10,7 +10,6 @@ public interface NodeUsageListener {
    * <p>
    * This is the properties that are used for a given bean in the object graph.
    * This information is used by autoTune to tune queries.
-   * </p>
    */
-  void collectNodeUsage(NodeUsageCollector collector);
+  void collectNodeUsage(NodeUsageCollector.State state);
 }

ebean-api/src/test/java/io/ebean/bean/NodeUsageCollectorTest.java

@@ -0,0 +1,42 @@
+package io.ebean.bean;
+
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class NodeUsageCollectorTest {
+
+  private final Listener listener = new Listener();
+
+  private final ObjectGraphNode node = new ObjectGraphNode((ObjectGraphOrigin) null, "foo");
+
+  /**
+   * Run this manually as we make use of explicit GC call here which is dubious.
+   */
+  @Disabled
+  @Test
+  void test() throws InterruptedException {
+    NodeUsageCollector c = new NodeUsageCollector(node, listener);
+    c.addUsed("a");
+    c.addUsed("b");
+    c = null;
+
+    System.gc();
+    Thread.sleep(100);
+
+    assertThat(listener.collectCount).isEqualTo(1);
+    assertThat(node).isNotNull();
+  }
+
+  static class Listener implements NodeUsageListener {
+
+    int collectCount;
+
+    @Override
+    public void collectNodeUsage(NodeUsageCollector.State collector) {
+      collectCount++;
+      System.out.println("collectNodeUsage " + collector);
+    }
+  }
+}

ebean-autotune/src/main/java/io/ebeaninternal/server/autotune/service/ProfileManager.java

@@ -91,8 +91,8 @@ public class ProfileManager implements ProfilingListener {
    * is called on the bean.
    */
   @Override
-  public void collectNodeUsage(NodeUsageCollector usageCollector) {
-    ProfileOrigin profileOrigin = getProfileOrigin(usageCollector.getNode().getOriginQueryPoint());
+  public void collectNodeUsage(NodeUsageCollector.State usageCollector) {
+    ProfileOrigin profileOrigin = getProfileOrigin(usageCollector.node().getOriginQueryPoint());
     profileOrigin.collectUsageInfo(usageCollector);
   }
 

ebean-autotune/src/main/java/io/ebeaninternal/server/autotune/service/ProfileOrigin.java

@@ -151,9 +151,9 @@ public class ProfileOrigin {
   /**
    * Collect the usage information for from a instance for this node.
    */
-  public void collectUsageInfo(NodeUsageCollector profile) {
+  public void collectUsageInfo(NodeUsageCollector.State profile) {
     if (!profile.isEmpty()) {
-      getNodeStats(profile.getNode().getPath()).collectUsageInfo(profile);
+      getNodeStats(profile.node().getPath()).collectUsageInfo(profile);
     }
   }
 

ebean-autotune/src/main/java/io/ebeaninternal/server/autotune/service/ProfileOriginNodeUsage.java

@@ -103,11 +103,10 @@ public class ProfileOriginNodeUsage {
   /**
    * Collect usage from a node.
    */
-  protected void collectUsageInfo(NodeUsageCollector profile) {
+  protected void collectUsageInfo(NodeUsageCollector.State profile) {
     lock.lock();
     try {
-      Set<String> used = profile.getUsed();
-
+      Set<String> used = profile.used();
       profileCount++;
       if (!used.isEmpty()) {
         profileUsedCount++;

ebean-autotune/src/test/java/io/ebeaninternal/server/autotune/service/ProfileOriginTest.java

@@ -1,6 +1,7 @@
 package io.ebeaninternal.server.autotune.service;
 
 import io.ebean.bean.NodeUsageCollector;
+import io.ebean.bean.NodeUsageListener;
 import io.ebean.bean.ObjectGraphNode;
 import io.ebean.bean.ObjectGraphOrigin;
 import io.ebeaninternal.server.deploy.BeanDescriptor;
@@ -13,6 +14,15 @@ import static org.assertj.core.api.Assertions.assertThat;
 
 public class ProfileOriginTest extends BaseTestCase {
 
+  static class Noop implements NodeUsageListener {
+    @Override
+    public void collectNodeUsage(NodeUsageCollector.State state) {
+      // do nothing
+    }
+  }
+
+  private final NodeUsageListener listener = new Noop();
+
   private final BeanDescriptor<Order> desc = getBeanDescriptor(Order.class);
 
   @Test
@@ -23,7 +33,7 @@ public class ProfileOriginTest extends BaseTestCase {
     c.addUsed("name");
 
     ProfileOrigin po = new ProfileOrigin(null, false, 1, 1);
-    po.collectUsageInfo(c);
+    po.collectUsageInfo(c.state());
 
     OrmQueryDetail detail = po.buildDetail(desc);
 
@@ -38,11 +48,11 @@ public class ProfileOriginTest extends BaseTestCase {
     c.addUsed("name");
 
     ProfileOrigin po = new ProfileOrigin(null, false, 1, 1);
-    po.collectUsageInfo(c);
+    po.collectUsageInfo(c.state());
 
     c = node(null);
     c.addUsed("orderDate");
-    po.collectUsageInfo(c);
+    po.collectUsageInfo(c.state());
 
     OrmQueryDetail detail = po.buildDetail(desc);
 
@@ -56,11 +66,11 @@ public class ProfileOriginTest extends BaseTestCase {
     c.addUsed("id");
 
     ProfileOrigin po = new ProfileOrigin(null, false, 1, 1);
-    po.collectUsageInfo(c);
+    po.collectUsageInfo(c.state());
 
     c = node(null);
     c.addUsed("orderDate");
-    po.collectUsageInfo(c);
+    po.collectUsageInfo(c.state());
 
     OrmQueryDetail detail = po.buildDetail(desc);
 
@@ -75,15 +85,15 @@ public class ProfileOriginTest extends BaseTestCase {
     NodeUsageCollector c = node(null);
     c.addUsed("orderDate");
     c.addUsed("customer");
-    po.collectUsageInfo(c);
+    po.collectUsageInfo(c.state());
 
     c = node("customer");
     c.addUsed("billingAddress");
-    po.collectUsageInfo(c);
+    po.collectUsageInfo(c.state());
 
     c = node("customer.billingAddress");
     c.addUsed("id");
-    po.collectUsageInfo(c);
+    po.collectUsageInfo(c.state());
 
     OrmQueryDetail detail = po.buildDetail(desc);
 
@@ -100,20 +110,20 @@ public class ProfileOriginTest extends BaseTestCase {
 
     NodeUsageCollector c = node(null);
     c.addUsed("customer");
-    po.collectUsageInfo(c);
+    po.collectUsageInfo(c.state());
 
     c = node("customer");
     c.addUsed("id");
     c.addUsed("name");
     c.addUsed("note");
     c.addUsed("billingAddress");
-    po.collectUsageInfo(c);
+    po.collectUsageInfo(c.state());
 
     //fetch details.product (id,name)
     c = node("customer.billingAddress");
     c.addUsed("id");
     c.addUsed("line1");
-    po.collectUsageInfo(c);
+    po.collectUsageInfo(c.state());
 
     OrmQueryDetail detail = po.buildDetail(desc);
     assertThat(detail.asString()).isEqualTo("fetch customer (name,note) fetch customer.billingAddress (line1)");
@@ -121,7 +131,7 @@ public class ProfileOriginTest extends BaseTestCase {
 
   private NodeUsageCollector node(String path) {
     ObjectGraphNode node = new ObjectGraphNode((ObjectGraphOrigin)null, path);
-    return new NodeUsageCollector(node, null);
+    return new NodeUsageCollector(node, listener);
   }
 
 //  @Test

ebean-core/src/main/java/io/ebeaninternal/server/query/CQuery.java

@@ -18,7 +18,6 @@ import io.ebeaninternal.server.core.SpiOrmQueryRequest;
 import io.ebeaninternal.server.deploy.*;
 
 import javax.persistence.PersistenceException;
-import java.lang.ref.WeakReference;
 import java.sql.Connection;
 import java.sql.PreparedStatement;
 import java.sql.ResultSet;
@@ -149,8 +148,6 @@ public final class CQuery<T> implements DbReadContext, CancelableQuery, SpiProfi
 
   private final ProfilingListener profilingListener;
 
-  private final WeakReference<NodeUsageListener> profilingListenerRef;
-
   private final Boolean readOnly;
 
   private long profileOffset;
@@ -189,7 +186,6 @@ public final class CQuery<T> implements DbReadContext, CancelableQuery, SpiProfi
     this.objectGraphNode = query.getParentNode();
     this.profilingListener = query.getProfilingListener();
     this.autoTuneProfiling = profilingListener != null;
-    this.profilingListenerRef = autoTuneProfiling ? new WeakReference<>(profilingListener) : null;
     // set the generated sql back to the query
     // so its available to the user...
     query.setGeneratedSql(queryPlan.getSql());
@@ -673,7 +669,7 @@ public final class CQuery<T> implements DbReadContext, CancelableQuery, SpiProfi
   @Override
   public void profileBean(EntityBeanIntercept ebi, String prefix) {
     ObjectGraphNode node = request.loadContext().getObjectGraphNode(prefix);
-    ebi.setNodeUsageCollector(new NodeUsageCollector(node, profilingListenerRef));
+    ebi.setNodeUsageCollector(new NodeUsageCollector(node, profilingListener));
   }
 
   @Override