PDA

View Full Version : Ext.forEach



LesJ
8 Feb 2012, 7:04 AM
I noticed that Ext.each is used very frequently. It's a very convenient and flexible way to iterate, but it is also slow.

I suggest adding this mapping and using Ext.forEach instead of Ext.each.


Ext.forEach = ExtArray.forEach

forEach is faster because it maps (in newer browsers) to native forEach. I checked the Ext code base... in many (if not almost all) places each could be replaced by faster forEach because we don't check the return value of the callback.

This shortcut would also save a few bytes because it could replace Ext.Array.forEach in a number of places

mitchellsimoens
8 Feb 2012, 7:43 AM
Ext.each is also slow, executing a function for every array/object iteration ("function iteration") comes at a cost and we are looking at removing it all internally. I have already gone through and done this but it's a massive change that needs to be tested and performance analyzed.

LesJ
8 Feb 2012, 11:00 AM
Ext.each is also slow, executing a function for every array/object iteration ("function iteration") comes at a cost and we are looking at removing it all internally. I have already gone through and done this but it's a massive change that needs to be tested and performance analyzed.

I found a post (http://triin.net/2011/02/27/Optimizing_JavaScript_array_iteration%20)that disagrees with this approach...

We really shouldn't put our efforts into doing that kind of micro-optimizations by hand. Instead we should build a JavaScript compiler that would do that kind of transformations for us.

evant
8 Feb 2012, 12:19 PM
Ideally, yes. Practically, not quite there yet! http://triin.net/2011/03/02/Optimizing_forEach:_Not_so_fast!

LesJ
10 Feb 2012, 5:58 AM
Ext.each is also slow, executing a function for every array/object iteration ("function iteration") comes at a cost and we are looking at removing it all internally. I have already gone through and done this but it's a massive change that needs to be tested and performance analyzed.

I found an Ext.each that should be converted to a plain for loop.


Ext.define('Ext.util.AbstractMixedCollection', {
...
removeAll : function(items){
Ext.each(items || [], function(item) {
this.remove(item);
}, this);

return this;
},
...

mschwartz
10 Feb 2012, 6:59 AM
If the 90/10 rule is really true, then 90% of these loops should be Ext.each() for maintainability and readability.

~o)

LesJ
10 Feb 2012, 7:05 AM
If the 90/10 rule is really true, then 90% of these loops should be Ext.each() for maintainability and readability.

~o)

You got this half right.;)

These loops should be Ext.forEach, not Ext.each.

Ext.forEach is much faster compared to Ext.each

mschwartz
10 Feb 2012, 7:26 AM
I'm not an Ext4 user yet, though I've dabbled a little.

Ideally, a forEach() type function should work on both arrays and objects.

For example:



var a = [ 1,2,3];
forEach(a, function(value, key) {
console.log(value);
});
// =>
// 1
// 2
// 3

var o = { foo: 'bar', baz: 'zaz' };
forEach(o, function(value, key) {
console.log(key + ' = ' + value);
});
// =>
// foo = bar
// baz = zaz

mitchellsimoens
10 Feb 2012, 7:29 AM
I personally wouldn't use Ext.each... Ext.forEach... store.each...

They all are going to execute a function for every iteration that is unneeded. Why not just do a for loop where possible?

LesJ
10 Feb 2012, 7:42 AM
I personally wouldn't use Ext.each... Ext.forEach... store.each...

They all are going to execute a function for every iteration that is unneeded. Why not just do a for loop where possible?

Performance-wise:

for loop > Ext.forEach > Ext.each

But, for loops are more prone to bugs and the code is harder to understand and maintain.

I'd use for loops only in performance critical code.

LesJ
10 Feb 2012, 10:00 AM
The cleaner and nicer the program, the faster it's going to run. And if it doesn't, it'll be easy to make it fast.

- Joshua Bloch (Google)

LesJ
14 Feb 2012, 5:27 AM
Ideally, yes. Practically, not quite there yet! http://triin.net/2011/03/02/Optimizing_forEach:_Not_so_fast!

There must be a way to do this sort of conversion. CoffeeScript converts their loops into traditional JavaScript loops, see this (http://coffeescript.org/#loops).

mschwartz
14 Feb 2012, 6:38 AM
There must be a way to do this sort of conversion. CoffeeScript converts their loops into traditional JavaScript loops, see this (http://coffeescript.org/#loops).

I'm just getting into CoffeeScript, having implemented it in SilkJS.

It seems like a lot of work to wrap ExtJS with CoffeeScript, but it could be interesting in the end. If you're just using client-side, though, it adds another build step and I think it might be difficult to debug generated JavaScript in the browser...

LesJ
21 Feb 2012, 9:37 AM
I found an Ext.each that should be converted to a plain for loop.


Ext.define('Ext.util.AbstractMixedCollection', {
...
removeAll : function(items){
Ext.each(items || [], function(item) {
this.remove(item);
}, this);

return this;
},
...

Fixed in nightly:


removeAll : function(items) {
items = [].concat(items);
var i, iLen = items.length;
for (i = 0; i < iLen; i++) {
this.remove(items[i]);
}

return this;
}

mitchellsimoens
21 Feb 2012, 9:50 AM
There are quite a lot of function iteration fixes we have in. Not all of mine mad it in I don't think but quite a few.