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

Use virtualNode as caching space #1096

Closed
2 of 14 tasks
WilcoFiers opened this issue Aug 24, 2018 · 7 comments
Closed
2 of 14 tasks

Use virtualNode as caching space #1096

WilcoFiers opened this issue Aug 24, 2018 · 7 comments
Labels
core Issues in the core code (lib/core)

Comments

@WilcoFiers
Copy link
Contributor

WilcoFiers commented Aug 24, 2018

Axe-core is increasingly moving away from directly working with the DOM and abstracting things out. Especially with the introduction of shadow DOM, many of the common methods are no longer in use, such as the usual querySelectorAll, most DOM walking (parentNode, children, etc.), getRoot() and quite a bit more.

One of the problems we have with axe-core is that there is no data sharing between check evaluate calls and between matches. Each runs in its own context and can't take any data from other evaluate / matches calls. This means that many things get computed over and over again. Axe-core does very little in terms of caching. When it comes to querying the DOM for the same thing hundreds of times, or computing the background of the same element thousands of times, these things add up.

The solution: We start putting memoization methods onto the virtualNode. For instance, instead of doing: axe.commons.text.accessibleTextVirtual(virtualNode) we would also allow virtualNode.accessibleText() which when called the first time would call accessibleTextVirtual, and would cache the result for any subsequent calls. I think that would save a bunch of time. Here's a list of methods I think we should move onto the virtualNode:

  • accessibleText
  • getRole
  • isFocusable
  • matchesSelector
  • getRootNode
  • getComposedParent
  • findUp
  • computedStyle
  • isVisible
  • getForegroundColor
  • getBackgroundColor
  • hasContent
  • getElementCoordinates
  • tabbableElements
@WilcoFiers
Copy link
Contributor Author

In addition to performance benefits, this also decreases our reliance on global methods which is a big bonus. Further more, we could start thinking about constraining matches/evaluate functions from having access to the DOM at all, which could be a big benefit for making sure custom rules can't do anything malicious.

@WilcoFiers WilcoFiers added the core Issues in the core code (lib/core) label Aug 27, 2018
@stephenmathieson
Copy link
Member

We had a quick chat about this and I'm of the opinion we may be better off using an existing memoization tool and just wrapping our existing functions.

For example:

const memoize = axe.imports.memoize

axe.utils.isFocusable = memoize(function (node) {
  // is focusable computation here
})

This leaves the "virtual DOM" abstraction alone and requires very little effort to achieve the same performance win.

NOTE - the memoization tool we use should rely on a WeakMap as its cache rather than trying to stringify DOM nodes.

@WilcoFiers
Copy link
Contributor Author

@stephenmathieson That would mean axe-core keeps all tested nodes in memory pretty much permanently. I don't think that's ideal.

@stephenmathieson
Copy link
Member

@WilcoFiers maybe we fork something like https://www.npmjs.com/package/@emotion/weak-memoize and expose the WeakMap's .clear() method then?

We could do something like:

axe.run = () => {
  memoizeThing.clear()

  // normal axe.run() logic
}

@dylanb
Copy link
Contributor

dylanb commented Jun 16, 2019

@stephenmathieson whatever solution we choose should clear the cache at the end of the run call, so the pseudo code should read

axe.run = () => {
   // normal axe.run() logic minus the return/resolve
   memoizeThing.clear()
   // normal axe.run() return/resolve
}

@straker straker added this to the Axe-core 3.5 milestone Oct 15, 2019
@straker
Copy link
Contributor

straker commented Oct 15, 2019

Let's memoize (with clearing cache)!

@straker
Copy link
Contributor

straker commented Nov 6, 2019

Do this for #908

@straker straker removed this from the Axe-core 3.5 milestone Nov 6, 2019
@straker straker closed this as completed Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Issues in the core code (lib/core)
Projects
None yet
Development

No branches or pull requests

4 participants