Skip to content

perf(state): batch append event log writes - #4095

Merged
huangruiteng merged 3 commits into
loopx-project:mainfrom
Duang777:codex/optimize-core-hotpath-2
Sep 10, 2026
Merged

huangruiteng merged 3 commits into
loopx-project:mainfrom
Duang777:codex/optimize-core-hotpath-2

Conversation

@Duang777

@Duang777 Duang777 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • load and normalize the existing event log once per append_many call instead of once per event
  • hold one file lock and one append stream for the batch
  • route single-event append through the same implementation while preserving event-id replay and sequence behavior

Performance evidence

Three-run median for appending 500 Todo events to a new temporary event log:

  • before: 3770.71 ms
  • after: 3.33 ms
  • improvement: approximately 1,130x

The previous implementation repeatedly re-read an ever-growing JSONL file, making batch append quadratic. The new path reads once and remains linear.

Correctness

The regression covers one log load per batch, new events, replay of an event already on disk, duplicate events within the same batch, and continuous append_sequence allocation. Existing conflict behavior and partial writes before a later failure remain intact.

Validation

  • 324 event-store and downstream control-plane tests passed before the final rebase
  • 26 focused tests passed after rebasing onto the latest main
  • event-sourced Markdown backfill smoke passed
  • Ruff and git diff checks passed
  • loopx canary premerge --from-git-diff passed (4/4 selected canaries)

Scope

The bounded refactor consolidates append and append_many on one implementation. No cache, index, schema, or migration was added.

@Duang777

Duang777 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

CI triage: the failing Python checks are not caused by this PR. The same two failures reproduce on current main in run https://github.com/huangruiteng/loopx/actions/runs/34224294038:

  • pytest collection sees two modules named test_native_codex_profile.py (benchmark/tests and tests/capabilities)
  • tests/test_sonarcloud_workflow.py still expects 3 guarded Sonar steps, while the workflow now has 4

The PR-specific DCO, dependency review, build, lint/type checks, installed-package checks, mutant checks, focused 324-test suite, and event-sourced canaries passed. Holding merge for the repository-wide CI repair.

@Duang777
Duang777 force-pushed the codex/optimize-core-hotpath-2 branch from fd22946 to 6510a3b Compare September 8, 2026 13:32
@Duang777

Duang777 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto 5008a30f2, the now-green main CI baseline, and force-pushed head 6510a3b7f to trigger a clean qualification run. Post-rebase local checks: focused event-store tests 6/6 passed, Ruff passed, diff check passed, and loopx canary premerge --from-git-diff passed all 4 selected canaries.

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

详细中文评审(REQUEST_CHANGES)

评审绑定 exact head:6510a3b7f526b7355e194528eb0df1557bd62aff

动机

这个 PR 指向一个真实且明显的性能问题:旧 append_many 对每个 event 都调用一次 append,反复读取不断变大的 JSONL、反复加锁和打开文件,使具体 batch 呈二次增长。作者的 500-event 数据从约 3771ms 降到 3.33ms,方向有价值;把单条和批量写入收敛到 AppendOnlyStateEventStore 这个既有 owner,也比引入 cache/index 更合适。

但当前方法的类型合同是 Iterable[dict],不只是预先物化的 list。旧实现会在每次向 iterator 请求下一项之前,完成上一项的写入、关闭/flush stream 并释放锁;head 则在持有一次锁和一个 buffered stream 时继续推进 iterator。作者声明“no functional changes”并声称保留 partial-write 行为,但这一细微的 lazy-Iterable 可观测语义没有被评估。

改动思路

正向的 materialized-list 路径设计是合理的:先取得首项以保持 empty batch 快返,然后持锁 load() 一次,构造 event-id map 和下一 sequence;每个 event 经过同一个 normalize_state_event,相同 fingerprint 的重复项复用 prior,新项写入同一 stream 并递增 sequence,finally 保证关闭。append 再通过一元素 tuple 复用这条实现,删除了重复规则。

问题出在锁/flush 边界和 iterator 生命周期被无条件绑在了一起。真实反例是一个 generator:yield event-1 后,在继续 yield event-2 前调用同一个 store 的 load()。base 在 generator 恢复前已关闭第一笔 append,因此能看到 event-1;head 的第一笔仍在 buffered stream 中,generator 看到空列表。若 generator 在两次 yield 之间调用 store.append(),head 还会在已持有的 lock 上再次等待并超时,留下部分前缀。现有 list-only 单测无法观察这个变化。

具体改动

  • loopx/event_sourced_state.py:44 行新增、16 行删除。append 改为委托 append_many;append_many 一次持锁、一次 load、一次 stream,维护内存中的 idempotency map 与 sequence。
  • tests/test_event_sourced_state_store.py:新增 38 行,验证 list batch 只 load 一次、磁盘已有重复项、batch 内重复项以及连续 sequence;正向覆盖很好,但输入已经完全物化。

关键代码讲解

  1. AppendOnlyStateEventStore.append(event_sourced_state.py:584)的收敛本身没有问题,单条 active caller 的最终结果保持。
  2. AppendOnlyStateEventStore.append_many(:587)在 exclusive_file_lock 内的 for event in chain((first,), iterator)(:603)推进未知 iterator;这是语义变化的 owner。
  3. stream.write(:620)后没有在请求下一 iterator item 前 flush,导致同进程真实 load() 看不到已经逻辑 append 的前缀。
  4. 新测试(test_event_sourced_state_store.py:35)只覆盖 list,不能 falsify lazy/reentrant input 的 postcondition。

对主干的风险

这是一个 P1 blocker。触发条件是 append_many 接收 lazy Iterable,且 producer 在两次 yield 之间根据 durable prefix 读取或写入同一个 store。错误结果不是单纯性能差异:下一 event 可能基于 stale state 生成;reentrant append 可能等待同一锁并在部分写入后失败。事件 ID 使纯 replay 可能安全,但如果第二个 event 内容是基于错误观测生成,重试会产生 fingerprint conflict 或错误的持久化事实。

我用相同临时 JSONL、相同两条 synthetic public-safe event 在 immutable base/head 上实际运行:base 的 mid-iteration read 为 ['event-1'],head 为 [],最终两边都持久化两条;这证明现有 final-state 测试会绿但中间语义已经漂移。与此同时,现有 6 个 focused tests、event-sourced Markdown backfill smoke、Ruff、diff check、4/4 event-sourced premerge canary 和全部远端 checks 都通过,说明 blocker 恰好位于现有覆盖之外。

最小修复不是放弃 batch 优化:只对明确的 concrete materialized Sequence/Collection 使用 one-lock fast path,对 generic lazy Iterable 保留旧的逐项 append 路径;或者提供一个明确的新 API 与兼容迁移,不能在原方法上静默收窄。请增加 generator 回归:第一项 yield 后 load() 必须看到该项,再 yield 第二项;再增加 reentrant append 行,证明不会 lock timeout 或产生未声明的 partial outcome。

我的整体评价

Owner、去重/sequence 规则和 list-batch 性能实现都选对了,代码量也很小;问题不是架构方向,而是优化越过了一个容易漏掉的 Iterable 行为边界。由于 PR 明确声称无行为变化,而 exported method 仍接受任意 Iterable,这个可复现差异必须在合入前处理。修复后应同时重跑 list/idempotency/conflict、lazy generator、partial failure、concurrent writer 以及 event-sourced downstream canaries。

English verdict: REQUEST_CHANGES for exact head 6510a3b7f526b7355e194528eb0df1557bd62aff. The list fast path is valuable, but advancing a lazy Iterable under the held lock and unflushed stream changes observable prefix/reentrant semantics; preserve the legacy iterator path (or introduce an explicit compatible API) and add generator regressions before merge.

@Duang777
Duang777 force-pushed the codex/optimize-core-hotpath-2 branch from 6510a3b to 97c8170 Compare September 9, 2026 00:04
@Duang777

Duang777 commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the requested lazy-Iterable blocker at exact head 97c81700fda04a89cf721026f447c359a9be4d4a.

  • Only concrete list/tuple inputs use the one-lock batch fast path.
  • Generic iterables retain the legacy per-item append boundary, so the durable prefix is visible before requesting the next item and generator-side reentrant appends do not self-lock.
  • Added one generator regression covering both mid-iteration load() visibility and a reentrant append; it also verifies final order and returned batch items.
  • The 500-event list benchmark remains 2.74 ms median versus the original 3770.71 ms baseline.

Validation after rebasing to current main: 345 related tests passed, focused store tests 7/7 passed, Ruff and diff checks passed, and all 4 selected premerge canaries passed.

@Duang777
Duang777 force-pushed the codex/optimize-core-hotpath-2 branch from 97c8170 to fd5dba5 Compare September 9, 2026 07:27
@Duang777

Duang777 commented Sep 9, 2026 •

Copy link
Copy Markdown
Collaborator Author

@huangruiteng 当前 head 5324927 已 rebase 到 main 23e6502。在原 lazy-Iterable 修复上补齐两个独立复核发现:仅精确内置 list/tuple 进入持锁快路径,避免可重入容器子类自锁;每条完整 JSONL 写入后 flush,恢复后续处理/进程退出前的前缀可见性。另修复 Node 26 暴露文件路径导致 fence parity 漂移:在共用 loader 仅剥离 Node error.path,保持既有公开错误合同。验证:Python 29/29、Node 24 parity 22/22、TypeScript typecheck、Ruff 与 diff check 通过,500-event 三次中位数 4.78ms;新 CI 全部通过,请按此 head 复审。

@Duang777
Duang777 force-pushed the codex/optimize-core-hotpath-2 branch from fd5dba5 to 8d641a0 Compare September 9, 2026 08:07
@Duang777
Duang777 force-pushed the codex/optimize-core-hotpath-2 branch 2 times, most recently from 79d25a5 to 8a6faf1 Compare September 10, 2026 02:58
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
@Duang777
Duang777 force-pushed the codex/optimize-core-hotpath-2 branch from ba0aef3 to 8a15366 Compare September 10, 2026 05:07
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

动机

复审结论:APPROVE,绑定完整 head 532492747577317f2f71f53d1694a3fd2df67967。未发现剩余阻塞项。

旧 append_many 每个事件都重新读取不断增长的 JSONL。这个批处理 API 的成本确实值得消除,但收益应准确限定:当前普通 Todo producer 主要调用单条 append,不能把合成 batch 的加速比宣传成整个 CLI 的加速比。无需新增 cache、索引或存储 provider;在既有 owner 内优化更合适。

改动思路

本轮重新检查整个三文件 diff,也检查了上一轮之后的修复。仅精确内置 list/tuple 在同一锁内处理,generic Iterable 和容器子类仍保持旧的逐项调用边界;单条 append 经一元素 tuple 复用写入实现。event ID、fingerprint、normalization、sequence 继续由原有 store 负责,没有引入第二份状态。

与之前的请求修改相比,当前代码已解决 lazy iterator 的可见性/重入问题,并补齐容器子类及逐条 flush。Node fence loader 的伴随修复只去除 error.path 对应的诊断片段,保持 reason_code 和失败关闭行为,不降低门禁。

具体改动

完整 diff 为 +222/-17,其中测试新增 176 行。没有新增 CLI、协议、持久化 schema 或未使用的模块。

关键代码讲解

  • AppendOnlyStateEventStore.append(event_sourced_state.py:583):一元素 tuple 委托同一写入实现;generic iterable 的兼容分支不会造成递归环。
  • append_many(:586):精确类型判断在进入锁之前;已有 event ID 使用原 fingerprint 判冲突,新事件才分配 sequence。每行写入后 flush,finally 关闭 stream;后续错误仍保留已接受前缀,并非全批次原子事务。
  • loadLegacyCoordinationWriterFence(legacy_writer_fence.ts:144):保留缺失与读取失败的区别,仅稳定 Node 版本差异引入的路径文案。

对主干的风险

最强反例不是最终文件是否相同,而是 producer 在两次 yield 之间读取或再次写入相同 store。除现有 mocked-lock 单测外,我使用真实 JSONL、真实文件锁进行了独立 base/head 对照:generator 读取已提交前缀、重入 append、重复 ID、后续冲突及部分前缀的完整结果一致;四个独立进程写入也保持四个唯一 ID 和连续 sequence。

验证:90 项 store/真实 Todo mutation/完成投影 Python 测试通过;22 项真实 fence caller parity 通过;Markdown backfill、downstream read-path、replay/compaction 三项 smoke 通过;TS typecheck、Ruff、diff check 通过。新增“一次 load”回归用同一测试在 baseline 23e650235 上实际失败(4 != 1),在 head 通过。初次验证环境缺 pytest、一次 smoke 路径拼写错误及临时 harness ID 不合法均已纠正后重跑,不计作产品失败。

未宣称 fsync/power-loss 持久性升级、全仓性能提升或 provider promotion。初次误装仓库范围外的 Ruff 0.16,换为项目支持的 0.15.22 后完整 changed-path lint 通过,没有修改规则或源码。主干近期其他变更未改变此 store 的业务规则;最新远端 checks 无失败。公共边界检查没有发现私有材料。

我的整体评价

这是比例合理的既有 API 优化。原评审的阻塞已实质修复,不需要为了继续重构引入泛化 batch 框架。未来面对更大的吞吐需求,应先找到实际批量调用者并测量真实工作负载;本 PR 无需为它预建结构。当前已完成 exact-head 证据检查,可按 maintainer 授权合并。

English verdict: APPROVE at 532492747577317f2f71f53d1694a3fd2df67967. The lazy-iterator, subclass reentrancy and prefix visibility blockers are fixed. Independent real-file baseline/head comparison and four-process contention pass; 90 Python tests, 22 fence caller tests, three event-sourced smokes, typecheck, Ruff and diff checks pass. Batch speedup is not a claim about ordinary single-event CLI performance; no new atomicity, durability or authority promise is introduced.

@huangruiteng
huangruiteng merged commit 41a95d9 into loopx-project:main Sep 10, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants