Skip to content

Commit

Permalink
prototype: Simplify the NWMatcher adapter and optimize it for browser…
Browse files Browse the repository at this point in the history
…s that don't need to extend DOM elements. [jddalton]
  • Loading branch information
jdalton authored and kangax committed Apr 6, 2010
1 parent ece6e0e commit 2f9bde3
Showing 1 changed file with 6 additions and 2 deletions.
8 changes: 6 additions & 2 deletions vendor/nwmatcher/selector_engine.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,12 @@ Prototype._original_property = window.NW;
//= require "repository/src/nwmatcher"

Prototype.Selector = (function(engine) {
function select(selector, scope) {
return engine.select(selector, scope || document, Element.extend);
var select = engine.select;

if (Element.extend !== Prototype.K) {
select = function select(selector, scope) {
return engine.select(selector, scope, Element.extend);
};
}

return {
Expand Down

12 comments on commit 2f9bde3

@gf3
Copy link
Contributor

@gf3 gf3 commented on 2f9bde3 Apr 6, 2010

Choose a reason for hiding this comment

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

Speed++

@staaky
Copy link

@staaky staaky commented on 2f9bde3 Apr 6, 2010

Choose a reason for hiding this comment

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

Nice :) Maybe give the Sizzle adapter the same treatment?

@tobie
Copy link
Collaborator

@tobie tobie commented on 2f9bde3 Apr 6, 2010

Choose a reason for hiding this comment

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

Yes. I believe this should be at a higher level if any.

@savetheclocktower
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have any actual numbers that show how much faster this approach is? If it reflects, say, a 10% speed increase, I'd be happy to see this moved from the adapter itself into our own code. If it's more like a 3% increase, I say it's not worth the bother.

@kangax
Copy link
Collaborator

@kangax kangax commented on 2f9bde3 Apr 6, 2010

Choose a reason for hiding this comment

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

John said it's about 10%, so I think it makes sense to have it.

@savetheclocktower
Copy link
Collaborator

Choose a reason for hiding this comment

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

I trust him. But I still want to see the numbers. :-)

@jdalton
Copy link
Contributor Author

@jdalton jdalton commented on 2f9bde3 Apr 7, 2010

Choose a reason for hiding this comment

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

@tobie
Copy link
Collaborator

@tobie tobie commented on 2f9bde3 Apr 7, 2010

Choose a reason for hiding this comment

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

@kangax: in the future, would appreciate if we discussed this within core before committing. Improving performance is essentially about improving bottlenecks. For selector engines, the bottle neck is in IE, not elsewhere.

@gf3
Copy link
Contributor

@gf3 gf3 commented on 2f9bde3 Apr 7, 2010

Choose a reason for hiding this comment

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

@tobie: Although, I must say, the public transparency is nice; for the rest of us.

@tobie
Copy link
Collaborator

@tobie tobie commented on 2f9bde3 Apr 7, 2010

Choose a reason for hiding this comment

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

@gf3: Yeah. Good point. For the record. I'm totally in favor of moving all of our internal discussions to public, read-only (I dislike bike-shedding more than opacity) mailing lists.

@kangax
Copy link
Collaborator

@kangax kangax commented on 2f9bde3 Apr 7, 2010

Choose a reason for hiding this comment

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

@tobie

It's a 3-line snippet of clean, easy-to-understand code that gives up to 10% boost. And packaged in a nice, ready-to-apply patch. I'm all for improving bottlenecks, but frankly don't see what the problem is in this case :)

As far as public discussion... Of course I'd love to see that too. But I don't agree with making it read-only. There must be other ways to prevent bike-shedding other than disallow input from the public side. For example, there are often fruitful discussions on WHATWG and es-discuss mailing lists, including non-commitee members pointing out flaws or providing better alternatives.

@tobie
Copy link
Collaborator

@tobie tobie commented on 2f9bde3 Apr 7, 2010

Choose a reason for hiding this comment

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

Woohoo! Meta bike-shedding.

Please sign in to comment.