ICEfaces
  1. ICEfaces
  2. ICE-5156

Improve performance for JS event listener cleanup

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.2, 1.8.2-EE-Beta
    • Fix Version/s: 1.8.2-EE-GA, 1.8.3
    • Component/s: Bridge, Framework
    • Labels:
      None
    • Environment:
      browser
    • Affects:
      Documentation (User Guide, Ref. Guide, etc.)

      Description

      Introduce heuristic in the event listener cleanup process that will cleanup only the listeners that are most often used by the components. This way the process will take much less time with the hope that ~90% of the registered listeners are cleaned up.

        Activity

        Hide
        Mircea Toma added a comment -

        Implemented partial listener cleanup. Only the following listeners are cleaned up: onkeypress, onmousedown, onmousemove, onmouseout, onmouseover, onclick, oncontextmenu, onchange, onfocus, onblur.

        Introduced "com.icesoft.faces.fullJSListenerCleanup" context parameter that can be used to turn back on a full listener cleanup. By default the parameter is set to false.

        Show
        Mircea Toma added a comment - Implemented partial listener cleanup. Only the following listeners are cleaned up: onkeypress, onmousedown, onmousemove, onmouseout, onmouseover, onclick, oncontextmenu, onchange, onfocus, onblur. Introduced "com.icesoft.faces.fullJSListenerCleanup" context parameter that can be used to turn back on a full listener cleanup. By default the parameter is set to false.
        Hide
        Ken Fyten added a comment -

        Adnan reports the following:

        I have listed the listeners that component render renders in the markup. Components using scriptaculos or Custom JS such as D&D or GMap must be registering dynamic listeners which would be lengthy to find out. Let me know if we should look at particular js libraries too.

        • Autocomplete
        • onmousedown
        • onfocus (focus management)
        • onblur (focus management)
        • selectInputDate + Popup
        • onkeypress (foir submit)
        • onfocus (focus management)
        • onblur (focus management)
        • Drag &Drop
        • onmousemove (create draggable)
        • onmouseout (remove draggable)

        *MenuBar
        -onmouseout (hide menus)

        • onmosueover (didn't see in the markup, but its obvious that JS creating it)
        • onclick (didn't see in the markup, but its obvious that JS creating it)

        *MenuPopup

        • oncontextmenu

        *inputRichText

        • onmouseover (set active editor)
        • onmouseout (set unsaved data)

        *Tree:

        • Tree uses commandLink, so same attributes as commandLink

        *rowSelector

        • onmouseover (set class)
        • onmouseout (enable text selection)
        • onclick (row click)

        *commandSortHeader

        • uses commandLink, so same attributes as commandLink

        *collapsiblePanel:

        • onclick (flip panel selection)
        • onfocus (focus management)
        • onblur (focus management)

        *Tabset
        -onmouseover (selected tab class)
        -onmouseout (deselect tab)

        • onfocus (focus management)
        • onblur (focus management)
        • tooltip:
          -onmouseover (to show tooltip)

        *panelConfirmation:

        • onclick (open confirmation)
        • onfocus (focus management)
        • onblur (focus management)
        Show
        Ken Fyten added a comment - Adnan reports the following: I have listed the listeners that component render renders in the markup. Components using scriptaculos or Custom JS such as D&D or GMap must be registering dynamic listeners which would be lengthy to find out. Let me know if we should look at particular js libraries too. Autocomplete onmousedown onfocus (focus management) onblur (focus management) selectInputDate + Popup onkeypress (foir submit) onfocus (focus management) onblur (focus management) Drag &Drop onmousemove (create draggable) onmouseout (remove draggable) *MenuBar -onmouseout (hide menus) onmosueover (didn't see in the markup, but its obvious that JS creating it) onclick (didn't see in the markup, but its obvious that JS creating it) *MenuPopup oncontextmenu *inputRichText onmouseover (set active editor) onmouseout (set unsaved data) *Tree: Tree uses commandLink, so same attributes as commandLink *rowSelector onmouseover (set class) onmouseout (enable text selection) onclick (row click) *commandSortHeader uses commandLink, so same attributes as commandLink *collapsiblePanel: onclick (flip panel selection) onfocus (focus management) onblur (focus management) *Tabset -onmouseover (selected tab class) -onmouseout (deselect tab) onfocus (focus management) onblur (focus management) tooltip: -onmouseover (to show tooltip) *panelConfirmation: onclick (open confirmation) onfocus (focus management) onblur (focus management)
        Hide
        Ken Fyten added a comment -

        Here's a distilled list of the unique event listeners in use:

        • onmousedown
        • onfocus (focus management)
        • onblur (focus management)
        • onkeypress (foir submit)
        • onmousemove (create draggable)
        • onmouseout (remove draggable)
        • onmosueover (didn't see in the markup, but its obvious that JS creating it)
        • onclick (didn't see in the markup, but its obvious that JS creating it)
        • oncontextmenu

        So it looks like the current implemented heuristic should suffice.

        Show
        Ken Fyten added a comment - Here's a distilled list of the unique event listeners in use: onmousedown onfocus (focus management) onblur (focus management) onkeypress (foir submit) onmousemove (create draggable) onmouseout (remove draggable) onmosueover (didn't see in the markup, but its obvious that JS creating it) onclick (didn't see in the markup, but its obvious that JS creating it) oncontextmenu So it looks like the current implemented heuristic should suffice.
        Hide
        Ted Goddard added a comment -

        Recording alternate approach for consideration in ICEfaces 2.0:

        $element(element).eachListenerName(
        function(listenerName)

        { element[listenerName.toLowerCase()] = null; }

        );

        fast: listenerName records exactly which listeners to remove
        slow: use of functional iteration
        slow: use of closure
        slow: toLowerCase()

        The optimal strategy may be to process the list of known listeners more quickly rather than to clear a smaller set of listeners on a larger set of elements.

        Show
        Ted Goddard added a comment - Recording alternate approach for consideration in ICEfaces 2.0: $element(element).eachListenerName( function(listenerName) { element[listenerName.toLowerCase()] = null; } ); fast: listenerName records exactly which listeners to remove slow: use of functional iteration slow: use of closure slow: toLowerCase() The optimal strategy may be to process the list of known listeners more quickly rather than to clear a smaller set of listeners on a larger set of elements.
        Hide
        Ken Fyten added a comment -

        Upon review I think we should be defaulting to the comprehensive listener cleanup strategy (old one) and only enabling the new (approx. 30% faster) approach if the config. parameter is enabled.

        The rationale for this is that there is a real risk that some of our custom components (gmap, inputRichText) or the application itself could add JS listeners that are not covered by the faster static listener cleanup approach, resulting in a browser memory leak.

        For customers with existing apps. this will be interpreted as a bug in the EE release. I would rather have things slightly slower and not crashing from memory leaks out of the box, with the option of them turning on "com.icesoft.faces.optimizedJSListenerCleanup" mode prior to them completing extension application testing (which will uncover any issues), than have them attempt to switch and run into problems that appear to be mysterious browser memory leaks that were never present previously.

        Note the suggested name change for the config. param above also.

        Show
        Ken Fyten added a comment - Upon review I think we should be defaulting to the comprehensive listener cleanup strategy (old one) and only enabling the new (approx. 30% faster) approach if the config. parameter is enabled. The rationale for this is that there is a real risk that some of our custom components (gmap, inputRichText) or the application itself could add JS listeners that are not covered by the faster static listener cleanup approach, resulting in a browser memory leak. For customers with existing apps. this will be interpreted as a bug in the EE release. I would rather have things slightly slower and not crashing from memory leaks out of the box, with the option of them turning on "com.icesoft.faces.optimizedJSListenerCleanup" mode prior to them completing extension application testing (which will uncover any issues), than have them attempt to switch and run into problems that appear to be mysterious browser memory leaks that were never present previously. Note the suggested name change for the config. param above also.
        Hide
        Ted Goddard added a comment -

        Interesting technique used by google via a central event listener:

        http://code.google.com/p/google-web-toolkit/wiki/DomEventsAndMemoryLeaks

        Show
        Ted Goddard added a comment - Interesting technique used by google via a central event listener: http://code.google.com/p/google-web-toolkit/wiki/DomEventsAndMemoryLeaks
        Hide
        Mircea Toma added a comment -

        Disabled optimized listener cleanup by default. Changed configuration parameter name to "com.icesoft.faces.optimizedJSListenerCleanup".

        Show
        Mircea Toma added a comment - Disabled optimized listener cleanup by default. Changed configuration parameter name to "com.icesoft.faces.optimizedJSListenerCleanup".
        Hide
        Mohamed Amghari added a comment -

        hi,

        Please tell us where we should do this change?
        < Changed configuration parameter name to "com.icesoft.faces.optimizedJSListenerCleanup" >

        Thank you.

        Show
        Mohamed Amghari added a comment - hi, Please tell us where we should do this change? < Changed configuration parameter name to "com.icesoft.faces.optimizedJSListenerCleanup" > Thank you.

          People

          • Assignee:
            Unassigned
            Reporter:
            Mircea Toma
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: