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

Chore: improve performance of :function selector #15181

Merged
merged 1 commit into from Oct 21, 2021

Conversation

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Oct 17, 2021

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:

:function selector is costly because getPossibleTypes doesn't assume that it matches only a particular set of node types, so NodeEventGenerator runs esquery.matches() on every node to check if it matches.

For example, running no-shadow-restricted-names on ESLint's codebase spends ~95% of the time in trying to match unrelated nodes with its "VariableDeclaration, :function, CatchClause" selector. Note that this isn't observable with TIMING because it tracks only time spent in visitors.

What changes did you make? (Give an overview)

As esquery hardcodes :function to only three exact types, I added them as the only possible types for :function. This improves the performance of no-shadow-restricted-names by the said 95%, and will also improve the execution time for any other rule with the :function selector in tail positions. We're not using that selector much in core rules, but it's used in plugins.

Is there anything you'd like reviewers to focus on?

Does this make sense? I don't expect any maintenance problems with this as the meaning of :function in esquery is unlikely to change in the foreseeable future.

nzakas
nzakas approved these changes Oct 19, 2021
Copy link
Member

@nzakas nzakas left a comment

LGTM. Can you also share the before/after perf test results?

Loading

@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Oct 20, 2021

Before:

Loading:
  Load performance Run #1:  296.659028ms
  Load performance Run #2:  307.43705ms
  Load performance Run #3:  281.093598ms
  Load performance Run #4:  373.192565ms
  Load performance Run #5:  288.10806ms

  Load Performance median:  296.659028ms


Single File:
  CPU Speed is 2095 with multiplier 13000000
  Performance Run #1:  11954.131095ms
  Performance Run #2:  12160.465663ms
  Performance Run #3:  12044.104171ms
  Performance Run #4:  11863.431794ms
  Performance Run #5:  12105.454938ms

  Performance budget exceeded: 12044.104171ms (limit: 6205.250596658711ms)


Multi Files (450 files):
  CPU Speed is 2095 with multiplier 39000000
  Performance Run #1:  31527.230707ms
  Performance Run #2:  31545.83885ms
  Performance Run #3:  31919.857825ms
  Performance Run #4:  32007.010552ms
  Performance Run #5:  31863.90682ms

  Performance budget exceeded: 31863.90682ms (limit: 18615.751789976133ms)

After:


Loading:
  Load performance Run #1:  300.61123ms
  Load performance Run #2:  282.375974ms
  Load performance Run #3:  279.61085ms
  Load performance Run #4:  290.072188ms
  Load performance Run #5:  289.147058ms

  Load Performance median:  289.147058ms


Single File:
  CPU Speed is 2095 with multiplier 13000000
  Performance Run #1:  11846.847890000001ms
  Performance Run #2:  12012.38588ms
  Performance Run #3:  11598.659174ms
  Performance Run #4:  11980.817622ms
  Performance Run #5:  11605.999607ms

  Performance budget exceeded: 11846.847890000001ms (limit: 6205.250596658711ms)


Multi Files (450 files):
  CPU Speed is 2095 with multiplier 39000000
  Performance Run #1:  30957.816461ms
  Performance Run #2:  30992.744603ms
  Performance Run #3:  31009.393017ms
  Performance Run #4:  31016.027751ms
  Performance Run #5:  30774.832482ms

  Performance budget exceeded: 30992.744603ms (limit: 18615.751789976133ms)


Loading

@nzakas
Copy link
Member

@nzakas nzakas commented Oct 21, 2021

Nice. That multi file use case is impressive.

Loading

@mdjermanovic mdjermanovic merged commit 90a5b6b into main Oct 21, 2021
13 checks passed
Loading
@mdjermanovic mdjermanovic deleted the functions-selector-performance branch Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants