ICEfaces
  1. ICEfaces
  2. ICE-11155

Performance analysis of radioButton and checkboxButton in dataTable and ui:repeat

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.0, EE-4.1.0.GA
    • Fix Version/s: 4.2.BETA, 4.2
    • Component/s: ACE-Components
    • Labels:
      None
    • Environment:
      jsf2 virtualVM, Google Chrome profiler
    • Assignee Priority:
      P2

      Description

      both checkboxButton and radioButton have 4 script tags.
      one for init, one for renderRest and the same script for onfocus and onhover.

      Decided to look at this in a vm profiler (used virtualVM) and the js profiler (used Google Chrome). Attaching the screenshots for these.

      Currently showcase has buttonGroup examples which show the buttons in columns of a dataTable as well as in rows for ui:repeat.
      these examples (now) show the ability to change the number of rows. Initially the number of rows is set to 10, but you can increase to 100, 500 and 1000. These screenshots show the results of 1000 for virtual-vm and the js console shows the example for by row (using ui:repeat) as it doesn't use the dataTable which may also show some performance lag.
      for example, the by row example is at showcase (myIP):-

      http://10.18.39.137:8080/showcase/showcase.jsf?grp=ace:buttonGroup&exp=Group%20In%20Row

      ALSO...if js is the issue--js could only toggle off the previously selected button instead of all the other buttons... (500 rows in dataTable takes about 5 seconds but only has 2 radioButtons and 2 checkboxButtons per row). 500 rows in row example takes about 8seconds but has 3 radioButtons and 3 checkboxButtons per row.
       
      1. buttonGroupByRow 1000 rows.png
        820 kB
      2. non-optimized.png
        271 kB
      3. optimized.png
        277 kB
      4. profiling js.png
        502 kB
      5. Screen Shot showcase buttonGroup 1000 rows CPU Column.png
        537 kB

        Activity

        Judy Guglielmin created issue -
        Hide
        Judy Guglielmin added a comment -

        screen shots for showcase examples with 1000 rows.

        Show
        Judy Guglielmin added a comment - screen shots for showcase examples with 1000 rows.
        Judy Guglielmin made changes -
        Field Original Value New Value
        Attachment buttonGroupByRow 1000 rows.png [ 22334 ]
        Attachment Screen Shot showcase buttonGroup 1000 rows CPU Column.png [ 22335 ]
        Hide
        Judy Guglielmin added a comment -

        Google js profiler for 1000 rows of the ui:repeat example for buttonGroup from the showcase link described above.

        Show
        Judy Guglielmin added a comment - Google js profiler for 1000 rows of the ui:repeat example for buttonGroup from the showcase link described above.
        Judy Guglielmin made changes -
        Attachment profiling js.png [ 22336 ]
        Judy Guglielmin made changes -
        Assignee Mircea Toma [ mircea.toma ]
        Judy Guglielmin made changes -
        Description both checkboxButton and radioButton have 4 script tags.
        one for init, one for renderRest and the same script for onfocus and onhover.

        Decided to look at this in a vm profiler (used virtualVM) and the js profiler (used Google Chrome). Attaching the screenshots for these.

        Currently showcase has buttonGroup examples which show the buttons in columns of a dataTable as well as in rows for ui:repeat.
        these examples (now) show the ability to change the number of rows. Initially the number of rows is set to 10, but you can increase to 100, 500 and 1000. These screenshots show the results of 1000 for virtual-vm and the js console shows the example for by row (using ui:repeat) as it doesn't use the dataTable which may also show some performance lag.
        for example, the by row example is at showcase (myIP):-

        http://10.18.39.137:8080/showcase/showcase.jsf?grp=ace:buttonGroup&exp=Group%20In%20Row
         
        both checkboxButton and radioButton have 4 script tags.
        one for init, one for renderRest and the same script for onfocus and onhover.

        Decided to look at this in a vm profiler (used virtualVM) and the js profiler (used Google Chrome). Attaching the screenshots for these.

        Currently showcase has buttonGroup examples which show the buttons in columns of a dataTable as well as in rows for ui:repeat.
        these examples (now) show the ability to change the number of rows. Initially the number of rows is set to 10, but you can increase to 100, 500 and 1000. These screenshots show the results of 1000 for virtual-vm and the js console shows the example for by row (using ui:repeat) as it doesn't use the dataTable which may also show some performance lag.
        for example, the by row example is at showcase (myIP):-

        http://10.18.39.137:8080/showcase/showcase.jsf?grp=ace:buttonGroup&exp=Group%20In%20Row

        ALSO...if js is the issue--js could only toggle off the previously selected button instead of all the other buttons... (500 rows in dataTable takes about 5 seconds but only has 2 radioButtons and 2 checkboxButtons per row). 500 rows in row example takes about 8seconds but has 3 radioButtons and 3 checkboxButtons per row.
         
        Ken Fyten made changes -
        Fix Version/s 4.2 [ 12870 ]
        Ken Fyten made changes -
        Assignee Priority P2 [ 10011 ]
        Ken Fyten made changes -
        Summary analysis of radioButton and checkboxButton in dataTable and ui:repeat Performance analysis of radioButton and checkboxButton in dataTable and ui:repeat
        Hide
        Mircea Toma added a comment -

        It turns out that the bottleneck is caused by the inline scripts rendered by radio and checkbox buttons, more precisely the <script> elements not the event handlers. This is mainly Mojarra's fault since their implementation of evaluating the scripts is slow, the Mojarra bridge adds the script element to the document to be evaluated and then it removes it.

        In the case of the checkbox button the ice.ace.checkboxbutton.register is invoked in the inline script. The function records which checkbox buttons are rendered so that later on it can implement the mutuallyExclusive feature. To avoid having the inline script we could invoke ice.ace.checkboxbutton.register function only in the initialization code (which is lazily executed).
        I have not found any cleanup code for the lists used ice.ace.checkboxbutton.register. This should be added since the list can change their content dynamically and any object accumulation along the time should be avoided.

        In the case tof the radio button I don't see lazy loading being implemented. Having the lazy loading implmented it would move the intialization code from the script element into the "onfocus" event handler, and thus workaround the bottleneck.

        Show
        Mircea Toma added a comment - It turns out that the bottleneck is caused by the inline scripts rendered by radio and checkbox buttons, more precisely the <script> elements not the event handlers. This is mainly Mojarra's fault since their implementation of evaluating the scripts is slow, the Mojarra bridge adds the script element to the document to be evaluated and then it removes it. In the case of the checkbox button the ice.ace.checkboxbutton.register is invoked in the inline script. The function records which checkbox buttons are rendered so that later on it can implement the mutuallyExclusive feature. To avoid having the inline script we could invoke ice.ace.checkboxbutton.register function only in the initialization code (which is lazily executed). I have not found any cleanup code for the lists used ice.ace.checkboxbutton.register . This should be added since the list can change their content dynamically and any object accumulation along the time should be avoided. In the case tof the radio button I don't see lazy loading being implemented. Having the lazy loading implmented it would move the intialization code from the script element into the "onfocus" event handler, and thus workaround the bottleneck.
        Hide
        Mircea Toma added a comment - - edited

        Modified ace:checkBoxButton to save initialization code in the data-init attribute of the component's root element to be used in multiple places (3 places to be exact) when widget needs to be lazy loaded. Factored out ice.ace.registerLazyComponent and ice.ace.evalInit utility functions to minimize rendered markup. Use JavaScriptRunner to render inline code to improve performance.

        Show
        Mircea Toma added a comment - - edited Modified ace:checkBoxButton to save initialization code in the data-init attribute of the component's root element to be used in multiple places (3 places to be exact) when widget needs to be lazy loaded. Factored out ice.ace.registerLazyComponent and ice.ace.evalInit utility functions to minimize rendered markup. Use JavaScriptRunner to render inline code to improve performance.
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #49395 Mon Oct 24 14:50:34 MDT 2016 mircea.toma ICE-11155 Save initialization code in 'data-init' to be used in multiple places when widget needs to be lazy loaded. Factored out ice.ace.registerLazyComponent and ice.ace.evalInit utility functions to minimize rendered markup. Use JavaScriptRunner to render inline code to improve performance.
        Files Changed
        Commit graph MODIFY /icefaces4/trunk/icefaces/ace/component/src/org/icefaces/ace/component/checkboxbuttons/CheckboxButtonsRenderer.java
        Commit graph MODIFY /icefaces4/trunk/icefaces/ace/component/resources/icefaces.ace/core/core.js
        Commit graph MODIFY /icefaces4/trunk/icefaces/ace/component/src/org/icefaces/ace/component/checkboxbutton/CheckboxButtonRenderer.java
        Hide
        Mircea Toma added a comment -

        Modified ace:radioButton renderer to lazy initialize the client-side component code. Also implemented the same strategies for minimizing rendered inline code.

        Show
        Mircea Toma added a comment - Modified ace:radioButton renderer to lazy initialize the client-side component code. Also implemented the same strategies for minimizing rendered inline code.
        Hide
        Mircea Toma added a comment -

        Cleanup client IDs from the registered checkbox map when the components are removed from DOM.

        Show
        Mircea Toma added a comment - Cleanup client IDs from the registered checkbox map when the components are removed from DOM.
        Mircea Toma made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #49402 Mon Oct 24 16:17:08 MDT 2016 mircea.toma ICE-11155 Cleanup client IDs from the registered checkbox map when the components are removed from DOM.
        Files Changed
        Commit graph MODIFY /icefaces4/trunk/icefaces/ace/component/resources/icefaces.ace/checkboxbutton/checkboxbutton.js
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #49403 Tue Oct 25 05:16:54 MDT 2016 mircea.toma ICE-11155 Register checkboxes or cleanup callbacks only once.
        Files Changed
        Commit graph MODIFY /icefaces4/trunk/icefaces/ace/component/resources/icefaces.ace/checkboxbutton/checkboxbutton.js
        Commit graph MODIFY /icefaces4/trunk/icefaces/ace/component/resources/icefaces.ace/core/core.js
        Hide
        Liana Munroe added a comment -

        Tested with ICEfaces 4 trunk r49407, Tomcat 8, IE 11, FF 47 Chrome 54
        Issues found:
        1.) ace:radioButton apps that are not using a dataTable or repeat structure are not functioning properly. mutuallyExclusive = true but is not respected.
        AN example of this can be found in the showcase ace:radioButton > Overview and Custom Style apps.
        To reproduce:
        Open either of the radioButton showcase demos and select the unselected radioButton. Both buttons stay selected. After they are both selected you can select one to force it to deselect (not proper behaviour). After that they become mutually exclusive.

        2.) ace:radioButtons no longer retain their itemValue once selected. They return false instead. This can be seen in the showcase ace:radioButtons > Overview.
        When first loading the demo radioButton One is selected, the value shown is "The selected radios are: One". After selecting any other radioButton the value shown is "The selected radios are: false". This issue is seen whether or not the radioButtons are inside a dataTable or repeat structure.

        3.) Showcase ace:checkboxButtons > all demos fail. No buttons can be selected. There are console errors when loading the demo
        Chrome: coalesced.js.jsf?ln=ice.core&dgst=kbiglm:14029 Uncaught TypeError: Cannot read property 'init' of undefined(…)
        FF:
        TypeError: a.dataset is undefined

        Show
        Liana Munroe added a comment - Tested with ICEfaces 4 trunk r49407, Tomcat 8, IE 11, FF 47 Chrome 54 Issues found: 1.) ace:radioButton apps that are not using a dataTable or repeat structure are not functioning properly. mutuallyExclusive = true but is not respected. AN example of this can be found in the showcase ace:radioButton > Overview and Custom Style apps. To reproduce: Open either of the radioButton showcase demos and select the unselected radioButton. Both buttons stay selected. After they are both selected you can select one to force it to deselect (not proper behaviour). After that they become mutually exclusive. 2.) ace:radioButtons no longer retain their itemValue once selected. They return false instead. This can be seen in the showcase ace:radioButtons > Overview. When first loading the demo radioButton One is selected, the value shown is "The selected radios are: One". After selecting any other radioButton the value shown is "The selected radios are: false". This issue is seen whether or not the radioButtons are inside a dataTable or repeat structure. 3.) Showcase ace:checkboxButtons > all demos fail. No buttons can be selected. There are console errors when loading the demo Chrome: coalesced.js.jsf?ln=ice.core&dgst=kbiglm:14029 Uncaught TypeError: Cannot read property 'init' of undefined(…) FF: TypeError: a.dataset is undefined
        Liana Munroe made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #49430 Mon Oct 31 15:05:13 MDT 2016 mircea.toma ICE-11155 Remove debugging code.
        Files Changed
        Commit graph MODIFY /icefaces4/trunk/icefaces/ace/component/resources/icefaces.ace/checkboxbutton/checkboxbutton.js
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #49431 Mon Oct 31 15:06:17 MDT 2016 mircea.toma ICE-11155 Fixed ice.ace.registerLazyComponent to return the widget reference when just created.
        Files Changed
        Commit graph MODIFY /icefaces4/trunk/icefaces/ace/component/resources/icefaces.ace/core/core.js
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #49432 Mon Oct 31 15:18:49 MDT 2016 mircea.toma ICE-11155 Modify CheckboxButtonsRenderer to replicate what CheckboxButtonRenderer is doing.
        Files Changed
        Commit graph MODIFY /icefaces4/trunk/icefaces/ace/component/src/org/icefaces/ace/component/checkboxbuttons/CheckboxButtonsRenderer.java
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #49433 Mon Oct 31 15:21:42 MDT 2016 mircea.toma ICE-11155 Add ice.ace.radiobutton.register to be used by the lazy loading of the radio button. Modify RadioButtonsRenderer to replicate what RadioButtonRenderer is doing.
        Files Changed
        Commit graph MODIFY /icefaces4/trunk/icefaces/ace/component/src/org/icefaces/ace/component/radiobutton/RadioButtonRenderer.java
        Commit graph MODIFY /icefaces4/trunk/icefaces/ace/component/src/org/icefaces/ace/component/radiobuttons/RadioButtonsRenderer.java
        Commit graph MODIFY /icefaces4/trunk/icefaces/ace/component/resources/icefaces.ace/radiobutton/radiobutton.js
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #49434 Mon Oct 31 15:40:31 MDT 2016 mircea.toma ICE-11155 Avoid using inherited RadioButtonRenderer.getConvertedValue method since the value is always cast to a boolean.
        Files Changed
        Commit graph MODIFY /icefaces4/trunk/icefaces/ace/component/src/org/icefaces/ace/component/radiobuttons/RadioButtonsRenderer.java
        Hide
        Mircea Toma added a comment -

        1) Fixed ice.ace.registerLazyComponent to return the widget reference when just created.

        2) Modified CheckboxButtonsRenderer to replicate what scripts are rendered by CheckboxButtonRenderer after the recent changes. Same was done for RadioButtonsRenderer. I didn't realize how little code is reussed by the buttons renderers.

        3) Changed RadioButtonsRenderer to avoid using the inherited RadioButtonRenderer.getConvertedValue method since the value is always cast to a boolean. This issue was actually a regression introduced by ICE-11153 refactoring.

        Show
        Mircea Toma added a comment - 1) Fixed ice.ace.registerLazyComponent to return the widget reference when just created. 2) Modified CheckboxButtonsRenderer to replicate what scripts are rendered by CheckboxButtonRenderer after the recent changes. Same was done for RadioButtonsRenderer . I didn't realize how little code is reussed by the buttons renderers. 3) Changed RadioButtonsRenderer to avoid using the inherited RadioButtonRenderer.getConvertedValue method since the value is always cast to a boolean. This issue was actually a regression introduced by ICE-11153 refactoring.
        Mircea Toma made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Mircea Toma made changes -
        Attachment non-optimized.png [ 22363 ]
        Attachment optimized.png [ 22364 ]
        Hide
        Mircea Toma added a comment - - edited

        Here are the optimisation results. The profiling was run during the switching from 10 to 1000 rows in showcase > buttonGroup > Group In Column demo.

        Before optimization the measured interval (using visual clues) was 8 seconds. The profiling shows that our JS scripts took about 5 seconds.

        After optimization the measured interval (using visual cues) was 2 seconds. The profiling show that only 890 milliseconds was used by JS, 360 milliseconds was the actual time used by our code.

        Show
        Mircea Toma added a comment - - edited Here are the optimisation results. The profiling was run during the switching from 10 to 1000 rows in showcase > buttonGroup > Group In Column demo. Before optimization the measured interval (using visual clues) was 8 seconds. The profiling shows that our JS scripts took about 5 seconds. After optimization the measured interval (using visual cues) was 2 seconds. The profiling show that only 890 milliseconds was used by JS, 360 milliseconds was the actual time used by our code.
        Ken Fyten made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Mircea Toma added a comment - - edited

        1) I don't see any difference in behaviour from EE 4.1.0 GA release. One still needs to click on "Submit" button (after "Clear" button is clicked) to actually clear the outputText that renders the selected item. This is needed because the "Clear" button does not issue a submit, it just clears the radio buttons locally.

        2) This is again a regression caused by ICE-11153 commits. The fix restores RadioButtonsRenderer.renderResetSettings method to properly invoke the ice.ace.radionbutton.clear function for multiple selection (instead of relying on the inherited method meant for RadioButton).

        Show
        Mircea Toma added a comment - - edited 1) I don't see any difference in behaviour from EE 4.1.0 GA release. One still needs to click on "Submit" button (after "Clear" button is clicked) to actually clear the outputText that renders the selected item. This is needed because the "Clear" button does not issue a submit, it just clears the radio buttons locally. 2) This is again a regression caused by ICE-11153 commits. The fix restores RadioButtonsRenderer.renderResetSettings method to properly invoke the ice.ace.radionbutton.clear function for multiple selection (instead of relying on the inherited method meant for RadioButton ).
        Mircea Toma made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #49448 Thu Nov 03 06:57:50 MDT 2016 mircea.toma ICE-11155 Restore RadioButtonsRenderer.renderResetSettings method to properly invoke the ice.ace.radionbuttons.clear function for multiple selection (instead of relying on the inherited method meant for RadioButton).
        Files Changed
        Commit graph MODIFY /icefaces4/trunk/icefaces/ace/component/src/org/icefaces/ace/component/radiobuttons/RadioButtonsRenderer.java
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #49449 Thu Nov 03 08:15:42 MDT 2016 mircea.toma ICE-11155 Optimized imports.
        Files Changed
        Commit graph MODIFY /icefaces4/trunk/icefaces/ace/component/src/org/icefaces/ace/component/radiobuttons/RadioButtonsRenderer.java
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #49633 Tue Dec 06 17:07:16 MST 2016 art.zambrano ICE-9500, ICE-11155 added inline documentation regarding lazy registry, to avoid confusion
        Files Changed
        Commit graph MODIFY /icefaces4/trunk/icefaces/ace/component/resources/icefaces.ace/core/core.js
        Ken Fyten made changes -
        Fix Version/s 4.2.BETA [ 13091 ]
        Ken Fyten made changes -
        Affects Version/s EE-4.1.0.GA [ 12171 ]
        Affects Version/s EE-4.1.1.BETA [ 13079 ]
        Ken Fyten made changes -
        Security Private [ 10001 ]
        Ken Fyten made changes -
        Affects Version/s 4.0 [ 11382 ]
        Ken Fyten made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Mircea Toma
            Reporter:
            Judy Guglielmin
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: