Skip to content
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

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
Labels
Milestone

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
Copy link
Member Author

@gibson042 gibson042 commented Aug 12, 2016

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
For both simple and complex selectors, use direct
filter in winnow

Closes jquerygh-3272
@SaptakS SaptakS mentioned this issue Dec 27, 2017
2 of 2 tasks complete
SaptakS added a commit to SaptakS/jquery that referenced this issue Jan 10, 2018
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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.