Unnecessary lock contention issues in ObjectInstantiator caching

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Profile an application that calls ObjenesisBase#getInstantiatorOf(Class 
clazz) several times across threads and observe high degree of lock contention 
on that call.

What is the expected output? What do you see instead?
Expected behavior is, once cached, the act of getting the instantiator should 
be lock-less.

What version of the product are you using? On what operating system?
1.2 (the patch is for master)

Please provide any additional information below.
The patch attached fixes this problem on trunk(it also cleanly applies on the 
1.2 tag). Its a git format-patch generated patch file, if working directly with 
svn, 'patch -p1 -i <file-patch>' can be used to apply it.

Original issue reported on code.google.com by singh.janmejay on 4 Nov 2010 at 2:18

Attachments:

• 0001-fixed-getInstantiatorOf-coarse-grained-lock-to-avoid.patch

[GoogleCodeExporter]

Thanks for this patch.

Your implementation is the clean way to do a double-checked locking. It is 
guaranteed to work on Java 5 and 6. However, not on Java 1.3 and 1.4. Which 
Objenesis still support. However, it
1) Might work anyway
2) Be useless to keep supporting these old java implementations
3) Might be possible to support both world nicely

Out of curiosity, why are you synchronizing on clazz.getName().intern()? It's a 
nice way to prevent using a transient?

Original comment by henri.tr...@gmail.com on 7 Nov 2010 at 3:08

• Changed state: Accepted
• Added labels: Milestone-1.3, Type-Defect

[GoogleCodeExporter]

I wanted something unique out of the class to take a lock(which doesn't affect 
callers getting instantiator for a different class, hence 
class.getName().intern(). I could have used class, but didn't want to worry 
about multi-classloader environments(re-loading of class while locked etc). 
Using name avoids all that trouble.

Does it make sense to branch out for java 1.4/1.3 and not apply this patch 
there? I don't know, its a little bit of effort, but since the patch is pretty 
localized, it may make sense. Just a suggestion.

Original comment by singh.janmejay on 9 Nov 2010 at 4:42

[GoogleCodeExporter]

class.getName().intern() is a recipe for disaster of memory leaks 

Original comment by dquint...@gmail.com on 5 Jan 2011 at 12:36

[GoogleCodeExporter]

well not exactly memory leaks, but memory exaustion

Original comment by dquint...@gmail.com on 5 Jan 2011 at 12:38

[GoogleCodeExporter]

It seems post java 1.2 it does get garbage-collected. Its a native call, and i 
haven't checked how it works myself, but almost all pages i came across 
indicate that interned strings are actually garbage collected(for instance 
http://mindprod.com/jgloss/interned.html page talks about it).

Original comment by singh.janmejay on 6 Jan 2011 at 2:51

[GoogleCodeExporter]

Original comment by henri.tr...@gmail.com on 23 Jan 2013 at 10:32

• Added labels: Milestone-2.0
• Removed labels: Milestone-1.3

[GoogleCodeExporter]

Here's a workaround I should have told way before.

You can do your own caching of the instances. For that, you should desactivate 
the one from ObjenesisBase and just do your own. 

You will still get a synchronized for any new class though. The next version of 
Objenesis will not synchronize if the cache is disabled. Right now, the best 
solution is to calling and instantiator used on your JVM directly instead of 
using the strategy.

Original comment by henri.tr...@gmail.com on 22 Jul 2013 at 11:20

[GoogleCodeExporter]

It is now possible to implement its own cache and so remove the synchronization 
in Objenesis.

Version 2.0 will be only Java 5 compatible and will be able to really fix this.

Original comment by henri.tr...@gmail.com on 14 Aug 2013 at 11:45

• Added labels: Milestone-1.4
• Removed labels: Milestone-2.0

[GoogleCodeExporter]

Original comment by henri.tr...@gmail.com on 14 Aug 2013 at 11:45

[GoogleCodeExporter]

Original comment by henri.tr...@gmail.com on 14 Aug 2013 at 11:51

• Changed state: Fixed