Details

    • Type: Task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.4
    • Security Level: Public (Public: Anyone can view)
    • Labels:
      None
    • Epic Link:
    • Rice Module:
      KRAD
    • Sprint:
      2.4.0-m3 KRAD UXI Sprint 3, 2.4.0-m3 KRAD UXI Sprint 4, 2.4.0-m4 KRAD UXI Sprint 2
    • KAI Review Status:
      Not Required
    • KTI Review Status:
      Not Required
    • Code Review Status:
      Not Required
    • Include in Release Notes?:
      Yes

      Description

      Do analysis and POC on following items:

      1. Reuse nested component lists. Initial timings with this approach several weeks ago were very positive, but I kept having to back it out due to stability issues. The lifecycle is much more organized now – once the current refactoring effort is stable, reusing nested component lists in a controlled manner will be quite a bit easier to accomplish.
      2. Incremental view indexing and cleaning. During the lifecycle, there are several full tree traversals taking place: one for each phase, one to index the entire view between phases, and another to clean the view at the end. There are also multiple smaller traversals within various lifecycle phases for different purposes – for example to search for data fields in a collection. Incremental indexing could minimize these traversals, and incremental cleaning could free resources immediately after the render phase.
      3. Component prototype “recycling”. If we can set up a reliable mechanism to recycle component instances, particularly those consumed as prototypes during the lifecycle, then heap consumption and GC overhead can be significantly reduced. The technique appears to have worked well to reverse the overhead added during lifecycle phase/task refactoring, and could work on a larger scale if applied to components. Have a look at LifecycleTaskFactory and LifecyclePhaseFactory for the general idea.

        Attachments

          Activity

          Hide
          mwfyffe Mark Fyffe (Inactive) added a comment -

          Cleanup items noted by Jerry for another pass at this issue:

          1) Quick style thing: We decided for ternary operators they should be very simple and fit on one line. For example, we would rather see this as an if:

          Component parentComponent = parent == null ? null
          : parent.getElement() instanceof Component ? (Component) parent.getElement()
          : parent.getParent();
          2) With the latest changes we moved task initialization more to the lifecycle element. I would rather keep them in the phase and have one place to look. Could we do this if we made tasks that were only applied to certain elements? In general, it would be great if we could isolate all the lifecycle code (besides the phase callbacks) to the lifecycle packages.

          3) The clearIndex(); call on View causes issues with getting components by id. For example in Action we need to get a component to set the refreshedByAction flag, but if the phase has not been run on that component yet it is not in the index:

          Action:

          else if (StringUtils.isNotBlank(refreshId)) {
          Component component = view.getViewIndex().getComponentById(refreshId);
          if (component != null)

          Unknown macro: { refreshComponent = component; }

          }

          4) We need to make another pass and cleanup javadocs and method ordering. Lots of incorrect Javadocs are present, and style problems. Does Eclipse highlight problems in the Javadoc (for example when the doc has a unmatched param tag)? Here is more on doc standards:

          https://wiki.kuali.org/display/KULRICE/Documentation+Standards

          Show
          mwfyffe Mark Fyffe (Inactive) added a comment - Cleanup items noted by Jerry for another pass at this issue: 1) Quick style thing: We decided for ternary operators they should be very simple and fit on one line. For example, we would rather see this as an if: Component parentComponent = parent == null ? null : parent.getElement() instanceof Component ? (Component) parent.getElement() : parent.getParent(); 2) With the latest changes we moved task initialization more to the lifecycle element. I would rather keep them in the phase and have one place to look. Could we do this if we made tasks that were only applied to certain elements? In general, it would be great if we could isolate all the lifecycle code (besides the phase callbacks) to the lifecycle packages. 3) The clearIndex(); call on View causes issues with getting components by id. For example in Action we need to get a component to set the refreshedByAction flag, but if the phase has not been run on that component yet it is not in the index: Action: else if (StringUtils.isNotBlank(refreshId)) { Component component = view.getViewIndex().getComponentById(refreshId); if (component != null) Unknown macro: { refreshComponent = component; } } 4) We need to make another pass and cleanup javadocs and method ordering. Lots of incorrect Javadocs are present, and style problems. Does Eclipse highlight problems in the Javadoc (for example when the doc has a unmatched param tag)? Here is more on doc standards: https://wiki.kuali.org/display/KULRICE/Documentation+Standards
          Hide
          mwfyffe Mark Fyffe (Inactive) added a comment -

          Addressed item #2:

          I have moved lifecycle task initialization back to the default methods in ComponentIntitializePhase, ComponentApplyModelPhase, and ComponentFinalizePhase. To eliminate instanceof checking, and other complex branching logic during this setup, I added a check while executing tasks to ensure that the lifecycle element for the current phase is an instance of an "elementClass" defined for the task.

          I've switched development over to IntelliJ to complete a final pass at JavaDoc cleanup. TODOs from 12/23 are still outstanding.

          Show
          mwfyffe Mark Fyffe (Inactive) added a comment - Addressed item #2: I have moved lifecycle task initialization back to the default methods in ComponentIntitializePhase, ComponentApplyModelPhase, and ComponentFinalizePhase. To eliminate instanceof checking, and other complex branching logic during this setup, I added a check while executing tasks to ensure that the lifecycle element for the current phase is an instance of an "elementClass" defined for the task. I've switched development over to IntelliJ to complete a final pass at JavaDoc cleanup. TODOs from 12/23 are still outstanding.
          Hide
          mwfyffe Mark Fyffe (Inactive) added a comment -

          Work is in progress on the 2.4 performance branch for converting the pre-process phase to a fully formed lifecycle phase.

          Implementation is fully complete, but I am working through a few null pointers - related to the model being null during the pre-process phase.

          I expect to have this issue resolved early this week.

          Show
          mwfyffe Mark Fyffe (Inactive) added a comment - Work is in progress on the 2.4 performance branch for converting the pre-process phase to a fully formed lifecycle phase. Implementation is fully complete, but I am working through a few null pointers - related to the model being null during the pre-process phase. I expect to have this issue resolved early this week.
          Hide
          mwfyffe Mark Fyffe (Inactive) added a comment -

          Committed updates to performance branch:

          1. Added "viewPath" bean property to both LifecycleElement and ViewLifecyclePhase for the full path relative to the view, and clarified the use of the "path" property as relative to the element's parent component.
          2. Converted pre-process to a fully formed lifecycle phase.
          3. Added recycling to lifecycle element maps.

          Last step is to investigate impact related to clearIndex()

          Show
          mwfyffe Mark Fyffe (Inactive) added a comment - Committed updates to performance branch: Added "viewPath" bean property to both LifecycleElement and ViewLifecyclePhase for the full path relative to the view, and clarified the use of the "path" property as relative to the element's parent component. Converted pre-process to a fully formed lifecycle phase. Added recycling to lifecycle element maps. Last step is to investigate impact related to clearIndex()
          Hide
          mwfyffe Mark Fyffe (Inactive) added a comment -

          Removed clearIndex() calls and performed regression testing to check for fallout. With the planned removal of ViewIndex, this should have minimal long-term impact.

          As a final item for this issue, I modified ProcessLoggingUnitTest to facilitate multiple repetitions of the test. It is up to the test itself to alter the test expectations for each run. I set each ViewLifecycleTest method to run 2 times: once with asynchronous lifecycle, and once synchronous.

          Show
          mwfyffe Mark Fyffe (Inactive) added a comment - Removed clearIndex() calls and performed regression testing to check for fallout. With the planned removal of ViewIndex, this should have minimal long-term impact. As a final item for this issue, I modified ProcessLoggingUnitTest to facilitate multiple repetitions of the test. It is up to the test itself to alter the test expectations for each run. I set each ViewLifecycleTest method to run 2 times: once with asynchronous lifecycle, and once synchronous.

            People

            • Assignee:
              mwfyffe Mark Fyffe (Inactive)
              Reporter:
              jkneal Jerry Neal (Inactive)
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 2 days Original Estimate - 2 days
                2d
                Remaining:
                Remaining Estimate - 0 minutes
                0m
                Logged:
                Time Spent - 1 week, 7 hours
                1w 7h