Issue Details (XML | Word | Printable)

Key: NUCAPIJDO-4
Type: Improvement Improvement
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Unassigned
Reporter: Fernando Padilla
Votes: 0
Watchers: 1
Operations

If you were logged in you would be able to see more operations.
DataNucleus API JDO

reduce global lock for JDOPersistenceManagerFactory get/releasePersistenceManager methods

Created: 28/Jan/11 11:36 PM   Updated: 08/Feb/11 03:56 AM   Resolved: 01/Feb/11 08:12 AM
Component/s: API
Affects Version/s: None
Fix Version/s: 3.0.0.m1


 Description  « Hide
While trying to track down a different bug, I was reviewing how locks are used in JDO stack.

I noticed that both getPersistenceManager and releasePersistenceManager methods are "synchronized". Reviewing that code, it looks like the only thing they are protecting are access to pmCache, and freezeConfiguration calls. But since we call getPM and releasePM for every single web request in our system, I think this would be a really nice/simple optimization to reduce contention and serialization of requests! :)

I'll work on getting you a patch, but if you review the code, I bet you will see what I'm talking about and can figure it out easily. :)


Sort Order: Ascending order - Click to sort in descending order
Andy Jefferson added a comment - 29/Jan/11 02:46 PM
SVN trunk (3.x) has changed synchronisation to just lock things that need locking; could be improved, and could be backported to 2.x, but that is left as an exercise to the reader

Andy Jefferson added a comment - 29/Jan/11 03:04 PM
And if coming across multithreading issues in core, make use of the existing JIRA
http://www.datanucleus.org/servlet/jira/browse/NUCCORE-606
that will be addressed sometime during 3.x

Fernando Padilla added a comment - 31/Jan/11 07:54 PM
great. I just reviewed the code in trunk, and you're right, you already did most of the changes I asked for. :)

But I would go further, within getPersistenceManager and releasePersistenceManager you're still locking against pCache, when you could change it to avoid a global lock, and use ConcurrentHasMap with lock striping; you can use Collections.newSetFromMap( new ConcurrentHashMap() ).

Also, within getPersistenceManager, I think you should wrap the freezeConfiguration call with a "if ( isConfigurable() )" check, to avoid attaining the lock, unless you really have to.

what do you think?

Andy Jefferson added a comment - 01/Feb/11 08:12 AM
freezeConfig is now only synced within isConfigurable() when we are definitely freezing it. The other suggestion is not applied since your way is JDK1.6 specific and DataNucleus is not, and 3.0 M1 happens today

Chris Colman added a comment - 03/Feb/11 03:16 AM
Luckily DataNucleus is still compatible with JDK 1.5! We have built a large web app for a rail corporation running WebSphere application server and they specify JDK 1.5 in their deployment specs.

Fernando Padilla added a comment - 03/Feb/11 05:48 AM
@chris So. Do you mean that Collections.newSetFromMap works in JDK 1.5?

Chris Colman added a comment - 08/Feb/11 03:56 AM
@Fernando: We haven't had any problems yet. Should we? We're still in development and using IntelliJ with 1.6 JDK switched into 1.5 source/1.5 target mode. We haven't had any issues with the use of any 1.6 specific features. Testing on a separate *pure* 1.5 JVM will occur within a few weeks and we're expecting no problems.