[jira] [Created] (JXPATH-181) ValueUtils modifies a static HashMap field without synchronization

["Michele Vivoda (JIRA)" <ji...@apache.org>]

Michele Vivoda created JXPATH-181:
-------------------------------------

             Summary: ValueUtils modifies a static HashMap field without synchronization
                 Key: JXPATH-181
                 URL: https://issues.apache.org/jira/browse/JXPATH-181
             Project: Commons JXPath
          Issue Type: Bug
            Reporter: Michele Vivoda


ValueUtils uses a HashMap to cache instances of DynamicPropertyHandler that otherwise creates from a java.lang.Class object.

https://github.com/apache/commons-jxpath/blob/JXPATH_1_3_RC4/src/java/org/apache/commons/jxpath/util/ValueUtils.java#L44

https://github.com/apache/commons-jxpath/blob/JXPATH_1_3_RC4/src/java/org/apache/commons/jxpath/util/ValueUtils.java#L549

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 




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)