1 2 Previous Next 21 Replies Latest reply on Feb 22, 2010 10:43 AM by kabirkhan Go to original post
      • 15. Re: Profiling the dependency project
        kabirkhan
        I removed the resolveContexts() states loop break, and adjusted the affected tests in http://fisheye.jboss.org/changelog/JBossAS/?cs=100882
        • 16. Re: Profiling the dependency project
          kabirkhan

          Since installCallbacks and uninstallCallbacks are ConcurrentHashMaps, I think the read lock is unnecessary here in AbstractController:

           

          ===================================================================

          --- src/main/java/org/jboss/dependency/plugins/AbstractController.java (revision 100970)

          +++ src/main/java/org/jboss/dependency/plugins/AbstractController.java (working copy)

          @@ -1833,17 +1833,9 @@

               */

              protected Set<CallbackItem<?>> getCallbacks(Object name, boolean isInstallPhase)

              {

          -      lockRead();

          -      try

          -      {

          -         Map<Object, Set<CallbackItem<?>>> map = (isInstallPhase ? installCallbacks : uninstallCallbacks);

          -         Set<CallbackItem<?>> callbacks = map.get(name);

          -         return callbacks != null ? callbacks : Collections.<CallbackItem<?>>emptySet();

          -      }

          -      finally

          -      {

          -         unlockRead();

          -      }

          +      Map<Object, Set<CallbackItem<?>>> map = (isInstallPhase ? installCallbacks : uninstallCallbacks);

          +      Set<CallbackItem<?>> callbacks = map.get(name);

          +      return callbacks != null ? callbacks : Collections.<CallbackItem<?>>emptySet();

              }

          • 17. Re: Profiling the dependency project
            alesj

            Since installCallbacks and uninstallCallbacks are ConcurrentHashMaps, I think the read lock is unnecessary here in AbstractController:

            Since I think callbacks are already used in some explicitly locked code (via Lock),

            what about if we leave the locks and just change the callbacks to plain HashMap?

             

            Like discussed on the call, what is faster in a really-rare-concurrent env?

            * lock + plain hash collection

            * concurrent hash collection

             

            Jason, DML?

            • 18. Re: Profiling the dependency project
              dmlloyd

              alesj wrote:

               


              Since installCallbacks and uninstallCallbacks are ConcurrentHashMaps, I think the read lock is unnecessary here in AbstractController:

              Since I think callbacks are already used in some explicitly locked code (via Lock),

              what about if we leave the locks and just change the callbacks to plain HashMap?

               

              Like discussed on the call, what is faster in a really-rare-concurrent env?

              * lock + plain hash collection

              * concurrent hash collection

               

              Jason, DML?

               

              Well, if you're already locking, concurrent hash maps will generally give you general throughput in the contended case; if there's only one contender then there's only one lock acquisition so it's a wash, on paper.  In reality, a CHM is actually a bit more complex, and it's also a really big structure (even if it's empty), consisting of a ReentrantLock for each lock stripe, each of which includes a bunch of stuff, so you get another kind of savings by using a simple synchronized map.  In fact I'm hesitant to ever use CHM just because of its excessive size, especially if you're creating a lot of them (if you ever do a memory profile of AS, you can see that CHM and its components tend to bubble to the top of the list).  So for low contention situations I'd always recommend a plain synchronized map.

               

              The best solution would of course be to eliminate the lock - if you're already protected, you could use a plain hashmap.  Otherwise consider if copy-on-write is suitable for you - Jason's FastCopyHashMap is good for that kind of thing.

              • 19. Re: Profiling the dependency project
                kabirkhan

                alesj wrote:

                 


                Since installCallbacks and uninstallCallbacks are ConcurrentHashMaps, I think the read lock is unnecessary here in AbstractController:

                Since I think callbacks are already used in some explicitly locked code (via Lock),

                what about if we leave the locks and just change the callbacks to plain HashMap?

                 

                Like discussed on the call, what is faster in a really-rare-concurrent env?

                * lock + plain hash collection

                * concurrent hash collection

                 

                Jason, DML?

                I see we are adding them here (and similarly for removing them)

                 

                   protected <T> void addCallback(Object name, boolean isInstallPhase, CallbackItem<T> callback)
                   {
                      lockWrite();
                      try
                      {
                         Map<Object, Set<CallbackItem<?>>> map = (isInstallPhase ? installCallbacks : uninstallCallbacks);
                         Set<CallbackItem<?>> callbacks = map.get(name);
                         if (callbacks == null)
                         {
                            callbacks = new HashSet<CallbackItem<?>>();
                            map.put(name, callbacks);
                         }
                         callbacks.add(callback);
                      }
                      finally
                      {
                         unlockWrite();
                      }
                   }
                 
                

                 

                So this needs some rethinking. Since callbacks is a plain HashSet and the user of getCallbacks()  is iterating over them this code is broken.

                 

                So we either make everything plain HashMap/Set, reintroduce the locks, and replace getContexts() with this method where we do the iteration (via addAll()) with the lock taken:

                 

                   /**
                    * Get the matching callbacks.
                    *
                    * @param result the set to put any callbacks into
                    * @param name demand name
                    * @param isInstallPhase install or uninstall phase
                    * @return The passed in result parameter. If it was null and callbacks were found a new set is created 
                    */
                   protected Set<CallbackItem<?>> addCallbacks(Set<CallbackItem<?>> result, Object name, boolean isInstallPhase)
                   {
                      lockRead();
                      try
                      {
                         Map<Object, Set<CallbackItem<?>>> map = (isInstallPhase ? installCallbacks : uninstallCallbacks);
                         Set<CallbackItem<?>> callbacks = map.get(name);
                         if (callbacks != null)
                         {
                            if (result == null)
                               result = new HashSet<CallbackItem<?>>();
                            result.addAll(callbacks);
                         }
                      }
                      finally
                      {
                         unlockRead();
                      }
                      return result;
                   }
                
                Or we make everything involved concurrent map set that would work too. I need to understand a bit better how this code is used.
                • 20. Re: Profiling the dependency project
                  kabirkhan
                  I've reverted what I did and will take a proper look on Monday
                  • 21. Re: Profiling the dependency project
                    kabirkhan

                    I tried a few different things, none of which really made a difference, so I have stuck with more or less what we had, but making sure we access the (un)installCallbacks in a thread safe way:

                     

                    ===================================================================
                    --- dependency/src/main/java/org/jboss/dependency/plugins/AbstractController.java     (revision 101181)
                    +++ dependency/src/main/java/org/jboss/dependency/plugins/AbstractController.java     (working copy)
                    @@ -23,6 +23,7 @@
                     
                     import java.util.Collection;
                     import java.util.Collections;
                    +import java.util.HashMap;
                     import java.util.HashSet;
                     import java.util.Iterator;
                     import java.util.LinkedHashSet;
                    @@ -107,8 +108,8 @@
                        private final Set<AbstractController> childControllers = new CopyOnWriteArraySet<AbstractController>();
                     
                        /** The callback items */
                    -   private final Map<Object, Set<CallbackItem<?>>> installCallbacks = new ConcurrentHashMap<Object, Set<CallbackItem<?>>>();
                    -   private final Map<Object, Set<CallbackItem<?>>> uninstallCallbacks = new ConcurrentHashMap<Object, Set<CallbackItem<?>>>();
                    +   private final Map<Object, Set<CallbackItem<?>>> installCallbacks = new HashMap<Object, Set<CallbackItem<?>>>();
                    +   private final Map<Object, Set<CallbackItem<?>>> uninstallCallbacks = new HashMap<Object, Set<CallbackItem<?>>>();
                     
                        /** Whether an on demand context has been enabled */
                        private boolean onDemandEnabled = true;
                    @@ -1831,14 +1832,20 @@
                         * @param isInstallPhase install or uninstall phase
                         * @return all matching registered callbacks or empty set if no such item
                         */
                    -   protected Set<CallbackItem<?>> getCallbacks(Object name, boolean isInstallPhase)
                    +   protected Set<CallbackItem<?>> getCallbacks(Set<CallbackItem<?>> result, Object name, boolean isInstallPhase)
                        {
                           lockRead();
                           try
                           {
                              Map<Object, Set<CallbackItem<?>>> map = (isInstallPhase ? installCallbacks : uninstallCallbacks);
                              Set<CallbackItem<?>> callbacks = map.get(name);
                    -         return callbacks != null ? callbacks : Collections.<CallbackItem<?>>emptySet();
                    +         if (callbacks != null)
                    +         {
                    +            if (result == null)
                    +               result = new HashSet<CallbackItem<?>>();
                    +            result.addAll(callbacks);
                    +         }
                    +         return result;
                           }
                           finally
                           {
                    @@ -1910,20 +1917,16 @@
                              if (dependencyInfo != null && dependencyInfo.isAutowireCandidate())
                              {
                                 // match callbacks by name
                    -            existingCallbacks = new HashSet<CallbackItem<?>>();
                    -            existingCallbacks.addAll(getCallbacks(context.getName(), isInstallPhase));
                    +            existingCallbacks = getCallbacks(existingCallbacks, context.getName(), isInstallPhase);
                                 // match by classes
                                 Collection<Class<?>> classes = getExposedClasses(context);
                                 if (classes != null && classes.isEmpty() == false)
                                 {
                                    for (Class<?> clazz : classes)
                                    {
                    -                  existingCallbacks.addAll(getCallbacks(clazz, isInstallPhase));
                    +                  existingCallbacks = getCallbacks(existingCallbacks, clazz, isInstallPhase);
                                    }
                                 }
                    -            
                    -            if (existingCallbacks.isEmpty())
                    -               existingCallbacks = null;
                              }
                              
                              if (installs != null || uninstalls != null || existingCallbacks != null)
                    
                    
                    
                    1 2 Previous Next