Slow event registry removal with a large number of objects

[rad-pat]

trafficstars

As discussed in Gitter in May 2023, with dynamic theming enabled and a large number of LayoutItems created. A simple fix was not apparent so I am logging the issue so it is recorded.

The lookup of the events in the event registry becomes exponentially slow. This was noticed for the destruction of a window containing many controls, compounded by the fact that some other elements in the application were not being leaked, adding to the number of event registrations.

I built a simple repl, although this is not something you would likely find in an application, but it does highlight the inefficiency of the event lookup.

Repl: https://tinyurl.com/yujzyz2t This creates 30,000 TextFields, which is already slow to destroy (about 5 seconds), but if you bump the number up to 60,000 then it is very much slower to destroy (around 20 seconds).

The problem seems to be related to looping/manipulation of the event registry array for each event removal in this piece of code: https://github.com/qooxdoo/qooxdoo/blob/49d5fbcc322ed71a6ab8d998048705766ef2e731/source/class/qx/event/Manager.js#L702-L715

My suggestion to improve was to possibly store the event listeners in a map so they could be removed by key.

Comment from @derrell at the time was:

Yeah, I can see how that would happen. There may be some O(n^2) looping going on there. The good news is that it should be refactorable in a backward-compatible fashion, I think. I'll look into it. It'll be a bit of work, so not just a 1-day project.

And then:

Unfortunately, it can't be refactored in a BC fashion. It had appeared that the data structures for maintaining all of the listeners was a purely internal matter, but unfortunately, there is a public interface for retrieving all listeners, which simply returns the existing top-level data structure. Although there are very few uses of that interface within qooxdoo, because it's public, that interface could be used in users' apps, so it can't be changed except with a major version number bump.

[cboulanger]

@derrell What if we change the internal implementation to be a hash and the public interface returns an ad-hoc constructed array that would be identical to the current top-level data structure?

[oetiker]

@derrell What if we change the internal implementation to be a hash and the public interface returns an ad-hoc constructed array that would be identical to the current top-level data structure?

I think the problem @derrell saw, is that once the data structure is public, someone could make changes to it which would then not be reflected in the hash … BUT! I think there is a way out. We could use a Proxy Object to monitor changes to the exported array:

  // our backing array
  let array = ["a", "b", "c", "d"];

  // a proxy for our array
  let proxy = new Proxy(array, {
    deleteProperty(target, property) {
      delete target[property];
      console.log("Deleted %s", property);
      return true;
    },
    set(target, property, value, receiver) {      
      target[property] = value;
      console.log("Set %s to %o", property, value);
      return true;
    }
  });

  console.log("Set a specific index..");
  proxy[0] = "x";

  console.log("Add via push()...");
  proxy.push("z");

  console.log("Add/remove via splice()...");
  proxy.splice(1, 3, "y");

  console.log("Current state of array: %o", array);

If changes are made to the objects inside the array, this would not matter as the array would refer the same objects as the hash. But if new ones are added or some are removed, the proxy code could easily duplicate these changes in the hash.

[cboulanger]

@oetiker Very cool, I was not aware it takes such little code to create a proxy for an array

[johnspackman]

I think the problem @derrell saw, is that once the data structure is public, someone could make changes to it which would then not be reflected in the hash

That data structure in __listeners looks like it should be entirely internal to Qooxdoo - even though it is exposed via .getAllListeners(), I would not have much sympathy if someone modified it and caused a problem in their code.

The only other place in Qooxdoo that uses .getAllListeners() is the unit tests, and it would make sense if the unit tests is the only reason why the data structure was exposed.

The serializeListeners() function looks like the public API - that function documents the return value, where getAllListeners() does not.

IMHO we mark getAllListeners() as deprecated, maybe rename it to getAllListenersInternalUseOnly() or similar.