fix eclipse-ee4j#1596 Major performance issue in case of many new ent…

…ities (eclipse-ee4j#1843) * Issue 1596: use a hash-based collection to lookup objects in the cache instead of a linear search Signed-off-by: Patrick Schmitt <Patrick.Schmitt@cpb-software.com> * Adjusted source according to review: - extended tests - updated copyright year - call removeObjectFromPrimaryKeyToNewObjects in preMergeChanges - remove list from primaryKeyToNewObjects if it is empty - simplify tests Signed-off-by: Patrick Schmitt <Patrick.Schmitt@cpb-software.com>
jmix-framework · Apr 11, 2023 · 76d8274 · 76d8274
1 parent 3f84c1c
commit 76d8274
Show file tree

Hide file tree

Showing 9 changed files with 134 additions and 16 deletions.
diff --git a/...eclipse/persistence/testing/tests/queries/ConformResultsWithPrimaryKeyExpressionTest.java b/...eclipse/persistence/testing/tests/queries/ConformResultsWithPrimaryKeyExpressionTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2021 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -158,7 +158,7 @@ public void prepareTest() {
         case CASE_NEW: {
             selectionObject = newEmployee;
             if (shouldCheckCacheByExactPrimaryKey()) {
-                expectedGetIdCallCount = 1;
+                expectedGetIdCallCount = 0;
             } else {
                 expectedGetIdCallCount = n + 1;
             }
@@ -192,7 +192,7 @@ public void prepareTest() {
                 // S.M. This went from 5 calls to 4, which is good.
                 // When checking the one new object + registration +
                 // building clone + building backup clone.
-                expectedGetIdCallCount = 3;
+                expectedGetIdCallCount = 2;
             } else {
                 expectedGetIdCallCount = n + 4;
             }

diff --git a/...ce/testing/tests/unitofwork/DoubleNestedUnitOfWorkDeleteConformedNestedNewObjectTest.java b/...ce/testing/tests/unitofwork/DoubleNestedUnitOfWorkDeleteConformedNestedNewObjectTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2021 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -15,6 +15,7 @@
 package org.eclipse.persistence.testing.tests.unitofwork;
 
 import java.math.BigDecimal;
+import java.util.Collection;
 
 import org.eclipse.persistence.internal.sessions.UnitOfWorkImpl;
 import org.eclipse.persistence.queries.ReadObjectQuery;
@@ -65,6 +66,10 @@ public void test() {
             throw new TestErrorException("Failed to unregister the Object in the nested unit of work");
         }
 
+        if (!((UnitOfWorkImpl)uow).getPrimaryKeyToNewObjects().isEmpty()) {
+            throw new TestErrorException("Failed to unregister the Object in the nested unit of work");
+        }
+
     }
 
     @Override

diff --git a/...pse/persistence/testing/tests/unitofwork/DoubleNestedUnitOfWorkRegisterNewObjectTest.java b/...pse/persistence/testing/tests/unitofwork/DoubleNestedUnitOfWorkRegisterNewObjectTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2021 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -15,6 +15,7 @@
 package org.eclipse.persistence.testing.tests.unitofwork;
 
 import java.math.BigDecimal;
+import java.util.Collection;
 
 import org.eclipse.persistence.internal.sessions.UnitOfWorkImpl;
 import org.eclipse.persistence.queries.ReadObjectQuery;
@@ -65,6 +66,10 @@ public void test() {
             throw new TestErrorException("Failed to unregister the Object in the nested unit of work");
         }
 
+        if (!((UnitOfWorkImpl)uow).getPrimaryKeyToNewObjects().isEmpty()) {
+            throw new TestErrorException("Failed to unregister the Object in the nested unit of work");
+        }
+
     }
 
     @Override

diff --git a/...sistence/testing/tests/unitofwork/NestedUnitOfWorkDeleteConformedNestedNewObjectTest.java b/...sistence/testing/tests/unitofwork/NestedUnitOfWorkDeleteConformedNestedNewObjectTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2021 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -16,6 +16,7 @@
 
 import java.math.BigDecimal;
 
+import java.util.Collection;
 import java.util.Vector;
 
 import org.eclipse.persistence.internal.sessions.UnitOfWorkImpl;
@@ -67,6 +68,10 @@ public void test() {
             throw new TestErrorException("Failed to unregister the Object in the nested unit of work");
         }
 
+        if (!((UnitOfWorkImpl)uow).getPrimaryKeyToNewObjects().isEmpty()) {
+            throw new TestErrorException("Failed to unregister the Object in the nested unit of work");
+        }
+
     }
 
     @Override

diff --git a/...lipse/persistence/testing/tests/unitofwork/NestedUnitOfWorkDeleteNestedNewObjectTest.java b/...lipse/persistence/testing/tests/unitofwork/NestedUnitOfWorkDeleteNestedNewObjectTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2021 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -49,6 +49,10 @@ public void test() {
             throw new TestErrorException("Failed to unregister the Object in the nested unit of work");
         }
 
+        if (((UnitOfWorkImpl)uow).getPrimaryKeyToNewObjects().values().stream().anyMatch(c -> c.contains(employee))) {
+            throw new TestErrorException("Failed to unregister the Object in the nested unit of work");
+        }
+
     }
 
     @Override

diff --git a/...org/eclipse/persistence/testing/tests/unitofwork/NestedUnitOfWorkDeleteNewObjectTest.java b/...org/eclipse/persistence/testing/tests/unitofwork/NestedUnitOfWorkDeleteNewObjectTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2021 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -44,6 +44,10 @@ public void test() {
             throw new TestErrorException("Failed to move the deleted new object into the parent UOW");
         }
 
+        if (((UnitOfWorkImpl)uow).getPrimaryKeyToNewObjects().values().stream().anyMatch(c -> c.contains(original))) {
+            throw new TestErrorException("Failed to move the deleted new object into the parent UOW");
+        }
+
     }
 
     @Override

diff --git a/...rc/it/java/org/eclipse/persistence/testing/tests/unitofwork/UnregisterUnitOfWorkTest.java b/...rc/it/java/org/eclipse/persistence/testing/tests/unitofwork/UnregisterUnitOfWorkTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2021 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -14,6 +14,7 @@
 //     Oracle - initial API and implementation from Oracle TopLink
 package org.eclipse.persistence.testing.tests.unitofwork;
 
+import java.util.Collection;
 import java.util.Enumeration;
 import java.util.Vector;
 
@@ -55,6 +56,10 @@ protected void test() {
             throw new TestErrorException("Deep unregister object did not work");
         }
 
+        if (!uow.getPrimaryKeyToNewObjects().isEmpty()) {
+            throw new TestErrorException("Deep unregister object did not work");
+        }
+
         uow.commit();
 
         uow = (UnitOfWorkImpl)getSession().acquireUnitOfWork();
@@ -76,6 +81,10 @@ protected void test() {
             throw new TestErrorException("Deep unregister object did not work");
         }
 
+        if (!uow.getPrimaryKeyToNewObjects().isEmpty()) {
+            throw new TestErrorException("Deep unregister object did not work");
+        }
+
         uow.commit();
 
         /*******************/

diff --git a/...sistence.core/src/main/java/org/eclipse/persistence/internal/sessions/UnitOfWorkImpl.java b/...sistence.core/src/main/java/org/eclipse/persistence/internal/sessions/UnitOfWorkImpl.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2021 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -155,6 +155,11 @@ public class UnitOfWorkImpl extends AbstractSession implements org.eclipse.persi
     protected Map<Object, Object> cloneMapping;
     protected Map<Object, Object> newObjectsCloneToOriginal;
     protected Map<Object, Object> newObjectsOriginalToClone;
+
+    /**
+     * Map of primary key to list of new objects. Used to speedup in-memory querying.
+     */
+    protected Map<Object, List<Object>> primaryKeyToNewObjects;
     /**
      * Stores a map from the clone to the original merged object, as a different instance is used as the original for merges.
      */
@@ -544,6 +549,8 @@ public Object assignSequenceNumber(Object object, ClassDescriptor descriptor) th
             ObjectBuilder builder = descriptor.getObjectBuilder();
             try {
                 value = builder.assignSequenceNumber(object, this);
+                getPrimaryKeyToNewObjects().putIfAbsent(value, new ArrayList<>());
+                getPrimaryKeyToNewObjects().get(value).add(object);
             } catch (RuntimeException exception) {
                 handleException(exception);
             } finally {
@@ -580,6 +587,7 @@ public void assignSequenceNumbers() throws DatabaseException {
         }
         if (hasNewObjects()) {
             assignSequenceNumbers(getNewObjectsCloneToOriginal());
+            setupPrimaryKeyToNewObjects();
         }
     }
 
@@ -2394,6 +2402,18 @@ public Map getNewObjectsCloneToOriginal() {
         return newObjectsCloneToOriginal;
     }
 
+    /**
+     * INTERNAL:
+     * The primaryKeyToNewObjects stores a list of objects for every primary key.
+     * It is used to speed up in-memory-querying.
+     */
+    public Map<Object, List<Object>> getPrimaryKeyToNewObjects() {
+        if (primaryKeyToNewObjects == null) {
+            primaryKeyToNewObjects = new HashMap<>();
+        }
+        return primaryKeyToNewObjects;
+    }
+
     /**
      * INTERNAL:
      * The stores a map from new object clones to the original object used from merge.
@@ -2544,15 +2564,12 @@ public Object getObjectFromNewObjects(Class<?> theClass, Object selectionKey) {
         boolean readSubclassesOrNoInheritance = (!descriptor.hasInheritance() || descriptor.getInheritancePolicy().shouldReadSubclasses());
 
         ObjectBuilder objectBuilder = descriptor.getObjectBuilder();
-        for (Iterator newObjectsEnum = getNewObjectsCloneToOriginal().keySet().iterator();
-                 newObjectsEnum.hasNext();) {
+        for (Iterator newObjectsEnum = getPrimaryKeyToNewObjects().getOrDefault(selectionKey, new ArrayList<>(0)).iterator();
+             newObjectsEnum.hasNext(); ) {
             Object object = newObjectsEnum.next();
             // bug 327900
             if ((object.getClass() == theClass) || (readSubclassesOrNoInheritance && (theClass.isInstance(object)))) {
-                Object primaryKey = objectBuilder.extractPrimaryKeyFromObject(object, this, true);
-                if ((primaryKey != null) && primaryKey.equals(selectionKey)) {
                     return object;
-                }
             }
         }
         return null;
@@ -3861,6 +3878,7 @@ protected void preMergeChanges() {
                     Object referenceObjectToRemove = getNewObjectsCloneToOriginal().get(removedObject);
                     if (referenceObjectToRemove != null) {
                         getNewObjectsCloneToOriginal().remove(removedObject);
+                        removeObjectFromPrimaryKeyToNewObjects(removedObject);
                         getNewObjectsOriginalToClone().remove(referenceObjectToRemove);
                     }
                 }
@@ -4480,6 +4498,7 @@ protected void registerNewObjectClone(Object clone, Object original, ClassDescri
         registerNewObjectInIdentityMap(clone, original, descriptor);
 
         getNewObjectsCloneToOriginal().put(clone, original);
+        addNewObjectToPrimaryKeyToNewObjects(clone,descriptor);
         if (original != null) {
             getNewObjectsOriginalToClone().put(original, clone);
         }
@@ -5000,6 +5019,69 @@ public void setMergeManager(MergeManager mergeManager) {
      */
     protected void setNewObjectsCloneToOriginal(Map newObjects) {
         this.newObjectsCloneToOriginal = newObjects;
+        setupPrimaryKeyToNewObjects();
+    }
+
+    /**
+     * INTERNAL:
+     * Create the map of primary key to new objects used to speed up in-memory querying.
+     */
+    protected void setupPrimaryKeyToNewObjects() {
+        primaryKeyToNewObjects = null;
+        if (hasNewObjects()) {
+            primaryKeyToNewObjects = new HashMap<>();
+            getNewObjectsCloneToOriginal().forEach((object, o2) -> {
+                addNewObjectToPrimaryKeyToNewObjects(object);
+            });
+        }
+    }
+
+    /**
+     * INTERNAL:
+     * Extracts the primary key from a new object and puts it in primaryKeyToNewObjects.
+     */
+    protected void addNewObjectToPrimaryKeyToNewObjects(Object newObject){
+        ClassDescriptor descriptor = getDescriptor(newObject.getClass());
+        addNewObjectToPrimaryKeyToNewObjects(newObject,descriptor);
+    }
+
+    /**
+     * INTERNAL:
+     * Extracts the primary key from a new object and puts it in primaryKeyToNewObjects.
+     * Allows passing of the ClassDescriptor.
+     */
+    protected void addNewObjectToPrimaryKeyToNewObjects(Object newObject, ClassDescriptor descriptor){
+        ObjectBuilder objectBuilder = descriptor.getObjectBuilder();
+        Object pk = objectBuilder.extractPrimaryKeyFromObject(newObject, this, true);
+        if(pk!=null) {
+            getPrimaryKeyToNewObjects().putIfAbsent(pk, new ArrayList<>());
+            getPrimaryKeyToNewObjects().get(pk).add(newObject);
+        }
+    }
+    /**
+     * INTERNAL:
+     * Extracts the primary key and removes an object from primaryKeyToNewObjects.
+     */
+    protected void removeObjectFromPrimaryKeyToNewObjects(Object object){
+        ClassDescriptor descriptor = getDescriptor(object.getClass());
+        ObjectBuilder objectBuilder = descriptor.getObjectBuilder();
+        Object pk = objectBuilder.extractPrimaryKeyFromObject(object, this, true);
+        removeObjectFromPrimaryKeyToNewObjects(object,pk);
+    }
+
+    /**
+     * INTERNAL:
+     * Removes an object from primaryKeyToNewObjects.
+     */
+    protected void removeObjectFromPrimaryKeyToNewObjects(Object object, Object primaryKey){
+        Map<Object, List<Object>> pkToNewObjects = getPrimaryKeyToNewObjects();
+        if (pkToNewObjects.containsKey(primaryKey)) {
+            List<Object> newObjects = pkToNewObjects.get(primaryKey);
+            newObjects.remove(object);
+            if (newObjects.isEmpty()) {
+               pkToNewObjects.remove(primaryKey);
+            }
+        }
     }
 
     /**
@@ -5459,6 +5541,7 @@ public void resumeUnitOfWork() {
             }
             this.newObjectsCloneToOriginal = null;
             this.newObjectsOriginalToClone = null;
+            this.primaryKeyToNewObjects = null;
         }
         this.unregisteredExistingObjects = null;
         this.unregisteredNewObjects = null;
@@ -5579,6 +5662,7 @@ public void iterate(Object object) {
                 // PERF: Avoid initialization of new objects if none.
                 if (hasNewObjects()) {
                     Object original = getNewObjectsCloneToOriginal().remove(object);
+                    removeObjectFromPrimaryKeyToNewObjects(object, primaryKey);
                     if (original != null) {
                         getNewObjectsOriginalToClone().remove(original);
                     }
@@ -5942,6 +6026,7 @@ public void clear(boolean shouldClearCache) {
         this.cloneMapping = null;
         this.newObjectsCloneToOriginal = null;
         this.newObjectsOriginalToClone = null;
+        this.primaryKeyToNewObjects = null;
         this.deletedObjects = null;
         this.allClones = null;
         this.objectsDeletedDuringCommit = null;

diff --git a/...core/src/main/java/org/eclipse/persistence/internal/sessions/remote/RemoteUnitOfWork.java b/...core/src/main/java/org/eclipse/persistence/internal/sessions/remote/RemoteUnitOfWork.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2021 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -659,6 +659,7 @@ protected void prepareForMergeIntoRemoteUnitOfWork() {
 
         this.newObjectsOriginalToClone = originalToClone;
         this.newObjectsCloneToOriginal = cloneToOriginal;
+        setupPrimaryKeyToNewObjects();
     }
 
     /**