Skip to content

Commit

Permalink
Landing some minor perf optimization to jQuery().
Browse files Browse the repository at this point in the history
  • Loading branch information
jeresig committed Jan 28, 2010
1 parent 0db207d commit b8076a9
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 5 deletions.
16 changes: 12 additions & 4 deletions src/core.js
Expand Up @@ -69,6 +69,14 @@ jQuery.fn = jQuery.prototype = {
this.length = 1;
return this;
}

// The body element only exists once, optimize finding it
if ( selector === "body" && !context ) {
this.context = this[0] = document.body;
this.selector = "body";
this.length = 1;
return this;
}

// Handle HTML strings
if ( typeof selector === "string" ) {
Expand Down Expand Up @@ -99,7 +107,7 @@ jQuery.fn = jQuery.prototype = {
ret = buildFragment( [ match[1] ], [ doc ] );
selector = (ret.cacheable ? ret.fragment.cloneNode(true) : ret.fragment).childNodes;
}

return jQuery.merge( this, selector );

// HANDLE: $("#id")
Expand Down Expand Up @@ -128,6 +136,7 @@ jQuery.fn = jQuery.prototype = {
this.selector = selector;
this.context = document;
selector = document.getElementsByTagName( selector );
return jQuery.merge( this, selector );

// HANDLE: $(expr, $(...))
} else if ( !context || context.jquery ) {
Expand All @@ -150,9 +159,7 @@ jQuery.fn = jQuery.prototype = {
this.context = selector.context;
}

return jQuery.isArray( selector ) ?
this.setArray( selector ) :
jQuery.makeArray( selector, this );
return jQuery.makeArray( selector, this );
},

// Start with an empty selector
Expand Down Expand Up @@ -604,6 +611,7 @@ jQuery.extend({
for ( var l = second.length; j < l; j++ ) {
first[ i++ ] = second[ j ];
}

} else {
while ( second[j] !== undefined ) {
first[ i++ ] = second[ j++ ];
Expand Down
2 changes: 1 addition & 1 deletion test/unit/core.js
Expand Up @@ -25,7 +25,7 @@ test("jQuery()", function() {
equals( jQuery(obj).selector, "div", "jQuery(jQueryObj) == jQueryObj" );

// can actually yield more than one, when iframes are included, the window is an array as well
equals( 1, jQuery(window).length, "Correct number of elements generated for jQuery(window)" );
equals( jQuery(window).length, 1, "Correct number of elements generated for jQuery(window)" );


var main = jQuery("#main");
Expand Down

17 comments on commit b8076a9

@DBJDBJ
Copy link

@DBJDBJ DBJDBJ commented on b8076a9 Jan 29, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, then this also might improve things :

 // The HEAD element only exists once, optimize finding it
  if ( selector === "head" && !context ) {
     this.context = this[0] =document.documentElement.firstChild ;
     this.selector = "head";
    this.length = 1;
   return this;
  }

--DBJ

@GarrettS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment node is allowed there. If a comment is present, then the code should fail.


Your code will produce your desired outcome Gecko and IE, but only due to browser bugs. It will fail in Chrome, Opera, Webkit, and Blackberry.

@GarrettS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try the code example again:-

<html lang="en">
<!-- comment node -->
<head<

What is document.document.firstChild? Is it a comment or the head? So when IE and Firefox get the head, that's a bug.

Garrett

@GarrettS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well close enough on the code example to convey the point (should be <head>

@DBJDBJ
Copy link

@DBJDBJ DBJDBJ commented on b8076a9 Jan 29, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// The HEAD element only exists once, optimize finding it
 if ( selector === "head" && !context ) {
   this.context = this[0] =document.getElementsByTagName('head')[0] ;
   this.selector = "head";
  this.length = 1;
 return this;
}

Easy to fix, don't get upset ... Concentrate on good things ...

@GarrettS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the thinking your response. I helped you avoid a bug. How does that make me "upset"?

@DBJDBJ
Copy link

@DBJDBJ DBJDBJ commented on b8076a9 Jan 30, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Garrets: I was talking to myself, so "don't get upset" ... ;o)

@GarrettS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, got it. I know some people really talk to themselves a lot. Well, I sometimes don't edit my words so well either, AYCS from the code examples.

But please try and get my name right, though -- it's "Garrett".

@DBJDBJ
Copy link

@DBJDBJ DBJDBJ commented on b8076a9 Feb 1, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My name is not DBJDBJ, either ...

@rkatic
Copy link
Contributor

@rkatic rkatic commented on b8076a9 Feb 1, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@john

'jQuery.setArray' is still significantly faster then 'jQuery.makeArray', and although it is not strictly necessary, 'jQuery(aArray)' is still heavily used internally.
Is this change motivated by the intention to deprecate/remove 'jQuery.setArray'? It is not used any more.
If so, then I would suggest to integrate 'jQuery.setArray' inside 'jQuery.makeArray' or inside 'jQuery.merge or even better inside' 'jQuery'.

@jeresig
Copy link
Member Author

@jeresig jeresig commented on b8076a9 Feb 1, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rkatic: Actually, that's not entirely true - the only place jQuery(array) is used is within pushStack - that can easily be rewritten to use the faster code path - and if it's being removed then it makes a lot of sense to deprecate/remove it as well (people should be using pushStack anyway).

@DBJDBJ
Copy link

@DBJDBJ DBJDBJ commented on b8076a9 Feb 1, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@john, I wellcome anyhting that untangles jQuery() , "multifunctional" single function interface.
That is If, I understood properly you want to deprecarte jQuery( array ) ...

@jeresig
Copy link
Member Author

@jeresig jeresig commented on b8076a9 Feb 1, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that's not what I said. I use want to remove jQuery.fn.setArray as it's no longer needed - it can be incorporated into jQuery.fn.pushStack.

@DBJDBJ
Copy link

@DBJDBJ DBJDBJ commented on b8076a9 Feb 1, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I got carried away. Although, name "pushStack()" dramaticaly improves the readability of the API. It clearly reveals the intent and the reason of existence of the method... I would like to see (much) more of that kind of activity, while moving towards 1.5

The Official jQuery Podcast – Episode 9 – David Artz, Aol.also reveals , what my little contribution (from few months ago) tried to explain too : not everyone is jQuery expert. Managing a large(ish) teams and/or Web 2.0 companies, becomes a nightmare, because of power and flexibility of jQuery. Which often produces a lot of headaches when "given" to those big teams developing and/or supporting large portals.

They rarely bother with api documentation, usually not before the whole mountain of javascript code starts disolving, because of code like : $("*").click() ...

All API ambiguity must be weeded-out, as much as possible ...

--DBJ

@rkatic
Copy link
Contributor

@rkatic rkatic commented on b8076a9 Feb 1, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Uf, I wrote jQuery.setArray instead of jQuery.fn.seArray. I have to sleep.)

the only place jQuery(array) is used is within pushStack

I know, but pushStack is heavily used. Had to be more clear telling that jQuery(array) is frequently evaluated.

I am not completely convinced about incorporating setArray into pushStack.
Internally, currently, pushStack is the only one that uses jQuery(array), but what about plugins? What about adding jQuery.fromArray then.

@jeresig
Copy link
Member Author

@jeresig jeresig commented on b8076a9 Feb 1, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rkatic: As a public-facing API setArray simply doesn't make sense (and it never has been an API that we've wanted to expose). It's a destructive operation which is completely against the rest of the jQuery API. Moving it into pushStack is really better all around.

@rkatic
Copy link
Contributor

@rkatic rkatic commented on b8076a9 Feb 1, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

John, I completely agree to remove setArray, but maybe jQuery.fn.init is more appropriate to integrate that.

Please sign in to comment.