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

Poor performance. a11yCheck() taking > 1min #240

Closed
paulirish opened this issue Oct 12, 2016 · 16 comments
Closed

Poor performance. a11yCheck() taking > 1min #240

paulirish opened this issue Oct 12, 2016 · 16 comments
Assignees
Labels
color contrast Color contrast issues

Comments

@paulirish
Copy link
Contributor

paulirish commented Oct 12, 2016

Hiya folks,

We use axe-core from within Lighthouse and we love it, however it takes a significant amount of time to run.

For example, the latest build tool ~3 mins to finish on this page https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise

I've profiled things and have some findings that should help... (details to follow)

@paulirish
Copy link
Contributor Author

paulirish commented Oct 12, 2016

Overall look

screen shot 2016-10-12 at 10 40 41 am

Here's the full recording. As you can see, it's taking 180s to complete. The purple on the bottom of each flame of javascript execution is layout. Aka forced layout aka reflow.

Common layout thrashing

Zooming in:
image

And closer:
image

Two important pieces of information here. Where layout was forced and where it was invalidated.

Layout was forced at this line (from elements-from-point.js):

    while ((current = doc.elementFromPoint(x, y)) && elements.indexOf(current) === -1 && current !== null) {

The elementFromPoint requires the layout of the page be recomputed if the layout potentially changed since last layout. And the "invalidation" is exactly that: where it was potentially changed.

That line looks like:

    current.style.setProperty('pointer-events', 'none', 'important');

and there's another invalidation just a few lines later

        for (i = previousPointerEvents.length; !!(d = previousPointerEvents[--i]); ) {
          elements[i].style.setProperty('pointer-events', d.value ? d.value : '', d.priority);
        }

While you and I know the pointer-events css property doesn't change the layout of objects on the page, unfortunately the browser is kinda dumb and considers this style change to be an invalidation.
And since we basically go back and forth between invalidation and forcing layout recomputing... that's how we end up in this pattern. Each time it happens it only takes like 3ms, but since it's done for all elements. that adds up.

More:

@paulirish
Copy link
Contributor Author

The fix

The fix is luckily pretty straightforward. Paul Lewis covers it in the above article.

Just do two loops, one where you invalidate and one where you measure.

(Or perhaps in your case it might be three.)

for (element of allElements) {
  getTopElementFromPoint();
}
for (element of allElements) {
  adjustPointerEventsForAllElements();
}
for (element of allElements) {
  getTopElementFromPointAgain();
}

(I'm just guessing. I am not reading this code totally correctly. :)

@paulirish
Copy link
Contributor Author

paulirish commented Oct 12, 2016

The workaround

Lastly, I realize that this work may not be even neccessary in a number of browsers..

document.elementsFromPoint() exists! (note the plural in there!) It's supported in everyone but safari.

Using this would mean you don't need to run this polyfill code at all. So.. that would actually be really straightforward to fix. :) (You could still fix the layout thrashing for Safari's sake if you want)

Good luck and let me know if you have any questions about this stuff!

@marcysutton
Copy link
Contributor

Thank you so much for this! We knew the color contrast rule was the bottleneck for sure, just haven't gotten around to fixing it yet. This is immensely helpful!

@dylanb
Copy link
Contributor

dylanb commented Oct 13, 2016

@paulirish thanks for the excellent analysis. It turns out that we have already made some of the changes you talked about and these will be in the 2.1 release of the library (probably to be released in the next month or so)

On that MDN page, we get the following performance in Chrome

image

Specifically, we are using elementsFromPoint where this is available. We'll look into whether we can make Safari perform better using some of the info you provided.

@paulirish
Copy link
Contributor Author

paulirish commented Oct 13, 2016

Yay excellent news. Can I build 2.1 myself?

@dylanb
Copy link
Contributor

dylanb commented Oct 13, 2016

@paulirish we do our internal development on a private fork ATM. Part of our commitment to our paying customers is early access to enhancements.

@paulirish
Copy link
Contributor Author

ah okay! coolness.

if you can get me a build (.min.js is fine), I can profile it to see if there are any more big perf opportunities. But no worries if you can't :)

@dylanb
Copy link
Contributor

dylanb commented Oct 13, 2016

Email me at the email address in my profile and I'll see what I can do

@marcysutton marcysutton self-assigned this Oct 27, 2016
@marcysutton marcysutton added the color contrast Color contrast issues label Nov 1, 2016
@paulirish
Copy link
Contributor Author

Any news of 2.1 shipping? Can't wait to roll the latest into Lighthouse

@marcysutton
Copy link
Contributor

We're getting close! I'm measuring the performance now and making sure any upgrades don't cause false positives or negatives before we commit to a release. At the very least, if our team is not satisfied with the current improvements I've been working on, we can ship the code already in our internal repository as Dylan mentioned before. I will update you as soon as I know more.

@mfairchild365
Copy link
Contributor

I'm also eagerly awaiting the 2.1 release for a performance fix. I'm willing to help wherever possible.

@marcysutton
Copy link
Contributor

marcysutton commented Dec 13, 2016

2.1.7 has been released! I'm going to close this issue, let us know if you still have any problems.

@paulirish
Copy link
Contributor Author

Nice. On a particularly tough page it looks like axe-core went from a runtime of 9.6s to 1.1s. :)

Thanks!

@paulirish
Copy link
Contributor Author

Whoa. Looks like you guys are splitting up all the tasks to yield very frequently:
image

That's totally awesome. Super performant. Great job on this!

@mfairchild365
Copy link
Contributor

So, noticed that performance in phantomjs is still pretty bad (presumably because it uses webkit which does not support getElementsFromPoint()). So I made a thing that uses chromium instead. I thought some folks might find it helpful. https://github.com/mfairchild365/the-axe-nightmare/

mrtnvh pushed a commit to mrtnvh/axe-core that referenced this issue Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
color contrast Color contrast issues
Projects
None yet
Development

No branches or pull requests

4 participants