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

Did removing selector-set have a adverse effect on performance. #22

Open
koddsson opened this issue May 18, 2020 · 5 comments
Open

Did removing selector-set have a adverse effect on performance. #22

koddsson opened this issue May 18, 2020 · 5 comments

Comments

@koddsson
Copy link
Contributor

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 simple WeakMap in #20.

I'm gonna try to make some performance tests to see if there's a noticeable difference and post them here.

@dgraham
Copy link
Contributor

dgraham commented May 18, 2020

// 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

git clone https://github.com/dgraham/delegated-events.git
cd delegated-events
npm install
npm run bootstrap
npm run bench

@koddsson
Copy link
Contributor Author

You also need to run npm run bootstrap in order to pull the submodules containing the vendored libraries.

@koddsson
Copy link
Contributor Author

I ended up taking the jsperf test linked on the selector-set README and adding a new case for the Map lookup and found it performs about the same as linear.

SelectorSet x 50,848 ops/sec ±0.87% (63 runs sampled)
Linear x 104 ops/sec ±2.31% (52 runs sampled)
Map x 122 ops/sec ±1.76% (51 runs sampled)
Fastest is SelectorSet

@koddsson
Copy link
Contributor Author

I wanted to check what the difference this change has on code on github.com and added the following performance measuring to the getMatches call and ran some form submissions. I ran these on a pull request in my local environment where there are approximately 100 forms.

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
Time running: 0.1300000585615635
Time running: 0.11499994434416294

SelectorSet

+  const start = window.performance.now()
   const matches = selectorSet && selectorSet.matches(form)
+  console.debug(`Time running: ${window.performance.now() - start}`)

Time running: 0.04000007174909115
Time running: 0.06000010762363672
Time running: 0.04000007174909115

So it seems in practice the current method is double the time SelectorSet takes to run the same operation but the difference is ~0.1 milliseconds. It might be worth being as performant as we can here but I would lean towards having less dependencies in remote-form.

selector-set is 1.5kB which takes 30ms to download on 3G according to Bundle Phobia.

@dgreif
Copy link
Contributor

dgreif commented Jun 17, 2022

@koddsson I just stumbled on this. Sounds like the perf differences were negligible. Good to close?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants