Make Fathom extraction more performant #319
Description
TL;DR: isVisible, which will likely be a common rule in many Fathom applications, accounts for the majority of the 460 ms of Fathom-related jank (67%). It may be possible to reduce this overall Fathom jank by as much as 374 ms (81%) by reducing style and DOM property accesses in isVisible, but this solution requires a privileged context.
Is this a feature request or a bug?
Feature request
What is the current behavior?
Fathom currently causes considerable jank on page load. Given that Price Tracker was an experiment, performance was not a major consideration in development of the current ruleset used to extract a product from a page.
Initial profile
Numbers
- The largest unresponsive chunk is caused by Fathom extraction, contributing 460 ms of jank right around page load.
isVisiblecalls Fathom’sancestorsfunction and a bunch ofCSS2Propertiesgetters, altogether taking up 311ms (67%) of this jank.
isVisible(fnode) {
for (const ancestor of ancestors(fnode.element)) {
const style = getComputedStyle(ancestor);
const isElementHidden = (
style.visibility === 'hidden'
|| style.display === 'none'
|| style.opacity === '0'
|| style.width === '0'
|| style.height === '0'
);
if (isElementHidden) {
return false;
}
}
return true;
}/**
* Yield an element and each of its ancestors.
*/
export function *ancestors(element) {
yield element;
let parent;
while ((parent = element.parentNode) !== null && parent.nodeType === parent.ELEMENT_NODE) {
yield parent;
element = parent;
}
}What is the expected or desired behavior?
Fathom should be able to run much faster with much less jank.
I shared this profile with some Firefox engineers, including a layout (Emilio) and a performance engineer (Greg).
Why is isVisible taking so much time?
- There is a known layout performance bug that causes
getComputedStyle()on adisplay: nonesubtree to be very slow.- Fix: Avoid checking
display: none.
- Fix: Avoid checking
- For each element, we are walking the ancestor chain. This means we are doing redundant work for many elements (e.g. for
nvisible descendants of<html>, we will check<html>'s stylesntimes).- Fix: Mitigate redundant work and/or memoize
isVisibleresult (e.g. in an HTML element => BooleanMapor anote()).
- Fix: Mitigate redundant work and/or memoize
getComputedStyletriggers a layout flush (not a natural layout flush).- Fix: Use the cheapest style accesses first (e.g.
getBoundingClientRect) and early return when possible. After a flush, some style accesses are O(1). - Fix: Leverage a natural flush rather than triggering a new flush using the Intersection Observer API for unprivileged and/or
promiseDocumentFlushedfor privileged code.
- Fix: Use the cheapest style accesses first (e.g.
Updating isVisible: first attempt
Revised profile: first attempt[1]
This focuses on the fix of using the cheapest style accesses first (e.g. getBoundingClientRect) and early return when possible in isVisible.
Numbers
- The largest unresponsive chunk is still caused by Fathom extraction, contributing 386 ms of jank right around page load.
isVisiblemade up 244ms (63%) of this jank.- This change reduced overall Fathom-related jank by 74 ms (16%) compared to the original implementation of
isVisible.
// Avoid reading `display: none` due to Bug 1381071
isVisible(fnode) {
const element = fnode.element;
if (element.getBoxQuads().length === 0) {
// The element has no box (display: none subtree?), checking
// getBoundingClientRect instead for a width and height of 0 only tells
// us it is a zero-sized box.
return false;
}
const eleStyle = getComputedStyle(element);
if (eleStyle.visibility === 'hidden') {
return false; // not painted.
}
// This is assuming inclusive ancestors, otherwise we need to check opacity
// above too.
for (const ancestor of ancestors(element)) {
const style = getComputedStyle(ancestor);
if (style.opacity === '0') {
return false;
}
if (style.display === 'contents') {
// display: contents elements have no box themselves, but children are
// still rendered.
continue;
}
const rect = ancestor.getBoundingClientRect();
if ((rect.width === 0 || rect.height === 0) && style.overflow === 'hidden') {
// This is a zero-sized box; zero-sized ancestors don’t make descendants
// hidden unless the descendant has overflow: hidden
return false;
}
}
return true;
}Conclusions
- While this removed much of the layout querying work, the proportion of jank caused by style accesses was smaller than expected, given the known performance Bug 1381071.
- Now
isVisibleis spending most of its time (roughly 62%) accessing properties through X-ray vision. We can inspect this in the profile by filtering for Native code for "js::NativeLookupOwnProperty", "XRayTraits" or "WeakMap".- Fix: Spend less time accessing DOM properties. Consider simply checking whether the element is clickable. This requires more work from the browser, but less complexity in
isVisible.
- Fix: Spend less time accessing DOM properties. Consider simply checking whether the element is clickable. This requires more work from the browser, but less complexity in
Updating isVisible: second attempt
Revised profile: second attempt[1]
This focuses on the fix of mitigating (actually, eliminating) redundant work, removing ancestors entirely to eliminate DOM accesses, and reducing the number of style accesses by checking if the element is clickable.
Numbers
- The largest unresponsive chunk is no longer caused by Fathom extraction; Fathom extraction contributes only 89 ms of jank right around page load.
isVisiblemade up 30 ms (34%) of this jank.- This change reduced overall Fathom-related jank by 374 ms (81%) compared to the original implementation of
isVisible.
isVisible(fnode) {
const element = fnode.element;
const rect = element.getBoundingClientRect();
return !!(rect.height && rect.width && (document.elementsFromPoint(rect.x
+ rect.width / 2, rect.y + rect.height / 2)).includes(element));
}Conclusions
- DOM property accesses were by far the largest contributor to jank (81% compared to 16% for style accesses in the previous profile). This is largely due to the X-Ray work associated with DOM property access.
- Unfortunately, as implemented, this misses some elements that can be visible but reside outside of the viewport in the -x and/or -y directions due to how
getBoundingClientRectandelementsFromPointwork.- Per Gijs and Emilio, in privileged code, there is a way to cope with out-of-viewport coordinates using
aIgnoreRootScrollFrame, so this solution may still be viable in Firefox applications. This needs further investigation.
- Per Gijs and Emilio, in privileged code, there is a way to cope with out-of-viewport coordinates using
Version information
- Firefox version: 70
- Your OS and version: Mac OSX 10.13.6
- Price Tracker extension version: PR Upgrade to Fathom 3.0 #317
[1]: Profiled on a locally hosted product page with specs from the "Version information" section in this comment.