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

feat(color-contrast): greatly improve performance for very large sites #1943

Merged
merged 21 commits into from
Jan 8, 2020

Conversation

straker
Copy link
Contributor

@straker straker commented Dec 16, 2019

Remove the scroll requirement from color-contrast and use the new performance api of dom.getElementStack (#1842). I also tried to milk the performance of the dom.getElementStack api for all it's worth (using TreeWalker, caching lookups, etc.), but I think it's the best I can get it. Lastly, to get correct color contrast, I needed every node on the page to be part of the 2d grid, even if they weren't in the axe context. So I had to create temporary VirtualNodes for any node that was missing from the virtual tree so I could use them in the stack.

This returns my favorite site https://web.archive.org/web/20190613132353/https://giveawaylisting.com/ in about 5s. Slower than I wanted but way better than never returning. All other sites seem to return in about the same time as the old code.

Looking at the flame chart, you can see that the initial setting up of the 2d grid takes 2s, and then it's very tiny stacks for each of the 50906 nodes. So nothing is taking too long to run, it's just processing that many nodes is the bottleneck and something that probably can't be overcome.

image

For #1685, the performance went from 23s to just a few seconds to run color-contrast (had to use an archived view again https://web.archive.org/web/20180625151005/https://users.math.msu.edu/users/wongwwy/expository/torsion.html).

Closes issue: #1588, #318, #1358, #1920, #1730, #1685, #1539, #1951

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Code is reviewed for security

@straker straker requested a review from a team as a code owner December 16, 2019 19:07
lib/commons/dom/visually-contains.js Outdated Show resolved Hide resolved
lib/commons/dom/visually-contains.js Outdated Show resolved Hide resolved
lib/commons/dom/visually-contains.js Show resolved Hide resolved
lib/commons/dom/visually-contains.js Outdated Show resolved Hide resolved
test/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
test/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
WilcoFiers
WilcoFiers previously approved these changes Jan 8, 2020
@straker straker merged commit 9ea0065 into develop Jan 8, 2020
@straker straker deleted the perfColorContrast branch January 8, 2020 19:48
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

Successfully merging this pull request may close these issues.

3 participants