Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[master] fix #1596 Major performance issue in case of many new entities #1843

Merged
merged 3 commits into from
Apr 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -158,7 +158,7 @@ public void prepareTest() {
case CASE_NEW: {
selectionObject = newEmployee;
if (shouldCheckCacheByExactPrimaryKey()) {
expectedGetIdCallCount = 1;
expectedGetIdCallCount = 0;
} else {
expectedGetIdCallCount = n + 1;
}
Expand Down Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -16,6 +16,7 @@

import java.math.BigDecimal;

import java.util.Collection;
import java.util.Vector;

import org.eclipse.persistence.internal.sessions.UnitOfWorkImpl;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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;

Expand Down Expand Up @@ -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();
Expand All @@ -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();

/*******************/
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -580,6 +587,7 @@ public void assignSequenceNumbers() throws DatabaseException {
}
if (hasNewObjects()) {
assignSequenceNumbers(getNewObjectsCloneToOriginal());
setupPrimaryKeyToNewObjects();
}
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -3861,6 +3878,7 @@ protected void preMergeChanges() {
Object referenceObjectToRemove = getNewObjectsCloneToOriginal().get(removedObject);
if (referenceObjectToRemove != null) {
getNewObjectsCloneToOriginal().remove(removedObject);
removeObjectFromPrimaryKeyToNewObjects(removedObject);
getNewObjectsOriginalToClone().remove(referenceObjectToRemove);
}
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
}
}

/**
Expand Down Expand Up @@ -5459,6 +5541,7 @@ public void resumeUnitOfWork() {
}
this.newObjectsCloneToOriginal = null;
this.newObjectsOriginalToClone = null;
this.primaryKeyToNewObjects = null;
}
this.unregisteredExistingObjects = null;
this.unregisteredNewObjects = null;
Expand Down Expand Up @@ -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);
rfelcman marked this conversation as resolved.
Show resolved Hide resolved
if (original != null) {
getNewObjectsOriginalToClone().remove(original);
}
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -659,6 +659,7 @@ protected void prepareForMergeIntoRemoteUnitOfWork() {

this.newObjectsOriginalToClone = originalToClone;
this.newObjectsCloneToOriginal = cloneToOriginal;
setupPrimaryKeyToNewObjects();
}

/**
Expand Down