Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-5169

Removing Javolution from framework components

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: Trunk
    • Fix Version/s: 16.11.01
    • Component/s: framework
    • Labels:
      None
    • Sprint:
      Bug Crush Event - 21/2/2015

      Description

      a. Remove static instances of Javolution objects.
      b. Remove other uses of Javolution objects.

        Issue Links

          Activity

          Hide
          vbhansaly Varun Bhansaly added a comment -

          This patch removes all instances of FastList and replaces them with ArrayList.

          Show
          vbhansaly Varun Bhansaly added a comment - This patch removes all instances of FastList and replaces them with ArrayList.
          Hide
          jacopoc Jacopo Cappellato added a comment -

          Using ArrayList, rather than LinkedList, as a default is a good approach because ArrayList will work better in most situations (in terms of algorithmic runtimes and memory footprints).
          However I think it is important to wisely choose ArrayList or LinkedList based on the context, because sometimes a linked list may be a better choice: I am not saying we should do it now and we can tackle this at a later moment, possibly focusing only on list that can be very big and analyzing their usage to see if array or linked list is better; it would be also nice, when we use ArrayList, to see if we can predict the capacity of the list and allocate it at creation: if we can do this we will have a more predictable memory usage and less unpredictable peaks under heavy load (when the array capacity is reached and the array is copied to a new one with double capacity).

          Show
          jacopoc Jacopo Cappellato added a comment - Using ArrayList, rather than LinkedList, as a default is a good approach because ArrayList will work better in most situations (in terms of algorithmic runtimes and memory footprints). However I think it is important to wisely choose ArrayList or LinkedList based on the context, because sometimes a linked list may be a better choice: I am not saying we should do it now and we can tackle this at a later moment, possibly focusing only on list that can be very big and analyzing their usage to see if array or linked list is better; it would be also nice, when we use ArrayList, to see if we can predict the capacity of the list and allocate it at creation: if we can do this we will have a more predictable memory usage and less unpredictable peaks under heavy load (when the array capacity is reached and the array is copied to a new one with double capacity).
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          Varun,

          Thank you for getting started on this.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - Varun, Thank you for getting started on this.
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          Jacopo,

          I agree we need to choose replacements wisely. I intended to create a Jira issue to provide some guidelines, but now that this one is here I will use it.

          ArrayLists are ideal for write-once-read-many situations. Linked lists are good for read/write scenarios. If you look carefully at Mini-language, you will see where I used ArrayLists in the model object fields - because the lists are written once and then read many times. I also used the trimToSize() method to keep them small.

          Linked lists are good to use in the execution path - like in local variables.

          Those are general guidelines, and there will be exceptions.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - Jacopo, I agree we need to choose replacements wisely. I intended to create a Jira issue to provide some guidelines, but now that this one is here I will use it. ArrayLists are ideal for write-once-read-many situations. Linked lists are good for read/write scenarios. If you look carefully at Mini-language, you will see where I used ArrayLists in the model object fields - because the lists are written once and then read many times. I also used the trimToSize() method to keep them small. Linked lists are good to use in the execution path - like in local variables. Those are general guidelines, and there will be exceptions.
          Hide
          vbhansaly Varun Bhansaly added a comment - - edited

          Adrian,

          Will you be committing this patch in the branch and shall I move ahead with removing FastSet & FastMap ?
          Anything specific you would like to be considered while creating patch for these ?
          FastMap changes may have to be handled relatively carefully.

          Show
          vbhansaly Varun Bhansaly added a comment - - edited Adrian, Will you be committing this patch in the branch and shall I move ahead with removing FastSet & FastMap ? Anything specific you would like to be considered while creating patch for these ? FastMap changes may have to be handled relatively carefully.
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          I will be committing the changes to the trunk. I came to the conclusion a branch will be too hard to maintain because of the scope of the effort.

          I will need time to review your patch and profile the changes to check for negative impact.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - I will be committing the changes to the trunk. I came to the conclusion a branch will be too hard to maintain because of the scope of the effort. I will need time to review your patch and profile the changes to check for negative impact.
          Hide
          jacopoc Jacopo Cappellato added a comment -

          Adrian,

          I think that gathering information from a profiler would indeed help.
          However, would it make sense, if there is a general agreement, to create a branch with the current trunk to keep a Javolution version of OFBiz and then commit the changes to remove Javolution (after your review/tests) to the trunk?
          I am basically saying that we could use the branch the other way round: in the future we could use it to run comparisons with the Javolution-free trunk.
          A TAG would also probably work (because the Javolution branch will be mostly read-only) but a branch may give us some flexibility: for example we could commit there some optimizations done in the trunk that are not related to Javolution and that may impact performance.
          In this way it will be easier to migrate out of Javolution in steps.
          Of course before doing this we should check with the community if there are still major concerns.

          Show
          jacopoc Jacopo Cappellato added a comment - Adrian, I think that gathering information from a profiler would indeed help. However, would it make sense, if there is a general agreement, to create a branch with the current trunk to keep a Javolution version of OFBiz and then commit the changes to remove Javolution (after your review/tests) to the trunk? I am basically saying that we could use the branch the other way round: in the future we could use it to run comparisons with the Javolution-free trunk. A TAG would also probably work (because the Javolution branch will be mostly read-only) but a branch may give us some flexibility: for example we could commit there some optimizations done in the trunk that are not related to Javolution and that may impact performance. In this way it will be easier to migrate out of Javolution in steps. Of course before doing this we should check with the community if there are still major concerns.
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          That's a great idea! I can use the one I already created and revert my changes (only one commit so far).

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - That's a great idea! I can use the one I already created and revert my changes (only one commit so far).
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          Varun,

          It would be easier for me to evaluate the changes if we replace all Javolution objects, instead of just Lists. Also, could you break up the patches by component please?

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - Varun, It would be easier for me to evaluate the changes if we replace all Javolution objects, instead of just Lists. Also, could you break up the patches by component please?
          Hide
          vbhansaly Varun Bhansaly added a comment -

          Adrian,

          Removing all Javolution objects would have taken more time hence had replaced only the lists.

          I will try to break up the patch into smaller patches by component.

          Show
          vbhansaly Varun Bhansaly added a comment - Adrian, Removing all Javolution objects would have taken more time hence had replaced only the lists. I will try to break up the patch into smaller patches by component.
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          Thinking about this more...

          It would be best if we worked on the components in this order: base, entity, service. I will run the profiler as each component is changed. When all three components have been changed, and if the profiler shows no adverse performance impact, then I believe we can remove Javolution from the rest of the project without concern.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - Thinking about this more... It would be best if we worked on the components in this order: base, entity, service. I will run the profiler as each component is changed. When all three components have been changed, and if the profiler shows no adverse performance impact, then I believe we can remove Javolution from the rest of the project without concern.
          Hide
          soledad Nicolas Malin added a comment -

          After close the OFBIZ-5781 we still face in code
          framework/entity/src/org/ofbiz/entity/util/EntitySaxReader.java:

          import javolution.text.CharArray;
          import javolution.text.Text;
          import javolution.xml.sax.Attributes;
          import javolution.xml.sax.XMLReaderImpl;
          public class EntitySaxReader implements javolution.xml.sax.ContentHandler, ErrorHandler {
          

          I will check how to replace it.

          Show
          soledad Nicolas Malin added a comment - After close the OFBIZ-5781 we still face in code framework/entity/src/org/ofbiz/entity/util/EntitySaxReader.java: import javolution.text.CharArray; import javolution.text.Text; import javolution.xml.sax.Attributes; import javolution.xml.sax.XMLReaderImpl; public class EntitySaxReader implements javolution.xml.sax.ContentHandler, ErrorHandler { I will check how to replace it.
          Hide
          soledad Nicolas Malin added a comment -

          I re read this thread and I am afraid that my commit to close OFBIZ-5781 has been untimely.

          Do you want that I revert it ?

          Show
          soledad Nicolas Malin added a comment - I re read this thread and I am afraid that my commit to close OFBIZ-5781 has been untimely. Do you want that I revert it ?
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          Yes maybe a profiler session would be a plus. But I have no ideas on how much OFBIZ-5781 changes performances. I note here Adrian's comment on dev ML about OFBIZ-5781 and r1672752

          This should be fine, but there might be cases where a LinkedHashMap is needed.

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited Yes maybe a profiler session would be a plus. But I have no ideas on how much OFBIZ-5781 changes performances. I note here Adrian's comment on dev ML about OFBIZ-5781 and r1672752 This should be fine, but there might be cases where a LinkedHashMap is needed.
          Hide
          soledad Nicolas Malin added a comment -

          Yesterday I continue to benchmark. On the ant clean-all load-demo run-tests between before and after the revision r1672752 on average I win 10s.
          If some body can to do it under an other plateform (I'm under linux and jdk 1.7), it's would be great

          For the LinkedHashMap, yes I use it when I detected an iteration on the map. I will realize a second pass to check if I can found other optimization, it's also the case for arrayList instead of linkedList. On many case the list use through foreach loop, but I didn't analyze all case.

          Show
          soledad Nicolas Malin added a comment - Yesterday I continue to benchmark. On the ant clean-all load-demo run-tests between before and after the revision r1672752 on average I win 10s. If some body can to do it under an other plateform (I'm under linux and jdk 1.7), it's would be great For the LinkedHashMap, yes I use it when I detected an iteration on the map. I will realize a second pass to check if I can found other optimization, it's also the case for arrayList instead of linkedList. On many case the list use through foreach loop, but I didn't analyze all case.
          Hide
          mbecker Martin Becker added a comment -

          I wonder why the LinkedList was chosen as the default replacement of FastList in Java code instead if an ArrayList (see comment from Jacopo Cappellato above). Although it could be a very complex question which implementation to choose respecting the concrete logic and the underlying environment, I would prefer to use ArrayList as default on the basis of the memory consumption and general performance impacts of LinkedList alone. The cases at OFBiz where the main benefit of faster insert/remove in the middle/beginning of a large LinkedList is relevant should be very rare, especially with moderate list sizes. The disadvantage of ArrayList in growing over the initial capacity should always be encountered by specifying an appropriate initial capacity where possible.

          See short description from oracle:
          https://docs.oracle.com/javase/tutorial/collections/implementations/list.html

          Another conspicuity: The migrated groovy scripts instead often using the default list implementation which is ArrayList by only using „[]“ without specifying LinkedList as concrete type.

          Show
          mbecker Martin Becker added a comment - I wonder why the LinkedList was chosen as the default replacement of FastList in Java code instead if an ArrayList (see comment from Jacopo Cappellato above). Although it could be a very complex question which implementation to choose respecting the concrete logic and the underlying environment, I would prefer to use ArrayList as default on the basis of the memory consumption and general performance impacts of LinkedList alone. The cases at OFBiz where the main benefit of faster insert/remove in the middle/beginning of a large LinkedList is relevant should be very rare, especially with moderate list sizes. The disadvantage of ArrayList in growing over the initial capacity should always be encountered by specifying an appropriate initial capacity where possible. See short description from oracle: https://docs.oracle.com/javase/tutorial/collections/implementations/list.html Another conspicuity: The migrated groovy scripts instead often using the default list implementation which is ArrayList by only using „[]“ without specifying LinkedList as concrete type.
          Hide
          jacopoc Jacopo Cappellato added a comment -

          I agree with Martin Becker

          Show
          jacopoc Jacopo Cappellato added a comment - I agree with Martin Becker
          Hide
          soledad Nicolas Malin added a comment -

          I also agree with you (really it's not a joke)

          I started the conversion on OFBIZ-5781 where Deepak Dixit began by groovy file.
          When I kept, I didn't read this thread so I checked what is more used on framework.

          Now I understand more each detail and I have two choices :

          • revert and restart the conversion (lot's of work and javolution come back)
          • keep like it and realize a second pass with the ArrayList improvement

          I really hesitated between both to finally says, ok I break nothing, I didn't impacted the framework, so move forward.
          And sure, it will be a pleasure for me to review your future patch

          Show
          soledad Nicolas Malin added a comment - I also agree with you (really it's not a joke) I started the conversion on OFBIZ-5781 where Deepak Dixit began by groovy file. When I kept, I didn't read this thread so I checked what is more used on framework. Now I understand more each detail and I have two choices : revert and restart the conversion (lot's of work and javolution come back) keep like it and realize a second pass with the ArrayList improvement I really hesitated between both to finally says, ok I break nothing, I didn't impacted the framework, so move forward. And sure, it will be a pleasure for me to review your future patch
          Hide
          jacopoc Jacopo Cappellato added a comment -

          Thank you Nicolas for your feedback.
          I think that a second pass would make more sense, we could file a ticket in Jira and then see if volunteers will help us.

          Show
          jacopoc Jacopo Cappellato added a comment - Thank you Nicolas for your feedback. I think that a second pass would make more sense, we could file a ticket in Jira and then see if volunteers will help us.
          Hide
          deepak.dixit Deepak Dixit added a comment -

          We should use the LinkedHashMap for controller pre/post processor eventLists (ConfigXMLReader.java:180) as HashMap does not maintain the iteration order.

          Show
          deepak.dixit Deepak Dixit added a comment - We should use the LinkedHashMap for controller pre/post processor eventLists (ConfigXMLReader.java:180) as HashMap does not maintain the iteration order.
          Hide
          soledad Nicolas Malin added a comment -

          Thanks Jacopo I appreciate to have an other opinion.
          I open the issue OFBIZ-6298 for that

          Show
          soledad Nicolas Malin added a comment - Thanks Jacopo I appreciate to have an other opinion. I open the issue OFBIZ-6298 for that
          Hide
          mbecker Martin Becker added a comment -

          Well, my intention is to overall switch the now used LinkedList to ArrayList, as it is the more proper default with a light performance drawback in rare cases, if at all, and to be in line with the types used in groovy scripts. No functional drawback compared to FastList is to be expected (in contrast to HashMap/HashSet, if preserving the insert-order for iteration is a requirement).

          That would be a good starting point for detailed analysis and changes in the scope of OFBIZ-6298, to optimize the use of collection types, also this will be a very exhausting activity over thousands of code places with very small expectable benefits in real world performance.

          Show
          mbecker Martin Becker added a comment - Well, my intention is to overall switch the now used LinkedList to ArrayList, as it is the more proper default with a light performance drawback in rare cases, if at all, and to be in line with the types used in groovy scripts. No functional drawback compared to FastList is to be expected (in contrast to HashMap/HashSet, if preserving the insert-order for iteration is a requirement). That would be a good starting point for detailed analysis and changes in the scope of OFBIZ-6298 , to optimize the use of collection types, also this will be a very exhausting activity over thousands of code places with very small expectable benefits in real world performance.
          Hide
          taher Taher Alkhateeb added a comment -

          I removed all javolution uses except in EntitySaxReader.java in r1710122.

          Now one file remains and we can take this library out. Working on it.

          Show
          taher Taher Alkhateeb added a comment - I removed all javolution uses except in EntitySaxReader.java in r1710122. Now one file remains and we can take this library out. Working on it.
          Hide
          soledad Nicolas Malin added a comment -

          greats! thanks Taher

          Show
          soledad Nicolas Malin added a comment - greats! thanks Taher
          Hide
          taher Taher Alkhateeb added a comment -

          I've investigated what to re implement to satisfy dependencies in other Java sources in the framework and I list them below for reference. Once we reimplement them in EntitySaxReader without javolution then I would assume it is safe to remove the javolution from the framework

          EntitySaxReader(delegator, timeout) // constructor
          EntitySaxReader(delegator) // constructor
          parse(content) // XML
          parse(location) // URL
          setCheckDataOnly(checkDataOnly) // boolean
          setCreateDummyFks(dummyFks) // boolean
          setMaintainTxStamps(maintainTxStamps) // boolean
          setTransactionTimeout(transactionTimeout) // int
          setUseTryInsertMethod(value) // boolean

          Show
          taher Taher Alkhateeb added a comment - I've investigated what to re implement to satisfy dependencies in other Java sources in the framework and I list them below for reference. Once we reimplement them in EntitySaxReader without javolution then I would assume it is safe to remove the javolution from the framework EntitySaxReader(delegator, timeout) // constructor EntitySaxReader(delegator) // constructor parse(content) // XML parse(location) // URL setCheckDataOnly(checkDataOnly) // boolean setCreateDummyFks(dummyFks) // boolean setMaintainTxStamps(maintainTxStamps) // boolean setTransactionTimeout(transactionTimeout) // int setUseTryInsertMethod(value) // boolean
          Hide
          jacopoc Jacopo Cappellato added a comment -

          EntitySaxReader has been converted and Javolution removed in rev. 1762079.

          Please help testing the xml data import (in its various forms) and report any issues you may discover since I had to implement several changes in EntitySaxReader (which is the class responsible for most of the xml data loading in OFBiz).

          Show
          jacopoc Jacopo Cappellato added a comment - EntitySaxReader has been converted and Javolution removed in rev. 1762079. Please help testing the xml data import (in its various forms) and report any issues you may discover since I had to implement several changes in EntitySaxReader (which is the class responsible for most of the xml data loading in OFBiz).

            People

            • Assignee:
              jacopoc Jacopo Cappellato
              Reporter:
              vbhansaly Varun Bhansaly
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development

                  Agile