perf(analyzer): index export lookups - #771
Conversation
|
Thanks for putting this together the implementation looks sound. One request before merge: could you add a deterministic regression guard for the performance property? The new tests validate preserved output, but they also pass against the previous per-exported-class full scan. A small instrumented mapping or scale probe that confirms self._global_type_map is indexed once, rather than .items() being scanned for every reachable class, would protect the actual O(n^2) fix without relying on wall-clock timing. Separately, and completely unrelated to review or merge.. we really value the depth of your profiling and feedback. Would you be interested in being a design partner for the paid version of Skylos? We would love occasional candid feedback on workflows, pain points, etc. There is no commitment or pressure at all. If you are interested, let us know and we can find a good way to chat privately. |
|
Ok, I added unit tests with spies to check the number of calls is consistent with expectations, both in the case you described and the logarithmic search. Note if future amendments to the function add more calls to the spied-upon attributes deliberately, the test thresholds would also have to be revised. I tried to email the aaron@skylos.dev email but my email bounced; is it listed correctly? I also sent one to the email in the pyproject.toml |
Follow-up to #769 applying analogous fixes to
_mark_exports.I started drafting this before I noticed the new PR #770 had merged, which already absorbs the vast majority of the perf win this was going to have. I decided to still PR it anyway, because it squashes the last O(n^2) bit of the method and so is still a meaningful speedup on very large repositories like homeassistant/core.
I also preserved some light refactor changes I was doing that reduce the measured cyclomatic and cognitive complexity a bit more, by doing early returns and fusing loops to make the crucial control flow ordering clear and avoid differently named variables that do exactly the same thing.
Bottom line
Before this raft of O(n^2) squashing performance optimizations, my first run of
homeassistant/coretook ~5 hours (which is why I was only runninghomeassistant/core/homeassistantas a baseline). Now, with the full it takes just over 6 minutes, taking running skylos on the full repo from unfeasible to merely a quick coffee break. This sequence of O(n^2) fixes therefore represents a categorical improvement in the kinds of repos that could feasibly use skylos.Summary
Complexity
A lot of the line diff in the PR is just unindenting due to early returns; it meaningfully reduces the complexity metrics skylos uses to score the function, although they are still high.
Behavior preservation
__all__remains authoritative per source fileliveness_primerchecked on expanded corpus, 0 diffs vs current mainPerformance
An isolated 5,000-class transitive-export benchmark improved from 0.541s to 0.049s.
_mark_exports_mark_exportshomeassistant/core/homeassistant)For a full-root
homeassistant/corescan with no cProfile isolation of_mark_exports, the speedup is 461.91s -> 384.77s, saving 77.14s.Validation
_mark_exportstests passedgit diff --checkpassedAI Use Disclosure
The quadratic paths were identified from cProfile data. I did most of the original refactoring by hand in analogy to the previous
_mark_refsspeedup, with guidance from Codex. Codex integrated the optimization with the newer export semantics and ran behavior-preservation validation; the resulting diff was reviewed by Codex and Opus 5.0 agents.