Uploaded image for project: 'Commons JXPath'
  1. Commons JXPath
  2. JXPATH-181

ValueUtils modifies a static HashMap field without synchronization



    • Bug
    • Status: Open
    • Major
    • Resolution: Unresolved
    • None
    • None


      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




            Unassigned Unassigned
            vivodamichele@hotmail.com Michele Vivoda
            0 Vote for this issue
            1 Start watching this issue