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.

Sign up for GitHub

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

Jump to bottom

perf: Fast path for NodeEventGenerator #17019

Closed
wants to merge 2 commits into from
Closed

perf: Fast path for NodeEventGenerator #17019

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99ae371
perf: Fast path for NodeEventGenerator
nzakas Mar 24, 2023
4eeb542
Switch to use isIdentifier for apply selector
nzakas Jun 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 27 additions & 5 deletions lib/linter/node-event-generator.js
View file
Open in desktop
Expand Up @@ -185,12 +185,24 @@ function compareSpecificity(selectorA, selectorB) {
/**
* Parses a raw selector string, and throws a useful error if parsing fails.
* @param {string} rawSelector A raw AST selector
* @param {Set<string>} knownTypes The known node types
* @returns {Object} An object (from esquery) describing the matching behavior of this selector
* @throws {Error} An error if the selector is invalid
*/
function tryParseSelector(rawSelector) {
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
};
}
Comment on lines +192 to +202
Copy link
Member

Choose a reason for hiding this comment

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

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.


try {
return esquery.parse(rawSelector.replace(/:exit$/u, ""));
return esquery.parse(rawSelectorWithoutExit);
} catch (err) {
if (err.location && err.location.start && typeof err.location.start.offset === "number") {
throw new SyntaxError(`Syntax error in selector "${rawSelector}" at position ${err.location.start.offset}: ${err.message}`);
Expand All @@ -204,19 +216,22 @@ const selectorCache = new Map();
/**
* Parses a raw selector string, and returns the parsed selector along with specificity and type information.
* @param {string} rawSelector A raw AST selector
* @param {Set<string>} knownTypes The known node types that don't require parsing.
* @returns {ASTSelector} A selector descriptor
*/
function parseSelector(rawSelector) {
function parseSelector(rawSelector, knownTypes) {
if (selectorCache.has(rawSelector)) {
return selectorCache.get(rawSelector);
}

const parsedSelector = tryParseSelector(rawSelector);
// no need to parse anything that matches node.type
const parsedSelector = tryParseSelector(rawSelector, knownTypes);

const result = {
rawSelector,
isExit: rawSelector.endsWith(":exit"),
parsedSelector,
isIdentifier: parseSelector.type === "identifier",
listenerTypes: getPossibleTypes(parsedSelector),
attributeCount: countClassAttributes(parsedSelector),
identifierCount: countIdentifiers(parsedSelector)
Expand Down Expand Up @@ -260,9 +275,10 @@ class NodeEventGenerator {
this.exitSelectorsByNodeType = new Map();
this.anyTypeEnterSelectors = [];
this.anyTypeExitSelectors = [];
this.knownTypes = new Set(Object.keys(esqueryOptions.visitorKeys));
Copy link
Member

@fasttime fasttime Apr 22, 2023 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.


emitter.eventNames().forEach(rawSelector => {
const selector = parseSelector(rawSelector);
const selector = parseSelector(rawSelector, this.knownTypes);

if (selector.listenerTypes) {
const typeMap = selector.isExit ? this.exitSelectorsByNodeType : this.enterSelectorsByNodeType;
Expand Down Expand Up @@ -293,6 +309,12 @@ class NodeEventGenerator {
* @returns {void}
*/
applySelector(node, selector) {

if (selector.isIdentifier) {
this.emitter.emit(selector.rawSelector, node);
return;
}

if (esquery.matches(node, selector.parsedSelector, this.currentAncestry, this.esqueryOptions)) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

this.emitter.emit(selector.rawSelector, node);
}
Expand Down