Skip to content

Commit

Permalink
perf: memoize DqElement (#4452)
Browse files Browse the repository at this point in the history
Noticed this when trying to debug perf issues in `duplicate-id-aria`.
We've run into problems on sites that have a module repeat 1000s of
times on the page and the module has an aria id that is also then
repeated. Axe-core would take a really long time to run the rule.
Looking into it what I discovered is that a majority of the time was
spent on resolving the `relatedNodes` for each check. Since each each
duplicate id node was also in the `relatedNodes` for every other node,
this caused the single node to [be converted to a
DqElement](https://github.com/dequelabs/axe-core/blob/develop/lib/core/utils/check-helper.js#L46)
_n_ times. This lead to many performance problems, but specifically
calling the `getSelector` of a DqElement would call `outerHTML` for the
node _n*2_ times which would be very slow.

To solve this I decided to memoize the DqElement creation. That way a
single node will only ever output a single DqElement, thus saving
significant time in creation. Not only that but every time a node
appears in a result (either as the check node or a related node) the
memory is now shared so this change also reduces the in-memory size of
the results object.

Testing a simple page with 5000 nodes of duplicate id, here are the
results for running the `duplicate-id-aria` check.

| Before change (in ms)  | After change (in ms) |
| ------------- | ------------- |
| 21,280.1  | 11,841.1  |

<img width="641" alt=""
src="https://github.com/dequelabs/axe-core/assets/2433219/05a456d8-2256-4e51-a0cb-f4a0d1c8c192">

_Flamechart before the change. The majority of the time is spent in
getSelector_

<img width="583" alt=""
src="https://github.com/dequelabs/axe-core/assets/2433219/1360de7c-5a65-448d-a3ee-327004d4c88c">

_Chrome performance timer of getSelector showing it spent 12000ms total
in the function_

<img width="640" alt=""
src="https://github.com/dequelabs/axe-core/assets/2433219/a5225444-0cfc-4ffb-850a-e428076de5e8">

_Flamechart after the change. Time is now spent mostly resolving the
cache which results in no time spent in getSelector_
  • Loading branch information
straker authored May 23, 2024
1 parent 3f13aa1 commit 9a22787
Show file tree
Hide file tree
Showing 3 changed files with 141 additions and 125 deletions.
10 changes: 8 additions & 2 deletions lib/core/utils/dq-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import getXpath from './get-xpath';
import getNodeFromTree from './get-node-from-tree';
import AbstractVirtualNode from '../base/virtual-node/abstract-virtual-node';
import cache from '../base/cache';
import memoize from './memoize';

const CACHE_KEY = 'DqElm.RunOptions';

Expand Down Expand Up @@ -36,7 +37,10 @@ function getSource(element) {
* @param {Object} options Propagated from axe.run/etc
* @param {Object} spec Properties to use in place of the element when instantiated on Elements from other frames
*/
function DqElement(elm, options = null, spec = {}) {
const DqElement = memoize(function DqElement(elm, options, spec) {
options ??= null;
spec ??= {};

if (!options) {
options = cache.get(CACHE_KEY) ?? {};
}
Expand Down Expand Up @@ -82,7 +86,9 @@ function DqElement(elm, options = null, spec = {}) {
if (!axe._audit.noHtml) {
this.source = this.spec.source ?? getSource(this._element);
}
}

return this;
});

DqElement.prototype = {
/**
Expand Down
Loading

0 comments on commit 9a22787

Please sign in to comment.