From 7e768623e489478237e04376b18be923d71e0914 Mon Sep 17 00:00:00 2001 From: rob bygrave Date: Wed, 12 Jun 2019 23:41:48 +1200 Subject: [PATCH] #1730 - Fix for wrong JDBC batch sorting / ordering This is a bit of a refactor that simplifies the ordering of batch execution (BatchedBeanHolder ordering). In short we batch by bean type and depth (rather than just type). This means that there are some cases we are not optimal (where we could batch by bean type in different depths) but that is expected to be rare and problematic for hierarchies of the same type. --- .../server/persist/BatchControl.java | 103 +++++++----------- .../server/persist/BatchDepthOrder.java | 33 ++++++ .../server/persist/BatchedBeanHolder.java | 24 ---- .../server/persist/BatchDepthOrderTest.java | 48 ++++++++ .../server/persist/BatchedBeanHolderTest.java | 45 -------- .../tests/cascade/TestMultiCascadeBatch.java | 1 + .../TestInsertBatchThenFlushThenUpdate.java | 8 +- 7 files changed, 126 insertions(+), 136 deletions(-) create mode 100644 src/main/java/io/ebeaninternal/server/persist/BatchDepthOrder.java create mode 100644 src/test/java/io/ebeaninternal/server/persist/BatchDepthOrderTest.java delete mode 100644 src/test/java/io/ebeaninternal/server/persist/BatchedBeanHolderTest.java diff --git a/src/main/java/io/ebeaninternal/server/persist/BatchControl.java b/src/main/java/io/ebeaninternal/server/persist/BatchControl.java index 0b6fd3a66..0b3bd28b3 100644 --- a/src/main/java/io/ebeaninternal/server/persist/BatchControl.java +++ b/src/main/java/io/ebeaninternal/server/persist/BatchControl.java @@ -5,12 +5,12 @@ import io.ebeaninternal.server.core.PersistRequest; import io.ebeaninternal.server.core.PersistRequestBean; import io.ebeaninternal.server.core.PersistRequestUpdateSql; import io.ebeaninternal.server.deploy.BeanDescriptor; -import io.ebeaninternal.server.deploy.BeanPropertyAssocOne; import java.sql.SQLException; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; +import java.util.IdentityHashMap; import java.util.List; /** @@ -29,6 +29,8 @@ import java.util.List; */ public final class BatchControl { + private static final Object DUMMY = new Object(); + /** * Used to sort queue entries by depth. */ @@ -46,6 +48,17 @@ public final class BatchControl { */ private final HashMap beanHoldMap = new HashMap<>(); + /** + * Set of beans in this batch. This is used to ensure that a single bean instance is not included + * in the batch twice (two separate insert requests etc). + */ + private final IdentityHashMap persistedBeans = new IdentityHashMap<>(); + + /** + * Helper to determine statement ordering based on depth (and type). + */ + private final BatchDepthOrder depthOrder = new BatchDepthOrder(); + private final SpiTransaction transaction; /** @@ -63,13 +76,10 @@ public final class BatchControl { private boolean batchFlushOnMixed = true; - private int maxDepth; - /** * Size of the largest buffer. */ private int bufferMax; - private int topCounter; private Queue earlyQueue; private Queue lateQueue; @@ -183,6 +193,14 @@ public final class BatchControl { */ private boolean addToBatch(PersistRequestBean request) throws BatchedSqlException { + Object alreadyInBatch = persistedBeans.put(request.getEntityBean(), DUMMY); + if (alreadyInBatch != null) { + // special case where the same bean instance has already been + // added to the batch (doesn't really occur with non-batching + // as the bean gets changed from dirty to loaded earlier) + return false; + } + BatchedBeanHolder beanHolder = getBeanHolder(request); int bufferSize = beanHolder.append(request); @@ -227,14 +245,14 @@ public final class BatchControl { } /** - * Flush without resetting the topOrder (maintains the depth info). + * Flush without resetting the depth info. */ public void flush() throws BatchedSqlException { flushBuffer(false); } /** - * Flush with a reset the topOrder (fully empty the batch). + * Flush with a reset of the depth info. */ public void flushReset() throws BatchedSqlException { flushBuffer(true); @@ -246,8 +264,8 @@ public final class BatchControl { public void clear() { pstmtHolder.clear(); beanHoldMap.clear(); - maxDepth = 0; - topCounter = 0; + depthOrder.clear(); + persistedBeans.clear(); } private void flushBuffer(boolean resetTop) throws BatchedSqlException { @@ -289,10 +307,10 @@ public final class BatchControl { for (BatchedBeanHolder beanHolder : bsArray) { beanHolder.executeNow(); } - + persistedBeans.clear(); if (resetTop) { beanHoldMap.clear(); - maxDepth = 0; + depthOrder.clear(); } } catch (BatchedSqlException e) { // clear the batch on error in case we want to @@ -306,69 +324,28 @@ public final class BatchControl { * Return an entry for the given type description. The type description is * typically the bean class name (or table name for MapBeans). */ - private BatchedBeanHolder getBeanHolder(PersistRequestBean request) throws BatchedSqlException { + private BatchedBeanHolder getBeanHolder(PersistRequestBean request) { - BeanDescriptor beanDescriptor = request.getBeanDescriptor(); - BatchedBeanHolder batchBeanHolder = beanHoldMap.get(beanDescriptor.rootName()); + int depth = transaction.depth(); + BeanDescriptor desc = request.getBeanDescriptor(); + + // batching by bean type AND depth + String key = desc.rootName() + ":" + depth; + + BatchedBeanHolder batchBeanHolder = beanHoldMap.get(key); if (batchBeanHolder == null) { - int relativeDepth = transaction.depth(); - - int beanDepth = 100 + relativeDepth; - if (relativeDepth == 0 && !beanHoldMap.isEmpty()) { - // could be non-cascading or uni-directional relationship - // so see look for a 'parent' in the beanHoldMap - int maybe = relativeToParentDepth(beanDescriptor); - if (maybe != -1) { - beanDepth = maybe; - } else { - // additional "top level" bean type ordered by save() order - beanDepth += ++topCounter; - } - } - - maxDepth = Math.max(maxDepth, beanDepth); - batchBeanHolder = new BatchedBeanHolder(this, beanDescriptor, beanDepth); - beanHoldMap.put(beanDescriptor.rootName(), batchBeanHolder); + int ordering = depthOrder.orderingFor(depth); + batchBeanHolder = new BatchedBeanHolder(this, desc, ordering); + beanHoldMap.put(key, batchBeanHolder); } return batchBeanHolder; } - /** - * Find a depth based on imported relationships (to a parent that is already in the buffer). - */ - private int relativeToParentDepth(BeanDescriptor beanDescriptor) { - - BeanPropertyAssocOne[] imported = beanDescriptor.propertiesOneImported(); - if (imported.length == 0) { - // a top level type so just maintain the order relative to the current depth - return maxDepth + 1; - } - - int parentMaxDepth = -1; - - for (BeanPropertyAssocOne parent : imported) { - BatchedBeanHolder parentBatch = beanHoldMap.get(parent.getTargetDescriptor().rootName()); - if (parentBatch != null) { - // deeper that the parent - parentMaxDepth = Math.max(parentMaxDepth, parentBatch.getOrder() + 1); - } - } - return parentMaxDepth; - } - /** * Return true if this holds no persist requests. */ private boolean isBeansEmpty() { - if (beanHoldMap.isEmpty()) { - return true; - } - for (BatchedBeanHolder beanHolder : beanHoldMap.values()) { - if (!beanHolder.isEmpty()) { - return false; - } - } - return true; + return persistedBeans.isEmpty(); } /** diff --git a/src/main/java/io/ebeaninternal/server/persist/BatchDepthOrder.java b/src/main/java/io/ebeaninternal/server/persist/BatchDepthOrder.java new file mode 100644 index 000000000..7fe9c10b5 --- /dev/null +++ b/src/main/java/io/ebeaninternal/server/persist/BatchDepthOrder.java @@ -0,0 +1,33 @@ +package io.ebeaninternal.server.persist; + +import java.util.HashMap; +import java.util.Map; + +/** + * Helper to determine batch execution order for BatchedBeanHolders. + */ +class BatchDepthOrder { + + private final Map map = new HashMap<>(); + + /** + * Return the batch order for the given depth. + */ + int orderingFor(int depth) { + final Counter slot = map.computeIfAbsent(depth, integer -> new Counter()); + return (depth * 100) + slot.increment(); + } + + void clear() { + map.clear(); + } + + private static class Counter { + + int count; + + public int increment() { + return count++; + } + } +} diff --git a/src/main/java/io/ebeaninternal/server/persist/BatchedBeanHolder.java b/src/main/java/io/ebeaninternal/server/persist/BatchedBeanHolder.java index 61c46ef0c..b32878494 100644 --- a/src/main/java/io/ebeaninternal/server/persist/BatchedBeanHolder.java +++ b/src/main/java/io/ebeaninternal/server/persist/BatchedBeanHolder.java @@ -5,7 +5,6 @@ import io.ebeaninternal.server.core.PersistRequestBean; import io.ebeaninternal.server.deploy.BeanDescriptor; import java.util.ArrayList; -import java.util.IdentityHashMap; /** * Holds lists of persist requests for beans of a given type. @@ -21,8 +20,6 @@ import java.util.IdentityHashMap; */ public class BatchedBeanHolder { - private static final Object DUMMY = new Object(); - /** * The owning queue. */ @@ -50,12 +47,6 @@ public class BatchedBeanHolder { */ private ArrayList deletes; - /** - * Set of beans in this batch. This is used to ensure that a single bean instance is not included - * in the batch twice (two separate insert requests etc). - */ - private final IdentityHashMap persistedBeans = new IdentityHashMap<>(); - /** * Create a new entry with a given type and depth. */ @@ -99,7 +90,6 @@ public class BatchedBeanHolder { updates = new ArrayList<>(); control.executeNow(bufferedUpdates); } - persistedBeans.clear(); } @Override @@ -123,14 +113,6 @@ public class BatchedBeanHolder { */ public int append(PersistRequestBean request) { - Object alreadyInBatch = persistedBeans.put(request.getEntityBean(), DUMMY); - if (alreadyInBatch != null) { - // special case where the same bean instance has already been - // added to the batch (doesn't really occur with non-batching - // as the bean gets changed from dirty to loaded earlier) - return 0; - } - request.setBatched(); switch (request.getType()) { @@ -161,10 +143,4 @@ public class BatchedBeanHolder { } } - /** - * Return true if this is empty containing no batched beans. - */ - public boolean isEmpty() { - return persistedBeans.isEmpty(); - } } diff --git a/src/test/java/io/ebeaninternal/server/persist/BatchDepthOrderTest.java b/src/test/java/io/ebeaninternal/server/persist/BatchDepthOrderTest.java new file mode 100644 index 000000000..d9640e7da --- /dev/null +++ b/src/test/java/io/ebeaninternal/server/persist/BatchDepthOrderTest.java @@ -0,0 +1,48 @@ +package io.ebeaninternal.server.persist; + +import org.junit.Test; + +import static org.junit.Assert.*; + +public class BatchDepthOrderTest { + + @Test + public void orderingFor() { + + BatchDepthOrder depthOrder = new BatchDepthOrder(); + + assertEquals(0, depthOrder.orderingFor(0)); + assertEquals(1, depthOrder.orderingFor(0)); + assertEquals(2, depthOrder.orderingFor(0)); + assertEquals(100, depthOrder.orderingFor(1)); + assertEquals(101, depthOrder.orderingFor(1)); + assertEquals(200, depthOrder.orderingFor(2)); + assertEquals(3, depthOrder.orderingFor(0)); + + assertEquals(-100, depthOrder.orderingFor(-1)); + assertEquals(-99, depthOrder.orderingFor(-1)); + assertEquals(-98, depthOrder.orderingFor(-1)); + + assertEquals(-200, depthOrder.orderingFor(-2)); + assertEquals(-199, depthOrder.orderingFor(-2)); + } + + + @Test + public void clear() { + + BatchDepthOrder depthOrder = new BatchDepthOrder(); + + assertEquals(0, depthOrder.orderingFor(0)); + assertEquals(1, depthOrder.orderingFor(0)); + assertEquals(100, depthOrder.orderingFor(1)); + assertEquals(101, depthOrder.orderingFor(1)); + + depthOrder.clear(); + + assertEquals(0, depthOrder.orderingFor(0)); + assertEquals(100, depthOrder.orderingFor(1)); + assertEquals(200, depthOrder.orderingFor(2)); + } + +} diff --git a/src/test/java/io/ebeaninternal/server/persist/BatchedBeanHolderTest.java b/src/test/java/io/ebeaninternal/server/persist/BatchedBeanHolderTest.java deleted file mode 100644 index 12a7acef0..000000000 --- a/src/test/java/io/ebeaninternal/server/persist/BatchedBeanHolderTest.java +++ /dev/null @@ -1,45 +0,0 @@ -package io.ebeaninternal.server.persist; - -import io.ebean.BaseTestCase; -import io.ebeaninternal.api.TDSpiEbeanServer; -import io.ebeaninternal.server.core.PersistRequest; -import io.ebeaninternal.server.core.PersistRequestBean; -import io.ebeaninternal.server.deploy.BeanDescriptor; -import io.ebeaninternal.server.deploy.BeanManager; -import org.junit.Test; -import org.tests.model.basic.Customer; - -import java.sql.Timestamp; - -import static org.junit.Assert.assertEquals; - -public class BatchedBeanHolderTest extends BaseTestCase { - - @SuppressWarnings({ "rawtypes", "unchecked" }) - @Test - public void testAppend() { - - TDSpiEbeanServer server = new TDSpiEbeanServer("foo"); - - BeanDescriptor beanDescriptor = getBeanDescriptor(Customer.class); - - //BeanDescriptor beanDescriptor = Mockito.mock(BeanDescriptor.class); - BeanManager beanManager = new BeanManager(beanDescriptor, null); - - - BatchedBeanHolder holder = new BatchedBeanHolder(null, beanDescriptor, 1); - - Customer customer = new Customer(); - customer.setUpdtime(new Timestamp(System.currentTimeMillis())); - PersistRequestBean req1 = new PersistRequestBean(server, customer, null, beanManager, null, null, PersistRequest.Type.INSERT, 0); - - - int size = holder.append(req1); - assertEquals(1, size); - - PersistRequestBean req2 = new PersistRequestBean(server, customer, null, beanManager, null, null, PersistRequest.Type.INSERT, 0); - size = holder.append(req2); - assertEquals(0, size); - - } -} diff --git a/src/test/java/org/tests/cascade/TestMultiCascadeBatch.java b/src/test/java/org/tests/cascade/TestMultiCascadeBatch.java index 1e23d0cb0..d2733ddc3 100644 --- a/src/test/java/org/tests/cascade/TestMultiCascadeBatch.java +++ b/src/test/java/org/tests/cascade/TestMultiCascadeBatch.java @@ -28,6 +28,7 @@ public class TestMultiCascadeBatch extends BaseTestCase { @Test public void testMultipleCascadeInsideTransaction() { + final Site mainSite = new Site(); mainSite.setName("mainSite"); Ebean.save(mainSite); diff --git a/src/test/java/org/tests/model/basic/xtra/TestInsertBatchThenFlushThenUpdate.java b/src/test/java/org/tests/model/basic/xtra/TestInsertBatchThenFlushThenUpdate.java index 294e8d898..6fe2e0d18 100644 --- a/src/test/java/org/tests/model/basic/xtra/TestInsertBatchThenFlushThenUpdate.java +++ b/src/test/java/org/tests/model/basic/xtra/TestInsertBatchThenFlushThenUpdate.java @@ -38,7 +38,7 @@ public class TestInsertBatchThenFlushThenUpdate extends BaseTestCase { Ebean.save(parent); // nothing flushed yet - assertEquals(0, LoggedSqlCollector.start().size()); + assertThat(LoggedSqlCollector.start()).isEmpty(); txn.flushBatch(); @@ -49,14 +49,14 @@ public class TestInsertBatchThenFlushThenUpdate extends BaseTestCase { Ebean.save(parent); // nothing flushed yet - assertEquals(0, LoggedSqlCollector.start().size()); + assertThat(LoggedSqlCollector.start()).isEmpty(); Ebean.commitTransaction(); // insert statements for EdExtendedParent List loggedSql2 = LoggedSqlCollector.start(); - assertEquals(2, loggedSql2.size()); - assertTrue(loggedSql2.get(0).contains(" update td_parent ")); + assertThat(loggedSql2).hasSize(2); + assertThat(loggedSql2.get(0)).contains(" update td_parent "); } }