Details
-
Bug
-
Status: Open
-
Major
-
Resolution: Unresolved
-
None
-
None
Description
ValueUtils uses a HashMap to cache instances of DynamicPropertyHandler that otherwise creates from a java.lang.Class object.
Since the getDynamicPropertyHandler method is static and modifies the HashMap, thread safety is broken.
The easiest option would be to use Collections.SynchronizedMap(), introducing more locking.
An other option is to remove cache: Class.newInstance() is not so heavy since all internal implementations of DynamicPropertyHandler perform almost no initialization in constructor. A side effect of removing cache could be external implementations of DynamicPropertyHandler not expecting to be created multiple times, but not sure if this is the case.
I did some tests all creating an instance from a almost-no-initialization class object using Class.newInstance(): 2 with cache (HashMap and Collections.synchronizedMap()) and one without cache.
First one thread, 50000x1000 loops
junit.perf.util.AccessorHashMap :259 (12.84%) 21.99% of max
junit.perf.util.AccessorSynchmap :1178 (58.4%) 100.0% of max
junit.perf.util.AccessorNoCache :580 (28.75%) 49.24% of max
then 1000 threads 50000 loops
junit.perf.util.AccessorHashMap :128 (3.82%) 4.51% of max
junit.perf.util.AccessorSynchmap :2840 (84.95%) 100.0% of max
junit.perf.util.AccessorNoCache :375 (11.21%) 13.2% of max
From results I see no cache is not so bad also compared to HashMap, perhaps more important is that with 1000 threads Synchmap uses much more time than with one thread (240%), while the other 2 implementations use less; test machine has more than one processor.
So I propose to remove caching from ValueUtils
If one day DynamicPropertyHandler implementations will be registered in some static way like using property files (and not with static methods like now) - then may be will be possible to cache only those registered in properties in an unmodifiable not synchronized map and just create the others