PDA

View Full Version : [FIXED-251][3.x] _getCacheIndex really inefficient



Condor
31 Aug 2009, 7:24 AM
After reading this post (http://extjs.com/forum/showthread.php?t=79129) I had a quick look at the current ext-base-event.js file.

I agree with the suggestions of the OP, but the changes are probably too big to make in the current Ext 3.0 branch.

I did however also notice that _getCacheIndex is really inefficient: It doesn't stop after finding the listener.

Suggested fix:

function _getCacheIndex(el, eventName, fn) {
var index = -1;
Ext.each(listeners, function (v,i) {
if(v && v[FN] == fn && v[EL] == el && v[TYPE] == eventName) {
index = i;
return false;
}
});
return index;
}

hendricd
3 Sep 2009, 9:15 AM
For that matter, Ext.each is too fat for that type of iteration. Should consider a reverse-iteration through the list, as well:



// private
function _getCacheIndex(el, eventName, fn) {
var index = -1;
var len=listeners.length;
for(var i=len-1; i>index; --i) {
var v=listeners[i];
if(v && v[FN] == fn && v[EL] == el && v[TYPE] == eventName) {
index = i;
break;
}
}
return index;
}
and removeListener has an optimization that purgeElement:



purgeElement : function(el, recurse, eventName) {
var me = this;
Ext.each( me.getListeners(el, eventName), function(v){
if(v){
me.removeListener(el, v.type, v.fn, v.index);
}
});

if (recurse && el && el.childNodes) {
Ext.each(el.childNodes, function(v){
me.purgeElement(v, recurse, eventName);
});
}
},

..does not take advantage of.

mystix
3 Sep 2009, 6:24 PM
really curious -- how does the reverse iteration help?

p.s. you could save on the extra vars to make it a wee bit thinner :)


function _getCacheIndex(el, eventName, fn) {
for (var index = -1, len = listeners.length, i = len - 1; i > index; --i) {
var v = listeners[i];
if (v && v[FN] == fn && v[EL] == el && v[TYPE] == eventName) {
index = i;
break;
}
}
return index;
}

Condor
3 Sep 2009, 10:04 PM
really curious -- how does the reverse iteration help?

_getCacheIndex is used by removeListener.

Now, what is more likely:
- You want to remove listeners from an element you created first (e.g. in onReady).
- You want to remove listeners from an element you created last.

IMHO the last one is more likely (e.g. throw-away windows etc.)

mystix
4 Sep 2009, 1:20 AM
got it. reverse iteration it is then. :)

Animal
4 Sep 2009, 1:33 AM
_getCacheIndex is used by removeListener.

Now, what is more likely:
- You want to remove listeners from an element you created first (e.g. in onReady).


On your specific point of last added being most likely to be removed, I'm with you.

A the general point about the onReady event handler...

You will have used { single: true } on your onReady listener, so it will not be there.

The next generation of the API docs for onReady recommends this. It's a once-only event, so no point clogging up the listeners array.

Condor
4 Sep 2009, 1:43 AM
You misunderstood. I didn't mean the registered onReady handler (that's not a DOM event).
I meant the DOM events that got registered (by Ext Components) when the page was loaded.

What do you guys think of the original concept proposed by the OP:

Instead of an array with registered event handlers, use an object with an array of event handlers for each element (keyed by the element id)?
It's probably a lot faster, but it fails if somebody modifies a dom id without notifying Ext.lib.Event.

Animal
4 Sep 2009, 1:51 AM
Suits me. I wouldn't change any element IDs.

Condor
4 Sep 2009, 1:56 AM
Ext itself modifies element ids (most noticeably in Ext.id(), but also in some other locations, e.g. Ext.Button.initButtonEl).

evant
4 Sep 2009, 2:02 AM
The other option would be to specify some other property on the element, eg:



el.extEventId = 'foo';


And have it generate in a similar manner to Ext.id().

Condor
4 Sep 2009, 2:11 AM
It pollutes the DOM a bit, but it does have the advantage that you can see from a DOM element if it has listeners attached.

hendricd
4 Sep 2009, 4:43 AM
really curious -- how does the reverse iteration help?



Indeed, the strategy was based on the potential for "LIFO action". ;)


... What do you guys think of the original concept proposed by the OP:

Instead of an array with registered event handlers, use an object with an array of event handlers for each element (keyed by the element id)?
It's probably a lot faster, but it fails if somebody modifies a dom id without notifying Ext.lib.Event.

Concerns I have with a strategy based solely on centralized Element hashes:

1) lib.Event is a STILL CLOSURED/PRIVATIZED singleton (bound to a single document instance).

2) id-hashes/arrays of listeners should be maintained on the Element instance of the dom reference of the HTMLElement to which it applies. (Then id changes can be handled with a higher degree of success.)

3) access to window.document is often restricted (by various browsers) for various reason, so assigning an 'id' to document may not be practical (or desirable). Ext.get(document) is a special instance of Element whose instance is NOT cached.

4) Flyweights would be a problem too, but (arguably) serve a purpose. 'Flies' do not assert an Id on the Elements they wrap. If you intended to set a listener on the those (using the hash model) you would require an id assertion on everything -- rendering the Flyweight pattern a 'no-gain' in the end.

Then, there is the 'garbage collector process'.... 8-|

So, pros, cons, performance issues prevail. ;)

evant
10 Sep 2009, 11:14 PM
On Niges point about Ext.onReady(), is there any reason for not adding single: true to the event declaration?



onDocumentReady : function(fn, scope, options){
options = Ext.apply({single: true}, options);
if(docReadyState){ // if it already fired
docReadyEvent.addListener(fn, scope, options);
docReadyEvent.fire();
docReadyEvent.clearListeners();
} else {
if(!docReadyEvent) initDocReady();
options.delay = options.delay || 1;
docReadyEvent.addListener(fn, scope, options);
}
}

Condor
10 Sep 2009, 11:51 PM
On Niges point about Ext.onReady(), is there any reason for not adding single: true to the event declaration?

It's completely irrelevant, because Ext always calls docReadyEvent.clearListeners() after docReadyEvent.fire(). So, even if you declare the event handler without single:true, it will still be removed after the first call.

evant
17 Sep 2009, 12:50 AM
Fix applied to svn in rev #65 for patch release 3.0.3.