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

Improve Start.java Component Loading

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 16.11.01
    • Fix Version/s: 16.11.01
    • Component/s: framework
    • Labels:
      None

      Description

      The current code for loading components parses configuration files twice. This issue is intended for review of code improvements.

      1. OFBIZ-6268.patch
        42 kB
        Adrian Crum
      2. OFBIZ-6268.patch
        34 kB
        Adrian Crum

        Activity

        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        The attached patch is the basic redesign. The instrumentation is not working, so it is commented out. I will continue working on that.

        The existing classes have been reworked slightly to provide better separation of concerns.

        Synchronization code has been added to ClassPath.java in preparation for multi-threaded component loading. I will add that feature after this review.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - The attached patch is the basic redesign. The instrumentation is not working, so it is commented out. I will continue working on that. The existing classes have been reworked slightly to provide better separation of concerns. Synchronization code has been added to ClassPath.java in preparation for multi-threaded component loading. I will add that feature after this review.
        Hide
        jacopoc Jacopo Cappellato added a comment -

        Thank you Adrian.
        What I don't like about this new approach is that it adds to the "framework/start" module a dependency on the inner layout of the "base" component:

                    classPath.addComponent(ofbizHomeTmp.concat("framework/base/config"));
                    classPath.addComponent(ofbizHomeTmp.concat("framework/base/dtd"));
                    classPath.addFilesFromPath(new File(ofbizHomeTmp.concat("framework/base/lib")));
                    classPath.addFilesFromPath(new File(ofbizHomeTmp.concat("framework/base/lib/commons")));
                    classPath.addComponent(ofbizHomeTmp.concat("framework/base/build/lib/ofbiz-base.jar"));
        

        while with the current code the dependency is just on the "layout" of the OFBiz folder:

                collectClasspathEntries(new File(home, "framework"), classPath, libraryPath);
                collectClasspathEntries(new File(home, "applications"), classPath, libraryPath);
                collectClasspathEntries(new File(home, "specialpurpose"), classPath, libraryPath);
                collectClasspathEntries(new File(home, "hot-deploy"), classPath, libraryPath);
        

        Moreover, does the new code introduce any changes to the ClassLoader tree? (i.e. a new ClassLoader in the parent-child path)
        I am asking because it is difficult for me to figure out by reviewing the patch.
        If it adds new levels, even if this is not a big deal, then we should decide if it is worth to improve startup performance and penalize runtime performance (both by tiny fractions).

        Show
        jacopoc Jacopo Cappellato added a comment - Thank you Adrian. What I don't like about this new approach is that it adds to the "framework/start" module a dependency on the inner layout of the "base" component: classPath.addComponent(ofbizHomeTmp.concat( "framework/base/config" )); classPath.addComponent(ofbizHomeTmp.concat( "framework/base/dtd" )); classPath.addFilesFromPath( new File(ofbizHomeTmp.concat( "framework/base/lib" ))); classPath.addFilesFromPath( new File(ofbizHomeTmp.concat( "framework/base/lib/commons" ))); classPath.addComponent(ofbizHomeTmp.concat( "framework/base/build/lib/ofbiz-base.jar" )); while with the current code the dependency is just on the "layout" of the OFBiz folder: collectClasspathEntries( new File(home, "framework" ), classPath, libraryPath); collectClasspathEntries( new File(home, "applications" ), classPath, libraryPath); collectClasspathEntries( new File(home, "specialpurpose" ), classPath, libraryPath); collectClasspathEntries( new File(home, "hot-deploy" ), classPath, libraryPath); Moreover, does the new code introduce any changes to the ClassLoader tree? (i.e. a new ClassLoader in the parent-child path) I am asking because it is difficult for me to figure out by reviewing the patch. If it adds new levels, even if this is not a big deal, then we should decide if it is worth to improve startup performance and penalize runtime performance (both by tiny fractions).
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        The start component has many dependencies on base - there is no way to avoid that. Keep in mind that start used to be IN base.

        No changes have been made to the class loader tree.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - The start component has many dependencies on base - there is no way to avoid that. Keep in mind that start used to be IN base. No changes have been made to the class loader tree.
        Hide
        jacopoc Jacopo Cappellato added a comment - - edited

        I am not aware of any code dependencies; there are a few in the loader configurations in start.properties, but that is a config file.
        Just out of curiosity, did you measure how much this change will improve performance at startup? (in terms of milliseconds)

        Show
        jacopoc Jacopo Cappellato added a comment - - edited I am not aware of any code dependencies; there are a few in the loader configurations in start.properties, but that is a config file. Just out of curiosity, did you measure how much this change will improve performance at startup? (in terms of milliseconds)
        Hide
        jacopoc Jacopo Cappellato added a comment -

        Moreover, the code that you have removed was honoring the classpath entries defined in the base component (ofbiz-component.xml) rather than just relying in the folder layout of it.

        Show
        jacopoc Jacopo Cappellato added a comment - Moreover, the code that you have removed was honoring the classpath entries defined in the base component (ofbiz-component.xml) rather than just relying in the folder layout of it.
        Hide
        jacopoc Jacopo Cappellato added a comment -

        The fact that this patch didn't change the class loader tree is a good thing! Thanks.

        Show
        jacopoc Jacopo Cappellato added a comment - The fact that this patch didn't change the class loader tree is a good thing! Thanks.
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        Let's back up for a second. I understand the changes you made and why you made them. The problem is with your implementation - you caused the same files to be parsed twice.

        I am merely removing the double parsing, your other changes (and your intent) are intact. It would help if you applied the patch and actually looked at the code.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - Let's back up for a second. I understand the changes you made and why you made them. The problem is with your implementation - you caused the same files to be parsed twice. I am merely removing the double parsing, your other changes (and your intent) are intact. It would help if you applied the patch and actually looked at the code.
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        Btw, in the current trunk, if I disable the specialpurpose folder

        <component-loader xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                xsi:noNamespaceSchemaLocation="http://ofbiz.apache.org/dtds/component-loader.xsd">
            <load-components parent-directory="framework"/>
            <load-components parent-directory="themes"/>
            <load-components parent-directory="applications"/>
            <!-- 
            <load-components parent-directory="specialpurpose"/>
             -->
            <load-components parent-directory="hot-deploy"/>
        </component-loader>
        

        the specialpurpose component class paths are loaded anyway.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - Btw, in the current trunk, if I disable the specialpurpose folder <component-loader xmlns:xsi= "http: //www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation= "http: //ofbiz.apache.org/dtds/component-loader.xsd" > <load-components parent-directory= "framework" /> <load-components parent-directory= "themes" /> <load-components parent-directory= "applications" /> <!-- <load-components parent-directory= "specialpurpose" /> --> <load-components parent-directory= "hot-deploy" /> </component-loader> the specialpurpose component class paths are loaded anyway.
        Hide
        jacopoc Jacopo Cappellato added a comment -

        This is indeed a good point and a good idea for an improvement.
        The current code that does this:

                collectClasspathEntries(new File(home, "framework"), classPath, libraryPath);
                collectClasspathEntries(new File(home, "applications"), classPath, libraryPath);
                collectClasspathEntries(new File(home, "specialpurpose"), classPath, libraryPath);
                collectClasspathEntries(new File(home, "hot-deploy"), classPath, libraryPath);
        

        should instead look at the content of the component-loader file you mention; I didn't think about it when I have implemented the code.

        Show
        jacopoc Jacopo Cappellato added a comment - This is indeed a good point and a good idea for an improvement. The current code that does this: collectClasspathEntries( new File(home, "framework" ), classPath, libraryPath); collectClasspathEntries( new File(home, "applications" ), classPath, libraryPath); collectClasspathEntries( new File(home, "specialpurpose" ), classPath, libraryPath); collectClasspathEntries( new File(home, "hot-deploy" ), classPath, libraryPath); should instead look at the content of the component-loader file you mention; I didn't think about it when I have implemented the code.
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        Improved patch. Hard-coded class paths have been replaced with properties.

        The instrumentation still doesn't work (enabled in this patch). I'm still struggling to figure out why.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - Improved patch. Hard-coded class paths have been replaced with properties. The instrumentation still doesn't work (enabled in this patch). I'm still struggling to figure out why.
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        Fixed in rev 1676746. Thank you Jacopo for the review and feedback.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - Fixed in rev 1676746. Thank you Jacopo for the review and feedback.

          People

          • Assignee:
            Unassigned
            Reporter:
            adrianc@hlmksw.com Adrian Crum
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development