Skip to main content

Bug Tracker

Side navigation

#2619 closed enhancement (fixed)

Opened March 29, 2008 08:04PM UTC

Closed April 28, 2008 01:55PM UTC

Extend makeArray

Reported by: flesler Owned by:
Priority: minor Milestone: 1.2.4
Component: core Version: 1.2.3
Keywords: makearray array Cc:
Blocked by: Blocking:
Description

Hi, don't you think makeArray should be a bit smarter?

Instead of doing $.makeArray( foo || [] ) all the time, it should handle that internally.

$.fn.setArray does a lot of checks that IMO, belong to $.makeArray.

By improving it, you can remove those pesky checks from many places.

This version handles pretty much any kind of argument, without using too many checks.

I know that internal functions have to as fast as possible, I optimized this as much as I could, but well, in the end, it's up to you (core team).

Attachments (1)
  • makeArray.diff (0.7 KB) - added by flesler March 29, 2008 08:07PM UTC.
Change History (13)

Changed March 29, 2008 08:11PM UTC by flesler comment:1

The check for array.call can be removed, if there's no interest on supporting functions, I just added because it may happen.

!array.split can be changed for typeof array != 'string' if object detection is not the best option.

Changed March 29, 2008 08:13PM UTC by flesler comment:2

I made a swift test runner, and after a few fixes (specially for safari), they all passed on IE6, FF2, Opera 9 and Safari 3.

These are the tests:

//nodelist
var scripts = document.getElementsByTagName('script');
if( $.makeArray(scripts).slice(0,1)[0].nodeName != 'SCRIPT' )
	alert('error-1');

//arguments	
if( (function(){ return $.makeArray(arguments); })(1,2).join('') != '12' )
	alert('error-2');

//real array
if( $.makeArray([1,2,3]).join('') != '123' )
	alert('error-3');

//empty
if( $.makeArray().length != 0 )
	alert('error-4');
	
//number
if( $.makeArray( 0 )[0] != 0 )
	alert('error-5');
	
//string
if( $.makeArray( 'foo' )[0] != 'foo' )
	alert('error-6');
	
//boolean
if( typeof $.makeArray( true )[0] != 'boolean' )
	alert('error-7');
	
//node
if( $.makeArray( scripts[0] )[0].nodeName != 'SCRIPT' )
	alert('error-8');
	
//array-like object
if( $.makeArray( {length:2, 0:'a', 1:'b'} ).join('') != 'ab' )
	alert('error-9');

//childnodes array
if( $.makeArray( document.documentElement.childNodes ).slice(-1)[0].nodeName != 'HEAD' )
	alert('error-10');
	
//function (tricky, they have length)
if( $.makeArray( function(){ return 1;} )[0]() != '1' )
	alert('error-11');		

Changed March 29, 2008 08:22PM UTC by flesler comment:3

You might say: "ok, slice is not used for regular arrays". I benchmarked my option against slice and concat (with an empty page w/o jquery or anything else) and the native alternative was only faster on Safari 3. Firefox 2 was actually like 10x times faster with a reversed for.

This is the benchmark:

(function(){
		  
	var
		arr = Array(30),
		times = 1e5,
		i, t;

	t = times;
	console.time('mine');	
	while( t-- ){
		var i = arr.length
		while( i )
			arr[--i] = arr[i];
	}
	console.timeEnd('mine');

	t = times;
	console.time('native');
	while( t-- )
		arr.slice();
	console.timeEnd('native');
})();

I made a fake console with time() and timeEnd() for all but Firefox.

Results:

Firefox 2.0.0.13 (turned firebug off)
    * mine: 641ms
    * native: 5312ms

IE 6
    * mine: 2078ms
    * native: 2422ms

Opera 9.22
    * mine: 1656ms
    * native: 1844ms

Safari 3.0.4
    * mine: 1281ms
    * native: 344ms

This should be considered for the rest of the code as well, native methods might not always be the fastest.

Changed March 29, 2008 08:26PM UTC by flesler comment:4

One correction: the while loop was an experiment, i was comparing loops.

The real loop (the one in the code) is:

	t = times;
	console.time('mine');	
	while( t-- ){
		for( i = arr.length; i; )
			arr[--i] = arr[i];
	}
	console.timeEnd('mine');

Benchmarked all back and the numbers were the same.

Changed April 22, 2008 10:22PM UTC by joern comment:5

Could you rewrite those tests using the jQuery testsuite?

Changed April 22, 2008 11:16PM UTC by flesler comment:6

Commited the required tests on [5284].

Changed April 24, 2008 09:25PM UTC by joern comment:7

Commited patch: [5314]. Keeping ticket open until a few other places make use of the new implementation.

Changed April 24, 2008 09:47PM UTC by flesler comment:8

Simplified the code using the new makeArray where possible, at [5137]

Changed April 24, 2008 09:53PM UTC by joern comment:9

That should have been [5317].

Changed April 24, 2008 09:57PM UTC by joern comment:10

I wonder if makeArray can really replace all those checks in the init method:

// Watch for when an array-like object, contains DOM nodes, is passed in as the selector 
(selector.jquery || selector.length && selector != window && !selector.nodeType
 && selector[0] != undefined && selector[0].nodeType) && jQuery.makeArray( selector )

Odds are good that the testsuite doesn't really cover all of them.

The other changes look fine.

Changed April 24, 2008 11:20PM UTC by flesler comment:11

I just checked, and 'setArray' is only used inside 'init'. 'init' is only used inside jQuery.

In consequence, setArray needs to handle whatever $() handles.

String selectors/html and functions are sniffed out before, that only leaves us with:

  • Arrays
  • Array-like (jQuery, nodelist)
  • DOM elements

The first two are of course handled. DOM elements are checked first, prior to this.

We could actually remove that prior check because makeArray handles it well.

Of course, the this[0] = selector part, is faster than a whole makeArray+setArray, so

that if can be kept to speed up that case.

This enhancement to makeArray, actually opens up some new possibilities.

Like $( 2 ), $( new Date ), $( {foo:'bar'} ), $( /[abc]/ ), $( new MyClass ), etc.

I think this is really good!

The only problem that might arise is with window, as it has length.

We can add a " && !array.location " or .setInterval, to detect the window.

The !array.call can be removed if we don't need to support functions.

Changed April 25, 2008 03:53AM UTC by flesler comment:12

Fixed the detection for window, and added a test case for window and a regex at [5318].

Changed April 28, 2008 01:55PM UTC by joern comment:13

resolution: → fixed
status: newclosed