perf: Fast path for NodeEventGenerator #17019

nzakas · 2023-03-24T21:07:44Z

Prerequisites checklist

I have read the contributing guidelines.

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 autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:

Perf changes

What changes did you make? (Give an overview)

Added a "fast path" for NodeEventGenerator that skips matching and parsing of selectors when they are simple.

Refs #16962

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

I'm not seeing any noticeable improvement with this work. Did I do something wrong?

Refs #16962

netlify · 2023-03-24T21:07:50Z

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	`99ae371`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/641e112285ae840008ebac44

mdjermanovic · 2023-03-27T21:16:27Z

lib/linter/node-event-generator.js

+function tryParseSelector(rawSelector, knownTypes) {
+
+    const rawSelectorWithoutExit = rawSelector.replace(/:exit$/u, "");
+
+    // no need to parse known types
+    if (knownTypes.has(rawSelectorWithoutExit)) {
+        return {
+            type: "identifier",
+            value: rawSelectorWithoutExit
+        };
+    }


With the ESLint configuration for our codebase (enabled rules have 195 different selectors, out of which 113 are identifiers), this change improves performance by a couple of ms. I'm getting different results for the same run, but it's like ~16ms instead of ~18ms in total spent for running this function. Since we are caching parsed selectors, this improvement doesn't scale with the number of files being linted, so it looks like it won't bring much.

nzakas · 2023-03-28T18:04:05Z

I'm going to tweet this out to see if anyone is willing to try this branch and post their results. It's possible we aren't seeing the change but others might.

github-actions · 2023-04-07T22:31:27Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

amareshsm · 2023-04-07T22:59:46Z

Don't close. Still this solution needs to be experimented.

github-actions · 2023-04-18T22:31:28Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

nzakas · 2023-04-19T20:01:38Z

Since there is a small speedup in our own tests, maybe it's worth merging this? I wonder if it might help others in the wild more than us.

fasttime · 2023-04-20T06:37:03Z

Thanks for working on this @nzakas! If you can show the method(s) used to measure execution time before and after the change, I'd like to try this on my machine and see the results I'm getting.

nzakas · 2023-04-20T18:59:39Z

I just ran a combination of npm run perf and time eslint lib/ tests/.

fasttime · 2023-04-22T09:52:21Z

Using VSCode profiler to time function execution in node bin/eslint.js lib/ tests/ suggests a small improvement at least in Node.js 16 and 18 (I'm getting inconclusive results in Node.js 20). For reference, here are the results of my last three runs in Node.js 16.

Before

	Run 1	Run 2	Run 3	Average
(root)	36,828.56ms	36,032.43ms	34,383.37ms	35,748.12ms
NodeEventGenerator	148.99ms	112.88ms	129.83ms	130.57ms
applySelector	25,226.03ms	24,344.69ms	22,915.58ms	24,162.10ms

After

	Run 1	Run 2	Run 3	Average
(root)	34,949.05ms	35,382.04ms	36,360.29ms	35,563.79ms
NodeEventGenerator	123.56ms	125.00ms	126.11ms	124.89ms
applySelector	23,787.20ms	24,016.11ms	24,909.85ms	24,237.72ms

Note that the NodeEventGenerator constructor also covers tryParseSelector().

The differences are really minimal though, and there is a large margin for errors.

fasttime · 2023-04-22T10:03:38Z

lib/linter/node-event-generator.js

@@ -260,9 +274,10 @@ class NodeEventGenerator {
        this.exitSelectorsByNodeType = new Map();
        this.anyTypeEnterSelectors = [];
        this.anyTypeExitSelectors = [];
+        this.knownTypes = new Set(Object.keys(esqueryOptions.visitorKeys));


I was wondering if it's really necessary to keep track of known node types when the goal is to detect simple selectors. Maybe we could just test the selector string (in tryParseSelector and applySelector) for special characters like >, +, [, ], :, ,, *, !, ~ white spaces, etc. A valid selector is a simple selector if it does not contain any special characters.

I'm not 100% sure, but I think doing such a check could be slower than keeping track of known Node types.

I was also not sure, so I decided to test it (spoiler: it didn't help)

I defined a regex

const SIMPLE_SELECTOR_REGEX = /^\w+$/;

It's not perfect, but it matches all standard ESTree node names plus the code path analysis events (which are intended to work like identifiers as it seems). Then I replaced this.knownTypes.has() with SIMPLE_SELECTOR_REGEX.test() here and here and cleared the remaining references to knownTypes.

I run the VSCode profiler on npm run lint to measure execution time and compared the results with knownTypes and with my regex. No difference though. The times are almost the same either way. I guess we can keep the knownTypes and move on.

github-actions · 2023-05-02T22:31:19Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

nzakas · 2023-05-03T20:45:34Z

@fasttime yeah, running on our own repo doesn't show any significant improvement, but there have been previous changes that didn't show a big improvement on our repo and yet did show a big improvement in someone else's. The way ESLint is configured makes a big difference so the only way we can really know is to try the change out on a few other repos that perhaps have more complicated configurations.

fasttime · 2023-05-04T04:52:06Z

lib/linter/node-event-generator.js

+            this.emitter.emit(selector.rawSelector, node);
+            return;
+        }
+
        if (esquery.matches(node, selector.parsedSelector, this.currentAncestry, this.esqueryOptions)) {


Can we merge this condition with the next one in order to avoid repeating this.emitter.emit(selector.rawSelector, node);?

Hmm, I think you meant to post this on line 312?

I prefer the way I have it written because it's easier to see that there are two distinct conditions being handled. I don't mind repeating that line.

Hmm, I think you meant to post this on line 312?

Yes, I should have clicked on that line rather than the whole changed block, sorry.

I prefer the way I have it written because it's easier to see that there are two distinct conditions being handled. I don't mind repeating that line.

Fair enough. That would also not affect performance, only style.

fasttime · 2023-05-10T05:33:05Z

Thanks! The logic LGTM, but somehow I don't see a significant improvement here, not even in applySelector() which is called very often and clearly makes a good candidate for optimization. I'll leave this to @mdjermanovic to verify.

mdjermanovic · 2023-05-17T11:08:04Z

lib/linter/node-event-generator.js

+        if (this.knownTypes.has(selector.rawSelector)) {
+            this.emitter.emit(selector.rawSelector, node);
+            return;
+        }


We could add a property isIdentifier: parsedSelector.type === "identifier" to selectors and just check if (selector.isIdentifier) here.

Sorry, I’m missing some context here. Why would we do that?

For performance reasons. Whether the selector is an identifier is already known at the parse time.

Oh wait, are you saying that instead of checking knownTypes we could just check to see if the selector is an identifier?

Yes, at this point if it's an identifier then it certainly matches.

Got it. Updated.

github-actions · 2023-05-27T22:31:30Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

Rec0iL99 · 2023-05-28T01:41:54Z

Not stale. @nzakas there's a review for you to look into. Thanks.

mdjermanovic

The logic LGTM, but I'm consistently getting a worse performance after this change when running npm run test:performance and time node bin/eslint ., so I'm not sure whether we should merge this. I checked the blog post again but couldn't find performance tests for this recommendation.

nzakas · 2023-06-19T17:48:24Z

Thanks for the update. Let's just close this then, it's not important.

perf: Fast path for NodeEventGenerator

99ae371

Refs #16962

eslint-github-bot bot added the chore This change is not user-facing label Mar 24, 2023

nzakas mentioned this pull request Mar 24, 2023

Speed up ESLint #16962

Closed

mdjermanovic reviewed Mar 27, 2023

View reviewed changes

github-actions bot added the Stale label Apr 7, 2023

amareshsm removed the Stale label Apr 7, 2023

github-actions bot added the Stale label Apr 18, 2023

nzakas marked this pull request as ready for review April 19, 2023 20:01

nzakas requested a review from a team as a code owner April 19, 2023 20:01

github-actions bot removed the Stale label Apr 19, 2023

fasttime reviewed Apr 22, 2023

View reviewed changes

github-actions bot added the Stale label May 2, 2023

github-actions bot removed the Stale label May 3, 2023

fasttime reviewed May 4, 2023

View reviewed changes

fasttime approved these changes May 10, 2023

View reviewed changes

mdjermanovic reviewed May 17, 2023

View reviewed changes

github-actions bot added the Stale label May 27, 2023

Rec0iL99 removed the Stale label May 28, 2023

mdjermanovic reviewed Jun 12, 2023

View reviewed changes

Switch to use isIdentifier for apply selector

4eeb542

nzakas closed this Jun 19, 2023

nzakas deleted the perf-node-event-generator branch July 4, 2023 18:44

eslint-github-bot bot locked and limited conversation to collaborators Dec 17, 2023

eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: Fast path for NodeEventGenerator #17019

perf: Fast path for NodeEventGenerator #17019

nzakas commented Mar 24, 2023

netlify bot commented Mar 24, 2023 •

edited

mdjermanovic Mar 27, 2023

nzakas commented Mar 28, 2023

github-actions bot commented Apr 7, 2023

amareshsm commented Apr 7, 2023

github-actions bot commented Apr 18, 2023

nzakas commented Apr 19, 2023

fasttime commented Apr 20, 2023

nzakas commented Apr 20, 2023

fasttime commented Apr 22, 2023

Before

After

fasttime Apr 22, 2023 •

edited

nzakas May 3, 2023

fasttime May 4, 2023

github-actions bot commented May 2, 2023

nzakas commented May 3, 2023

fasttime May 4, 2023

nzakas May 8, 2023

fasttime May 10, 2023

fasttime commented May 10, 2023

mdjermanovic May 17, 2023

nzakas Jun 6, 2023

mdjermanovic Jun 12, 2023

nzakas Jun 13, 2023

mdjermanovic Jun 16, 2023

nzakas Jun 19, 2023

github-actions bot commented May 27, 2023

Rec0iL99 commented May 28, 2023

mdjermanovic left a comment

nzakas commented Jun 19, 2023

perf: Fast path for NodeEventGenerator #17019

perf: Fast path for NodeEventGenerator #17019

Conversation

nzakas commented Mar 24, 2023

Prerequisites checklist

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

What changes did you make? (Give an overview)

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

netlify bot commented Mar 24, 2023 • edited

✅ Deploy Preview for docs-eslint canceled.

Choose a reason for hiding this comment

nzakas commented Mar 28, 2023

github-actions bot commented Apr 7, 2023

amareshsm commented Apr 7, 2023

github-actions bot commented Apr 18, 2023

nzakas commented Apr 19, 2023

fasttime commented Apr 20, 2023

nzakas commented Apr 20, 2023

fasttime commented Apr 22, 2023

Before

After

fasttime Apr 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 2, 2023

nzakas commented May 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fasttime commented May 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 27, 2023

Rec0iL99 commented May 28, 2023

mdjermanovic left a comment

Choose a reason for hiding this comment

nzakas commented Jun 19, 2023

netlify bot commented Mar 24, 2023 •

edited

fasttime Apr 22, 2023 •

edited