Skip to content

Commit

Permalink
Replace Prototype's Selector implementation with Sizzle
Browse files Browse the repository at this point in the history
  • Loading branch information
@sstephenson
sstephenson committed Sep 19, 2009
1 parent 9e4a7ce commit 4dd878f
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 798 deletions.
3 changes: 3 additions & 0 deletions .gitmodules
View file
Expand Up @@ -11,3 +11,6 @@
path = vendor/sprockets
url = git://github.com/sstephenson/sprockets.git

[submodule "vendor/sizzle"]
path = vendor/sizzle
url = git://github.com/jeresig/sizzle.git
3 changes: 2 additions & 1 deletion Rakefile
View file
Expand Up @@ -9,6 +9,7 @@ module PrototypeHelper
DOC_DIR = File.join(ROOT_DIR, 'doc')
TEMPLATES_DIR = File.join(ROOT_DIR, 'templates')
PKG_DIR = File.join(ROOT_DIR, 'pkg')
SIZZLE_DIR = File.join(ROOT_DIR, 'vendor', 'sizzle')
TEST_DIR = File.join(ROOT_DIR, 'test')
TEST_UNIT_DIR = File.join(TEST_DIR, 'unit')
TMP_DIR = File.join(TEST_UNIT_DIR, 'tmp')
Expand All @@ -18,7 +19,7 @@ module PrototypeHelper
require_sprockets
secretary = Sprockets::Secretary.new(
:root => File.join(ROOT_DIR, path),
:load_path => [SRC_DIR],
:load_path => [SRC_DIR, SIZZLE_DIR],
:source_files => [source],
:strip_comments => strip_comments
)
Expand Down

16 comments on commit 4dd878f

@nel
Copy link

@nel nel commented on 4dd878f Oct 3, 2009

Choose a reason for hiding this comment

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

Does it feel good to get rid of this ?

Sizzle implementation is not only efficient at replacing $$, it is also particularly efficient at matching ancestors element against a css selector. It makes it a breath to use for event delegation. By replacing old implementation of Selector.findElements() by Sizzle.matches() you increase performance of event delegation by an order of magnitude on Firefox and particularly IE

Very happy to see this coming on prototype, thank you!

@kangax
Copy link
Collaborator

@kangax kangax commented on 4dd878f Oct 5, 2009

Choose a reason for hiding this comment

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

Strange. I always thought Sizzle uses top-down, not bottom-up selector engine.

@nel
Copy link

@nel nel commented on 4dd878f Oct 5, 2009

Choose a reason for hiding this comment

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

Actually it depends of the selector, but Sizzle.matches (which is a replacement for Selector.findElements) actually search bottom-up in most real case (i.e. not nth-child and stuff like that).

Look at sizzle.js L66-96, it does pop() the selector chunk backward. If you have a query like $('foo').up('.class1 .class2') what it does is:

- Fetch foo.ancestors (it is still real prototype extended Element which is not needed but it's ok for now)
- Search for class2Elt = element matching '.class2' selector in foo.ancestors() (quick match on the className)
- Search for parentNode of class2Elt that match '.class1' (still very quick)
- return class2Elt if it is success or nothing otherwise

It cannot be quicker in that particular case, and that particular case is 99% of the event delegation case.

Prototype previous was Searching all elements matching $$('.class1 .class2') (which lead to 2 huge intersection) and then Intersect with ancestors() which was awfully slow in case of frequent event like mouseout, mousemove ...

@jdalton
Copy link
Contributor

@jdalton jdalton commented on 4dd878f Oct 5, 2009

Choose a reason for hiding this comment

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

Shame they didn't use NWMatcher http://dl.getdropbox.com/u/513327/nwmatcher_match_performance.png

@kangax
Copy link
Collaborator

@kangax kangax commented on 4dd878f Oct 5, 2009

Choose a reason for hiding this comment

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

@jdalton It is a shame. I brought it up few times, but no one seemed to be interested (I think an argument was that Sizzle is more popular and doesn't compile functions - so is compatible with Caja).

@nel
Copy link

@nel nel commented on 4dd878f Oct 6, 2009

Choose a reason for hiding this comment

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

$$ search pattern is just a tiny part of the selector problem. A part which will become more and more irrelevant given native QSA will soon be the norm. .up(expr) .next(expr) and related methods are the limiting methods but no one is publishing any bench on those, results would probably be surprising and not kind to current Prototype master.

Sizzle is currently good enough at them, NWMatcher probably too but we have to be more careful given its compiled nature to see if it fits real world use (i.e. short lived Element instance anywhere traversing the dom in many direction with many selector).

Whatever the choice is, Prototype current implementation could support several adapter, so we are not stuck to Sizzle. IMHO the important step is to go forward and get rid of the current engine to get decent performance without crazy caching.

@jdalton
Copy link
Contributor

@jdalton jdalton commented on 4dd878f Oct 6, 2009

Choose a reason for hiding this comment

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

@nel - early alpha benchmarks of a work in progress framework using NWMatcher http://twitter.com/fusejs/status/3166496154. NWMatcher also passes the unit tests of Prototype and jQuery.

@jdalton
Copy link
Contributor

@jdalton jdalton commented on 4dd878f Oct 6, 2009

Choose a reason for hiding this comment

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

@nel also it's not "crazy" caching it's "intelligent" caching :D

@gf3
Copy link
Contributor

@gf3 gf3 commented on 4dd878f Oct 6, 2009

Choose a reason for hiding this comment

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

I was secretly hoping for NWMatcher as well, but Sizzle is still a vast improvement over Selector.

@samleb
Copy link
Contributor

@samleb samleb commented on 4dd878f Oct 9, 2009

Choose a reason for hiding this comment

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

NWMatcher is blazing fast, and it's a robust and complete selector implementation.
We just cannot ship as part of framework because function compilation at runtime isn't compatible with Caja (we're already trying to get rid of all eval calls).
As @nel pointed out, what's important for Prototype is to have a selector adapter API so any implementation could be plugged in depending on your needs (application with lot of event delegation vs. embeddable widget with security considerations).
This is planned for future releases.

@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 could not have said it better myself.

@dperini
Copy link

Choose a reason for hiding this comment

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

First of all thanks to all the NWMatcher supporters here and to the team for its consideration !

At many selectors your current engine is faster than Sizzle, some other selectors are incredibly slow though.
I have even thought about not being the Selector engine by itself the real slow down in those specific ones but wrappers and prototyping overhead and extending elements. I say this because I have the numbers of the few tests around under my eyes everyday.

The point should be moved to have a real matcher in Prototype one that can be used as a fall back to the already implemented webkit and moz MatchesSelector(), the priority is not changing the selector engine, having a matcher will automatically let you improve your current selector engine.

I say this knowing that NWMatcher will not be considered for the above reasons: Caja new Function() blah blah, I am trying hard to convince them this is so useful they can't just forget/ignore it. An alternative should be available this is just pure ECMAScript code it's not a virus !

Still, what you need in Prototype is a matcher, so see what are the alternatives, the adapter one seems good for me too. Hope this helps !

PS: we have now the very first tests of the match() method running vs. original browsers implementations so you may have a peek here:

http://dl.getdropbox.com/u/598365/matchspeed/matchspeed-custom.html

as a bonus I recently added a "data" parameter and "callback" capabilities to the engine, you will love these additions...promised... ;-)

@tobie
Copy link
Collaborator

@tobie tobie commented on 4dd878f Oct 22, 2009

Choose a reason for hiding this comment

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

@kangax: I've been following NWMatcher since it's inception and I know it's merits. As Andrew and Samuel both mentioned above, we're all very keen to make Prototype selector agnostic in the near future. That being said, Sizzle seems like a good default options for the various reasons already discussed (popularity, Caja compliance, etc.).

@diego: Im totally with you on the Selector / Matcher distinction. I wasn't aware that there was a matcher implemented in WebKit and Firefox. What is the API like? Where can I find some info on it? It would be great to have a common API for all selector engines so that switching engine could be a drop-in or evan a runtime replacement.

@kangax
Copy link
Collaborator

@kangax kangax commented on 4dd878f Oct 22, 2009

Choose a reason for hiding this comment

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

@tobie, both links are here — http://twitter.com/kangax/status/4617283006

@dperini
Copy link

Choose a reason for hiding this comment

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

@tobie,
the API are quite simple and in-line with how my match() method work. I also already submitted a ticket to Webkit asking them to improve speed ;-)

I am so happy they introduced those, this is like a reward for me and my work. I can finally test things Vs. native implementations and be able to show.
I did some more test on combinators and className resolution and seems Prototype is faster or at least on par with other selector engines on those.

Is Prototype using XPATH to resolve classNames internally ?
I know XPATH being very fast for those expensive operations...but it is not available everywhere though.

An adapter will be very easy for NWMatcher, both for the select() and match() methods, and you may opt to only adopt the match() method. The work Samuel did in my NWMatcher to make the unit tests work was already an attempt to this, the remaining parts are probably in Juriy "traversal" API he adapted for NWMatcher (up/down/next/prev).

Let me know if I can be of help in this process, I would love too see my NWMatcher used intensively, even if it has to be trough an adapter.

@tobie
Copy link
Collaborator

@tobie tobie commented on 4dd878f Oct 23, 2009

Choose a reason for hiding this comment

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

You'll hopefully like http://github.com/sstephenson/prototype/commit/7762e002cb0dd757b347494c896b6aa520b21c16

Please sign in to comment.