core: optimize tap-targets audit

[brendankenny]

Optimize the tap-targets audit for pages with a large number of tap targets. Many pages don't hit a problem, but searching for the longest wikipedia pages found https://en.wikipedia.org/wiki/List_of_2017_albums, which takes 18s to process in the current audit. That might seem kind of unreasonable as a test, but as we've discovered from some of our axe timeouts, people do indeed make and then test pages with giganto tables of links.

Some results:
avclub
before: 87.30 ms
after: 10.80 ms

https://github.com/GoogleChrome/lighthouse/pull/5846
before: 99.22 ms
after: 22.86 ms

https://en.wikipedia.org/wiki/List_of_2017_albums
before: 18,772.44 ms
after: 502.01 ms

(numbers updated from changes after feedback/bug fix)

Note that I was optimizing for a page with a ton of non-overlapping tap targets. There may still be some important asymptotic behavior of overlapping targets to optimize, but I couldn't find a good example page.

The primary optimization was to hoist getTappableRectsFromClientRects out of the inner loops so it only needs to run n times, not n^2.

The secondary optimization was to add a quick bounding box to each tap target of at least FINGER_SIZE_PX size (containing all of that target's client rects). If two bounding boxes don't overlap at all there is no point in testing further (though if they do overlap, there still may be no overlap of the constituent client rects).

Everything else was just some speeding up of hot functions without changing any behavior.

Functionality/flow/concepts should mostly be unchanged.

Remaining possibilities if we find it's a problem anywhere:

• the bounding box test is still O(n^2). Can do it n log n if we cared enough.
• we could halve the number of overlap failure tests if we handed the symmetrical situation simultaneously instead of intersecting and then filtering later
• allRectsContainedWithinEachOther is about 10% of the runtime of audit of the wikipedia page and doesn't appear to ever do anything. Conversation in new_audit(tap-targets): check that tap targets are big enough and don't overlap #5846 (comment) made it seem like it might only be needed when we add position:absolute?
• all of the seemingly tight numeric functions (e.g. rectContains, rectsTouchOrOverlap) are actually a mess at runtime due to v8's int vs double optimizations (some rects have integer coordinates, others don't, and v8 can't always tell they're supposed to be the same object shape regardless). These functions are all super hot so we might benefit from optimizing for this, but it's going to be ugly so likely not worth it compared to the above.

---

initial numbers before feedback/bug fix
Some results:
avclub
before:66.51 ms
after: 21.48 ms

https://github.com/GoogleChrome/lighthouse/pull/5846
before: 101.30 ms
after: 39.43 ms

https://en.wikipedia.org/wiki/List_of_2017_albums
before: 18,432.65 ms
after: 1,192.31 ms

[brendankenny]

https://en.wikipedia.org/wiki/List_of_2017_albums

note, to actually test this, you have to do --emulated-form-factor desktop, otherwise wikipedia collapses the sections of the article and the table isn't actually on screen to be tested. But then there's not a mobile viewport, so you have to disable the viewport check in the audit so it actually runs.

[mattzeunert]

Nice, that's a pretty significant speed-up 😍

allRectsContainedWithinEachOther is about 10% of the runtime of audit of the wikipedia page and doesn't appear to ever do anything. Conversation in #5846 (comment) made it seem like it might only be needed when we add position:absolute?

Yeah, right now we just ignore position: absolute tap targets, and the gatherer already ignores elements that have an ancestor that's a tap target. We could only run that check if either tap target is actually position: absolute.

[mattzeunert]

We could have a case where a target has two small CRs that won't be merged together by getTappableRectsFromClientRects. A finger placed on the one of the CRs would reach outside the bounding box and we would no longer report a failure when previously we did. getBoundingRectWithPadding would be more accurate.

This is super hypothetical though, especially since we allow some finger overlap without failing.

[brendankenny]

getBoundingRectWithPadding would be more accurate.

d'oh, good catch. We should totally do that.

There shouldn't be much more overhead since most cases should still reject (on a normal page with links and buttons, the vast majority of the time any two randomly picked tap targets on the page aren't that near each other), so we should be precise. This also lets us tweak the overlap thresholds or rules later without worrying we're missing cases.

[brendankenny]

yeah, it's about 15% slower on the wikipedia case, but it's still 93% faster than before, so we're still good :)

[brendankenny]

yeah, it's about 15% slower on the wikipedia case

er, no, reversed signs is what you get when you don't write tests yet :) With the fixed bounding box code, we're down to 500ms for the wikipedia page

[patrickhulce]

LGTM! 🏃 💨💨

[patrickhulce]

nit: should the prop be called paddedBoundingRect? seems like it might eliminate the need for part of this comment

[brendankenny]

nit: should the prop be called paddedBoundingRect

👍