Core: Ajax performance (cleanseDomElement)

[cocorossello]

Description

Since #11722 I can see a noticeable performance degradation in client side when there is an ajax and there are many inputs or DOM elements.

I can trace the problem down to the last remove in this function:

        cleanseDomElement: function(jq, clearData = true, removeElement = true) {
            if (!jq || !jq.length) {
                return;
            }
            // Recursively remove events from children elements
            jq.children().each(function() {
                PrimeFaces.utils.cleanseDomElement($(this), clearData, removeElement);
            });

            // Remove inline event attributes
            var attributes = jq[0].attributes;
            for (var i = 0; i < attributes.length; i++) {
                var attributeName = attributes[i].name;
                if (attributeName.startsWith("on")) {
                    jq.removeAttr(attributeName);
                }
            }
            // Remove event listeners
            jq.off();
            
            // Clear data
            if (clearData) {
                jq.removeData();
            }
            // Remove elements itself from memory
            if (removeElement) {
                jq.remove();
            }
        },

If I replace the last
jq.remove();
with
jq.each(function() {this.remove()});

I can see that performance improves very much. In my test I have an ajax that takes about 1.3seconds scripting and with the change it only takes 180ms.

Looks like a safe change, since children are removed before.
Do you think it's worthy to integrate this in PF?

Describe the solution you would like

CleanseDomElement should be faster. Of course another solution is to shrink the DOM tree to update, but there is room to improve cleanseDomElement

Additional context

No response

[melloware]

let me test this change with my client that has the major memory issues and get back to you. If it continues to clean up memory I will implement your change!

[melloware]

@cocorossello actually that conflicts with this recent change I made.

PR: #11998

Issue: #11994

The reason jq.remove() must be called is to make sure widgets register their destroyListeners when their DOM element is removed. Your code now skips that and in my testing the memory leaks are back.

In that ticket above with your fix:

Memory: 18.8 MB
After (with your fix): 15.5.MB with 6MB of detached elements leaking
After (with current master) 10.5 MB with no detached elements leaking

[cocorossello]

So the problem might be in some destroyListener, interesting

[melloware]

you can always just MonkeyPatch your code to override removeElement to false

cleanseDomElement: function(jq, clearData = true, removeElement = false) {

[tandraschko]

maybe you can identify a slow destroyListener
and even post some numbers, how long destroy a component (time + component type) takes
maybe we can even do async

if like all components are slow, we need to think about a config

[melloware]

@cocorossello that would be good because the client i am fixing this for has an INSANE amount of components on their page and tabs and I am not noticing much of a slowdown at all.

Probably almost 100+ inputs on their tab or pages each.

[melloware]

OK so I added timer to the AJAX update....

console.time('AJAX updateElement'); // Start the timer with a label
      var removed = target.replaceWith(content);
      PrimeFaces.utils.cleanseDomElement(removed);
console.timeEnd('AJAX updateElement'); // Stop the timer and print the elapsed time

And on my clients crazy pages I am seeing all sub 300ms executions.

AJAX updateElement: 1.2080078125 ms
VM43:117 AJAX updateElement: 69.680908203125 ms
VM43:117 AJAX updateElement: 210.800048828125 ms
VM43:117 AJAX updateElement: 35.72607421875 ms
VM43:117 AJAX updateElement: 47.4208984375 ms
VM43:117 AJAX updateElement: 63.093994140625 ms
VM43:117 AJAX updateElement: 32.046875 ms
VM43:117 AJAX updateElement: 22.20703125 ms

[tandraschko]

1m is without cleanse, all others with cleanse? or what are the numbers?

[melloware]

sorry that was just me clicking around through the app on heavy pages with AJAX updates. opening large dynamic tabs with tons of elements and closing them etc. the very first one. I was trying to see if I could get anywhere close to @cocorossello numbers and I have not seen it.

In fact this one...

AJAX updateElement: 203.463134765625 ms

Is recursively calling remove() on 11,615 DOM elements!

[melloware]

for example

AJAX updateElement: 63.39501953125 ms
Removed 11,475 DOM elements

[melloware]

@cocorossello if you want to do the same test just use these MonkeyPatches.

/**
 * Updates an element with the given ID by applying a change set that was returned by an AJAX request. This
 * involves replacing the HTML content of the element with the new content.
 * @param {string} id ID of the element that is to be updated.
 * @param {string} content The new content of the changeset as returned by an AJAX request.
 * @param {PrimeFaces.ajax.pfXHR} [xhr] Optional XHR request with `pfSettings` or `pfArgs` with further
 * data, such as which forms should be updated.
 */
PrimeFaces.ajax.Utils.updateElement = function (id, content, xhr) {
  if (id.indexOf(PrimeFaces.VIEW_STATE) !== -1) {
    PrimeFaces.ajax.Utils.updateFormStateInput(PrimeFaces.VIEW_STATE, content, xhr);
  } else if (id.indexOf(PrimeFaces.CLIENT_WINDOW) !== -1) {
    PrimeFaces.ajax.Utils.updateFormStateInput(PrimeFaces.CLIENT_WINDOW, content, xhr);
  }
  // used by @all
  else if (id === PrimeFaces.VIEW_ROOT) {
    // backup our utils, we reset it soon
    var ajaxUtils = PrimeFaces.ajax.Utils;

    // reset PrimeFaces JS state because the view is completely replaced with a new one
    window.PrimeFaces.resetState();

    ajaxUtils.updateHead(content);
    ajaxUtils.updateBody(content);
  } else if (id === PrimeFaces.ajax.VIEW_HEAD) {
    PrimeFaces.ajax.Utils.updateHead(content);
  } else if (id === PrimeFaces.ajax.VIEW_BODY) {
    PrimeFaces.ajax.Utils.updateBody(content);
  } else if (id === PrimeFaces.ajax.RESOURCE) {
    $("head").append(content);
  } else if (id === $("head")[0].id) {
    PrimeFaces.ajax.Utils.updateHead(content);
  } else {
    var target = $(PrimeFaces.escapeClientId(id));
    if (target.length === 0) {
      PrimeFaces.warn("DOM element with id '" + id + "' cant be found; skip update...");
    } else {
	  count = 0;
	  console.time('AJAX updateElement'); // Start the timer with a label
      var removed = target.replaceWith(content);
      PrimeFaces.utils.cleanseDomElement(removed);
	  console.timeEnd('AJAX updateElement'); // Stop the timer and print the elapsed time
	  console.log("Removed " + count +" DOM elements");
    }
  }
};

/**
 * Deletes all events, 'on' attributes, data, and the element itself in a recursive manner,
 * ensuring that the garbage collector does not retain any references to this element or its children.
 *
 * @param {JQuery | undefined} jq jQuery object to cleanse
 * @param {boolean} [clearData] flag to clear data off elements (default to true)
 * @param {boolean} [removeElement] flag to remove the element from DOM (default to true)
 * @see {@link https://github.com/primefaces/primefaces/issues/11696|GitHub Issue 11696}
 * @see {@link https://github.com/primefaces/primefaces/issues/11702|GitHub Issue 11702}
 */

PrimeFaces.utils.cleanseDomElement = function (jq, clearData = true, removeElement = true) {
  if (!jq || !jq.length) {
    return;
  }
  count++;
  
  // Recursively remove events from children elements
  jq.children().each(function () {
    PrimeFaces.utils.cleanseDomElement($(this), clearData, removeElement);
  });

  // Remove inline event attributes
  var attributes = jq[0].attributes;
  for (var i = 0; i < attributes.length; i++) {
    var attributeName = attributes[i].name;
    if (attributeName.startsWith("on")) {
      jq.removeAttr(attributeName);
    }
  }

  // Remove the element from the DOM and trigger onRemove events for widget.destroy.
  // IMPORTANT: This must occur before jq.off() to ensure the on("remove") events remain registered.
  if (removeElement) {
    jq.remove();
  }

  // Clear data
  if (clearData) {
    jq.removeData();
  }

  // Remove event listeners
  jq.off();
};

[cocorossello]

I can see the problem now.

The page itself has a (long) list of fees (money input) that the user can modify. The page has 380 money input. Each money input has a currency selector (<h:selectOneMenu) and each select has 80 options. So that makes 30k options to cleanse when the whole list of fees have to be updated:

AJAX updateElement: 115.52197265625 ms
common.js:758 Removed 269 DOM elements
common.js:757 AJAX updateElement: 2.77587890625 ms
common.js:758 Removed 1 DOM elements
common.js:757 AJAX updateElement: 2680.80810546875 ms
common.js:758 Removed 33208 DOM elements

(its still much slower than your 63ms for around 11k elements, I have no idea why. It is a chrome without extensions in a good machine)

Maybe I should exclude those plain html elements that cannot have listeners or even children from the cleanse

[melloware]

33,000 DOM elements 😨

[cocorossello]

Yes, 30.000 of those are options. Excluding them by tagname from the jquery remove works, I can can just do the patch myself, this should not be a problem in general. Should I close the issue?

[melloware]

Let me know how you exclude them. I think it's worth adding to this patch as I doubt options will ever be the reason for a leak.

[melloware]

Can you provide a PR with your exclusion fix?