feat: Improve perf of axe.run [WWD-1821] #1503
Conversation
lib/core/utils/flattened-tree.js
Outdated
| }); | ||
| return found; | ||
| // TODO: removing vNode is a breaking change, but we can skirt that | ||
| // for now by ignoring it if passed in |
There was a problem hiding this comment.
I'm wondering if it isn't better to keep getNodeFromTree as is and deprecate it, and have a replacement method that uses cache? Would like to discuss.
There was a problem hiding this comment.
👎 from me. I see this as an opportunity for people (us) to continue using the "wrong method". Additionally, as discussed yesterday, there are 3 different methods for figuring out if something is "hidden". Let's not create the same problem here.
There was a problem hiding this comment.
This is a breaking change getNodeFromTree is something that rules can use. Why is this change necessary?
lib/core/utils/flattened-tree.js
Outdated
| }); | ||
| return found; | ||
| // TODO: removing vNode is a breaking change, but we can skirt that | ||
| // for now by ignoring it if passed in |
There was a problem hiding this comment.
This is a breaking change getNodeFromTree is something that rules can use. Why is this change necessary?
What are the breaking changes? Please elaborate. The
Yes it does. Please elaborate. |
…utils function to setup axe._tree
straker
dismissed
dylanb’s stale review
Removed deprecation comment for a separate commit
| @@ -45,12 +46,16 @@ dom.isVisible = function(el, screenReader, recursed) { | |||
| el = el.host; // grab the host Node | |||
| } | |||
|
|
|||
| style = window.getComputedStyle(el, null); | |||
| if (node && typeof node[cacheName] !== 'undefined') { | |||
| return node[cacheName]; | |||
There was a problem hiding this comment.
Can you add 2 tests:
- that the cache is set after first accessing this method
- that the cache is used when accessing this a second time
There was a problem hiding this comment.
I'm going to use this comment to group all the tests for caching comments.
We already have code that tests that all of these functions are working as expected. Is there a particular reason you would like the unit tests to be tied to how the function was implemented internally?
One of the nice things about these drastic code changes (especially for getNodeFromTree) is that I could completely swap out the internal implementation and I didn't have to update any of the tests. If we start adding tests that make sure a node is added to the cache, or retrieved from the cache, or that private cache properties are added, then we tie a lot of tests to the implementation of the internal code. We also force changes to that implementation to require changes to the unit tests.
IMO, we shouldn't test the internals of a function as the internals can easily change, creating fragile tests. We should only test that the function returns an expected set of outputs for a given set of inputs.
| @@ -47,6 +48,8 @@ function virtualDOMfromNode(node, shadowId) { | |||
| return vNodeCache._tabbableElements; | |||
| } | |||
| }; | |||
| axe._cache.nodeMap.set(node, vNode); | |||
There was a problem hiding this comment.
Verify in a test that all virtual nodes are put in cache.
| }); | ||
| return found; | ||
| const el = node || vNode; | ||
| return axe._cache.nodeMap.get(el); |
There was a problem hiding this comment.
Create a test to show this is accessed from cache.
| @@ -20,7 +21,11 @@ axe.utils.isHidden = function isHidden(el, recursed) { | |||
| el = el.host; // grab the host Node | |||
| } | |||
|
|
|||
| var style = window.getComputedStyle(el, null); | |||
| if (node && node._isHidden !== null) { | |||
| return node._isHidden; | |||
There was a problem hiding this comment.
As before, please add tests for the caching.
| return node; | ||
| const idRefsRegex = /^idrefs?$/; | ||
|
|
||
| function cacheIdRefs(node, refAttrs) { |
There was a problem hiding this comment.
Can you add tests for axe._cache.idRefs?
WilcoFiers
changed the title
|
This is amazing ! Thank you very much @straker 💡It would be great if we can add some kind of perf tests to validate changes. If there are pointers on how to go about them, please let me know. |
* develop: (50 commits) docs: add jsdom example and tests (dequelabs#1530) fix(aria-valid-attr-value): allow aria-owns to pass when element is not in the DOM (dequelabs#1526) fix(isSkipLink): cache first page link (dequelabs#1525) chore: update Babel dependencies (dequelabs#1527) chore(package): Update karma to version 4.1.0 (dequelabs#1528) feat: Improve perf of axe.run [WWD-1821] (dequelabs#1503) fix(prettier): ignore generated api doc files (dequelabs#1522) fix(skip-link,region): Allow multiple skiplinks at page top (dequelabs#1496) fix(raw-reporter): do not output `DqElement`s (dequelabs#1513) fix: Scroll state had top and left properties flipped (dequelabs#1469) refactor: commons.color.getBackgroundColor method (dequelabs#1451) fix(aria-valid-attr-value): allow aria-controls to pass when element is not in the DOM chore: Update husky to the latest version 🚀 (dequelabs#1514) style: format HTML files with Prettier (dequelabs#1508) test: Fix invalid test html (dequelabs#1502) feat(rule): Inline text spacing must be adjustable with custom stylesheets (dequelabs#1446) chore: Remove version number from axe.d.ts (dequelabs#1499) chore: Update make-dir to the latest version 🚀 (dequelabs#1465) fix: Exclude iframe for html-has-lang rule (Issue 1424) (dequelabs#1430) feat(utils): add support for complex CSS selectors (dequelabs#1494) ...
First off, sorry for the massive pr. There's only a handful of files you need to look at, everything else is a refactor.
Here are the important files:
Summary
Cached
axe.utils.isHidden,axe.commons.dom.isVisible,axe.commons.aria.isAccessibleRef, andaxe.utils.getNodeFromTree. These changes alone cut the time ofaxe.runin half on a page with >11k DOM nodes: from ~6s to ~3s in Chrome and from ~10s to ~4s in Firefox.Longdesc
Added a global cache on
axe._cachethat can be used internally to cache things. The idea is to allow axe to cache results from tree searches so we don't have to do the tree search again (which is usually the slowest part of the code). The cache uses aWeakMapso I also had to add a WeakMap polyfill. Core-js can't be used withoutimportwith v3 (norequirepossible), so I decided against using it (plus it's so big).Refactored every
test/full/**/*.htmlfile where I had to move the include of thetest/testutils.jsfile to after thediv#mochaelement sobeforeEachwas defined when testutils needed it so I could default theaxe._cachefor each run.Also, a lot of tests and files removed the
vNodeargument ofaxe.utils.getNodeFromTreeas it was no longer needed. The function itself will still accept the value and ignore it so we don't have to release a breaking change, but I figured we could update the internal code now not to need it. At the next breaking change release we can consider dropping the parameter from the function.We've decided to hold off on merging the 3 different
isHiddenfunctions in axe, mainly because the many different options we have for determining what constitutes hidden pose problems for caching (as thedom.isVisiblecache shows). Improving this would help thecolor-contrastandimage-redundant-altrules as they usedom.isVisibledifferently, andisHiddenwas already run on the same nodes earlier.Here is the before/after of
axe.runwithperformanceTimer: trueCloses: #166
Closes: #726
Closes: #1193
Closes: #1464
Closes: WWD-1821
Should help #1497, though need more work to get under the 60s benchmark (mostly in our generate unique selector code)
Might invalidate some vNode caching on #1096
Reviewer checks
Required fields, to be filled out by PR reviewer(s)