-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
perf: Fast path for NodeEventGenerator #17019
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
}; | ||
} | ||
|
||
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}`); | ||
|
@@ -204,14 +216,16 @@ 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, | ||
|
@@ -260,9 +274,10 @@ class NodeEventGenerator { | |
this.exitSelectorsByNodeType = new Map(); | ||
this.anyTypeEnterSelectors = []; | ||
this.anyTypeExitSelectors = []; | ||
this.knownTypes = new Set(Object.keys(esqueryOptions.visitorKeys)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I run the VSCode profiler 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; | ||
|
@@ -293,6 +308,12 @@ class NodeEventGenerator { | |
* @returns {void} | ||
*/ | ||
applySelector(node, selector) { | ||
|
||
if (this.knownTypes.has(selector.rawSelector)) { | ||
this.emitter.emit(selector.rawSelector, node); | ||
return; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could add a property There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I’m missing some context here. Why would we do that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For performance reasons. Whether the selector is an identifier is already known at the parse time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh wait, are you saying that instead of checking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, at this point if it's an identifier then it certainly matches. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. Updated. |
||
|
||
if (esquery.matches(node, selector.parsedSelector, this.currentAncestry, this.esqueryOptions)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, I should have clicked on that line rather than the whole changed block, sorry.
Fair enough. That would also not affect performance, only style. |
||
this.emitter.emit(selector.rawSelector, node); | ||
} | ||
|
There was a problem hiding this comment.
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.