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

perf: memoize DqElement #4452

Merged
merged 2 commits into from
May 23, 2024
Merged

perf: memoize DqElement #4452

merged 2 commits into from
May 23, 2024

Conversation

straker
Copy link
Contributor

@straker straker commented May 7, 2024

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 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

Flamechart before the change. The majority of the time is spent in getSelector

Chrome performance timer of getSelector showing it spent 12000ms total in the function

Flamechart after the change. Time is now spent mostly resolving the cache which results in no time spent in getSelector

@straker straker requested a review from a team as a code owner May 7, 2024 20:11
@@ -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) {
Copy link
Contributor Author

@straker straker May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default parameters get transformed out of the function arguments after babel, which meant that only elm was left in the argument list for memoize, so options and spec were ignored.

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

return this;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memoize requires something to be returned otherwise it caches the function as undefined

fixture,
{},
{
selector: { get: throws },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With memoize this style of define would propagate to all tests and cause issues. Since DqElement takes the spec as truth, just passed the functions in as the spec.

straker added a commit that referenced this pull request May 8, 2024
These tests failed while in-development of
#4452 and it really confused
me. Based on how the title is written, I initially thought that it was
making sure that `.selector` (etc.) for the DqElement prototype function
was never called. But what it's really making sure is that the
`.selector` (etc.) property is not called for the one node.

The reason that distinction matters is because `processAggregate` calls
`nodeSerializer` which in turn calls the `toJSON` method of a DqElement,
which calls the `.selector` (etc.) property. So it would be impossible
to never call the DqElement properties.
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test showing that when you instantiate a DqElm for the second time you get the same object back?

@straker straker merged commit 9a22787 into develop May 23, 2024
22 checks passed
@straker straker deleted the perf-dqelm branch May 23, 2024 15:42
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.

2 participants