Wicket
  1. Wicket
  2. WICKET-2741

non-performant Collections.synchronizedMap() should be replaced with ConcurrentMap

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.4.6
    • Fix Version/s: 1.4.9, 1.5-M1
    • Component/s: wicket
    • Labels:

      Description

      The following two classes use Collections.synchronizedMap() --> lots of contention @ concurrent load.

      • RequestListenerInterface.interfaces
      • WebApplication.addBufferedResponse
      • Injector.inject() - 'cache' field.

      The last instance makes @SpringBean unusable in production (too much contention @ concurrent load)

      Here's a sample output of contention around monitor of class java.util.Collections$SynchronizedMap:

      +--------------------------------------------------------------------------------------+------------------+-----------------+
      |                                         Name                                         |    Time (ms)     |      Count      |
      +--------------------------------------------------------------------------------------+------------------+-----------------+
      |  +---java.util.Collections$SynchronizedMap.get(Object)                               |  126,101   51 %  |  19,145   52 %  |
      |  | |                                                                                 |                  |                 |
      |  | +---org.apache.wicket.injection.Injector.inject(Object, IFieldValueFactory)       |  109,912   45 %  |  16,285   44 %  |
      |  | |                                                                                 |                  |                 |
      |  | +---org.springframework.beans.CachedIntrospectionResults.forClass(Class)          |   16,188    7 %  |   2,860    8 %  |
      |  |                                                                                   |                  |                 |
      |  +---java.util.Collections$SynchronizedMap.put(Object, Object)                       |  119,343   49 %  |  17,676   48 %  |
      |    |                                                                                 |                  |                 |
      |    +---org.apache.wicket.injection.Injector.inject(Object, IFieldValueFactory)       |                  |                 |
      |      |                                                                               |                  |                 |
      |      +---org.apache.wicket.injection.ConfigurableInjector.inject(Object)             |                  |                 |
      |        |                                                                             |                  |                 |
      |        +---org.apache.wicket.injection.ComponentInjector.onInstantiation(Component)  |  118,060   48 %  |  17,467   47 %  |
      |        |                                                                             |                  |                 |
      |        +---com.castanealabs.gui.model.ZipCodeValidator.<init>()                      |      686    0 %  |     106    0 %  |
      |        |                                                                             |                  |                 |
      |        +---com.castanealabs.gui.model.ProductSkuProgramDataProvider.<init>(IModel)   |      337    0 %  |      59    0 %  |
      |        |                                                                             |                  |                 |
      |        +---com.castanealabs.gui.model.ProductTypeProgramDataProvider.<init>(IModel)  |      259    0 %  |      44    0 %  |
      +--------------------------------------------------------------------------------------+------------------+-----------------+
      
      Generated by YourKit Java Profiler 8.0.22 Feb 12, 2010 3:52:25 PM
      
      1. wicket-2741.patch
        4 kB
        Stefan Fussenegger

        Issue Links

          Activity

          Hide
          Hudson added a comment -
          Show
          Hudson added a comment - Integrated in Apache Wicket 1.5.x #54 (See http://hudson.zones.apache.org/hudson/job/Apache%20Wicket%201.5.x/54/ )
          Hide
          Stefan Fussenegger added a comment -

          this patch uses a copy-on-write approach to avoid synchronization for reads. Only in the rare event of a yet unmapped ClassLoader it's going to be a bit slower. While the patch seems to work very well a second or third pair of eyes would be very well appreciated.

          This patch also contains changes by patch for WICKET-2875 and reuses an empty array (new Field[0]) for classes without any injectable fields.

          Show
          Stefan Fussenegger added a comment - this patch uses a copy-on-write approach to avoid synchronization for reads. Only in the rare event of a yet unmapped ClassLoader it's going to be a bit slower. While the patch seems to work very well a second or third pair of eyes would be very well appreciated. This patch also contains changes by patch for WICKET-2875 and reuses an empty array (new Field [0] ) for classes without any injectable fields.
          Hide
          Johan Compagner added a comment -

          cant we just copy the specific ConcurrentMap impl we need from google code and add it to our own package?

          Else we could just use a ConcurrentHashMap and then use WeakReference keys ourself (but then we need to make sure that we clean it now and then)

          Show
          Johan Compagner added a comment - cant we just copy the specific ConcurrentMap impl we need from google code and add it to our own package? Else we could just use a ConcurrentHashMap and then use WeakReference keys ourself (but then we need to make sure that we clean it now and then)
          Hide
          Juergen Donnerstag added a comment -

          It's not that we are completely against it, but we careful in adding dependencies. A rule which we treat more strict for wicket-core than subproject such as wicket-spring. But if there is a good reason to add a lib, we'll do.

          Since we are using maven, it's a clear benefit if that lib is available via a maven repository (such as http://repo2.maven.org/maven2/com/google/collections/google-collections/)

          Show
          Juergen Donnerstag added a comment - It's not that we are completely against it, but we careful in adding dependencies. A rule which we treat more strict for wicket-core than subproject such as wicket-spring. But if there is a good reason to add a lib, we'll do. Since we are using maven, it's a clear benefit if that lib is available via a maven repository (such as http://repo2.maven.org/maven2/com/google/collections/google-collections/ )
          Hide
          Nikita Tovstoles added a comment -

          OK. I've found another perf issue with @SpringBean. Switching back to a locator pattern (injecting beans into WebApplication bean only) improved throughput 5x @ 20 concurrent users. I am happy to help, but would need to understand your reluctance to use a 3rd party library, given the current perf impact. For the above, Google Collections' MapMaker looks like a solid fit.

          Show
          Nikita Tovstoles added a comment - OK. I've found another perf issue with @SpringBean. Switching back to a locator pattern (injecting beans into WebApplication bean only) improved throughput 5x @ 20 concurrent users. I am happy to help, but would need to understand your reluctance to use a 3rd party library, given the current perf impact. For the above, Google Collections' MapMaker looks like a solid fit.
          Hide
          Juergen Donnerstag added a comment -

          Obviously with Inspector we can not simple replace Collections.synchronizedMap(new WeakHashMap()) with ConcurrentWeakHashMap since such a class does not exist. And we don't want to add a dependency on a 3rd party library just for that purpose. Since you seem to have all the tests and tools properly set up to validate, may be you could also provide a patch. I'm more than happy to apply it later on to the repository.

          Thanks for your help

          Juergen

          Show
          Juergen Donnerstag added a comment - Obviously with Inspector we can not simple replace Collections.synchronizedMap(new WeakHashMap()) with ConcurrentWeakHashMap since such a class does not exist. And we don't want to add a dependency on a 3rd party library just for that purpose. Since you seem to have all the tests and tools properly set up to validate, may be you could also provide a patch. I'm more than happy to apply it later on to the repository. Thanks for your help Juergen

            People

            • Assignee:
              Igor Vaynberg
              Reporter:
              Nikita Tovstoles
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development