perf(analysis): share parsed ASTs across rule passes - #779
Merged
duriantaco merged 2 commits intoSep 5, 2026
Conversation
mcdigman
marked this pull request as ready for review
August 28, 2026 22:23
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #775
Closes #778
Summary
Why?
The ai-defects rules contain 2 full parses and 4 full AST walks that are completely duplicative.
That could be improved in isolation, and indeed accounts for most of the perf improvement of this PR, but this PR proposes to build indices that unify all the AST walks in the same scope. In measured counts of ast.walk calls, with --ai-defects the PR reduces the total full AST walks from ~12 to ~2.
I can de-scope unifying all the other AST walks if desired.
As implemented, the fix would inevitably create a merge conflict with an isolated fix to #775, so I just incorporated a fix to that into this PR as well; it could be done as a separate PR that then has its code overwritten by this one, or fixed in isolation if the full unification is not desired.
Also, as implemented, the cache is not thread safe if the outer
analyzewas called separately from multiple different threads. That isn't how skylos uses it so I thought that was an acceptable caveat, but it can be fixed or modified if desired by the maintainer; I didn't want to get too far ahead without knowing if merging all the other AST.walks outside ai-defects is ok in the first place.AI Use Disclosure
The duplicate walks were found by analysis of cProfile results, and scoped the way to unify them. Opus 5.0 drafted the changes and they were reviewed according to skylos repository standards by codex GPT-5.6 Sol and claude Fable 5.
Performance impact
About a ~10% reduction in overall time for
liveness_primerscans including--ai-defectsand--quality. In the mcp-context-forge example, the biggest component of the perf impact is shaving ~30s from the ai-defects scan. For mcp-context-forge, cuts ~31 million calls to ast.walk.How to test
Results on this head: