Skip to content

/ jquery

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unify filtering by selector in `winnow` #3272

Closed
gibson042 opened this issue Aug 11, 2016 · 3 comments
Closed

Unify filtering by selector in `winnow` #3272

gibson042 opened this issue Aug 11, 2016 · 3 comments
Assignees
@gibson042
Labels
Traversing
Milestone
3.3.0

Comments

@gibson042
Copy link
Member

@gibson042 gibson042 commented Aug 11, 2016

Per https://github.com/jquery/jquery/pull/3261/files/c1339665c27440a23a5576ca1dfdad9c38567288#r74370245 , the complexity of separate branches for simple vs. non-simple selectors doesn't seem worthwhile. Both of them should defer directly to jQuery.filter.
@gibson042 gibson042 mentioned this issue Aug 11, 2016
Closed
4 of 4 tasks complete
@gibson042
Copy link
Member Author

@gibson042 gibson042 commented Aug 12, 2016 •
edited

Poor man's jsPerf benchmark: https://jsfiddle.net/uo2j7syk/
Typical run:

# {method}_{selector complexity}_{implementation}

find_simple_master: 1,125 ±2% (1,105–1,145) ops/sec
find_simple_direct: 1,140 ±2% (1,122–1,157) ops/sec
find_simple_intersect: 190 ±2% (186–193) ops/sec

not_simple_master: 524 ±2% (515–532) ops/sec
not_simple_direct: 529 ±2% (521–537) ops/sec
not_simple_intersect: 180 ±2% (177–183) ops/sec

find_complex_master: 483 ±2% (476–491) ops/sec
find_complex_direct: 743 ±1% (732–754) ops/sec
find_complex_intersect: 477 ±2% (470–485) ops/sec

not_complex_master: 440 ±2% (433–447) ops/sec
not_complex_direct: 393 ±2% (386–399) ops/sec
not_complex_intersect: 441 ±2% (433–449) ops/sec

The current code is taking a huge performance hit vs. always using only jQuery.filter( qualifier, elements, not ) for .find(nonSimple), but is noticeably faster for .not(nonSimple) (albeit by a smaller margin). I'm still in favor of completely dropping the set-intersection branch, but a case could be made for restricting it to the latter cases. Thoughts, @jquery/core?
@dmethvin
Copy link
Member

@dmethvin dmethvin commented Aug 13, 2016

Nice benchmark! I still like the idea of simplifying the code as well.
@markelog
Copy link
Member

@markelog markelog commented Aug 19, 2016

+1 for simplifying
@timmywil timmywil added this to the 3.2.0 milestone Sep 26, 2016
@timmywil timmywil modified the milestones: 3.3.0, 3.2.0 Mar 6, 2017
SaptakS added a commit to SaptakS/jquery that referenced this issue Dec 27, 2017
@SaptakS
Filter: Use direct filter in winnow
128bee7 
For both simple and complex selectors, use direct
filter in winnow

Closes jquerygh-3272
@SaptakS SaptakS mentioned this issue Dec 27, 2017
Closed
2 of 2 tasks complete
SaptakS added a commit to SaptakS/jquery that referenced this issue Jan 10, 2018
@SaptakS
Filter: Use direct filter in winnow
Unverified
This user has not uploaded their public key yet.
GPG key ID: B4A863E36FF55696 Learn about signing commits
5a8e33c 
For both simple and complex selectors, use direct
filter in winnow

Closes jquerygh-3272
@timmywil timmywil modified the milestones: 3.3.0, 3.4.0 Jan 16, 2018
@timmywil timmywil closed this in 4765bb5 Jan 17, 2018
@timmywil timmywil modified the milestones: 3.4.0, 3.3.0 Jan 17, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@gibson042 gibson042

Labels
Traversing
Projects
None yet
Milestone
3.3.0
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
@dmethvin @timmywil @markelog @gibson042
You can’t perform that action at this time.