Wicket
  1. Wicket
  2. WICKET-5132

Evaluation of returned data (which includes alot of javascript) very slow after ajax call in IE10.

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 6.0.0
    • Fix Version/s: 6.7.0
    • Component/s: wicket
    • Environment:
      IE10

      Description

      We're using ajax to update a treetable. In IE10, when the ajax call returns the data, which contains alot of javascript code, processEvaluation in wicket-ajax-jquery.js stalls on var text = jQuery(node).text();
      In jQuery procedure Sizzle.getText is called, which eventually returns property elem.textContent. In IE10 this last statement takes about 26 seconds for our case while the same statement in chrome takes about 6 ms.

      It seems strange that IE10 takes this long to return the textContent of a node, but why is this necessary in the first place?
      The purpose of var text = jQuery(node).text() would be to get the text of the node. I think it would suffice to state var text = node.childNodes[0].nodeValue instead of var text = Query(node).text(), because the text is in the node itself, and it's not necessary to get the text from its descendants, because it has no descendants.

      In our case this modification improves the duration from 25915 ms to 8 ms in IE10.

        Activity

        Bas Veenema created issue -
        Hide
        Martin Grigorov added a comment -

        This report looks like it should be reported to jQuery project instead.

        Can you attach a sample response that takes that long time ? I'd like to reproduce this because it looks like a problem in your installation of IE10. I cannot imagine how such simple thing can take that long time. Have you tried with IE9 too ?

        Show
        Martin Grigorov added a comment - This report looks like it should be reported to jQuery project instead. Can you attach a sample response that takes that long time ? I'd like to reproduce this because it looks like a problem in your installation of IE10. I cannot imagine how such simple thing can take that long time. Have you tried with IE9 too ?
        Hide
        Bas Veenema added a comment -

        I'll attach the javascript that is in the response. Javascript part is being tuned... but this is the example that takes about 25 seconds to process in IE10.

        In IE9 there's no performance issue because elem.textContent is undefined, which results in traversing its children and eventually returning elem.nodeValue, which performs as expected.

        I understand what you're saying about reporting to the jQuery project instead, however, it seems a bit overkill to call a jQuery function to get the text from a node when you can get the text from the node directly.

        Maybe you can take a look at http://jsperf.com/xml-text-selection-jquery-vs-textcontent-vs-nodevalue. Performance of $(node).text() on my machine (using chrome) is 93% slower than node.firstChild.nodeValue.

        So unless of course there's a good reason to use jQuery's .text() it seems to me that it's a bit overkill for this particular usage.

        Show
        Bas Veenema added a comment - I'll attach the javascript that is in the response. Javascript part is being tuned... but this is the example that takes about 25 seconds to process in IE10. In IE9 there's no performance issue because elem.textContent is undefined, which results in traversing its children and eventually returning elem.nodeValue, which performs as expected. I understand what you're saying about reporting to the jQuery project instead, however, it seems a bit overkill to call a jQuery function to get the text from a node when you can get the text from the node directly. Maybe you can take a look at http://jsperf.com/xml-text-selection-jquery-vs-textcontent-vs-nodevalue . Performance of $(node).text() on my machine (using chrome) is 93% slower than node.firstChild.nodeValue. So unless of course there's a good reason to use jQuery's .text() it seems to me that it's a bit overkill for this particular usage.
        Hide
        Bas Veenema added a comment -

        response (only the javascript) which takes 25 secondes to process.

        Show
        Bas Veenema added a comment - response (only the javascript) which takes 25 secondes to process.
        Bas Veenema made changes -
        Field Original Value New Value
        Attachment response_script.txt [ 12577204 ]
        Hide
        Martin Grigorov added a comment -

        The reason to use jQuery is that IE is notorious of not following W3C standards and jQuery task is to handle browser inconsistencies.
        In your example you use node.textContent but in older IE versions there is no such property and one has to use node.text instead.
        I ran the jsperf comparison and I see the big benefit. The suggested improvement will be applied! I just need to install IE 7/8 here to test that they still work.

        Show
        Martin Grigorov added a comment - The reason to use jQuery is that IE is notorious of not following W3C standards and jQuery task is to handle browser inconsistencies. In your example you use node.textContent but in older IE versions there is no such property and one has to use node.text instead. I ran the jsperf comparison and I see the big benefit. The suggested improvement will be applied! I just need to install IE 7/8 here to test that they still work.
        Hide
        Bas Veenema added a comment -

        Great! Thanks!

        Show
        Bas Veenema added a comment - Great! Thanks!
        Hide
        Andrea Del Bene added a comment -

        @Martin
        I'm testing IE 7/8 right now. I will wrote the result as soon as possible.

        Show
        Andrea Del Bene added a comment - @Martin I'm testing IE 7/8 right now. I will wrote the result as soon as possible.
        Hide
        Andrea Del Bene added a comment -

        If used 'node.childNodes[0].nodeValue' in place of ' jQuery(node).text()' as suggested by Bas and I successfully tested it with IE7/8 and Firefox 20

        Show
        Andrea Del Bene added a comment - If used 'node.childNodes [0] .nodeValue' in place of ' jQuery(node).text()' as suggested by Bas and I successfully tested it with IE7/8 and Firefox 20
        Hide
        Martin Grigorov added a comment -

        Thanks, Andrea!

        In the JSPerf link Bas provided there is no usage of childNodes[0].nodeValue.
        Can you check with s = node.textContent and s = node.firstChild.nodeValue ?
        It would be nice if you run the test on IE 7 and 8 so we can see the results. Scroll to the bottom in the JSPerf page and you will see the current results for different browsers.

        Show
        Martin Grigorov added a comment - Thanks, Andrea! In the JSPerf link Bas provided there is no usage of childNodes [0] .nodeValue. Can you check with s = node.textContent and s = node.firstChild.nodeValue ? It would be nice if you run the test on IE 7 and 8 so we can see the results. Scroll to the bottom in the JSPerf page and you will see the current results for different browsers.
        Hide
        Andrea Del Bene added a comment -

        Unfortunately it is not possible running Bas's test with IE 7/8 because they don't define method createDocument (see http://help.dottoro.com/ljqlhagh.php). However, node.firstChild.nodeValue works fine with both 7 and 8.

        Show
        Andrea Del Bene added a comment - Unfortunately it is not possible running Bas's test with IE 7/8 because they don't define method createDocument (see http://help.dottoro.com/ljqlhagh.php ). However, node.firstChild.nodeValue works fine with both 7 and 8.
        Martin Grigorov made changes -
        Assignee Martin Grigorov [ mgrigorov ]
        Hide
        Martin Grigorov added a comment -

        From now on node.firstChild.nodeValue will be used.
        jQuery(node).text() will be used as a fallback if an error occurs. The fallback will be removed later if there are no issues reported.

        Thanks all for the help!

        Show
        Martin Grigorov added a comment - From now on node.firstChild.nodeValue will be used. jQuery(node).text() will be used as a fallback if an error occurs. The fallback will be removed later if there are no issues reported. Thanks all for the help!
        Martin Grigorov made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 6.7.0 [ 12323964 ]
        Resolution Fixed [ 1 ]
        Hide
        Damien Hollis added a comment -

        Martin,

        This change seems to be causing us problems. When we open a popup, we get the following error:

        ERROR: Wicket.Ajax.Call.processEvaluation: Exception evaluating javascript: SyntaxError: Unexpected end of input, text: (function()

        {var e = Wicket.$('ok198--ajax-indicator'); if (e != null && typeof(e.parentNode) != 'undefined') e.parentNode.removeChild(e);})();(function(){var e = Wicket.$('cancel199--ajax-indicator'); if (e != null && typeof(e.parentNode) != 'undefined') e.parentNode.removeChild(e);})();(function(){

        When I debug the javascript code I see that the result of evaluating node.firstChild.nodeValue is

        "(function(){var e = Wicket.$('ok198--ajax-indicator'); if (e != null && typeof(e.parentNode) != 'undefined') e.parentNode.removeChild(e);}

        )();(function()

        {var e = Wicket.$('cancel199--ajax-indicator'); if (e != null && typeof(e.parentNode) != 'undefined') e.parentNode.removeChild(e);})();(function(){"

        but evaluating jQuery(node).text() provides the expected javascript:

        "(function(){var e = Wicket.$('ok198--ajax-indicator'); if (e != null && typeof(e.parentNode) != 'undefined') e.parentNode.removeChild(e);})();(function(){var e = Wicket.$('cancel199--ajax-indicator'); if (e != null && typeof(e.parentNode) != 'undefined') e.parentNode.removeChild(e);}

        )();(function()

        {var e = Wicket.$('link19a--ajax-indicator'); if (e != null && typeof(e.parentNode) != 'undefined') e.parentNode.removeChild(e);}

        )();(function()

        {var e = Wicket.$('link19b--ajax-indicator'); if (e != null && typeof(e.parentNode) != 'undefined') e.parentNode.removeChild(e);}

        )();"

        So it seems that the two approaches do not produce the same result.

        I'm testing on Chrome but our users are reporting problems on a variety of browsers.

        Can this change be reverted?

        Show
        Damien Hollis added a comment - Martin, This change seems to be causing us problems. When we open a popup, we get the following error: ERROR: Wicket.Ajax.Call.processEvaluation: Exception evaluating javascript: SyntaxError: Unexpected end of input, text: (function() {var e = Wicket.$('ok198--ajax-indicator'); if (e != null && typeof(e.parentNode) != 'undefined') e.parentNode.removeChild(e);})();(function(){var e = Wicket.$('cancel199--ajax-indicator'); if (e != null && typeof(e.parentNode) != 'undefined') e.parentNode.removeChild(e);})();(function(){ When I debug the javascript code I see that the result of evaluating node.firstChild.nodeValue is "(function(){var e = Wicket.$('ok198--ajax-indicator'); if (e != null && typeof(e.parentNode) != 'undefined') e.parentNode.removeChild(e);} )();(function() {var e = Wicket.$('cancel199--ajax-indicator'); if (e != null && typeof(e.parentNode) != 'undefined') e.parentNode.removeChild(e);})();(function(){" but evaluating jQuery(node).text() provides the expected javascript: "(function(){var e = Wicket.$('ok198--ajax-indicator'); if (e != null && typeof(e.parentNode) != 'undefined') e.parentNode.removeChild(e);})();(function(){var e = Wicket.$('cancel199--ajax-indicator'); if (e != null && typeof(e.parentNode) != 'undefined') e.parentNode.removeChild(e);} )();(function() {var e = Wicket.$('link19a--ajax-indicator'); if (e != null && typeof(e.parentNode) != 'undefined') e.parentNode.removeChild(e);} )();(function() {var e = Wicket.$('link19b--ajax-indicator'); if (e != null && typeof(e.parentNode) != 'undefined') e.parentNode.removeChild(e);} )();" So it seems that the two approaches do not produce the same result. I'm testing on Chrome but our users are reporting problems on a variety of browsers. Can this change be reverted?
        Hide
        Martin Grigorov added a comment -

        Can you provide a test case ?

        Show
        Martin Grigorov added a comment - Can you provide a test case ?
        Martin Grigorov made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Emmeran Seehuber added a comment -

        I can confirm this problem. I just upgraded to Wicket 6.7.0 and got this error.

        Within my application i can reproduce this error very easy: Just open a (Ajax) ModalWindow and add many Javascript behaviors to it, so that the Javascript to evaluate is not too small (i.e. > 2 kb).

        The problem here is, that according to the Chrome "Network"-Tab the <evaluate> element has one CDATA. But in the DOM it is split (for whatever reason) into 3 CDATA nodes.

        jquery(node).text() will concat the text of all those nodes. "text = node.firstChild.nodeValue; " will not ....

        You need to put a loop here. Something like

        var text =""; var n = node.firstChild; while

        { text = text + n.nodeValue; n = n.nextSibling; }

        I'll downgrade to Wicket 6.6 till this is fixed.

        Show
        Emmeran Seehuber added a comment - I can confirm this problem. I just upgraded to Wicket 6.7.0 and got this error. Within my application i can reproduce this error very easy: Just open a (Ajax) ModalWindow and add many Javascript behaviors to it, so that the Javascript to evaluate is not too small (i.e. > 2 kb). The problem here is, that according to the Chrome "Network"-Tab the <evaluate> element has one CDATA. But in the DOM it is split (for whatever reason) into 3 CDATA nodes. jquery(node).text() will concat the text of all those nodes. "text = node.firstChild.nodeValue; " will not .... You need to put a loop here. Something like var text =""; var n = node.firstChild; while { text = text + n.nodeValue; n = n.nextSibling; } I'll downgrade to Wicket 6.6 till this is fixed.
        Hide
        Martin Grigorov added a comment -

        The issue will be solved with WICKET-5185.

        Show
        Martin Grigorov added a comment - The issue will be solved with WICKET-5185 .
        Martin Grigorov made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Martin Grigorov added a comment -

        The improvement is applied.
        Please test with 6.8.0-SNAPSHOT/7.0.0-SNAPSHOT if you can.

        https://git-wip-us.apache.org/repos/asf/wicket/repo?p=wicket.git;a=commitdiff;h=62562dc4581046ad268c11a66f0dc71d92ddf456

        Show
        Martin Grigorov added a comment - The improvement is applied. Please test with 6.8.0-SNAPSHOT/7.0.0-SNAPSHOT if you can. https://git-wip-us.apache.org/repos/asf/wicket/repo?p=wicket.git;a=commitdiff;h=62562dc4581046ad268c11a66f0dc71d92ddf456
        Hide
        Emmeran Seehuber added a comment -

        With the snapshot wicket-core-6.8.0-20130513.120220-37.jar it works for me. Thanks!

        Just a small nitpick: When using an old ancient browser (IE8 ...), concating text is slow. Using an array and join() is faster there ... I.e.

        var result = [];

        result.push(...);

        result.join("");

        (Und guess what browser one of my customers is using ...)

        Show
        Emmeran Seehuber added a comment - With the snapshot wicket-core-6.8.0-20130513.120220-37.jar it works for me. Thanks! Just a small nitpick: When using an old ancient browser (IE8 ...), concating text is slow. Using an array and join() is faster there ... I.e. var result = []; result.push(...); result.join(""); (Und guess what browser one of my customers is using ...)
        Hide
        Martin Grigorov added a comment -

        Good point, Emmeran!
        I'll improve it.

        Show
        Martin Grigorov added a comment - Good point, Emmeran! I'll improve it.
        Show
        Martin Grigorov added a comment - https://git-wip-us.apache.org/repos/asf/wicket/repo?p=wicket.git;a=commitdiff;h=3c7d371ea7cd787b0c931779c30e54d7b34b9f6d Improved. Thanks!
        Hide
        Nikolaus Kühn added a comment -

        To be honest, I'm a little confused whether the issue is solved or just the subtask.

        we are currently experiencing a massive performance problem with IE11 on Win8 when applying large (ca. 800kb) wicket ajax updates under wicket 6.8.0 release (ca 26 seconds to apply the server answer). The issue is in the IE11 implementation of .textcontent - if we switch the IE mode to 10 or 9 the issue is gone.

        Looks like there's no really clean way to circumvent this in the application code.

        Is this the same Issue or a new one? (this one is officially labeled IE10, but it's the same context of things).

        Show
        Nikolaus Kühn added a comment - To be honest, I'm a little confused whether the issue is solved or just the subtask. we are currently experiencing a massive performance problem with IE11 on Win8 when applying large (ca. 800kb) wicket ajax updates under wicket 6.8.0 release (ca 26 seconds to apply the server answer). The issue is in the IE11 implementation of .textcontent - if we switch the IE mode to 10 or 9 the issue is gone. Looks like there's no really clean way to circumvent this in the application code. Is this the same Issue or a new one? (this one is officially labeled IE10, but it's the same context of things).
        Hide
        Martin Grigorov added a comment -

        Can you identify which code exactly is slow in IE11 ?
        I see Wicket uses jQuery.text() in three other places. I can send you patched wicket-ajax-jquery.js that uses the same solution as for the above problem so you can test it locally ?

        Show
        Martin Grigorov added a comment - Can you identify which code exactly is slow in IE11 ? I see Wicket uses jQuery.text() in three other places. I can send you patched wicket-ajax-jquery.js that uses the same solution as for the above problem so you can test it locally ?
        Hide
        Nikolaus Kühn added a comment -

        It's a little hard to debug because the debugger itself interferes with the behavior (hover over "node" and it tries to generate a preview which calls .textcontent itself etc...).

        A more specialized colleague of mine is looking into the issue, too, but as far as I could find out this https://github.com/apache/wicket/blob/build/wicket-6.8.0/wicket-core/src/main/java/org/apache/wicket/ajax/res/js/wicket-ajax-jquery.js#L996 is the place where "it" happens. The access is still via jquery text() (which again uses Sizzle for getText) in the master repo.

        is there a chance that the issue is resolved with the update to jQuery 1.9 in newer wicket versions? We're currently hesitating from migrating, but if that fixes it we would.

        Greets and thank you!

        Show
        Nikolaus Kühn added a comment - It's a little hard to debug because the debugger itself interferes with the behavior (hover over "node" and it tries to generate a preview which calls .textcontent itself etc...). A more specialized colleague of mine is looking into the issue, too, but as far as I could find out this https://github.com/apache/wicket/blob/build/wicket-6.8.0/wicket-core/src/main/java/org/apache/wicket/ajax/res/js/wicket-ajax-jquery.js#L996 is the place where "it" happens. The access is still via jquery text() (which again uses Sizzle for getText) in the master repo. is there a chance that the issue is resolved with the update to jQuery 1.9 in newer wicket versions? We're currently hesitating from migrating, but if that fixes it we would. Greets and thank you!
        Hide
        Marcel Lucas added a comment -

        Colleague here - well it is indeed the fact that jquery getText() invokes the problematic .textContent lines:

        getText = Sizzle.getText = function( elem ) {
                ...
        	if ( nodeType ) {
        		if ( nodeType === 1 || nodeType === 9 || nodeType === 11 ) {
        			// Use textContent for elements
        			// innerText usage removed for consistency of new lines (see #11153)
        			if ( typeof elem.textContent === "string" ) {
        				return elem.textContent;
                                }
                         ...
                         }
                 ...
                }
        ...
        }
        

        In our case of the mentioned 800kb XML each access to .textContent takes at least 10 seconds+
        the call stack looks like this

        getText [Line: 3997, Col: 4], jquery-ver-1391007748983.js
        getText [Line: 4014, Col: 4], jquery-ver-1391007748983.js
        Anonymous function [Line: 5705, Col: 4], jquery-ver-1391007748983.js
        access [Line: 828, Col: 3], jquery-ver-1391007748983.js
        text [Line: 5704, Col: 3], jquery-ver-1391007748983.js
        parse [Line: 1873, Col: 6], wicket-ajax-jquery-ver-1391007748983.js
        processContribution [Line: 1906, Col: 6], wicket-ajax-jquery-ver-1391007748983.js
        processHeaderContribution [Line: 1123, Col: 4], wicket-ajax-jquery-ver-1391007748983.js
        loadedCallback [Line: 914, Col: 7], wicket-ajax-jquery-ver-1391007748983.js
        processAjaxResponse [Line: 729, Col: 6], wicket-ajax-jquery-ver-1391007748983.js
        success [Line: 610, Col: 7], wicket-ajax-jquery-ver-1391007748983.js
        fire [Line: 974, Col: 5], jquery-ver-1391007748983.js
        fireWith [Line: 1084, Col: 7], jquery-ver-1391007748983.js
        done [Line: 7803, Col: 5], jquery-ver-1391007748983.js
        callback [Line: 8518, Col: 8], jquery-ver-1391007748983.js
        

        thanks and regards,

        Marcel Lucas

        Show
        Marcel Lucas added a comment - Colleague here - well it is indeed the fact that jquery getText() invokes the problematic .textContent lines: getText = Sizzle.getText = function ( elem ) { ... if ( nodeType ) { if ( nodeType === 1 || nodeType === 9 || nodeType === 11 ) { // Use textContent for elements // innerText usage removed for consistency of new lines (see #11153) if ( typeof elem.textContent === "string" ) { return elem.textContent; } ... } ... } ... } In our case of the mentioned 800kb XML each access to .textContent takes at least 10 seconds+ the call stack looks like this getText [Line: 3997, Col: 4], jquery-ver-1391007748983.js getText [Line: 4014, Col: 4], jquery-ver-1391007748983.js Anonymous function [Line: 5705, Col: 4], jquery-ver-1391007748983.js access [Line: 828, Col: 3], jquery-ver-1391007748983.js text [Line: 5704, Col: 3], jquery-ver-1391007748983.js parse [Line: 1873, Col: 6], wicket-ajax-jquery-ver-1391007748983.js processContribution [Line: 1906, Col: 6], wicket-ajax-jquery-ver-1391007748983.js processHeaderContribution [Line: 1123, Col: 4], wicket-ajax-jquery-ver-1391007748983.js loadedCallback [Line: 914, Col: 7], wicket-ajax-jquery-ver-1391007748983.js processAjaxResponse [Line: 729, Col: 6], wicket-ajax-jquery-ver-1391007748983.js success [Line: 610, Col: 7], wicket-ajax-jquery-ver-1391007748983.js fire [Line: 974, Col: 5], jquery-ver-1391007748983.js fireWith [Line: 1084, Col: 7], jquery-ver-1391007748983.js done [Line: 7803, Col: 5], jquery-ver-1391007748983.js callback [Line: 8518, Col: 8], jquery-ver-1391007748983.js thanks and regards, Marcel Lucas
        Hide
        Martin Grigorov added a comment -

        I've created https://issues.apache.org/jira/browse/WICKET-5510.
        Please add yourselves as watchers. I'll attach an improved wicket-ajax-jquery.js so you can test it.
        Thanks!

        Show
        Martin Grigorov added a comment - I've created https://issues.apache.org/jira/browse/WICKET-5510 . Please add yourselves as watchers. I'll attach an improved wicket-ajax-jquery.js so you can test it. Thanks!
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        3d 22h 53m 1 Martin Grigorov 09/Apr/13 08:08
        Resolved Resolved Reopened Reopened
        22d 22h 18m 1 Martin Grigorov 02/May/13 06:26
        Reopened Reopened Resolved Resolved
        11d 4h 55m 1 Martin Grigorov 13/May/13 11:22

          People

          • Assignee:
            Martin Grigorov
            Reporter:
            Bas Veenema
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 2h
              2h
              Remaining:
              Remaining Estimate - 2h
              2h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development