From 76d82744b14f9a3ad11c1a5215542955b43bea1a Mon Sep 17 00:00:00 2001 From: Patrick Schmitt <13404745+Zuplyx@users.noreply.github.com> Date: Tue, 11 Apr 2023 08:53:27 +0200 Subject: [PATCH] fix #1596 Major performance issue in case of many new entities (#1843) * Issue 1596: use a hash-based collection to lookup objects in the cache instead of a linear search Signed-off-by: Patrick Schmitt * 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 --- ...rmResultsWithPrimaryKeyExpressionTest.java | 6 +- ...orkDeleteConformedNestedNewObjectTest.java | 7 +- ...NestedUnitOfWorkRegisterNewObjectTest.java | 7 +- ...orkDeleteConformedNestedNewObjectTest.java | 7 +- ...edUnitOfWorkDeleteNestedNewObjectTest.java | 6 +- .../NestedUnitOfWorkDeleteNewObjectTest.java | 6 +- .../unitofwork/UnregisterUnitOfWorkTest.java | 11 ++- .../internal/sessions/UnitOfWorkImpl.java | 97 +++++++++++++++++-- .../sessions/remote/RemoteUnitOfWork.java | 3 +- 9 files changed, 134 insertions(+), 16 deletions(-) diff --git a/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/queries/ConformResultsWithPrimaryKeyExpressionTest.java b/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/queries/ConformResultsWithPrimaryKeyExpressionTest.java index 010949c73bd..4687304f1b2 100644 --- a/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/queries/ConformResultsWithPrimaryKeyExpressionTest.java +++ b/foundation/eclipselink.core.test/src/it/java/org/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/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/DoubleNestedUnitOfWorkDeleteConformedNestedNewObjectTest.java b/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/DoubleNestedUnitOfWorkDeleteConformedNestedNewObjectTest.java index e927ee7e7d8..cdcd4dfae16 100644 --- a/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/DoubleNestedUnitOfWorkDeleteConformedNestedNewObjectTest.java +++ b/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/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/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/DoubleNestedUnitOfWorkRegisterNewObjectTest.java b/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/DoubleNestedUnitOfWorkRegisterNewObjectTest.java index e2ab035404b..ffd034331c3 100644 --- a/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/DoubleNestedUnitOfWorkRegisterNewObjectTest.java +++ b/foundation/eclipselink.core.test/src/it/java/org/eclipse/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/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/NestedUnitOfWorkDeleteConformedNestedNewObjectTest.java b/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/NestedUnitOfWorkDeleteConformedNestedNewObjectTest.java index 3e0da4259a8..3127df2eabf 100644 --- a/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/NestedUnitOfWorkDeleteConformedNestedNewObjectTest.java +++ b/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/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/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/NestedUnitOfWorkDeleteNestedNewObjectTest.java b/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/NestedUnitOfWorkDeleteNestedNewObjectTest.java index e65ea315fe5..85922a55751 100644 --- a/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/NestedUnitOfWorkDeleteNestedNewObjectTest.java +++ b/foundation/eclipselink.core.test/src/it/java/org/eclipse/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/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/NestedUnitOfWorkDeleteNewObjectTest.java b/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/NestedUnitOfWorkDeleteNewObjectTest.java index b5ea2dc4710..12322e680f3 100644 --- a/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/NestedUnitOfWorkDeleteNewObjectTest.java +++ b/foundation/eclipselink.core.test/src/it/java/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/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/UnregisterUnitOfWorkTest.java b/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/UnregisterUnitOfWorkTest.java index c0ffc31e80e..bd75bea9231 100644 --- a/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork/UnregisterUnitOfWorkTest.java +++ b/foundation/eclipselink.core.test/src/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/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/UnitOfWorkImpl.java b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/UnitOfWorkImpl.java index 0bb03fbfbf4..8d8a218dbe9 100644 --- a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/UnitOfWorkImpl.java +++ b/foundation/org.eclipse.persistence.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 cloneMapping; protected Map newObjectsCloneToOriginal; protected Map newObjectsOriginalToClone; + + /** + * Map of primary key to list of new objects. Used to speedup in-memory querying. + */ + protected Map> 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> 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> pkToNewObjects = getPrimaryKeyToNewObjects(); + if (pkToNewObjects.containsKey(primaryKey)) { + List 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/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/remote/RemoteUnitOfWork.java b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/remote/RemoteUnitOfWork.java index 3279d8b94e4..60006dbff3b 100644 --- a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/sessions/remote/RemoteUnitOfWork.java +++ b/foundation/org.eclipse.persistence.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(); } /**