perf(js-plugins): optimize CFG walker with SoA pattern and custom traverser#17423
perf(js-plugins): optimize CFG walker with SoA pattern and custom traverser#17423re-taro wants to merge 2 commits intooxc-project:mainfrom
Conversation
19b0bc3 to
71a255d
Compare
There was a problem hiding this comment.
Pull request overview
This PR optimizes the walkProgramWithCfg function by replacing ESLint's data structures and traverser with more efficient custom implementations. The optimization focuses on reducing object allocations, improving memory locality, and eliminating redundant lookups.
Key changes:
- Replaced class-based step encoding with plain numeric constants and Struct of Arrays (SoA) pattern
- Pre-computed type IDs during step building to eliminate Map lookups during AST walking
- Implemented a lightweight custom
traverseNodefunction to replace ESLint'sTraverser
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/oxlint/src-js/plugins/cfg.ts | Core optimization implementation: replaced step classes with SoA pattern, added custom traverser, pre-computed type IDs |
| apps/oxlint/package.json | Removed unused @eslint/plugin-kit dependency |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isArray(node)) { | ||
| const len = node.length; | ||
| for (let i = 0; i < len; i++) { | ||
| traverseNode(node[i], enter, leave); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
The isArray check treats arrays as nodes to traverse recursively, but this will not call enter/leave callbacks on array elements that are actual AST nodes. The function signature accepts Node | null | undefined, not arrays, so this check appears to handle child node arrays incorrectly. Arrays should be iterated and their elements passed to traverseNode, but the type signature should clarify this or the logic should ensure enter/leave are called appropriately for node elements.
| for (const [name, id] of NODE_TYPE_IDS_MAP) { | ||
| if (id === eventTypeId) { | ||
| eventNames.push(name); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Reverse lookup iterating through the entire NODE_TYPE_IDS_MAP is inefficient in debug builds. Consider creating a reverse map (ID to name) at module initialization for O(1) lookups instead of O(n) iteration for each event.
c8a6284 to
31dc842
Compare
…verser Apply multiple performance optimizations to `walkProgramWithCfg`: - Replace `VisitNodeStep`/`CallMethodStep` classes with plain number constants - Pre-compute type IDs in first pass to eliminate Map lookups in second pass - Use Struct of Arrays (SoA) pattern for step storage to improve memory locality - Replace ESLint's Traverser with lightweight custom `traverseNode` function - Remove unused `@eslint/plugin-kit` and ESLint Traverser imports
31dc842 to
cf1fc5f
Compare
There was a problem hiding this comment.
Thank you for working on this.
I'm on holiday for 10 days, so I'm afraid I won't be able to review properly for some time. Sorry for delay.
But a couple of notes in meantime:
- Would it be possible to break this up into multiple PRs, split (roughly) along the lines of the points in the original TODO comment? That'd make this easier to review and get parts of it merged while we discuss other parts. I'm not sure from first glance if AI review suggesting flaws in new traversal impl are valid or not.
You can make a "stack" by basing each PR on the previous PR. I can handle rebasing each onto main before merging.
- I think the SOA arrays can probably be cut down from 4 to 2:
stepTargetsandstepArgscould be combined. When step type is "enter node" or "exit node", the array element would be the node object. When it's a CFG event, the array element would be an array of args to call the visitor function with.typeIdand step type could be combined by usingtypeIdunchanged for "enter node" andtypeId + Nfor "exit node" (whereNis some number greater than maximum type ID e.g. 256).
|
@re-taro I'm back from being away. Wondered if you have an interest in continuing with this? |
|
Yes! I am interested!! However, I am quite busy myself and it seems I can respond starting this weekend. |
|
That's great to hear! Please ping me when you've had time to make some progress. |
|
Hi @overlookmotel, thanks for your feedback! I've split the original PR into 3 separate PRs as you suggested, following the TODO comments:
Since GitHub doesn't support PR stacks across forks, I created them as independent PRs targeting main. They should be merged in order (1 → 2 → 3). I'll rebase each one after the previous is merged. I also incorporated your suggestion to reduce the SoA arrays from 4 to 2:
Let me know if you'd like any changes! |
|
Superceded by the 3 PRs listed above. |
Summary
Optimize
walkProgramWithCfginapps/oxlint/src-js/plugins/cfg.tsby applying multiple performance improvements based on TODO comments in the original codeClose #17232
VisitNodeStep/CallMethodStepclasses from@eslint/plugin-kitwith plain number constantsTraverserwith a lightweight customtraverseNodefunction that avoids unnecessary overheadChanges
VisitNodeStep/CallMethodStepclassesSTEP_TYPE_*steps[])stepTypes[],stepTargets[],stepTypeIds[],stepArgs[])Traverser.traverse()traverseNode()usingvisitorKeysRemoved dependencies
@eslint/plugin-kit(no longer needed)Traversermodule