commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michele Vivoda (JIRA)" <j...@apache.org>
Subject [jira] [Created] (JXPATH-181) ValueUtils modifies a static HashMap field without synchronization
Date Mon, 03 Aug 2015 15:15:05 GMT
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)

Mime
View raw message