1 2 Previous Next 27 Replies Latest reply on Feb 15, 2010 10:13 AM by kabirkhan Go to original post
      • 15. Re: Optimizing ControllerState
        kabirkhan

        Similarly, it also seems like INSTALLED is taken to always be the final state, e.g.

         public ControllerContext getInstalledContext(Object name)
         {
         return getContext(name, ControllerState.INSTALLED);
         }
        

        Since INSTALLED has special meaning, if there are states beyond INSTALLED, contexts in those states would not be reported as being installed. Should I enforce that INSTALLED is the final state?

        • 16. Re: Optimizing ControllerState
          alesj

           

          "kabir.khan@jboss.com" wrote:

          Should this be enforced, i.e. we throw an error if someone tries to add a state before NOT_INSTALLED?

          I would just have CSModel::getInitialState().

          • 17. Re: Optimizing ControllerState
            alesj

             

            "kabir.khan@jboss.com" wrote:
            Similarly, it also seems like INSTALLED is taken to always be the final state, e.g.
             public ControllerContext getInstalledContext(Object name)
             {
             return getContext(name, ControllerState.INSTALLED);
             }
            

            Since INSTALLED has special meaning, if there are states beyond INSTALLED, contexts in those states would not be reported as being installed. Should I enforce that INSTALLED is the final state?

            Actually they are reported, see AbstractController::getContext
             ControllerContext result = getRegisteredControllerContext(name, false);
             if (result != null && state != null && isBeforeState(result.getState(), state))
             {
             return null;
             }
            


            But AbstractKernelController::getContext could be fixed to apply this the same way.


            • 18. Re: Optimizing ControllerState
              alesj

              About that CSModel::isValid(ControllerState), I don't think it's needed,
              perhaps only for sanity, but not as part of interface.

              As ControllerState creation should already be controlled via single CS registry,
              hence would almost be impossible to get an invalid instance.

              • 19. Re: Optimizing ControllerState
                kabirkhan

                 

                "alesj" wrote:

                I would just have CSModel::getInitialState().

                Yeah, I was thinking something along the same lines after posting.

                "alesj" wrote:

                But AbstractKernelController::getContext could be fixed to apply this the same way.

                Will do

                "alesj" wrote:

                About that CSModel::isValid(ControllerState), I don't think it's needed,
                perhaps only for sanity, but not as part of interface.

                As ControllerState creation should already be controlled via single CS registry,
                hence would almost be impossible to get an invalid instance.

                Although a state has been created via a registry, as I understand it it that does not mean it is valid in a particular controller, e.g.:
                Controller controllerA = ..
                Controller controllerB = ..
                
                ControllerState stateA = StateRegistry.getState("A");
                controllerA.addState(stateA, null);
                ControllerState stateB = StateRegistry.getState("B");
                controllerB.addState(stateB, null);
                

                stateA will not be valid in controllerB, and vice versa. Unless the getState() belongs to the controller and adds it automatically, in which case Controller.addState() becomes superfluous.

                I've fixed a problem in incrementState() where when called initially with a context in the ERROR state getting moved from ERROR->NOT_INSTALLED in the first install() call and then from ERROR->NOT_INSTALLED again in the second call to install()



                • 20. Re: Optimizing ControllerState
                  kabirkhan

                  I have committed the optimized controller state model and updated AbstractController to use it.
                  https://jira.jboss.org/jira/browse/JBKERNEL-57

                  I removed ControllerStateModel from AbstractController's interfaces and removed the methods. The new ControllerStateModel lives here:
                  https://svn.jboss.org/repos/jbossas/projects/kernel/trunk/dependency/src/main/java/org/jboss/dependency/plugins/MapControllerStateModel.java
                  and the legacy one lives here"
                  https://svn.jboss.org/repos/jbossas/projects/kernel/trunk/dependency/src/main/java/org/jboss/dependency/plugins/ListControllerStateModel.java

                  "kabir" wrote:

                  One minor issue is the iterator() and listIterator() methods, which are not thread safe at the moment. The other methods are called with a lock taken in AbstractController. For the iterator methods I need to readLock, take a snapshot into a list and return iterators to that list.


                  This was wrong, although AbstractController holds locks when accessing the states, others may use it without locks taken, e.g.
                  controller.getStates.isAfterState(state1, state2)
                  

                  Since MapControllerStateModel keeps tracks of a few different things (as opposed to ListControllerStateModel which is backed by a single concurrent list), I am unsure whether read/write locks should be used there. I am pretty sure that the way I have set it up, it should be safe to use, but you might want to review this. In any case having read/write locks in MapCSM should still outperform ListCSM although I haven't actually measured that. I'll check that if consensus is that MapCSM needs locks


                  • 21. Re: Optimizing ControllerState
                    kabirkhan

                     

                    "kabir.khan@jboss.com" wrote:
                    "alesj" wrote:

                    I would just have CSModel::getInitialState().

                    Yeah, I was thinking something along the same lines after posting.

                    Done https://jira.jboss.org/jira/browse/JBKERNEL-58

                    • 22. Re: Optimizing ControllerState
                      kabirkhan

                      https://jira.jboss.org/jira/browse/JBKERNEL-59

                      "alesj" wrote:

                      Actually they are reported, see AbstractController::getContext
                       ControllerContext result = getRegisteredControllerContext(name, false);
                       if (result != null && state != null && isBeforeState(result.getState(), state))
                       {
                       return null;
                       }
                      


                      I updated the javadoc of Controller.getContext(Object, ControllerState) to show what is actually happening:
                       /**
                       * Get a context that has reached at least the passed in state
                       *
                       * @param name the name of the component
                       * @param state the state (pass null for any state)
                       * @return the context
                       */
                      


                      "alesj" wrote:

                      But AbstractKernelController::getContext could be fixed to apply this the same way.

                      I modified it to look for kernel registry entries is state >= INSTALLED and added StateAfterInstalledTestCase.

                      • 23. Re: Optimizing ControllerState
                        alesj

                         

                        "kabir.khan@jboss.com" wrote:

                        Although a state has been created via a registry, as I understand it it that does not mean it is valid in a particular controller, e.g.:
                        Controller controllerA = ..
                        Controller controllerB = ..
                        
                        ControllerState stateA = StateRegistry.getState("A");
                        controllerA.addState(stateA, null);
                        ControllerState stateB = StateRegistry.getState("B");
                        controllerB.addState(stateB, null);
                        

                        stateA will not be valid in controllerB, and vice versa. Unless the getState() belongs to the controller and adds it automatically, in which case Controller.addState() becomes superfluous.

                        Registry is an impl detail of CSModel.
                        But yeah, I see the point of CSM::isValid(CS) now.


                        • 24. Re: Optimizing ControllerState
                          kabirkhan
                          None of us did the work on avoiding zillions of ControllerState instances during xml parsing,
                          public class ControllerStateValueAdapter implements ValueAdapter
                          {
                             @SuppressWarnings("unchecked")
                             public Object cast(Object o, Class c)
                             {
                                return new ControllerState((String)o);
                             }
                          }
                          

                           

                          While the improved ControllerStateModel I created ide-steps the need for being able to compare states via object equality, I think it would make sense to make this cast() method try to use one of the existing instances? Something along the lines of
                             @SuppressWarnings("unchecked")
                             public Object cast(Object o, Class c)
                             {
                                ControllerState state = ControllerState.getExisitingState((String)o);
                                if (state == null)
                                {
                                   state = new ControllerState((String)o);
                                   ControllerState.addExistingState(state); //Or throw exception since the state does not exist?
                                } 
                          
                                return state;
                          
                          
                          
                          }
                           
                          
                          
                          
                          • 25. Re: Optimizing ControllerState
                            alesj

                            Ah, I thought you did this. :-)

                            I would just have ControllerState::getInstance somehow tied to the CSModel.

                            • 26. Re: Optimizing ControllerState
                              kabirkhan
                              • 27. Re: Optimizing ControllerState
                                kabirkhan

                                Although this was a significant improvement, it is still a bottleneck in benchmarks/AS startup since isBeforeState() and isAfterState() are called so many times. They both need to look up both states in the map to get the individual indexes to be able to compare the indexes.

                                 

                                I am reworking MapControllerStateModel so that MapControllerStateModel.ControllerStateWrapper extends ControllerState, and to index the states set in the ControllerContexts by AbstractController so that when MapControllerStateModel is used it uses the ControllerStateWrappers directly which have the index there, meaning we don't need to look things up in the map. The results look good, but I also need to index the states stored in AbstractDependencyItems since they get passed in to Controller.getContext() which calls ControllerStateModel.isBeforeState() internally.\

                                 

                                Committed against https://jira.jboss.org/jira/browse/JBKERNEL-99

                                1 2 Previous Next