More performance improvements for OpenJPA (lib and kernel)

• Key: OPENJPA-138
• Type: Sub-task
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Kevin Sutter
• Reporter: Kevin Sutter
• Component/s: kernel, lib
• Fix Version/s: 0.9.7

Description

As we continue pushing OpenJPA though its paces, we're finding a few additional tweaks that help with the performance throughput. This Issue will be used to track this next set of improvements. I'll document more detail as we determine the correct fixes. Several of the changes relate to caching of objects in a hashmap instead of re-creating the objects every time we call some of these methods.

Comments

[Kevin Sutter]

To give everyone a heads up on the type of changes I plan to introduce via this Issue... Please comment if you see a problem with any of these approaches. Our current OpenJPA test bucket still passes, but that doesn't always mean that these are safe...

o Cache the TransactionManager in JNDIManagedRuntime
o Cache the Class instances for Configurations.newInstance()
o Cache the Type hashcodes for OpenJPAId.hashcode()
o Cache the Type's ClassLoader instances in ObjectValue.newInstance()
o Cache the assignable "from" types and associated value "to" types in FetchConfigurationImpl.isAssignable
o Clean up the close/IllegalStateException processing in AbstractBrokerFactory and BrokerImpl (gate with TRACE)
o Cache the assignable "from" types and associated value "to" types in BrokerImpl.newObjectId()
o Cache the brokers being created along with appropriate cleanup and reopen processing in AbstractBrokerFactory (and BrokerImpl)

The details can be found with the SVN commit. Hopefully, this gives enough of a flavor to possibly raise any red flags...

Thanks!
Kevin

[Patrick Linskey]

> Cache the Class instances for Configurations.newInstance()
> Cache the Type's ClassLoader instances in ObjectValue.newInstance()
> Cache the assignable "from" types and associated value "to" types in FetchConfigurationImpl.isAssignable
> Cache the assignable "from" types and associated value "to" types in BrokerImpl.newObjectId()

These are all a bit concerning.... whenever Class objects get involved, there's always the possibility of different thread context classloaders or entities loaded in different classloaders. We should probably be careful here.

> Cache the brokers being created along with appropriate cleanup and reopen processing in AbstractBrokerFactory (and BrokerImpl)

It sounds like what this really means is that you're planning to create a pool of Brokers. Where will this be beneficial? I'm concerned about the cost of and uncertainty introduced by a pooling strategy, and the corresponding pool maintenance logic.

[Kevin Sutter]

Thanks for the insights, Patrick. Here are some new observations and assessments...

> Cache the Class instances for Configurations.newInstance()

On further review, I don't think my original idea is going to work as expected. I was thinking about caching the Class object based on the input clsName parameter. But, given your warning about multiple classloaders, this won't work. I suppose I could cache the Class instance based on the both the clsName and the derivedClassLoader, but due to the processing in the derivedClassLoader method, we might not save much.

> Cache the Type's ClassLoader instances in ObjectValue.newInstance()

This one I still comfortable with. I was looking to cache the ClassLoader from the Class parameter (type) passed in with the "type" instance as the key. This is should be safe, no? Unless you're saying that the Class instance (type) is not a suitable key.

> Cache the assignable "from" types and associated value "to" types in FetchConfigurationImpl.isAssignable
> Cache the assignable "from" types and associated value "to" types in BrokerImpl.newObjectId()

Here again, if I am using the "from" and "to" Class instances as keys to the hashmaps, shouldn't this caching be safe? I'm looking to keep a hashmap of "from" Classes that contains another hashmap of the valid "to" Classes that have been verified. Here again, unless we can't count on these Class instances to be used as keys, this optimization should be okay.

> Cache the brokers being created along with appropriate cleanup and reopen processing in AbstractBrokerFactory (and BrokerImpl)

Correct, I was looking to pool the Brokers. But, the more I look into this, I'm not as confident on my proposed solution. Let me think about this one a bit more...

I'm assuming that you are okay with the other ideas (since you didn't highlight any concerns). Is that a valid assumption?
Specifically, the following optimizations:

o Cache the TransactionManager in JNDIManagedRuntime
o Cache the Type hashcodes for OpenJPAId.hashcode()
o Clean up the close/IllegalStateException processing in AbstractBrokerFactory and BrokerImpl (gate with TRACE)

Thanks for your help in working through these performance improvements!
Kevin

[Patrick Linskey]

> This is should be safe, no? Unless you're saying that the Class instance (type) is not a suitable key.

To be honest, I didn't look at the source for any of the code that you mentioned. My concerns about classes and classloaders were based solely on seeing "cache" and "class" and "classloader" all occurring together. I expect that some of the hotspots that you have identified can be safely cached.

> > Cache the brokers being created along with appropriate cleanup and reopen processing in
> > AbstractBrokerFactory (and BrokerImpl)
>
> Correct, I was looking to pool the Brokers. But, the more I look into this, I'm not as confident on
> my proposed solution. Let me think about this one a bit more...

Generally-speaking, pooling concerns me. It's tough to get it right without introducing scalability bottlenecks. I think that we should be sure to demonstrate that there's a big performance win involved before implementing something like this.

Also, theoretically, creating a new Broker should be pretty quick. There are a number of bookkeeping data structures in a Broker, but not *that* many.

> Cache the TransactionManager in JNDIManagedRuntime

Seems safe to me, although I'm neither a JNDI expert nor a JTA expert, so can't comment on any issues that might come up with security principals or other contextual data if we do cache things.

I'm curious, though -- I thought that in Websphere, there was a direct API that you could invoke to get the TM. To the extent that we can avoid using JNDI altogether for commonly-used appservers, I imagine that JNDI optimization isn't going to bring that much bang for the buck.

> Cache the Type hashcodes for OpenJPAId.hashcode()

Seems legit. An ID refers to an instance; the type of an instance doesn't change over time.

> Clean up the close/IllegalStateException processing in AbstractBrokerFactory and
> BrokerImpl (gate with TRACE)

Yeah, this should definitely help things out.

Of course, with all the above issues, numbers don't lie. So, if there are significant performance improvements to be had, we should look into doing the work to make them available, whether that means additional code complexity or additional configuration options etc.

[Abe White]

o Cache the TransactionManager in JNDIManagedRuntime
+1

o Cache the Class instances for Configurations.newInstance()
-1

o Cache the Type hashcodes for OpenJPAId.hashcode()
Already being done as far as I can tell, unless you mean a static Class->hashcode map, in which case -1.

o Cache the Type's ClassLoader instances in ObjectValue.newInstance()
I have a hard time believing that Class.getClassLoader() for the few things that we dynamically create more than once via ObjectValues is a significant performance problem.

o Cache the assignable "from" types and associated value "to" types in FetchConfigurationImpl.isAssignable
Again, I have a hard time believing this is significant. What JVM is this that methods on Class seem to be so ridiculously slow, to the point that you want to create and maintain Maps of Maps to work around it?
 
o Clean up the close/IllegalStateException processing in AbstractBrokerFactory and BrokerImpl (gate with TRACE)
+1
 
o Cache the assignable "from" types and associated value "to" types in BrokerImpl.newObjectId()
Same comments as for FetchConfigurationImpl.isAssignable.

o Cache the brokers being created along with appropriate cleanup and reopen processing in AbstractBrokerFactory (and BrokerImpl)
-1

[Kevin Sutter]

I was waiting for Abe to weigh in... :-)

> o Cache the Type hashcodes for OpenJPAId.hashcode()
> Already being done as far as I can tell, unless you mean a static Class->hashcode map, in which case -1.

Yes, I am using a static ConcurrentReferenceHashMap to store the hashcodes. Why the -1 with this change?

> o Cache the Type's ClassLoader instances in ObjectValue.newInstance()
> I have a hard time believing that Class.getClassLoader() for the few things that we dynamically create more than once > via ObjectValues is a significant performance problem.

The caching definitely seems to have helped. More information at end of this append.

> o Cache the assignable "from" types and associated value "to" types in FetchConfigurationImpl.isAssignable
> Again, I have a hard time believing this is significant. What JVM is this that methods on Class seem to be so
> ridiculously slow, to the point that you want to create and maintain Maps of Maps to work around it?

:-) Not sure if it's due to the IBM JDK or not, but the Class.isAssignableFrom method is very CPU intensive.

Patrick earlier indicted that "numbers don't lie". We've been running an internal benchmark using OpenJPA (and other JPA vendors). Our performance numbers more than doubled just with the changes listed in this Issue (minus the Broker pooling and the Configurations.newInstance caching). Can't give any more details at this time. You'll have to trust me. :-)

Thanks,
Kevin

[Kevin Sutter]

Thanks for the input from everybody on this topic. I've decided on the following items to include with this Issue. Please review the SVN commit for any possible hiccups. I've run the OpenJPA test buckets and, of course, we have been running our benchmarks with continued success. But, that doesn't mean that I didn't miss something, especially with the Kodo JDO dependencies. Thanks for your patience.

o Cache the TransactionManager in JNDIManagedRuntime
o Cache the Type hashcodes for OpenJPAId.hashcode()
o Cache the Type's ClassLoader instances in ObjectValue.newInstance()
o Cache the assignable "from" types and associated value "to" types in FetchConfigurationImpl.isAssignable
o Clean up the close/IllegalStateException processing in AbstractBrokerFactory and BrokerImpl (gate with TRACE)
o Cache the assignable "from" types and associated value "to" types in BrokerImpl.newObjectId()

Kevin

[Kevin Sutter]

Code changes per Issue comments are complete.