-
Notifications
You must be signed in to change notification settings - Fork 19
-
Notifications
You must be signed in to change notification settings - Fork 19
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
Did removing selector-set
have a adverse effect on performance.
#22
Comments
// Naive loop
// O(n x m) -> at a tree depth of 20 and selector list of 100
// this is 2,000 matches calls.
for (const el of elements) {
for (const sel of selectors) {
if (el.matches(sel)) {
// do something
}
}
}
// SelectorSet
for (const el of elements) {
// set calls matches only against selectors that have a chance of
// matching, dropping 2,000 calls to just a handful.
if (set.matches(el)) {
// do something
}
}
} Prior art
|
You also need to run |
I ended up taking the jsperf test linked on the
|
I wanted to check what the difference this change has on code on github.com and added the following performance measuring to the Note that all the results are in milliseconds WeakMap function getMatches(el) {
+ const start = window.performance.now()
const results = []
for (const selector of formHandlers.keys()) {
if (el.matches(selector)) {
const handlers = formHandlers.get(selector) || []
results.push(...handlers)
}
}
+ console.debug(`Time running: ${window.performance.now() - start}`)
return results
} Time running: 0.17500005196779966 SelectorSet+ const start = window.performance.now()
const matches = selectorSet && selectorSet.matches(form)
+ console.debug(`Time running: ${window.performance.now() - start}`) Time running: 0.04000007174909115 So it seems in practice the current method is double the time
|
@koddsson I just stumbled on this. Sounds like the perf differences were negligible. Good to close? |
selector-set
is supposed to be very performant when you have large number of elements to match on. We might have taken a performance hit when we remove it as a dependency in favor of simpleWeakMap
in #20.I'm gonna try to make some performance tests to see if there's a noticeable difference and post them here.
The text was updated successfully, but these errors were encountered: