[WIP] fix(qsa): greatly improve qsa performance #2679

straker · 2020-12-07T17:45:59Z

Following the QSA proposal, this greatly improves the speed of QSA as it no longer has to scan the entire DOM tree for top-level queries. For large sites the speed increase is substantial.

For https://js.tensorflow.org/api/latest/, the top slowest performanceTimer results were as follow:

Timer name	Time (ms)
rule_aria-input-field-name#gather	358.93
rule_aria-input-field-name	360.51
rule_aria-toggle-field-name#gather	364.21
rule_aria-toggle-field-name	364.47
rule_heading-order#gather	417.78
rule_empty-heading#gather	424.87
rule_empty-heading	425.52
runchecks_page-has-heading-one	477.66
rule_page-has-heading-one	477.83
rule_link-name	483.33
runchecks_heading-order	501.92
rule_identical-links-same-purpose	586.62
rule_landmark-unique#gather	747.97
rule_landmark-unique	750.19
runchecks_region	752.90
runchecks_bypass	801.04
rule_heading-order	919.92
rule_bypass	953.06
rule_region	999.91
rule_color-contrast#matches	1,080.26
runchecks_color-contrast	7,518.19
rule_color-contrast	8,598.69
audit_start_to_end	25,047.47

We can see that the #gather step is what causes most rules to be slow (and not so much the #runchecks step), causing axe-core to take 25 seconds to just run through all the rules.

By caching node information ahead of time, the top slowest results are now as follows:

Timer name	Time (ms)
rule_aria-valid-attr	37.63
rule_list	38.20
rule_aria-valid-attr-value	39.39
rule_autocomplete-valid	40.15
runchecks_button-name	40.44
rule_button-name	44.93
rule_scrollable-region-focusable#matches	65.09
rule_aria-allowed-attr#gather_axe.utils.isHidden	88.74
rule_scrollable-region-focusable	102.22
runchecks_identical-links-same-purpose	150.08
rule_identical-links-same-purpose#matches	155.87
rule_region#gather	164.47
runchecks_link-name	201.40
rule_link-name	220.39
rule_aria-allowed-attr#gather	256.62
rule_aria-allowed-attr	277.43
rule_identical-links-same-purpose	331.73
runchecks_region	576.94
rule_region	747.61
rule_color-contrast#matches	1,041.21
runchecks_color-contrast	7,414.11
rule_color-contrast	8,455.56
audit_start_to_end	10,537.04

This change shaves off 15 seconds from the time needed to run all of axe rules on the site.

In terms of memory, this only increases the size of an axe-core run by a few MB.

Name	Size (MB)
Tensorflow default size	4.6
axe._tree before QSA cache	48.8
axe._tree after QSA cache	51.2

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

lib/core/utils/pollyfills.js

straker · 2020-12-08T20:01:53Z

test/core/utils/qsa.js

+      if (describeName === 'without cache') {
+        it('should find nodes using id, but not in shadow DOM', function() {
+          var result = axe.utils.querySelectorAllFilter(
+            dom[0].children[0],
+            '#one'
+          );
+          assert.equal(result.length, 1);
+        });
+        it('should find nodes using id, within a shadow DOM', function() {
+          var result = axe.utils.querySelectorAllFilter(
+            dom[0].children[0].children[2],
+            '#one'
+          );
+          assert.equal(result.length, 1);
+        });
+      }


The wrapping if was added.

Why does this not get tested with the cache?

Because you cannot query by ID through the shadow dom

straker · 2020-12-08T20:02:11Z

test/core/utils/qsa.js

-      node
-    ) {
-      return node.actualNode.nodeName !== 'UL';
+


This file is mostly just adding the wrapping describe. The first 30 lines are new code, then one additional if statement added in the middle (marked in another comment). The rest is no change

lib/commons/aria/arialabelledby-text.js

WilcoFiers

I have tried really hard to make sense of this PR, but the way you're using global cache all over the place, and the reliance on things getting executed in a very specific order that is in no way enforce. I'm not confident that this code works, and I wouldn't trust myself to make changes to this.

Please put some effort into writing this in a way where each function has a single clear purpose, with a strong preference on pure functions, and where if there are any preconditions to execution, those preconditions are obvious and enforced.

lib/core/utils/get-flattened-tree.js

lib/core/utils/selector-cache.js

lib/core/utils/get-flattened-tree.js

Resolved

WilcoFiers · 2021-01-19T12:41:06Z

lib/core/utils/query-selector-all-filter.js

+  // see if the passed in node is the root node of the tree and can
+  // find nodes using the cache rather than looping through the
+  // the entire tree
+  const nodes = getNodesMatchingSelector(domTree, selector, filter);


From the look of it, you might as well pass in the expressions variable instead of the selector. That way you're not converting it twice, and it makes for a more consistent function signature.

Passing an expression makes testing harder as instead of a simple string I need to parse the selector into the expression first then pass it to the function. I don't feel that passing an expression makes it any more consistent either.

WilcoFiers · 2021-01-19T16:19:50Z

lib/core/utils/query-selector-all-filter.js

+  // see if the passed in node is the root node of the tree and can
+  // find nodes using the cache rather than looping through the
+  // the entire tree
+  const nodes = getNodesMatchingSelector(domTree, selector, filter);


Suggested change

const nodes = getNodesMatchingSelector(domTree, selector, filter);

if (axe._tree === domTree) {

return matchSelectorOnTreeRoot(domTree, selector, filter);

}

WilcoFiers · 2021-01-19T16:32:00Z

lib/core/utils/selector-cache.js

+  const selectorMap = domTree[0]._selectorMap;
+  if (!selectorMap) {
+    return;
+  }


Suggested change

const selectorMap = domTree[0]._selectorMap;

if (!selectorMap) {

return;

}

let selectorMap = cache.get('selectorMap')

if (!selectorMap || cache.get('selectorMapRoot') !== domTree) {

selectorMap = getSelectorMap(domTree)

cache.set('selectorMap', selectorMap)

cache.set('selectorMapRoot', domTree)

}

dylanb

Concerned about use of the cache with shadow DOM given that a shadow DOM test case was excluded from that. Why is this ok?

CLAassistant · 2021-08-27T14:44:04Z

All committers have signed the CLA.

straker · 2022-03-24T23:54:23Z

Closing in favor of #3423

fix(qsq): greatly improve qsa performance

49bd6f6

straker requested a review from a team as a code owner December 7, 2020 17:45

straker added 2 commits December 7, 2020 16:34

fix

76a4253

fix Array.from polyfill

7a08d34

straker commented Dec 8, 2020

View reviewed changes

lib/core/utils/pollyfills.js Outdated Show resolved Hide resolved

no Set

eeea814

straker changed the title ~~fix(qsq): greatly improve qsa performance~~ fix(qsa): greatly improve qsa performance Dec 8, 2020

straker added 3 commits December 8, 2020 12:55

refactor and tests

f9989f3

remove log

bfa341a

fix

f0e7cfe

straker commented Dec 8, 2020

View reviewed changes

fix tests

b65fda1

dylanb reviewed Dec 15, 2020

View reviewed changes

lib/commons/aria/arialabelledby-text.js Outdated Show resolved Hide resolved

straker and others added 2 commits December 18, 2020 08:41

Merge branch 'develop' into qsa-perf

abc3bc6

resolve eslint

06687d4

WilcoFiers previously requested changes Jan 4, 2021

View reviewed changes

lib/core/utils/get-flattened-tree.js Outdated Show resolved Hide resolved

lib/core/utils/get-flattened-tree.js Outdated Show resolved Hide resolved

lib/core/utils/selector-cache.js Outdated Show resolved Hide resolved

lib/core/utils/get-flattened-tree.js Outdated Show resolved Hide resolved

straker added 2 commits January 12, 2021 10:14

Merge branch 'develop' into qsa-perf

1ff51f4

refactor

eaab92d

straker added 2 commits January 12, 2021 11:09

typos

7039be8

comments

f9cfe09

WilcoFiers requested changes Jan 21, 2021

View reviewed changes

dylanb reviewed Jan 25, 2021

View reviewed changes

straker changed the title ~~fix(qsa): greatly improve qsa performance~~ [WIP] fix(qsa): greatly improve qsa performance Apr 26, 2021

straker closed this Mar 24, 2022

straker reopened this Mar 24, 2022

straker closed this Mar 24, 2022

WilcoFiers deleted the qsa-perf branch January 30, 2023 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] fix(qsa): greatly improve qsa performance #2679

[WIP] fix(qsa): greatly improve qsa performance #2679

straker commented Dec 7, 2020

straker Dec 8, 2020 •

edited

Loading

dylanb Jan 25, 2021 •

edited

Loading

straker Jan 25, 2021

straker Dec 8, 2020

WilcoFiers left a comment

WilcoFiers Jan 19, 2021

straker Feb 2, 2021

WilcoFiers Jan 19, 2021

WilcoFiers Jan 19, 2021

dylanb left a comment

CLAassistant commented Aug 27, 2021 •

edited

Loading

straker commented Mar 24, 2022

-  const nodes = getNodesMatchingSelector(domTree, selector, filter);
+  if (axe._tree === domTree) {
+    return matchSelectorOnTreeRoot(domTree, selector, filter);
+  }

-  const selectorMap = domTree[0]._selectorMap;
-  if (!selectorMap) {
-    return;
-  }
+  let selectorMap = cache.get('selectorMap')
+  if (!selectorMap || cache.get('selectorMapRoot') !== domTree) {
+    selectorMap = getSelectorMap(domTree)
+    cache.set('selectorMap', selectorMap)
+    cache.set('selectorMapRoot', domTree)
+  }

[WIP] fix(qsa): greatly improve qsa performance #2679

[WIP] fix(qsa): greatly improve qsa performance #2679

Conversation

straker commented Dec 7, 2020

Reviewer checks

straker Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

dylanb Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

straker Jan 25, 2021

Choose a reason for hiding this comment

straker Dec 8, 2020

Choose a reason for hiding this comment

WilcoFiers left a comment

Choose a reason for hiding this comment

WilcoFiers Jan 19, 2021

Choose a reason for hiding this comment

straker Feb 2, 2021

Choose a reason for hiding this comment

WilcoFiers Jan 19, 2021

Choose a reason for hiding this comment

WilcoFiers Jan 19, 2021

Choose a reason for hiding this comment

dylanb left a comment

Choose a reason for hiding this comment

CLAassistant commented Aug 27, 2021 • edited Loading

straker commented Mar 24, 2022

straker Dec 8, 2020 •

edited

Loading

dylanb Jan 25, 2021 •

edited

Loading

CLAassistant commented Aug 27, 2021 •

edited

Loading