Skip to content

[master] OPENJPA-2767: Incomplete ValueMapDiscriminatorStrategy cache and Meta… #45

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.

Sign up for GitHub

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

Merged
dazey3 merged 1 commit into from
May 16, 2019
Merged

[master] OPENJPA-2767: Incomplete ValueMapDiscriminatorStrategy cache and Meta… #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 63 additions & 21 deletions ...jdbc/src/main/java/org/apache/openjpa/jdbc/meta/strats/ValueMapDiscriminatorStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import java.util.List;
import java.util.Map;
import java.util.TreeSet;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantReadWriteLock;

import org.apache.openjpa.jdbc.kernel.JDBCStore;
import org.apache.openjpa.jdbc.meta.ClassMapping;
Expand All @@ -43,15 +45,17 @@
public class ValueMapDiscriminatorStrategy
extends InValueDiscriminatorStrategy {


private static final long serialVersionUID = 1L;

public static final String ALIAS = "value-map";

private static final Localizer _loc = Localizer.forPackage
(ValueMapDiscriminatorStrategy.class);

private Map _vals = null;
private Map<String, Class<?>> _vals;
private final ReentrantReadWriteLock rwl = new ReentrantReadWriteLock(true);
private final Lock _readLock = rwl.readLock();
private final Lock _writeLock = rwl.writeLock();

@Override
public String getAlias() {
Expand All @@ -67,8 +71,8 @@ protected int getJavaType() {
// if the user wants the type to be null, we need a jdbc-type
// on the column or an existing column to figure out the java type
DiscriminatorMappingInfo info = disc.getMappingInfo();
List cols = info.getColumns();
Column col = (cols.isEmpty()) ? null : (Column) cols.get(0);
List<Column> cols = info.getColumns();
Column col = (cols.isEmpty()) ? null : cols.get(0);
if (col != null) {
if (col.getJavaType() != JavaTypes.OBJECT)
return col.getJavaType();
Expand All @@ -88,37 +92,75 @@ protected Object getDiscriminatorValue(ClassMapping cls) {
@Override
protected Class getClass(Object val, JDBCStore store)
throws ClassNotFoundException {
if (_vals == null) {
ClassMapping cls = disc.getClassMapping();
ClassMapping[] subs = cls.getJoinablePCSubclassMappings();
Map map = new HashMap((int) ((subs.length + 1) * 1.33 + 1));
mapDiscriminatorValue(cls, map);
for (int i = 0; i < subs.length; i++)
mapDiscriminatorValue(subs[i], map);
_vals = map;

if(_vals == null) {
_writeLock.lock();
try {
if(_vals == null) {
_vals = constructCache(disc);
}
} finally {
_writeLock.unlock();
}
}

String className = (val == null) ? null : val.toString();
_readLock.lock();
try {
Class<?> clz = _vals.get(className);
if (clz != null)
return clz;
} finally {
_readLock.unlock();
}

_writeLock.lock();
try {
Class<?> clz = _vals.get(className);
if (clz != null)
return clz;

//Rebuild the cache to check for updates
_vals = constructCache(disc);

//Try get again
clz = _vals.get(className);
if (clz != null)
return clz;
throw new ClassNotFoundException(_loc.get("unknown-discrim-value",
new Object[]{ className, disc.getClassMapping().getDescribedType().
getName(), new TreeSet<String>(_vals.keySet()) }).getMessage());
} finally {
_writeLock.unlock();
}
}

String str = (val == null) ? null : val.toString();
Class cls = (Class) _vals.get(str);
if (cls != null)
return cls;
throw new ClassNotFoundException(_loc.get("unknown-discrim-value",
new Object[]{ str, disc.getClassMapping().getDescribedType().
getName(), new TreeSet(_vals.keySet()) }).getMessage());
/**
* Build a class cache map from the discriminator
*/
private static Map<String, Class<?>> constructCache(Discriminator disc) {
//Build the cache map
ClassMapping cls = disc.getClassMapping();
ClassMapping[] subs = cls.getJoinablePCSubclassMappings();
Map<String, Class<?>> map = new HashMap<String, Class<?>>((int) ((subs.length + 1) * 1.33 + 1));
mapDiscriminatorValue(cls, map);
for (int i = 0; i < subs.length; i++)
mapDiscriminatorValue(subs[i], map);
return map;
}

/**
* Map the stringified version of the discriminator value of the given type.
*/
private static void mapDiscriminatorValue(ClassMapping cls, Map map) {
private static void mapDiscriminatorValue(ClassMapping cls, Map<String, Class<?>> map) {
// possible that some types will never be persisted and therefore
// can have no discriminator value
Object val = cls.getDiscriminator().getValue();
if (val == null)
return;

String str = (val == Discriminator.NULL) ? null : val.toString();
Class exist = (Class) map.get(str);
Class<?> exist = map.get(str);
if (exist != null)
throw new MetaDataException(_loc.get("dup-discrim-value",
str, exist, cls));
Expand Down
34 changes: 18 additions & 16 deletions openjpa-kernel/src/main/java/org/apache/openjpa/meta/MetaDataRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,8 @@ public class MetaDataRepository implements PCRegistry.RegisterClassListener, Con
private Map<Class<?>, Class<?>> _metamodel = Collections.synchronizedMap(new HashMap<Class<?>, Class<?>>());

// map of classes to lists of their subclasses
private Map<Class<?>, List<Class<?>>> _subs = Collections.synchronizedMap(new HashMap<Class<?>, List<Class<?>>>());
private Map<Class<?>, Collection<Class<?>>> _subs =
Collections.synchronizedMap(new HashMap<Class<?>, Collection<Class<?>>>());

// xml mapping
protected final XMLMetaData[] EMPTY_XMLMETAS;
Expand Down Expand Up @@ -1625,19 +1626,20 @@ private void loadRegisteredClassMetaData(ClassLoader envLoader) {
}

/**
* Updates our datastructures with the latest registered classes.
* Updates our data structures with the latest registered classes.
*
* This method is synchronized to make sure that all data structures are fully updated
* before other threads attempt to call this method
*/
Class<?>[] processRegisteredClasses(ClassLoader envLoader) {
if (_registered.isEmpty())
synchronized Class<?>[] processRegisteredClasses(ClassLoader envLoader) {
Copy link
Member

@ilgrosso ilgrosso May 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance to avoid synchronization at method level?

Copy link
Contributor Author

@dazey3 dazey3 May 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilgrosso I had written a possible implementation using read/write locks similar to my ValueMapDiscriminator impl, but so much of the code ended up being write locked that it almost seemed pointless to have that fine grained locking. Almost all the time, _registered is empty anyway, so little to no blocking will occur.
The issue really ends up being that all of processRegisteredClasses needs to block until it's done processing all of the classes out of the temporary array. I realize the temp array was added to reduce blocking on the _registered array in the first place, but that only opens a window for other threads to start initializing other caches WHILE the MetaDataRepository is still processing and building. If the ORM is sufficiently large (I was seeing this processing taking up to 700ms for my customer), it can stretch into application code as queries begin executing. During that time, reads are potentially dirty from MetaDataRepository._subs and we were seeing CNFE even though MetaDataRepository was STILL in the process of processing the class it threw a CNFE for.
That being said, if there is a strong performance concern from synchonizing the method (something that may not be as troubling nowadays with newer jdks), I can re-investigate implementing locks. This change was just infinitely cleaner and I have an interest for my customer to get this fixes as soon as possible.

Copy link
Member

@ilgrosso ilgrosso May 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dazey3 I see: given what you say, method synchronization could be acceptable then - I was just wondering why...

Copy link
Contributor Author

@dazey3 dazey3 May 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilgrosso I understand the concern and I appreciate all the review comments I can get! Thank you for taking a look.

if (_registered.isEmpty()) {
return EMPTY_CLASSES;
}

// copy into new collection to avoid concurrent mod errors on reentrant
// registrations
Class<?>[] reg;
synchronized (_registered) {
reg = _registered.toArray(new Class[_registered.size()]);
_registered.clear();
}
Class<?>[] reg = _registered.toArray(new Class[_registered.size()]);
_registered.clear();

Collection<String> pcNames = getPersistentTypeNames(false, envLoader);
Collection<Class<?>> failed = null;
Expand All @@ -1652,8 +1654,8 @@ Class<?>[] processRegisteredClasses(ClassLoader envLoader) {
if (_filterRegisteredClasses) {
Log log = (_conf == null) ? null : _conf.getLog(OpenJPAConfiguration.LOG_RUNTIME);
ClassLoader loadCL = (envLoader != null) ?
envLoader :
AccessController.doPrivileged(J2DoPrivHelper.getContextClassLoaderAction());
envLoader :
AccessController.doPrivileged(J2DoPrivHelper.getContextClassLoaderAction());

try {
Class<?> classFromAppClassLoader = Class.forName(reg[i].getName(), true, loadCL);
Expand Down Expand Up @@ -1840,7 +1842,7 @@ private boolean isLeastDerivedImpl(Class<?> inter, Class<?> cls) {
/**
* Add the given value to the collection cached in the given map under the given key.
*/
private void addToCollection(Map map, Class<?> key, Class<?> value, boolean inheritance) {
private void addToCollection(Map<Class<?>, Collection<Class<?>>> map, Class<?> key, Class<?> value, boolean inheritance) {
if (_locking) {
synchronized (map) {
addToCollectionInternal(map, key, value, inheritance);
Expand All @@ -1850,8 +1852,8 @@ private void addToCollection(Map map, Class<?> key, Class<?> value, boolean inhe
}
}

private void addToCollectionInternal(Map map, Class<?> key, Class<?> value, boolean inheritance) {
Collection coll = (Collection) map.get(key);
private void addToCollectionInternal(Map<Class<?>, Collection<Class<?>>> map, Class<?> key, Class<?> value, boolean inheritance) {
Collection<Class<?>> coll = map.get(key);
if (coll == null) {
if (inheritance) {
InheritanceComparator comp = new InheritanceComparator();
Expand Down Expand Up @@ -1927,8 +1929,8 @@ public void startConfiguration() {
@Override
public void endConfiguration() {
initializeMetaDataFactory();
if (_implGen == null)
_implGen = new InterfaceImplGenerator(this);
if (_implGen == null)
_implGen = new InterfaceImplGenerator(this);
if (_preload == true) {
_oids = new HashMap<>();
_impls = new HashMap<>();
Expand Down