From e1b9bf62a3d2581ada5bdc84e3f7e43e472a5903 Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Thu, 21 Apr 2022 08:40:00 +1200 Subject: [PATCH 1/2] NodeUsageCollector migrate from using finalize() to Cleaner --- .../io/ebean/bean/NodeUsageCollector.java | 165 +++++++++--------- .../java/io/ebean/bean/NodeUsageListener.java | 3 +- .../io/ebean/bean/NodeUsageCollectorTest.java | 44 +++++ .../autotune/service/ProfileManager.java | 4 +- .../autotune/service/ProfileOrigin.java | 4 +- .../service/ProfileOriginNodeUsage.java | 5 +- .../autotune/service/ProfileOriginTest.java | 22 +-- 7 files changed, 143 insertions(+), 104 deletions(-) create mode 100644 ebean-api/src/test/java/io/ebean/bean/NodeUsageCollectorTest.java diff --git a/ebean-api/src/main/java/io/ebean/bean/NodeUsageCollector.java b/ebean-api/src/main/java/io/ebean/bean/NodeUsageCollector.java index bb65f6735..708f6acf1 100644 --- a/ebean-api/src/main/java/io/ebean/bean/NodeUsageCollector.java +++ b/ebean-api/src/main/java/io/ebean/bean/NodeUsageCollector.java @@ -1,5 +1,6 @@ package io.ebean.bean; +import java.lang.ref.Cleaner; import java.lang.ref.WeakReference; import java.util.LinkedHashSet; import java.util.Set; @@ -8,120 +9,116 @@ import java.util.Set; * Collects profile information for a bean (or reference/proxy bean) at a given node. *

* The node identifies the location of the bean in the object graph. - *

- *

- * It has to use a weak reference so as to ensure that it does not stop the - * associated bean from being garbage collected. - *

*/ 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(); - /** - * Weak to allow garbage collection. - */ - private final WeakReference managerRef; + public static final class State implements Runnable { + private final WeakReference managerRef; + /** + * The properties used at this profile point. + */ + private final Set 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; - /** - * The properties used at this profile point. - */ - private final Set used = new LinkedHashSet<>(); + /** + * The property that cause a reference to lazy load. + */ + private String loadProperty; - /** - * set to true if the bean is modified (setter called) - */ - private boolean modified; + private State(ObjectGraphNode node, WeakReference managerRef) { + this.node = node; + this.managerRef = managerRef; + } - /** - * The property that cause a reference to lazy load. - */ - private String loadProperty; + @Override + public String toString() { + return node + " read:" + used + " modified:" + modified; + } + + @Override + public void run() { + NodeUsageListener manager = managerRef.get(); + if (manager != null) { + manager.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 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, WeakReference managerRef) { - this.node = node; - // weak to allow garbage collection. - this.managerRef = managerRef; + this.state = new State(node, managerRef); + cleaner.register(this, state); + } + + /** + * Return the underlying state. + */ + 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); + state.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 getUsed() { - return used; - } - - /** - * Return true if the bean was modified by a setter. - */ - public boolean isModified() { - return modified; - } - - public String getLoadProperty() { - return loadProperty; + state.loadProperty = loadProperty; } @Override public String toString() { - return node + " read:" + used + " modified:" + modified; + return state.toString(); } } diff --git a/ebean-api/src/main/java/io/ebean/bean/NodeUsageListener.java b/ebean-api/src/main/java/io/ebean/bean/NodeUsageListener.java index bfa2c1892..0e4897e90 100644 --- a/ebean-api/src/main/java/io/ebean/bean/NodeUsageListener.java +++ b/ebean-api/src/main/java/io/ebean/bean/NodeUsageListener.java @@ -10,7 +10,6 @@ public interface NodeUsageListener { *

* This is the properties that are used for a given bean in the object graph. * This information is used by autoTune to tune queries. - *

*/ - void collectNodeUsage(NodeUsageCollector collector); + void collectNodeUsage(NodeUsageCollector.State state); } diff --git a/ebean-api/src/test/java/io/ebean/bean/NodeUsageCollectorTest.java b/ebean-api/src/test/java/io/ebean/bean/NodeUsageCollectorTest.java new file mode 100644 index 000000000..851922142 --- /dev/null +++ b/ebean-api/src/test/java/io/ebean/bean/NodeUsageCollectorTest.java @@ -0,0 +1,44 @@ +package io.ebean.bean; + +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; + +import java.lang.ref.WeakReference; + +import static org.assertj.core.api.Assertions.assertThat; + +class NodeUsageCollectorTest { + + private final Listener listener = new Listener(); + + /** + * Run this manually as we make explicit GC call here. + */ + @Disabled + @Test + void test() throws InterruptedException { + WeakReference profilingListenerRef = new WeakReference<>(listener); + + ObjectGraphNode node = new ObjectGraphNode((ObjectGraphOrigin)null, "foo"); + NodeUsageCollector c = new NodeUsageCollector(node, profilingListenerRef); + c.addUsed("a"); + c.addUsed("b"); + c = null; + + System.gc(); + Thread.sleep(100); + + assertThat(listener.collectCount).isEqualTo(1); + } + + static class Listener implements NodeUsageListener { + + int collectCount; + + @Override + public void collectNodeUsage(NodeUsageCollector.State collector) { + collectCount++; + System.out.println("collectNodeUsage " + collector); + } + } +} diff --git a/ebean-autotune/src/main/java/io/ebeaninternal/server/autotune/service/ProfileManager.java b/ebean-autotune/src/main/java/io/ebeaninternal/server/autotune/service/ProfileManager.java index 019955114..24469ee75 100644 --- a/ebean-autotune/src/main/java/io/ebeaninternal/server/autotune/service/ProfileManager.java +++ b/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); } diff --git a/ebean-autotune/src/main/java/io/ebeaninternal/server/autotune/service/ProfileOrigin.java b/ebean-autotune/src/main/java/io/ebeaninternal/server/autotune/service/ProfileOrigin.java index b9935f558..4e2427524 100644 --- a/ebean-autotune/src/main/java/io/ebeaninternal/server/autotune/service/ProfileOrigin.java +++ b/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); } } diff --git a/ebean-autotune/src/main/java/io/ebeaninternal/server/autotune/service/ProfileOriginNodeUsage.java b/ebean-autotune/src/main/java/io/ebeaninternal/server/autotune/service/ProfileOriginNodeUsage.java index 1f2a655dc..c5a3c17db 100644 --- a/ebean-autotune/src/main/java/io/ebeaninternal/server/autotune/service/ProfileOriginNodeUsage.java +++ b/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 used = profile.getUsed(); - + Set used = profile.used(); profileCount++; if (!used.isEmpty()) { profileUsedCount++; diff --git a/ebean-autotune/src/test/java/io/ebeaninternal/server/autotune/service/ProfileOriginTest.java b/ebean-autotune/src/test/java/io/ebeaninternal/server/autotune/service/ProfileOriginTest.java index 9f981a404..e9069c9b4 100644 --- a/ebean-autotune/src/test/java/io/ebeaninternal/server/autotune/service/ProfileOriginTest.java +++ b/ebean-autotune/src/test/java/io/ebeaninternal/server/autotune/service/ProfileOriginTest.java @@ -23,7 +23,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 +38,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 +56,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 +75,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 +100,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)"); From cacbee5fd5cf5e9eb22620488d4f7633b015cd64 Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Thu, 21 Apr 2022 08:59:53 +1200 Subject: [PATCH 2/2] Refactor tidy NodeUsageCollector, remove unused loadProperty and unnecessary WeakReference for State.listener --- .../io/ebean/bean/EntityBeanIntercept.java | 3 -- .../io/ebean/bean/NodeUsageCollector.java | 29 +++++-------------- .../io/ebean/bean/NodeUsageCollectorTest.java | 12 ++++---- .../autotune/service/ProfileOriginTest.java | 12 +++++++- .../io/ebeaninternal/server/query/CQuery.java | 6 +--- 5 files changed, 24 insertions(+), 38 deletions(-) diff --git a/ebean-api/src/main/java/io/ebean/bean/EntityBeanIntercept.java b/ebean-api/src/main/java/io/ebean/bean/EntityBeanIntercept.java index 888f8f23e..833d7cb31 100644 --- a/ebean-api/src/main/java/io/ebean/bean/EntityBeanIntercept.java +++ b/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 diff --git a/ebean-api/src/main/java/io/ebean/bean/NodeUsageCollector.java b/ebean-api/src/main/java/io/ebean/bean/NodeUsageCollector.java index 708f6acf1..c168b3a66 100644 --- a/ebean-api/src/main/java/io/ebean/bean/NodeUsageCollector.java +++ b/ebean-api/src/main/java/io/ebean/bean/NodeUsageCollector.java @@ -1,7 +1,6 @@ package io.ebean.bean; import java.lang.ref.Cleaner; -import java.lang.ref.WeakReference; import java.util.LinkedHashSet; import java.util.Set; @@ -15,7 +14,8 @@ public final class NodeUsageCollector { private final static Cleaner cleaner = Cleaner.create(); public static final class State implements Runnable { - private final WeakReference managerRef; + + private final NodeUsageListener listener; /** * The properties used at this profile point. */ @@ -29,14 +29,9 @@ public final class NodeUsageCollector { */ private boolean modified; - /** - * The property that cause a reference to lazy load. - */ - private String loadProperty; - - private State(ObjectGraphNode node, WeakReference managerRef) { + private State(ObjectGraphNode node, NodeUsageListener listener) { this.node = node; - this.managerRef = managerRef; + this.listener = listener; } @Override @@ -46,10 +41,7 @@ public final class NodeUsageCollector { @Override public void run() { - NodeUsageListener manager = managerRef.get(); - if (manager != null) { - manager.collectNodeUsage(this); - } + listener.collectNodeUsage(this); } /** @@ -84,8 +76,8 @@ public final class NodeUsageCollector { private final State state; - public NodeUsageCollector(ObjectGraphNode node, WeakReference managerRef) { - this.state = new State(node, managerRef); + public NodeUsageCollector(ObjectGraphNode node, NodeUsageListener listener) { + this.state = new State(node, listener); cleaner.register(this, state); } @@ -110,13 +102,6 @@ public final class NodeUsageCollector { state.used.add(property); } - /** - * The property that invoked a lazy load. - */ - public void setLoadProperty(String loadProperty) { - state.loadProperty = loadProperty; - } - @Override public String toString() { return state.toString(); diff --git a/ebean-api/src/test/java/io/ebean/bean/NodeUsageCollectorTest.java b/ebean-api/src/test/java/io/ebean/bean/NodeUsageCollectorTest.java index 851922142..103772249 100644 --- a/ebean-api/src/test/java/io/ebean/bean/NodeUsageCollectorTest.java +++ b/ebean-api/src/test/java/io/ebean/bean/NodeUsageCollectorTest.java @@ -3,24 +3,21 @@ package io.ebean.bean; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; -import java.lang.ref.WeakReference; - 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 explicit GC call here. + * Run this manually as we make use of explicit GC call here which is dubious. */ @Disabled @Test void test() throws InterruptedException { - WeakReference profilingListenerRef = new WeakReference<>(listener); - - ObjectGraphNode node = new ObjectGraphNode((ObjectGraphOrigin)null, "foo"); - NodeUsageCollector c = new NodeUsageCollector(node, profilingListenerRef); + NodeUsageCollector c = new NodeUsageCollector(node, listener); c.addUsed("a"); c.addUsed("b"); c = null; @@ -29,6 +26,7 @@ class NodeUsageCollectorTest { Thread.sleep(100); assertThat(listener.collectCount).isEqualTo(1); + assertThat(node).isNotNull(); } static class Listener implements NodeUsageListener { diff --git a/ebean-autotune/src/test/java/io/ebeaninternal/server/autotune/service/ProfileOriginTest.java b/ebean-autotune/src/test/java/io/ebeaninternal/server/autotune/service/ProfileOriginTest.java index e9069c9b4..371d1976b 100644 --- a/ebean-autotune/src/test/java/io/ebeaninternal/server/autotune/service/ProfileOriginTest.java +++ b/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 desc = getBeanDescriptor(Order.class); @Test @@ -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 diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQuery.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQuery.java index 22e2e1f37..3566ee08a 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQuery.java +++ b/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 implements DbReadContext, CancelableQuery, SpiProfi private final ProfilingListener profilingListener; - private final WeakReference profilingListenerRef; - private final Boolean readOnly; private long profileOffset; @@ -189,7 +186,6 @@ public final class CQuery 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 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