Selector speedup by using John Resig's Sizzle
Arnar Birgisson
Some of you may have seen on reddit that John Resig (of jQuery) is
working on a new, ultra-fast, css selector module. It is called Sizzle
and although it is not released yet, John uploaded a version to
github: http://github.com/jeresig/sizzle/tree/master
MochiKit's Selector module (which is ported from early versions of
Prototype) is unbearably slow, and thus many people steer clear of it.
I asked John about the possibility of including Sizzle in MochiKit and
he's ok with that, Sizzle will be released under the MIT license.
I did a quick test, just deleted most of the Selector module and
replaced with John's code, and modified the exported functions of the
Selector module to use that instead. The "MochiKit.Selector.Selector"
object has to go though, so this would not be an entirely
backwards-compatible change. The functions findChildElements,
findDocElements and $$ would be unchanged though.
You can check out the speed test (included with Sizzle) where I've
added both the trunk version of MochiKit and the MochiKit+Sizzle
fusion here:
http://www.hvergi.net/arnar/public/sizzle/speed/#
For this benchmark, regular MochiKit completed all tests in 3983
milliseconds. The MochiKit+Sizzle combination does it in 61. That
means we are talking about a speedup by a factor of roughly 65!
It doesn't come without faults though. Sizzle didn't support all
queries in MochiKit's unit tests, namely these are the ones that fail
(I'm cc-ing John in case he wants to add support for any of them):
a[fakeattribute] - i.e. checking for presence of attribute
p[lang|="en"] - membership test of hyphen-seperated string collections
:nth-of-type(...) pseudo-class
:enabled, :disabled and :checked pseudo-classes
:root pseudo-class
This change would increase the size of the packed version by about
1700 bytes (currently at 173.5 KB).
Now, how do people feel about committing a change like this to the
trunk? Of course, we'd wait until a fairly stable version of Sizzle is
released. John told us that Sizzle will become the main selector
engine behind jQuery, but will also remain a standalone component. All
bugfixes will be backported to Sizzle also. As long as MochiKit keeps
up, this means we'd benefit from the bugs reported by the jQuery
community.
A rough test, just plomping in the Sizzle source code into Selector.js
is available on my website:
http://www.hvergi.net/arnar/public/mochikit/MochiKit/Selector.js
cheers,
Arnar
Arnar Birgisson
On Mon, Aug 25, 2008 at 16:06, Arnar Birgisson <arn...@gmail.com> wrote:
> For this benchmark, regular MochiKit completed all tests in 3983
> milliseconds. The MochiKit+Sizzle combination does it in 61. That
> means we are talking about a speedup by a factor of roughly 65!
Sorry, forgot to mention that this is running on my computer on
Firefox 3.0. For browsers that support the querySelectorAll method
(such as FF 3.1), I'd expect an even greater speedup.
cheers,
Arnar
JasonBunting
Chris Lee-Messer
addition fixes the errors in MochiKit.Selector. Does it break any of
the other (non-selector related) tests in MochiKit?
-Chris
p.s. I'm running firefox 3.1pre now with the JIT running, and
slickspeed tests are interesting as your version of MochiKit.Selector
is reported as faster than Sizzle: most of the response times are 1ms
for a total time of 45ms vs 174ms for Sizzle itself. I would guess
that some compiled code is being cached. MochiKit's current Selector
comes in with a time of 4045ms.
Arnar Birgisson
On Tue, Aug 26, 2008 at 13:38, Chris Lee-Messer
<chris.l...@gmail.com> wrote:
> Adding Sizzle sounds like a substantial plus, I noticed that your
> addition fixes the errors in MochiKit.Selector. Does it break any of
> the other (non-selector related) tests in MochiKit?
No, no tests outside the Selector module are broken. You can run the
test suite here:
http://www.hvergi.net/arnar/public/mochikit/tests/
There is some namespace pollution, i.e. the Sizzle code defines a few
globals. Maybe we should hide them inside some
MochiKit.Selector.__internal__ object or similar?
The Selector module has limited usability now imo, so I think this
would be a good move even though it breaks backwards compatibility a
bit.
> p.s. I'm running firefox 3.1pre now with the JIT running, and
> slickspeed tests are interesting as your version of MochiKit.Selector
> is reported as faster than Sizzle: most of the response times are 1ms
> for a total time of 45ms vs 174ms for Sizzle itself. I would guess
> that some compiled code is being cached. MochiKit's current Selector
> comes in with a time of 4045ms.
Yes, I noticed the same when I ran the benchmark, but not so drastically.
I added a second instance Sizzle to the test suite, at the very end.
That one gives lower times so indeed there seems to be some caching of
compiled code going on.
http://www.hvergi.net/arnar/public/sizzle/speed/#
cheers,
Arnar
Arnar Birgisson
On Tue, Aug 26, 2008 at 13:38, Chris Lee-Messer
<chris.l...@gmail.com> wrote:
> ... I noticed that your addition fixes the errors in MochiKit.Selector.
I just realized, do you mean you noticed that I fixed the errors that
were causing the tests in Selector to fail?
If so, then no.. i "fixed" them by commenting out the tests that were
failing. They were stopping the test-suite from continuing to other
tests.
cheers,
Arnar
Amit Mendapara
in another post, I am implementing jQuery style Query module which is
currently using MochiKit.Selector (the code is not made public yet).
As my proposed module is totally new, I'm not afraid of backward
compatibility. That's why I have decided to implement new selector for
the Query module. Though if MochiKit.Selector is going to be Sizzle
based, it would be great...
Another problem to which I am not sure is licensing issue, Sizzle is
MIT licensed while MochiKit has dual MIT/AFL license. I myself
thinking of releasing my code under any FSF approved licenses like MIT
or GLP. Do you see any difficulties of including these sources to
MochiKit particularly under AFL?
Regards
..
Amit Mendapara
Arnar Birgisson
> Another problem to which I am not sure is licensing issue, Sizzle is
> MIT licensed while MochiKit has dual MIT/AFL license. I myself
> thinking of releasing my code under any FSF approved licenses like MIT
> or GLP. Do you see any difficulties of including these sources to
> MochiKit particularly under AFL?
To be honest, I don't know.
Bob, or anyone, do you know if there are problems with this?
If there are, John Resig was keen on the idea so he might make some
arrangements for us.
cheers,
Arnar
Bob Ippolito
I don't think there should be a problem with that, since AFL doesn't
give any permissions that don't already exist in MIT, but IANAL.
-bob
JasonBunting
"JavaScript Library Overview" slides from "Ajax Experience
'08" (http://www.slideshare.net/jeresig/javascript-library-overview-
presentation/), this is being integrated into MochiKit.
On Aug 25, 10:06 am, "Arnar Birgisson" <arna...@gmail.com> wrote:
> Hey all,
>
> Some of you may have seen on reddit that John Resig (of jQuery) is
> working on a new, ultra-fast, css selector module. It is calledSizzle
> and although it is not released yet, John uploaded a version to
> github:http://github.com/jeresig/sizzle/tree/master
>
> MochiKit's Selector module (which is ported from early versions of
> Prototype) is unbearably slow, and thus many people steer clear of it.
> he's ok with that,Sizzlewill be released under the MIT license.
> I did a quick test, just deleted most of the Selector module and
> replaced with John's code, and modified the exported functions of the
> Selector module to use that instead. The "MochiKit.Selector.Selector"
> object has to go though, so this would not be an entirely
> backwards-compatible change. The functions findChildElements,
> findDocElements and $$ would be unchanged though.
>
> fusion here:http://www.hvergi.net/arnar/public/sizzle/speed/#
>
> For this benchmark, regular MochiKit completed all tests in 3983
>
> (I'm cc-ing John in case he wants to add support for any of them):
>
> a[fakeattribute] - i.e. checking for presence of attribute
> p[lang|="en"] - membership test of hyphen-seperated string collections
> :nth-of-type(...) pseudo-class
> :enabled, :disabled and :checked pseudo-classes
> :root pseudo-class
>
> This change would increase the size of the packed version by about
> 1700 bytes (currently at 173.5 KB).
>
> Now, how do people feel about committing a change like this to the
> trunk? Of course, we'd wait until a fairly stable version ofSizzleis
> community.
>
Arnar Birgisson
On Tue, Oct 7, 2008 at 06:21, JasonBunting <jason....@gmail.com> wrote:
> Any update on this? According to slide 92 of John Resig's recent
> "JavaScript Library Overview" slides from "Ajax Experience
> '08" (http://www.slideshare.net/jeresig/javascript-library-overview-
> presentation/), this is being integrated into MochiKit.
Sorry, a sudden onslaught of thesis work has been keeping me busy. I
was just thinking about this last night though.
I can upload a patch for review tonight. There are two main problems,
both of which might be considered acceptable:
1. A few existing unit-tests for Mochikit.Selector fail, because
Sizzle doesn't support some (rarely used) selectors. The list of
failed tests is earlier in this thread.
2. The global name space is polluted a bit.
If someone is willing to review the patch and either make suggestions
how to fix these, or if the ML is ok with both problems - then I can
commit it.
cheers,
Arnar
Amit Mendapara
If you have noticed, I have published MochiKit.Query module on
Launchpad (https://launchpad.net/mochikit-ext). The goal of this
module is to provide functionalities similar to jQuery (not 100%
compatible). Currently, it uses MochiKit.Selector. Initially, I was
thinking on implementing jQuery style selector but then I come across
to this post and I changed my mind to wait for the integration of
Sizzle in MochiKit.
It seems that you are trying to make the integration into existing
MochiKit.Selector module with full backward compatibility. I think,
you should integrate Sizzle as a separate MochiKit.Sizzle module. And
we should kindly request Mr. Bob to deprecate MochiKit.Selector (or
just remove it as 1.4 is not officially released).
Regards
--
Amit Mendapara
Arnar Birgisson
On Tue, Oct 7, 2008 at 10:34, Amit Mendapara <mendapa...@gmail.com> wrote:
> It seems that you are trying to make the integration into existing
> MochiKit.Selector module with full backward compatibility. I think,
> you should integrate Sizzle as a separate MochiKit.Sizzle module. And
> we should kindly request Mr. Bob to deprecate MochiKit.Selector (or
> just remove it as 1.4 is not officially released).
Personally, I don't mind if the change is backwards incompatible - but
it's not my decision alone if it is ok. I would greatly prefer to keep
Mochikit.Selector - because someday something better than Sizzle might
come along.
To be clear, I'm basically asking the mailing list: Would you be OK
with it if the following selectors would stop working?
a[fakeattribute] - i.e. checking for presence of attribute
p[lang|="en"] - membership test of hyphen-seperated string collections
:nth-of-type(...) pseudo-class
:enabled, :disabled and :checked pseudo-classes
:root pseudo-class
cheers,
Arnar
Amit Mendapara
On Oct 7, 1:49 pm, "Arnar Birgisson" <arna...@gmail.com> wrote:
> To be clear, I'm basically asking the mailing list: Would you be OK
> with it if the following selectors would stop working?
>
> a[fakeattribute] - i.e. checking for presence of attribute
> p[lang|="en"] - membership test of hyphen-seperated string collections
> :nth-of-type(...) pseudo-class
> :enabled, :disabled and :checked pseudo-classes
> :root pseudo-class
>
Drop these (not in jQuery, so may be not supported by Sizzle):
1) p[lang|="en"]
2) :nth-of-type(...)
Rename this:
1) :root to :first
Should be there:
1) a[fakeattribute]
2) :enabled
3) :disabled
4) :checked
5) ... more form selectors (see jQuery doc)
Regards
--
Amit Mendapara
Arnar Birgisson
> Should be there:
>
> 1) a[fakeattribute]
> 2) :enabled
> 3) :disabled
> 4) :checked
> 5) ... more form selectors (see jQuery doc)
Right, last time I looked at this was with a pre-release of Sizzle, so
things might have improved. I will update to the latest Sizzle version
when I get home tonight and prepare a patch.
cheers,
Arnar
Jason Bunting
> Arnar Birgisson wrote:
>
> To be clear, I'm basically asking the mailing list: Would you be OK
> with it if the following selectors would stop working?
>
> a[fakeattribute] - i.e. checking for presence of attribute
> p[lang|="en"] - membership test of hyphen-seperated string
> collections
> :nth-of-type(...) pseudo-class
> :enabled, :disabled and :checked pseudo-classes
> :root pseudo-class
Sounds good to me - would it be too hard to hook into Sizzle and add these
ourselves so that the tests pass? I don't know squat about the current code
and what is involved in doing this; would it be much work?
Jason
Arnar Birgisson
I intend to find out tonight :)
cheers,
Arnar
Arnar Birgisson
Experimental version ready in seperate branch, see below.
On Tue, Oct 7, 2008 at 19:11, Arnar Birgisson <arn...@gmail.com> wrote:
>> Sounds good to me - would it be too hard to hook into Sizzle and add these
>> ourselves so that the tests pass? I don't know squat about the current code
>> and what is involved in doing this; would it be much work?
>
> I intend to find out tonight :)
As it turns out, most of these were easy to implement. Sizzle actually
pulls some implementation from jQuery if it is available. I copied and
adapted the relevant parts from jQuery (which is also MIT licensed).
Sizzle had a few bugs (or at least incorrect behaviour imo) which were
easy to fix, among those was support for the |= selector. I created a
branch called selector_sizzle [1] and committed this there, ready for
review - with the following notes:
[1] http://svn.mochikit.com/mochikit/branches/selector_sizzle
1. All unit tests pass in FF except those for three types of
selectors. As they were stopping the test suite I had to comment them
out. The three selector types that do not work are:
- :nth-of-type(...)
This one is not supported by Sizzle, and it is not straight forward
to do so.
I think we should just accept that, they are CSS3 features and
probably not used a lot.
- a[someattribute]
Now this is quite common, and I think that it is the intention of
Sizzle to support this.
However, the implementation is broken - I couldn't figure out what
was wrong so I emailed
John about it, I'm currently waiting for a reply.
If nothing else, MochiKit should thus lead to one bugfix in Sizzle. :)
My opinion here is to wait for John, either he fixes it or helps me to do it.
- :root
This selector is also from CSS3 and is not supported by Sizzle. I
think it is possible, I
tried adding support for it but it didn't work out too nicely. This
has to do with the fact
that Sizzle assumes one is working with "document" (the main doc)
but MochiKit.Selector, and
thus the unit test for :root, tries to use this on other documents
with the functionality
in MochiKit.DOM (withDocument, currentDocument etc.). My opinion is
that we should simply
skip this as well.
2. The change in the branch introduces a few global names. Namely:
chunker, cache, Sizzle, Expr, makeArray, dir, dirNode, dirCheck. This
is a bit messy since many of these names are non-distinct and could be
used by users or other libraries. We should probably enclose them
somehow in the MochiKit.Selector namespace. However, I will suggest to
John that Sizzle only binds one global name (Sizzle) and tucks the
others away in that namespace.
Some unit tests fail on Safari, will have to look at that tomorrow
(unless someone else wants to give it a go). Please run the unit tests
on other platforms and browsers as I only have access to FF3 and
Safari 3.
My speed measures, using the SlickSpeed framework, indicate a 55x
speedup - going from 3.9 sec with the old Selector module to 71 ms for
the new Sizzle based one. The SlickSpeed benchmark tests many
selectors, but far from all of them (i.e. it does not has enough
coverage to be a unit test). Please run the benchmark on your browser
if you like, just visit [2] and everything should be set up already.
[2] http://www.hvergi.net/arnar/public/sizzle/speed/#
So.. please try this out and comment. What changes do you think are
necessary (if any), including my suggestions above, before comitting
this to trunk?
cheers,
Arnar
Per Cederberg
here comes a bunch of random comments:
1. The test result from p[lang!=is-IS] has been modified. The
MochiKit.Selector implementation was recently fixed just here,
changing the semantics of all attribute operators to imply attribute
existance. One can argue either way, I guess, but I kind of liked the
existing semantics here.
2. From your speed comparison page for the various framework
implementations, I noded a number of issues in IE 6 (have not tested
IE 7 yet). Sizzle returns errors in 4 of the tests there. In the
README it also says: "It's a work in progress! Not ready for use yet!"
3. Also on the speed page, I noted that the trunk MochiKit.Selector is
reported to throw errors on a bunch of tests. And there are also a few
places where it returns a different number of results than the others.
This is problematic, since it implies that the exising
MochiKit.Selector is not only slow but also incorrect. I have no great
desire for fixing more issues there, but neither am I a fan of pushing
new (and also not finished) stuff immediately out into a release.
Finally, I'm really trying to push for a release real-soon-now (see my
other mails on the list)...
4. Have you investigated all the cases where the frameworks differ in
the number of returned elements? Just so that we can be reasonably
sure that the new implementation is indeed correct. Also... I seem to
get 1 result for nearly all tests on MochiKit.Selector w/ Sizzle when
running in Safari 3. Could it be that querySelectorAll returns a
NodeList, not an Array?
5. I agree that the global namespace pollution is an issue. If only
the Sizzle namespace was used in the code, we could easily refactor it
to use MochiKit.Selector.Sizzle or similar to further avoid any
issues.
6. There seems to be an unnecessary whitespace change in MochiKit/MochiKit.js.
7. The MochiKit.Selector.findDocElements function is not is the
current API docs. So why do we even export it? Or is that omission by
mistake? (This is a trunk issue)
8. The Sizzle result cache seems to break if people start manipulating
the returned arrays (using shift, pop or similar). Basically this:
return cache && doCache ?
(cache[ selector ] = results) :
results;
should become:
if (cache && doCache) {
cache[selector] = results.slice(0);
}
return results;
9. Note that when testing $$ in Safari, you are really only using the
built-in support for document.querySelectorAll (unless that function
throws an error). So adding or modifying functionality requires that
the query is checked before using document.querySelectorAll.
10. I'm not afraid of regexps, but the "chunker" one in the code is
just ridiculous. It is much too dense to be in any kind of production
code without proper comments and/or explanations as to its function
and use.
11. This line (118):
if ( parts.length > 0 && !checkSet ) {
should be just:
if ( parts.length > 0) {
it seems.
... and now I've got to take a break. This email is already quite
long. I'll continue looking at the code some other day.
Cheers,
/Per
PS. I just discovered that Google Groups silently dropped all my
emails that used another sender address, so I'm currently resending
all my recent postings. Hence the sudden email bombing...
Arnar Birgisson
On Wed, Oct 8, 2008 at 15:49, Per Cederberg <cede...@gmail.com> wrote:
> This is great work! But since I just took on my code-review-glasses,
> here comes a bunch of random comments:
>
> 1. The test result from p[lang!=is-IS] has been modified. The
> MochiKit.Selector implementation was recently fixed just here,
> changing the semantics of all attribute operators to imply attribute
> existance. One can argue either way, I guess, but I kind of liked the
> existing semantics here.
Ah, ok. I just thought the test was buggy. != is not actually defined
by CSS3, so I went by the jQuery definition which is this:
"Matches elements that either don't have the specified attribute or do
have the specified attribute but not with a certain value."
I.e. it explicitly says it matches elements w/o that attribute. Of
course this is no standard, if you think we should use the other
semantics I don't mind.
> 2. From your speed comparison page for the various framework
> implementations, I noded a number of issues in IE 6 (have not tested
> IE 7 yet). Sizzle returns errors in 4 of the tests there. In the
> README it also says: "It's a work in progress! Not ready for use yet!"
Yes, I'm actually not sure what the status on Sizzle development is -
but John has at least not updated the github version since posting it
originally. Unfortunately I don't have access to IE6 or time to set up
a virtual machine - but if you try debugging it I'd be happy to help
with the little insight I gained last night.
> 3. Also on the speed page, I noted that the trunk MochiKit.Selector is
> reported to throw errors on a bunch of tests. And there are also a few
> places where it returns a different number of results than the others.
>
> This is problematic, since it implies that the exising
> MochiKit.Selector is not only slow but also incorrect. I have no great
> desire for fixing more issues there, but neither am I a fan of pushing
> new (and also not finished) stuff immediately out into a release.
> Finally, I'm really trying to push for a release real-soon-now (see my
> other mails on the list)...
Yes. If the benchmark tests are to be trusted, that simply means that
the trunk version (which should behave like Prototype did, although
they may have changed the selector engine in Prototype now) is
incorrect. I'll look into what tests are failing and what the
standards have to say about them. If it really is incorrect, all the
more reason to move to a new implementation.
I would also vote against pushing this to a release as it stands now.
> 4. Have you investigated all the cases where the frameworks differ in
> the number of returned elements? Just so that we can be reasonably
> sure that the new implementation is indeed correct. Also... I seem to
> get 1 result for nearly all tests on MochiKit.Selector w/ Sizzle when
> running in Safari 3. Could it be that querySelectorAll returns a
> NodeList, not an Array?
I will have a look at the tests and confirm what is the correct
outcome. Safari was also failing on several unit tests for me, I will
also look at that tonight.
> 5. I agree that the global namespace pollution is an issue. If only
> the Sizzle namespace was used in the code, we could easily refactor it
> to use MochiKit.Selector.Sizzle or similar to further avoid any
> issues.
Ok, I'll start work on that too. I think I would move everything
inside Sizzle (and submit a patch to John) and then move Sizzle into
MochiKit.Selector. It should be easy, just a little legwork.
> 6. There seems to be an unnecessary whitespace change in MochiKit/MochiKit.js.
There are? The only change I can see is the reording of submodules -
since Selector now depends on Style I had to switch their order. Or do
you mean packed/MochiKit/MochiKit.js?
> 7. The MochiKit.Selector.findDocElements function is not is the
> current API docs. So why do we even export it? Or is that omission by
> mistake? (This is a trunk issue)
That is probably a mistake, it should definitely be in the docs. I
will fix this in the trunk.
> 8. The Sizzle result cache seems to break if people start manipulating
> the returned arrays (using shift, pop or similar). Basically this:
>
> return cache && doCache ?
> (cache[ selector ] = results) :
> results;
>
> should become:
>
> if (cache && doCache) {
> cache[selector] = results.slice(0);
> }
> return results;
Ok, I'll have a look. Otherwise this is a comment for John I guess.
> 9. Note that when testing $$ in Safari, you are really only using the
> built-in support for document.querySelectorAll (unless that function
> throws an error). So adding or modifying functionality requires that
> the query is checked before using document.querySelectorAll.
Right, I'm not sure what is best to do here. Should we rely on
querySelectorAll where it is available - simply to have the best
performance, or is identical semantics on all browsers more important?
For the latter there are two ways, don't use querySelectorAll (and
take the performance penalty) or strip out the features not supported
by it (and take the feature penalty).
> 10. I'm not afraid of regexps, but the "chunker" one in the code is
> just ridiculous. It is much too dense to be in any kind of production
> code without proper comments and/or explanations as to its function
> and use.
This is another comment for John I guess. I don't necessarily agree
with you though, its function was relatively clear after pasting it
into http://regexpal.com/ and playing with it for a minute.
> 11. This line (118):
>
> if ( parts.length > 0 && !checkSet ) {
>
> should be just:
>
> if ( parts.length > 0) {
>
> it seems.
Right, there are several "bugs" of this kind in Sizzle already.
> ... and now I've got to take a break. This email is already quite
> long. I'll continue looking at the code some other day.
Thank you very much for excellent, detailed comments. Reviewing is
just as hard as coding, but more boring :)
cheers,
Arnar
jer...@gmail.com
> Ah, ok. I just thought the test was buggy. != is not actually defined
> by CSS3, so I went by the jQuery definition which is this:
>
> "Matches elements that either don't have the specified attribute or do
> have the specified attribute but not with a certain value."
>
> I.e. it explicitly says it matches elements w/o that attribute. Of
> course this is no standard, if you think we should use the other
> semantics I don't mind.
=value] was that it was equivalent to :not([attr=value]).
> Yes, I'm actually not sure what the status on Sizzle development is -
> but John has at least not updated the github version since posting it
> originally. Unfortunately I don't have access to IE6 or time to set up
> a virtual machine - but if you try debugging it I'd be happy to help
> with the little insight I gained last night.
to be able to land this for jQuery 1.3 this month). What sort of
timeline are you guys on for your 1.4 release?
> I would also vote against pushing this to a release as it stands now.
> I will have a look at the tests and confirm what is the correct
> outcome. Safari was also failing on several unit tests for me, I will
> also look at that tonight.
that.
> Ok, I'll start work on that too. I think I would move everything
> inside Sizzle (and submit a patch to John) and then move Sizzle into
> MochiKit.Selector. It should be easy, just a little legwork.
selectively exposing it where needed, specifically:
(function(){
...
var Sizzle = this.Sizzle = function(){
...
};
})();
No need to namespace the random state variables that are used
internally.
> > 8. The Sizzle result cache seems to break if people start manipulating
> > the returned arrays (using shift, pop or similar). Basically this:
>
> > return cache && doCache ?
> > (cache[ selector ] = results) :
> > results;
>
> > should become:
>
> > if (cache && doCache) {
> > cache[selector] = results.slice(0);
> > }
> > return results;
>
> Ok, I'll have a look. Otherwise this is a comment for John I guess.
> Right, I'm not sure what is best to do here. Should we rely on
> querySelectorAll where it is available - simply to have the best
> performance, or is identical semantics on all browsers more important?
> For the latter there are two ways, don't use querySelectorAll (and
> take the performance penalty) or strip out the features not supported
> by it (and take the feature penalty).
Sizzle just falls back to the old selector code, if qSA doesn't know
how to handle a selector. If something isn't working correctly in this
regard then it should definitely be fixed.
> This is another comment for John I guess. I don't necessarily agree
> with you though, its function was relatively clear after pasting it
I could attempt to explain it a little bit more with comments, I
guess. I'll see what I can do.
> Right, there are several "bugs" of this kind in Sizzle already.
Thanks for the review guys, I appreciate it!
--John
Arnar Birgisson
Glad to see you are following our discussion. Btw. are you on the
MochiKit mailinglist, or should I keep cc-ing you explicitly when
there are issues for you?
On Wed, Oct 8, 2008 at 16:41, jer...@gmail.com <jer...@gmail.com> wrote:
> Obviously we can discuss this some more but my interpretation of [attr!
> =value] was that it was equivalent to :not([attr=value]).
My intuition is to agree with this.
> I got bogged down here recently but I'm starting back up (since I want
> to be able to land this for jQuery 1.3 this month). What sort of
> timeline are you guys on for your 1.4 release?
The current suggestion is to go into a two-week feature freeze asap
(so in they very next days I guess) and then release 1.4. The
Sizzle-based Selector will most likely not be a part of that, but I
think the intention is to release 1.5 pretty soon.
>> Right, I'm not sure what is best to do here. Should we rely on
>> querySelectorAll where it is available - simply to have the best
>> performance, or is identical semantics on all browsers more important?
>> For the latter there are two ways, don't use querySelectorAll (and
>> take the performance penalty) or strip out the features not supported
>> by it (and take the feature penalty).
>
> Right now there is no feature penalty to using querySelectorAll.
> Sizzle just falls back to the old selector code, if qSA doesn't know
> how to handle a selector. If something isn't working correctly in this
> regard then it should definitely be fixed.
Ah ok, I will have a look at Safari tonight and figure out what's failing.
>> Right, there are several "bugs" of this kind in Sizzle already.
>
> I'll look in to the one mentioned here.
I'll send you a patch with a few corrections (such as function (elem,
i, match) { .. m[i] .. // should be match[i] }) and stuff that I
noticed. Also see the bug about not removing processed parts from the
string which I sent you yesterday.
cheers,
Arnar
Per Cederberg
> Ah, ok. I just thought the test was buggy. != is not actually defined
> by CSS3, so I went by the jQuery definition which is this:
>
> "Matches elements that either don't have the specified attribute or do
> have the specified attribute but not with a certain value."
>
> I.e. it explicitly says it matches elements w/o that attribute. Of
> course this is no standard, if you think we should use the other
> semantics I don't mind.
I'm used to XPath semantics, which seem to require existence. Now, if
everybody else use a different interpretation, that is of course ok
with me.
> Yes, I'm actually not sure what the status on Sizzle development is -
> but John has at least not updated the github version since posting it
> originally. Unfortunately I don't have access to IE6 or time to set up
> a virtual machine - but if you try debugging it I'd be happy to help
> with the little insight I gained last night.
I run IE 6 in CodeWeavers CrossOver Mac. Quite nice to have the full
battery of browsers available... (except IE 7 though)
> Yes. If the benchmark tests are to be trusted, that simply means that
> the trunk version (which should behave like Prototype did, although
> they may have changed the selector engine in Prototype now) is
> incorrect. I'll look into what tests are failing and what the
> standards have to say about them. If it really is incorrect, all the
> more reason to move to a new implementation.
Naturally, but I would have to fix the old implementation for the 1.4
release. Or not include it at all. Or document it as "experimental".
Or something.
>> 6. There seems to be an unnecessary whitespace change in MochiKit/MochiKit.js.
>
> There are? The only change I can see is the reording of submodules -
> since Selector now depends on Style I had to switch their order. Or do
> you mean packed/MochiKit/MochiKit.js?
My bad. I got confused by the web diff view in Trac.
> That is probably a mistake, it should definitely be in the docs. I
> will fix this in the trunk.
Great, thanks!
>> 9. Note that when testing $$ in Safari, you are really only using the
>> built-in support for document.querySelectorAll (unless that function
>> throws an error). So adding or modifying functionality requires that
>> the query is checked before using document.querySelectorAll.
>
> Right, I'm not sure what is best to do here. Should we rely on
> querySelectorAll where it is available - simply to have the best
> performance, or is identical semantics on all browsers more important?
> For the latter there are two ways, don't use querySelectorAll (and
> take the performance penalty) or strip out the features not supported
> by it (and take the feature penalty).
What I was trying to say is that if querySelectorAll returns a result
that is inconsistent with the JS implementation, then we're in
trouble. If it throws an error (as it presumably does when using
non-standard stuff), we already fallback to the JS-implementation, so
that should work.
We really need to make sure that our test suite has good test
coverage. Then if we encounter issues in Safari, we can handle those
by searching for those patterns in the query before calling
querySelectorAll.
>> 10. I'm not afraid of regexps, but the "chunker" one in the code is
>> just ridiculous. It is much too dense to be in any kind of production
>> code without proper comments and/or explanations as to its function
>> and use.
>
> This is another comment for John I guess. I don't necessarily agree
> with you though, its function was relatively clear after pasting it
> into http://regexpal.com/ and playing with it for a minute.
Possibly. But I'd still like some comments... :-)
BTW, John. I saw your recent Sizzle commit and commented it on github.
Cheers,
/Per
Arnar Birgisson
On Wed, Oct 8, 2008 at 18:07, Per Cederberg <cede...@gmail.com> wrote:
> What I was trying to say is that if querySelectorAll returns a result
> that is inconsistent with the JS implementation, then we're in
> trouble. If it throws an error (as it presumably does when using
> non-standard stuff), we already fallback to the JS-implementation, so
> that should work.
Ah yes. Right, the fallback seems to work. The problem on Safari was
indeed that it returns a NodeList object but is treated as an array. I
simply added a call to convert it to an array.
As for the incorrectness in the current MochiKit.Selector module in
trunk, these are the tests in the SlickSpeed benchmark that fail or
return an incorrect number of elements:
div p
MK trunk returns 142 elements while others return 140 (both numbers
are reported incorrect though). The *set* of elements is the same, but
MK repeats some elements, which is of course not correct. This seems
to happen in the situation where one has
<div><div><p>...</p></div></div>. If one forgets about performance,
this is trivial to fix in MK - but a reasonably performing one might
be more tricky.
div ~ p
MK returns 4120 (!) elements here where others return 183 (both
numbers are again considered wrong by SlickSpeed). This is clearly a
bug
div, p, a
div.example, div.note
This fails in MK trunk, and rightly so, as it does not support
multiple selectors in one string. Multiple selectors should be passed
by using an array argument to findDocElements.
h1[id]:contains(Selectors)
This fails, as :contains(..) is not supported.
p:contains(selectors)
This returns an invalid number of elements (presumably all p elements)
since :contains is not selected. Prototype has the same behavior
p:nth-child(2n)
This fails as 2n is not recognized, should be easy to fix (just
interpret as 2n+0).
p:nth-child(n)
Fails, and frankly I don't understand the reasoning here (what does
this mean?). Should be interpreted as (1n+0) I guess.
p:only-child
returns wrong number of results, but then again - so do all the
others. Perhaps SlickSpeed is incorrect
(the empty string)
Fails with an error, which I think is better than returning all
elements as some libraries do. Could be handled as a special case but
I don't see the need.
cheers,
Arnar
Amit Mendapara
MochiKit.Query module. As `Per Cederberg` is preparing for 1.4
release, I think MochiKit.Selector should not be included in 1.4, but
let we get something really useful with Sizzle which is going to be
integrated in MochiKit (hopefully MochiKit 1.5)...
- Amit Mendapara
Arnar Birgisson
> I have seen many problems with MochiKit.Selector while testing
> MochiKit.Query module.
You would be helping if you submitted these as bug-reports.
Arnar
Per Cederberg
here on the list. I've got 1-2 previously here in this thread that I
intend to have a look at soon.
Either way, I think we are beyond removing MochiKit.Selector entirely
for 1.4. I'll update the docs to point out that it is an
*experimental* module that is subject to change. Also, I'll add
specific notes for those selectors that won't be compatible with the
new Sizzle-powered version.
Cheers,
/Per
Arnar Birgisson
> Also, I'll add
> specific notes for those selectors that won't be compatible with the
> new Sizzle-powered version.
Assuming the bug in Sizzle that causes [attribute] to fail will be
fixed, the only ones that will not work are :root and :*-of-type. I
think we are decided to Sizzle and these are both rare and probably
take an effort to add to Sizzle. -- so, you could go further than
adding a notice and just deprecate them right away.
Arnar
Amit Mendapara
On Oct 13, 5:52 pm, "Per Cederberg" <cederb...@gmail.com> wrote:
> I'd appreciate bug reports for the MochiKit.Selector module in Trac or
> here on the list. I've got 1-2 previously here in this thread that I
> intend to have a look at soon.
was unable to change/comment it later. That's why I never submitted
again.
Anyway, I will prepare one on MochiKit.Selector this night and will
post here in this thread instead...
- Amit Mendapara
Per Cederberg
<mendapa...@gmail.com> wrote:
> Once, I filed a bug report on the trac (related to Sortables), but I
> was unable to change/comment it later. That's why I never submitted
> again.
Yes, this is very unfortunate. I used to have the same problem, so I
hear you loud and clear.
The problem is the amount of spam that we'd otherwise receive in bug
reports. Don't know if there is some newer version of Trac that fixes
this that we could install on the server. Or if there is some other
solution that would work. Until that happens, emailing to this list or
directly to the bug owner should work. Sorry about that.
Cheers,
/Per
Arnar Birgisson
Do we have a procedure for adding user accounts to Trac, other than
simply "Ask Bob"?
cheers,
Arnar
Amit Mendapara
you can easily prevent those spams. You can see how TurboGears trac is
configured...
You can also think about moving MochiKit to Launchpad.net. It's really
a good platform to host OpenSource projects with distributed vcs, bug
tracking, blueprints and more. Launchpad team has already created a
project for MochiKit (so that no one then MochiKit team can claim the
ownership, you can contact Launchpad team to get ownership rights).
- Amit Mendapara
On Oct 13, 6:24 pm, "Per Cederberg" <cederb...@gmail.com> wrote:
> On Mon, Oct 13, 2008 at 3:13 PM, Amit Mendapara
>
Bob Ippolito
auth to login and they are the same credentials that give svn commit
access. Disabling anonymous commenting is something that I did because
I couldn't be bothered to implement a better spam filter or maintain
it.
I'm not really sold on launchpad, I think bzr would be too much of a
barrier to entry for many people. I would certainly consider moving to
google code though, because that would be easy enough. All of our
other open source projects are there these days. People that want to
use distributed vcs to keep a local branch can do so easily enough
with a central svn repo.
Arnar Birgisson
> I'm not really sold on launchpad, I think bzr would be too much of a
> barrier to entry for many people. I would certainly consider moving to
> google code though, because that would be easy enough. All of our
> other open source projects are there these days. People that want to
> use distributed vcs to keep a local branch can do so easily enough
> with a central svn repo.
I want to support this, i.e. if the plan is to abandon Trac - Google
Code is much more fitting than Launchpad.
cheers,
Arnar
Amit Mendapara
codding, reporting/fixing bugs, suggesting new ideas or anything
should never been restricted anyway because of those few spammers...
- Amit Mendapara
On Oct 13, 8:19 pm, "Bob Ippolito" <b...@redivi.com> wrote:
> Well, the login database is outside of trac since we're using basic
> auth to login and they are the same credentials that give svn commit
> access. Disabling anonymous commenting is something that I did because
> I couldn't be bothered to implement a better spam filter or maintain
> it.
>
> I'm not really sold on launchpad, I think bzr would be too much of a
> barrier to entry for many people. I would certainly consider moving to
> google code though, because that would be easy enough. All of our
> other open source projects are there these days. People that want to
> use distributed vcs to keep a local branch can do so easily enough
> with a central svn repo.
>
> On Mon, Oct 13, 2008 at 6:47 AM, Amit Mendapara
>
>
> > The version of trac is okay. Seehttp://trac.edgewall.org/wiki/TracPermissions,
Bob Ippolito
Per Cederberg
deprecations. More comments inlined below:
On Wed, Oct 8, 2008 at 9:28 PM, Arnar Birgisson <arn...@gmail.com> wrote:
> div p
> MK trunk returns 142 elements while others return 140 (both numbers
> are reported incorrect though). The *set* of elements is the same, but
> MK repeats some elements, which is of course not correct. This seems
> to happen in the situation where one has
> <div><div><p>...</p></div></div>. If one forgets about performance,
> this is trivial to fix in MK - but a reasonably performing one might
> be more tricky.
I'll let this one pass. It is slightly incorrect, but can be
worked-around by users until 1.5 comes along.
> div ~ p
> MK returns 4120 (!) elements here where others return 183 (both
> numbers are again considered wrong by SlickSpeed). This is clearly a
> bug
I'll try to fix this. Filed as http://trac.mochikit.com/ticket/321
The remaining issues seem to be feature omissions in the current
MochiKit.Selector implementation, so I'll just ignore those.
Cheers,
/Per
Arnar Birgisson
On Wed, Oct 15, 2008 at 10:15, Per Cederberg <cede...@gmail.com> wrote:
> On Wed, Oct 8, 2008 at 9:28 PM, Arnar Birgisson <arn...@gmail.com> wrote:
>> div p
>> MK trunk returns 142 elements while others return 140 (both numbers
>> are reported incorrect though). The *set* of elements is the same, but
>> MK repeats some elements, which is of course not correct. This seems
>> to happen in the situation where one has
>> <div><div><p>...</p></div></div>. If one forgets about performance,
>> this is trivial to fix in MK - but a reasonably performing one might
>> be more tricky.
>
> I'll let this one pass. It is slightly incorrect, but can be
> worked-around by users until 1.5 comes along.
I agree.
>> div ~ p
>> MK returns 4120 (!) elements here where others return 183 (both
>> numbers are again considered wrong by SlickSpeed). This is clearly a
>> bug
>
> I'll try to fix this. Filed as http://trac.mochikit.com/ticket/321
Thanks, let me know if you have problems. I'm sorry I don't have time
to help more these days, I have a few deadlines and a conference
coming up in the next two weeks.
cheers,
Arnar
Per Cederberg
adding a uniqueness filter on the results from
MochiKit.Selector.findChildElements. Unless the number of returned
results is very high it should not degrade performance much. And
naturally, this should also fix the issue for "div p".
Arnar, could you add a new trunk MochiKit to your speed comparison page?
Otherwise, I look forward to replacing the current implementation that
is based on eval usage, poor query parsing and slow set handling. Not
much fun to debug at all, actually...
Cheers,
/Per
Arnar Birgisson
> I think I fixed the MochiKit.Selector issue with "div ~ p" in r1429 by
> adding a uniqueness filter on the results from
> MochiKit.Selector.findChildElements. Unless the number of returned
> results is very high it should not degrade performance much. And
> naturally, this should also fix the issue for "div p".
>
> Arnar, could you add a new trunk MochiKit to your speed comparison page?
Done. Yup, the number of nodes is not correct (or at least as correct
as th other frameworks). Performance takes a minor hit, mainly because
it is so bad to begin with :)
> Otherwise, I look forward to replacing the current implementation that
> is based on eval usage, poor query parsing and slow set handling. Not
> much fun to debug at all, actually...
Me too. The current implementation is quite bad and it is my fault for
not updating it since the early Prototype port. Sizzle is quite
simple, there are no specific "tricks" to make it fast, but the design
is nice.
cheers,
Arnar
Per Cederberg
> Done. Yup, the number of nodes is not correct (or at least as correct
> as th other frameworks). Performance takes a minor hit, mainly because
> it is so bad to begin with :)
Ah, you mean "now correct"... :-)
Disturbing that this fix actually affects performance in a noticable
way. It really shouldn't, except when more than ~100 elements are
matched. Perhaps there are some obvious improvements to be made in my
uniq() implementation:
var uniq = function(arr) {
var res = [];
for (var i = 0; i < arr.length; i++) {
if (MochiKit.Base.findIdentical(res, arr[i]) < 0) {
res.push(arr[i]);
}
}
return res;
};
I think the above should be O(n^2). Not optimal, but fixing the root
cause means rewriting the module altogether. :-(
> Me too. The current implementation is quite bad and it is my fault for
> not updating it since the early Prototype port. Sizzle is quite
> simple, there are no specific "tricks" to make it fast, but the design
> is nice.
Yes, it is much clearer. I have a few opinions about it of course, but
I think I'll fork the Sizzle project on github to fix those when/if I
have time.
Many thanks for your help, Arnar!
Cheers,
/Per
Arnar Birgisson
> On Wed, Oct 15, 2008 at 1:01 PM, Arnar Birgisson <arn...@gmail.com> wrote:
>> Done. Yup, the number of nodes is not correct (or at least as correct
>> as th other frameworks). Performance takes a minor hit, mainly because
>> it is so bad to begin with :)
>
> Ah, you mean "now correct"... :-)
Heh, yes - I do :) I think I left my typing fu in Iceland.
> Disturbing that this fix actually affects performance in a noticable
> way. It really shouldn't, except when more than ~100 elements are
> matched. Perhaps there are some obvious improvements to be made in my
> uniq() implementation:
>
> var uniq = function(arr) {
> var res = [];
> for (var i = 0; i < arr.length; i++) {
> if (MochiKit.Base.findIdentical(res, arr[i]) < 0) {
> res.push(arr[i]);
> }
> }
> return res;
> };
>
> I think the above should be O(n^2). Not optimal, but fixing the root
> cause means rewriting the module altogether. :-(
We should definitely not invest in a big rewrite at this point. As for
the uniq, this is probably as good as it gets unless we can assume
that identical items are always consecutive. If they are, there's the
obvious O(n) method of course.
> Many thanks for your help, Arnar!
You are the one that deserves thanks for your efforts :)
cheers,
Arnar
Arnar Birgisson
> On Tue, Aug 26, 2008 at 13:38, Chris Lee-Messer
>> p.s. I'm running firefox 3.1pre now with the JIT running, and
>> slickspeed tests are interesting as your version of MochiKit.Selector
>> is reported as faster than Sizzle: most of the response times are 1ms
>> for a total time of 45ms vs 174ms for Sizzle itself. I would guess
>> that some compiled code is being cached. MochiKit's current Selector
>> comes in with a time of 4045ms.
I just upgraded to FF 3.1b1 and I'm seeing this strange thing. For me,
MochiKit trunk completes in 1893ms, MochiKit w/Sizzle in 54ms but
Sizzle itself in ~100ms - no matter if it is run before or after the
MochiKit/Sizzle combo.
So, MochiKit+Sizzle is almost twice as fast as Sizzle standalone.
John, can you think of a good reason for this?
cheers,
Arnar
John Resig
Maybe something is failing?
--John
Arnar Birgisson
On Mon, Oct 20, 2008 at 15:52, John Resig <jer...@gmail.com> wrote:
> That's... odd. Are there any selectors that are noticeably faster?
Yes, it seems that nested queries are to blame. By nested queries I
mean queries that uses the axis combinator, either the implicit
"descendant" axis (like "div p") or an explicit axis combinator such
as ~, > or +.
"div ~ p" is 2ms on MK+Sizzle vs. 13ms on Sizzle.
"div p" is 2ms on MK+Sizzle vs. 4ms on Sizzle.
"div > p" is 1ms vs. 3ms
"div + p" is 1ms vs. 5ms
"div p a" is 1ms vs. 8ms
Also, a[href][lang][class] is 1ms vs. 9ms.
> Maybe something is failing?
I don't think so, at least the number of elements returned by each is
the same in every test.
You can run the test benchmark yourself here:
http://www.hvergi.net/arnar/public/sizzle/speed/
cheers,
Arnar
John Resig
Do you have a diff of any change(s) that you've made to your copy of Sizzle?
--John
Per Cederberg
http://ejohn.org/blog/queryselectorall-in-firefox-31/
And in fact, there is a slight bug in Sizzle here, causing it to not
use that version when not sending in an explicit 2:nd argument:
Sizzle("...", document)
The problem is here:
if ( document.querySelectorAll ) (function(){
var oldSizzle = Sizzle;
Sizzle = function(query, context, extra){
if ( context === document ) {
try {
return makeArray(context.querySelectorAll(query));
} catch(e){}
}
return oldSizzle(query, context, extra);
};
Sizzle.find = oldSizzle.find;
Sizzle.filter = oldSizzle.filter;
})();
Cheers,
/Per
Arnar Birgisson
On Mon, Oct 20, 2008 at 16:43, John Resig <jer...@gmail.com> wrote:
> Just to clarify: Are you turning on JIT in 3.1?
JIT is off.
> Do you have a diff of any change(s) that you've made to your copy of Sizzle?
Yup. http://www.hvergi.net/arnar/public/sizzle/diff.txt
cheers,
Arnar
John Resig
fast without using querySelectorAll, in that case. Just committed the
fix:
http://github.com/jeresig/sizzle/commit/6239a25918f8fd7d56fc97c22815418833a64e00
--John
John Resig
http://github.com/jeresig/sizzle/commit/93e33dc2a41e2b0aa0e1e1c66368f5d224da80e1
The exception is :visible and :hidden - which should be handled by the
host library.
Also, I wrapped the entireity of Sizzle withing a (function(){ ...
})() which means that no global variables are exposed (save for
Sizzle).
What specific code do you use to hook Sizzle in to your engine? What
I'll probably do is just hook it directly in to the right spot (for
example, overwrite jQuery.find, in the case of jQuery) rather than
introduce a new global variable.
--John
Arnar Birgisson
On Mon, Oct 20, 2008 at 17:19, John Resig <jer...@gmail.com> wrote:
> Excellent list - I just integrated virtually all of your points:
> http://github.com/jeresig/sizzle/commit/93e33dc2a41e2b0aa0e1e1c66368f5d224da80e1
Excellent. Thanks!
> The exception is :visible and :hidden - which should be handled by the
> host library.
I agree.
> Also, I wrapped the entireity of Sizzle withing a (function(){ ...
> })() which means that no global variables are exposed (save for
> Sizzle).
>
> What specific code do you use to hook Sizzle in to your engine? What
> I'll probably do is just hook it directly in to the right spot (for
> example, overwrite jQuery.find, in the case of jQuery) rather than
> introduce a new global variable.
Actually, I copied the contents of Sizzle into Selector.js which is
part of MochiKit [1]. Integrating is then just a matter of calling
Sizzle(...) in the correct place in the Selector API. Our plan is to
completely remove the old Selector implementation, i.e. using Sizzle
won't be optional like it looks like your plan for jQuery is.
[1] http://trac.mochikit.com/browser/mochikit/branches/selector_sizzle/MochiKit/Selector.js
I don't know what other MochiKitters say about including Sizzle.js as
a seperate file. Per, Bob?
cheers,
Arnar
Arnar Birgisson
On Mon, Oct 20, 2008 at 17:01, John Resig <jer...@gmail.com> wrote:
> Wow, thanks for catching that. That's amusing that it was still so
> fast without using querySelectorAll, in that case. Just committed the
> fix:
> http://github.com/jeresig/sizzle/commit/6239a25918f8fd7d56fc97c22815418833a64e00
Well, that makes a big difference :) Now both MK+Sizzle and Sizzle
standalone do the benchmark in 53 ms.
John, this is an interesting testament to your implementation:
On FF 3.1b1 with jit turned OFF the Sizzle selector code (i.e not
using querySelectorAll) completes in 55ms.
With jit turned ON *and* using the querySelectorAll - it improves only
a tiny bit to 53ms :)
cheers,
Arnar
John Resig
> part of MochiKit [1]. Integrating is then just a matter of calling
> Sizzle(...) in the correct place in the Selector API. Our plan is to
> completely remove the old Selector implementation, i.e. using Sizzle
> won't be optional like it looks like your plan for jQuery is.
It's only going to be optional during this development process - I'm
planning on integrating it (and making it mandatory) in the upcoming
jQuery 1.3 release.
> [1] http://trac.mochikit.com/browser/mochikit/branches/selector_sizzle/MochiKit/Selector.js
>
> I don't know what other MochiKitters say about including Sizzle.js as
> a seperate file. Per, Bob?
So it seems like the major difference is that your selector method
(findChildElements) is able to take an array of results, correct?
--John
John Resig
> standalone do the benchmark in 53 ms.
>
> John, this is an interesting testament to your implementation:
>
> On FF 3.1b1 with jit turned OFF the Sizzle selector code (i.e not
> using querySelectorAll) completes in 55ms.
> With jit turned ON *and* using the querySelectorAll - it improves only
> a tiny bit to 53ms :)
Ha! I certainly won't complain with those types of numbers.
It's funny, I was struggling to find out why some of my selectors were
slower when qSA was supposed to be used - I didn't realize that it was
a product of how the test suite was working (it always passes in the
extra document argument) and how the perf suite works (it doesn't pass
in document). Oh well, all better now!
--John
Arnar Birgisson
>> I don't know what other MochiKitters say about including Sizzle.js as
>> a seperate file. Per, Bob?
>
> So it seems like the major difference is that your selector method
> (findChildElements) is able to take an array of results, correct?
Yes, correct. This is something we inherited from Prototype, from
where the current Selector module was ported. Perhaps we should change
the API, I don't know..
cheers,
Arnar
John Resig
Sizzle is now passing the test suite 100% in IE 6, Firefox 3, and Safari 3.1. There is one failing test in Firefox 3.1b1 and in Opera 9.6 (both are specific browser bugs, and relatively minor, so I'm filing those with the vendors).
I've fixed all of the previously-discussed issues in this thread.
With compliance in order I want to look back and tackle two things:
- Library-specific hook code (it's looking likely that Sizzle might make its way in to jQuery, MochiKit, Dojo, and Prototype).
- Speed (the performance in IE can still use some work so I'm looking in to that)
--John
John Resig
http://ejohn.org/apps/sizzle/test/
and the performance suite is here:
http://ejohn.org/apps/sizzle/speed/
Taking a quick peek at IE 6 I see a lot of areas in which small improvements could yield large results ("div p", "div + p", ".class"). As it is it's faster than all the other major libraries. DOMAssistant has some tricks which could definitely help here so I'll investigate and report back.
--John
Per Cederberg
strategy of searching inside-out from the expression. I.e. using the
last part of the query first, and then filtering that set using the
previous parts. Perhaps other libraries search outside-in?
In the case of "ul .tocline2" the approach taken by Sizzle seems to
fare especially poorly (compared to the others). Of course, this issue
would only affect cases where we are using parent-child relations.
Cheers,
/Per - running IE6 inside CrossOver, so that might slow down my
results a bit too
John Resig
My guess would be that the speed difference stems from the Sizzle
strategy of searching inside-out from the expression. I.e. using the
last part of the query first, and then filtering that set using the
previous parts. Perhaps other libraries search outside-in?
Possibly - it definitely depends on the test page. It tends to fare poorly on this particular test page but since it's the de facto standard, not much we can do :-/
In the case of "ul .tocline2" the approach taken by Sizzle seems to
fare especially poorly (compared to the others). Of course, this issue
would only affect cases where we are using parent-child relations.
Well, I'd be more interested in watching cases like "div p" since that's two pure tag searches and would expose performance flaws in depth selectors pretty quickly. "ui .tocline2" is probably especially slow due to the fact that all class selectors are slow in IE 6, right now.
Giulio Cesare Solaroli
I would like to ask if the new implementation is "currentWindow" aware
or (like it seems form a quick look at the code) is still accessing
directly the document object.
I will try to make a few test case to see if I am doing something
wrong with my current code or there is really a problem here.
Regards,
Giulio Cesare
Arnar Birgisson
First: wow - your name is awesome :)
On Mon, Nov 3, 2008 at 11:56, Giulio Cesare Solaroli
<giulio...@gmail.com> wrote:
> I would like to ask if the new implementation is "currentWindow" aware
> or (like it seems form a quick look at the code) is still accessing
> directly the document object.
>
> I will try to make a few test case to see if I am doing something
> wrong with my current code or there is really a problem here.
I want the new implementation to be currentWindow aware (or rather
currentDocument) - and I tried to put the call in the right places.
However, I'm not entirely sure that Sizzle handles this correctly.
I.e. Sizzle might implicitly make the assumption that one is using
window.document in some places. I pointed this out to John and he
could not confirm either way, I think he means to have a look at it.
In the meantime, test-cases for this would be greatly appreciated. If
you find specific problems, please make a track issue and/or submit a
patch if you can.
cheers,
Arnar
Per Cederberg
remember having seem some check-ins dealing with similar issues.
Perhaps the time is right now to just go ahead and merge the new
selector branch into the 1.5 trunk? With the appropriate updates from
the base Sizzle implementation of course.
Also, please note that the MochiKit Customizer assumes that each
module starts with a call to _deps and ends with a call to _export, so
please make sure that the new Selector module also follows that
pattern.
Cheers,
/Per
Arnar Birgisson
On Tue, Nov 18, 2008 at 23:04, Per Cederberg <cede...@gmail.com> wrote:
> Perhaps the time is right now to just go ahead and merge the new
> selector branch into the 1.5 trunk? With the appropriate updates from
> the base Sizzle implementation of course.
If Sizzle is considered stable, I'd say it is time to do so. I haven't
been able to give any attention to this for the last few weeks as I've
been travelling in the States, Italy and the Netherlands -- with
various levels of connection -- and will not be able to until December
at least.
> Also, please note that the MochiKit Customizer assumes that each
> module starts with a call to _deps and ends with a call to _export, so
> please make sure that the new Selector module also follows that
> pattern.
Could you add a ticket for it? If no-one else takes care of it, then
as I said I won't be available until December.
cheers,
Arnar
Per Cederberg
other things to focus on. :-)
Hope you have a nice trip!
Cheers,
/Per