Skip to content

perf(gather): Faster shadowDOM testing#3327

Merged
michael-siek merged 4 commits intodevelopfrom
gather-perf-improvement
Dec 22, 2021
Merged

perf(gather): Faster shadowDOM testing#3327
michael-siek merged 4 commits intodevelopfrom
gather-perf-improvement

Conversation

@WilcoFiers
Copy link
Copy Markdown
Contributor

Testing if deeply nested shadow DOM nodes are in context can be a very slow operation, especially on sites that make extreme use of shadow DOM (think 30+ layers of nested shadow trees). For a very large part, this is because calling containsShadowChild thousands of times recursively is very slow.

This PR avoids calling isNodeInContext when it isn't necessary (when there are no excluded nodes), and makes containsShadowChild significantly faster by walking down to the root, rather than walking the entire tree.

@WilcoFiers WilcoFiers force-pushed the gather-perf-improvement branch from d1b93bd to 1203736 Compare December 13, 2021 13:14
@WilcoFiers WilcoFiers changed the title perf(gather): Fast shadowDOM testing without context.exclude perf(gather): Faster shadowDOM testing Dec 13, 2021
@WilcoFiers WilcoFiers marked this pull request as ready for review December 13, 2021 13:43
@WilcoFiers WilcoFiers requested a review from a team as a code owner December 13, 2021 13:43
var results = axe.runVirtualRule('nested-interactive', node);

assert.lengthOf(results.passes, 0);
assert.lengthOf(results.passes, 1);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have no explanation for why this test wasn't erroring before... There are two applicable nodes in this tree, one passes, the other fails.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this blew my mind but I gave up trying to understand why

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, likewise.

* @param {Context}
* @return {Function|null}
*/
function getContextFilter(context) {
Copy link
Copy Markdown
Contributor Author

@WilcoFiers WilcoFiers Dec 13, 2021

Choose a reason for hiding this comment

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

I have no idea what tests to write for this. I'm not sure it's necessary, since PR doesn't change output. It just makes some things faster. If you feel tests are necessary, please let me know and I'll figure something out. Will probably need to put something on the global axe object so that I can spy on stuff.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You could create a very deep shadow tree that times out the tests with the old code and doesn't now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I suppose so... Didn't try that. I'll play around with that for a bit, see if that works.

Comment on lines -16 to -18
return !!vNode.children.find(child => {
return containsShadowChild(child, otherVNode);
});
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was the biggest problem. Searching all descendants of vNode instead of just looking at the ancestors of ovetherVNode.

@michael-siek
Copy link
Copy Markdown
Member

Merging #3332

@michael-siek michael-siek merged commit 26adc1b into develop Dec 22, 2021
@michael-siek michael-siek deleted the gather-perf-improvement branch December 22, 2021 13:32
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.

3 participants