remote-form icon indicating copy to clipboard operation
remote-form copied to clipboard

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

Open koddsson opened this issue 5 years ago • 5 comments

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.

koddsson avatar May 18 '20 15:05 koddsson

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

dgraham avatar May 18 '20 16:05 dgraham

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

koddsson avatar May 19 '20 12:05 koddsson

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 avatar May 19 '20 13:05 koddsson

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.

koddsson avatar May 19 '20 14:05 koddsson

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

dgreif avatar Jun 17 '22 03:06 dgreif