perf(semantics): parse each Python source once per inventory run - #4628
Conversation
python_facts now memoizes on the frozen SourceFile (path, suffix, text), so owner_values and build_inventory share one parse per file instead of two. Measured on the drift smoke: 2120 -> 1060 ast.parse calls (1088 cache hits), wall time 19s -> 17s, output byte-identical apart from the budget lines. A mutated file is a different cache key, so a test that edits a SourceFile still sees fresh facts; the full architecture suite (288 tests) passes unchanged. Signed-off-by: song <liusongstep@gmail.com>
…memoization Signed-off-by: song <22676124+songoow@users.noreply.github.com>
本 PR 在 #4447 计划中的位置issue #4447 现在有一节统一协调(中英双语),把这 13 个在开 PR 作为一个计划列出:各自修什么、为何必要、以及实测出的合并顺序。 冲突实测:对全部 78 对做了试合并,9 对冲突,分四簇,每一处都是文本相邻,没有一处是语义分歧。
建议顺序(代价从低到高):#4628 → #4625、#4626 → #4627 → #4619、#4621 → #4630 → #4614 → #4631 → #4629 → #4617 → #4606 → #4608。四个棘轮 PR 放最后,因为每落地一个,下一个的数字就从估算变成确定值。 全部 13 个 PR 现已同步到 |
…memoization Signed-off-by: song <22676124+songoow@users.noreply.github.com>
huangruiteng
left a comment
There was a problem hiding this comment.
Reviewed exact head: 4e737a206aed82a86bb3b9a734f1d6776495a4ce (codex/semantic-parse-memoization).
动机
这次改动的目标是把「同一个文件在一次 inventory 运行里被解析多次」这个重复成本去掉,属于仓库内语义护栏自身的成本优化,不是产品行为变更。改动前 python_facts 每次调用都会重新解析源码;漂移 smoke 先按 owner 逐个调用 owner_values,随后 build_inventory 又扫全树,于是同一批文件在一次运行里被解析两遍以上。这个重复成本会随 registry 条目增长,所以它不是一个随 diff 大小变化的常数。
作者的声明(2120 → 1060 次 ast.parse、19s → 17s、输出逐字节一致)方向正确。我在同一棵树上把 9 行补丁反向应用后独立计数:ast.parse 调用 6186 → 5107(差 1079),wall time 14.2s → 12.2s,stdout 逐字节一致。我的绝对数字与作者不同(我的统计口径覆盖整次 smoke,而不只是两次 inventory 扫描),因此效果方向与量级被独立复现,作者给出的绝对值仍然只是作者的测量。
改动思路
入口是 python examples/semantic-vocabulary-drift-smoke.py --report(scripts/generate_semantic_inventory.py 与 tests/architecture/ 复用同一模块)。权威输入是 load_sources 依据 git ls-files 生成的冻结 SourceFile(path, suffix, text),唯一影响行为的字段是 text。决策边界在 loopx/semantics/inventory.py:python_facts 拥有 Python 事实抽取规则,改动只在这条纯函数上加一层有界 memo,没有把规则搬到调用方。
复用上,仓库已有同形状先例:scripts/computer_use_runtime_contract_validator.py、loopx/control_plane/effect_program.py、loopx/control_plane/agents/capability_gate.py、loopx/control_plane/todos/completion_state.py 都用 functools.lru_cache 加有界 maxsize 缓存纯 helper。因此这里没有引入新的缓存设施或第二份事实抽取器,符合既有约定。
我认真找了「不发布」的最强理由:收益只是维护者/CI 的几秒,属于可选优化;调用方也可以自己缓存。但重复解析规则同时存在于两个独立调用方(smoke 的 owner_values 与 build_inventory/collect_string_constants),放到调用方会把同一条规则写两遍,而这里只用一个装饰器就同时覆盖两处,输出逐字节一致且可一行回退。因此它不足以构成阻塞理由。
具体改动
1 个生产文件、9 行新增、0 行删除:新增 functools.lru_cache 导入、@lru_cache(maxsize=4096) 装饰器、以及解释缓存键与「文件变了就是不同键」的 docstring。没有测试文件改动,因为该模块已被现有架构套件覆盖。loopx/ 下 .py 文件 1061 个,低于 maxsize=4096,因此一次全树扫描不会发生淘汰;缓存只存在于进程内(仅 scripts/、examples/、tests/ 引用该模块,产品运行时不引用)。
关键代码讲解
SourceFile(loopx/semantics/inventory.py:29):冻结 dataclass,身份就是(path, suffix, text),因此可以直接当作缓存键。它同时是「不会读到陈旧事实」这一不变量的依据:文本变了就是不同键,不需要额外失效逻辑。python_facts(loopx/semantics/inventory.py:119):函数体一行未改,只是加了有界 memo。命中时返回的是同一份对象图(dict 及其嵌套 list/dict),这是本次唯一新增的隐含契约,也是下面风险一节的核心。build_inventory/collect_string_constants(loopx/semantics/inventory.py:277、loopx/semantics/inventory.py:368)与 smoke 的owner_values:三个调用点都只读,用list.extend把缓存里的条目 dict 引用聚合进新列表,没有原地修改;multi_value_carriers用{**entry, "kind": kind}生成新 dict。
对主干的风险
我跑的证据:pytest -q tests/architecture/test_semantic_inventory.py tests/architecture/test_semantic_vocabulary_drift.py 76 passed(23s,exact head);smoke rc 0;同一棵树上加/不加装饰器的 stdout diff 为空;反向验证「文本变了换键」与「非法源码仍抛 ValueError」两条负路径。头部自身检查为 SUCCESS,当前 merge_state=BLOCKED 是分支保护/待合并状态,不是冲突。
有一个非阻塞 P2,我独立复现过:lru_cache 命中时把同一份可变结构交给所有调用方,而 python_facts 不做拷贝。任何调用方原地修改返回的条目,都会在同一进程内污染后续扫描——两次扫描同一棵树会给出不同结果,且没有任何检查能发现。当前仓库内的调用点都不修改,所以这是一条休眠路径而不是已触发的缺陷,因此不阻塞;但仓库已有的最近似先例(capability_gate._missing 缓存私有 helper 并返回 immutable tuple,公开包装返回新 list)恰好给出了推荐修法。建议缓存私有 helper、公开函数返回拷贝(或在 docstring 中明确「返回值不得修改」),并在 tests/architecture/test_semantic_inventory.py 增加一条断言:第二次调用的结果不是同一对象,或修改一次结果不影响下一次调用。
另外记录一个未验证维度:typescript_facts 未缓存。这是有意的范围边界(TS 侧是正则扫描,成本低),不构成问题,但意味着缓存收益只覆盖 Python 侧。
我的整体评价
结论 APPROVE。这是一个 9 行、可一行回退、无行为变更的成本优化:独立复现显示解析次数与耗时下降且输出逐字节一致,机制复用了仓库既有 lru_cache 约定,边界正确地留在拥有该规则的模块里,也没有新增 CLI、schema、持久化字段或权威面。唯一的 P2 是缓存返回共享可变对象这一隐含契约,当前无触发者,可用「私有缓存 + 公开拷贝」这一仓库已有模式修掉,作为非阻塞建议保留在本评论中。
English verdict: APPROVE - exact head 4e737a2; the 9-line lru_cache memoization of the pure python_facts helper reproduces the claimed cost drop (6186 -> 5107 ast.parse calls, byte-identical drift-smoke stdout when the decorator is removed on the same tree, 76 architecture tests pass). One non-blocking P2: the cached dict/list graph is returned uncopied, so a future in-place consumer could poison later scans in the same process; repair by caching a private helper and returning a copy, matching the existing capability_gate pattern. Merge readiness is a separate qualification and is not granted by this review.
|
Review evidence correction (no verdict change), still at exact head In the review above I reported the head's own checks as SUCCESS. That snapshot was taken while the check list was still filling in for the freshly pushed head; as of now the head shows 17 checks with most still queued (no failures observed). The code verdict stands on the local evidence I executed at this head (drift smoke rc 0, byte-identical stdout with the decorator removed on the same tree, 76 architecture tests passing); final CI completion belongs to the merge-readiness gate, not to this review conclusion. |
Six tracker PRs landed while this branch was open. Three touched files it also edits, in the way loopx-project#4447's merge-order note predicted: - loopx-project#4626 and loopx-project#4625 append to the end of `test_semantic_vocabulary_drift.py`; both blocks are kept, theirs first. - loopx-project#4627 replaced the Section 11 target table with a *Measured by* column and a rule that the table carries no dated values, since those belong to the tracker. This branch's row had added dated numbers, so the resolution takes loopx-project#4627's table and puts the migration surface in *Measured by* as the `--report` line that prints it. The dated table stays in Appendix A. - loopx-project#4628 memoized `python_facts`. The retirement scan needs the tree rather than the facts, so `parse_python` is factored out for one error path and left uncached: caching the trees held about two million AST nodes for the rest of the run and measured 0.7s worse overall, while slowing `check_inventory` from 2.4s to 5.4s -- the pass loopx-project#4628 had just made cheaper. Remeasured on the integrated tree: every role count is unchanged, and `dynamic_mapping_key_sites` moved 1704 to 1712 with the new code. Both mirrors carry the new number. `loopx/semantics/field_use.py` also had to stop spelling the six field names in its own docstrings. The scan reads tracked sources under `loopx/`, this module is one of them, and committing it pushed `heartbeat_recommendation` to 18 of a budget of 17 -- the check catching its own module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: song <22676124+songoow@users.noreply.github.com>
No conflict; the branch was only behind. loopx-project#4628 memoized python_facts in the same module this branch extends, and the two changes compose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: song <22676124+songoow@users.noreply.github.com>
What
python_factsnow memoizes on the frozenSourceFile(path, suffix, text).owner_valuesandbuild_inventoryin the drift smoke each parsed every file once per run; they now share one parse per file.Why
Measured on
examples/semantic-vocabulary-drift-smoke.py: 2120 → 1060ast.parsecalls (1088 cache hits), wall time 19s → 17s, output byte-identical apart from the budget lines. A mutated file is a different cache key (frozen dataclass), so tests that edit aSourceFilestill see fresh facts — no stale-read window.Validation
python3.11 examples/semantic-vocabulary-drift-smoke.py— green, output matches the pre-change run except the budget anchor lines this branch does not touchpython3.11 -m pytest -q tests/architecture/— 288 passedcodex/lock-inventory-ratchets,codex/merge-candidate-visibility,codex/invariant-domain, and open semantics PRs refactor(semantics): single-source five duplicated multi-value vocabularies #4617 / refactor(status): single-source six duplicated status vocabulary constants #4606 / refactor(todos): import the Todo task-class vocabulary from its owner #4608 / feat(semantics): report the name-keyed value-set divergence a rename hides #4614Scope
One file, nine lines, no behavior change. Pure cost reduction of the guard itself.