perf: memoize DqElement #4452
Merged
perf: memoize DqElement #4452
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
straker
commented
May 7, 2024
| @@ -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) { | |||
There was a problem hiding this comment.
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; | |||
There was a problem hiding this comment.
Memoize requires something to be returned otherwise it caches the function as undefined
| fixture, | ||
| {}, | ||
| { | ||
| selector: { get: throws }, |
There was a problem hiding this comment.
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.
WilcoFiers
previously requested changes
May 10, 2024
WilcoFiers
approved these changes
May 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 therelatedNodesfor each check. Since each each duplicate id node was also in therelatedNodesfor 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 thegetSelectorof a DqElement would callouterHTMLfor 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-ariacheck.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