Faster implementation of filter.

[manolo]

This implementation improves performance a lot:

• Not enabling filtering detached elements flag (like jQuery does) is as faster as jQuery

• Enabling the flag is only 3 times slower instead of 50, It performs better in loops, specially decreasing time when creating ghost parents.

  Fixes issue #177 I have used Julien's test example for measuring times. https://github.com/manolo/testissue146/commits/master

[manolo]

I forgot to say that this approach reduces times from 400ms to 13ms, jQuery is 3ms though

[manolo]

I have tested jQuery and seems that Sizzle does not consider detached elements. The implementation now does the same by default and the performance with production code is the same than Sizzle. The new implementation supports an extra boolean parameter to enable old behaviour for compatibility with old gwtquery.

[christiangoudreau]

LGTM

[manolo]

This breaks a couple of tests, I will fix them, but right now I want to focus on the implementation. BTW, support for detached elements was introduced in a01efe08f9de2b60a99b6504d9c14cd5bb7dcc4b byt Julien , I'm asking Julien if it's ok to change the behaviour or we are going to break something else

[christiangoudreau]

Yeah, I though the test breaking was the ones that you wanted to fix already... Anyway, my LGTM is superficial, wait for Julien :D It's merely a : "The code looks good"

I don't think I have enough knowledge of the code base yet to do anything else than superficial code review, sorry :D

[jDramaix]

I don't remember why I introduce filter on detached element but I guess it's used in one of my plugin. Anyway, I think your implementation (using a boolean indicating if we have to take into account detached element) is correct

IIRC (maybe I'm wrong, it's been a long time since I worked on that), the running time of the filter method in jquery is constant and is independent of the number of element to filter. It doesn't seems to be the case here (could be 0(n2) in the wrongest case). I know you don't want to use Collection classes in jQuery but I'm pretty sure that using a HashSet instead of the double for loop should improve the performance when filter is used on a huge amount of elements.

[manolo]

Last code fixes broken tests. @jDramaix @christiangoudreau could you take a look? @ibaca can you test this code now? it does not use parentElements by default unless you specifically set the 'consider detached' flag to true. BTW, live methods in top tree with very repetitive events like mouseover, is known to be an issue not only in gQuery but jQuery, see section 'Event Performance' in http://api.jquery.com/on/

[manolo]

Doing more testing, I come to the conclusion that we should consider detached elements by default.

The thing is that gQuery is profusely used with detached widgets, and for example we use filter inside children(filter), etc which could break some code.

Also we use gQuery filters for xml databinding, so although jQuery does not considers filtering non detached element, it's possible that they don't use filter for other things like we do in many methods.

Finally the implementation has overloaded filter method to pass a flag to disable detached elements, or even we can disable it globally setting GQuery.filterDetached = true.

@ibaca could you test this ?

[ibaca]

Running filter method on +/- 500 elements with GQuery default: 74ms Running filter method on +/- 500 elements with GQuery old: 621ms Running filter method on +/- 500 elements with GQuery new (considerDetached=false): 3ms Running filter method on +/- 500 elements with GQuery new (considerDetached=true): 3ms More info here https://github.com/manolo/testissue146/pull/1

And the delegated event test with a 10x1000 table is below 25ms.

[manolo]

So IMO, and based on these numbers, this patch is pretty OK to go to production, do you all agree?

When @ibaca modifies his patch https://github.com/ArcBees/gwtquery/pull/302, we can have a way to inject customised filters when the user needs more performance.

[christiangoudreau]

Agreed

[manolo]

Merging, this to master. Note that filter implementation has been moved to SelectorEngine like @ibaca suggested at https://github.com/ArcBees/gwtquery/pull/302 so as users could override it in case.

[manolo]

@jDramaix changed ArrayLists by HashSets #316 in a quick test it performs better, thanks