Skip to content

refactor(stream): index parked readers by offset instead of full scans - #3048

Open
ILoveScratch2 wants to merge 1 commit into
mainfrom
refactor/stream-continuation-reader-index
Open

refactor(stream): index parked readers by offset instead of full scans#3048
ILoveScratch2 wants to merge 1 commit into
mainfrom
refactor/stream-continuation-reader-index

Conversation

@ILoveScratch2

Copy link
Copy Markdown
Member

Summary / 摘要

RangeReadReadAtSeeker 中存储 continuation reader 的 sync.Map 替换,参考 #3010 ,优化效率。

无用户可感知的行为变化。

  • This PR has breaking changes.
    / 此 PR 包含破坏性变更。
  • This PR changes public API, config, storage format, or migration behavior.
    / 此 PR 修改了公开 API、配置、存储格式或迁移行为。
  • This PR requires corresponding changes in related repositories.
    / 此 PR 需要关联仓库同步修改。

Related repository PRs / 关联仓库 PR:

  • OpenList-Frontend:
  • OpenList-Docs:

Related Issues / 关联 Issue

Relates to #3010

Testing / 测试

  • go test ./...
  • gofmt / go vet
  • Manual test / 手动测试:

Checklist / 检查清单

  • I have read CONTRIBUTING.
    / 我已阅读 CONTRIBUTING
  • I confirm this contribution follows the repository license, contribution policy, and code of conduct.
    / 我确认此贡献符合仓库许可证、贡献规范和行为准则。
  • I have formatted the changed code with gofmt, go fmt, or prettier where applicable.
    / 我已按适用情况使用 gofmtgo fmtprettier 格式化变更代码。
  • I have requested review from relevant maintainers or code owners where applicable.
    / 我已在适用情况下请求相关维护者或代码所有者审查。

AI Disclosure / AI 使用声明

  • This PR includes AI-assisted content.
    / 此 PR 包含 AI 辅助内容。

Tools used / 使用工具:

  • ChatGPT
  • Codex
  • GitHub Copilot
  • Claude
  • Gemini
  • Other (please specify) / 其他(请注明): DeepSeek V4 Flash

Usage scope / 使用范围:

  • Code generation / 代码生成

  • Refactoring / 重构

  • Documentation / 文档

  • Tests / 测试

  • Translation / 翻译

  • Review assistance / 审查辅助

  • I have reviewed and validated all AI-assisted content included in this PR.
    / 我已审核并验证此 PR 中的所有 AI 辅助内容。

  • I have ensured that all AI-assisted commits include Co-Authored-By attribution.
    / 我已确保所有 AI 辅助提交都包含 Co-Authored-By 归属信息。

  • I can reproduce all AI-assisted content included in this PR without any AI tools.
    / 我可以在没有任何 AI 工具的情况下重现此 PR 中包含的所有 AI 辅助内容。

RangeReadReadAtSeeker kept continuation readers in a sync.Map and every
ReadAt scanned all entries to find an exact or within-window match,
degrading to O(N) as parked readers accumulate under random access.

Store readers in a mutex-guarded map with sorted keys instead: exact
hits are O(1) and nearest-within-window lookups are O(log N). Finding
and removing a reader is atomic, which also drops the stale-range retry
loop of the old sync.Map snapshot walk.

Behavior is unchanged: parked readers stay single-use, the 4 MiB reuse
window and fresh range request on miss are kept.

Co-authored-by: DeepSeek V4 Flash

@pikachuren pikachuren 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.

🙏 感谢 @ILoveScratch2 提交!
🤖 AI 自动审核声明:本评审报告由 AI 自动生成,当前使用 Claude Opus 5 模型进行分析。
⚠️ AI 分析结果仅供参考,可能存在误判或遗漏。如您发现任何问题或有不同意见,欢迎随时提出讨论和纠正。
⚠️ 重要提醒:即使 AI 评审认为代码质量良好且建议合并,最终是否合并仍需由项目维护者进行人工判定。项目维护者会综合考虑代码质量、项目规划、技术方向、团队资源等多方面因素做出是否合并的决策。

🎯 结论

✅ Approve — 代码质量优秀,性能优化合理,测试覆盖充分

📖 概要

refactor(stream): index parked readers by offset instead of full scans · 将 RangeReadReadAtSeeker 中存储 continuation reader 的 sync.Map 替换为有序索引结构,消除 O(n) 全量扫描,优化为 O(log n) 二分查找

无用户可感知的行为变化

🧭 整体方案

采用 排序键数组 + map 的组合数据结构,替代原有的 sync.Map 全量遍历:

  • 核心优化:通过维护有序的 keys []int64 切片,使用 sort.Search 实现 O(log n) 的二分查找,避免了原有的 O(n) 全量扫描
  • 层次清晰:新增 orderedReaders 类型封装了索引和查找逻辑,接口设计合理(storetakeExacttakeBest
  • 测试充分:新增 179 行测试代码,覆盖顺序读、窗口内跳跃、窗口外跳跃、向后跳跃、随机读等核心场景

方案与 issue #3010 中的建议一致,技术路线正确。

📊 变更统计

2 个文件(+250 / -33 行) | 功能 ⭐⭐⭐⭐⭐ | 最小改动 ⭐⭐⭐⭐ | 前向兼容 ⭐⭐⭐⭐⭐ | 方案设计 ⭐⭐⭐⭐⭐

🚨 关键问题

无 P0/P1 问题

P2(可选)

  • 💡 orderedReaders.removeKey 中的线性删除:当前使用 copy(o.keys[i:], o.keys[i+1:]) 执行 O(n) 的切片移动操作。虽然在实际场景中 parked readers 数量很少(通常 < 10),但如果未来需要支持更多并发读取流,可以考虑:
    • 使用延迟删除(标记为删除,定期压缩)
    • 或使用更高效的数据结构(如跳表)
    • 但目前场景下,这个优化的收益很小,代码简洁性更重要
  • 💡 测试中的随机数生成器randomData 函数使用固定种子 42 的 LCG(线性同余生成器),测试足够确定性。但 TestReadAtSeekerRandomReads 中使用 rand.New(rand.NewSource(7)) 而不是复用同一生成器,这是合理的(避免测试间干扰),建议在注释中说明为何使用不同种子

📂 逐文件分析

internal/stream/readat_test.go(新增 +179 行)

改动意图:为 RangeReadReadAtSeeker 的优化行为添加完整测试覆盖
代码逻辑

  • 辅助函数
    • newMockSeekableStream:构造可计数的 mock stream,精确跟踪上游 range 请求次数
    • readAtFull:封装 ReadAt 调用并验证数据正确性
    • randomData:使用固定种子 LCG 生成确定性随机数据(避免测试抖动)
  • 测试用例
    • TestReadAtSeekerSequentialReuse:验证顺序读只触发 1 次上游请求(continuation reader 复用)
    • TestReadAtSeekerSkipsAheadWithinWindow:验证窗口内(≤4MB)跳跃能复用 parked reader,通过 io.Discard 跳过中间数据而不触发新请求
    • TestReadAtSeekerFarJumpOpensNewRequest:验证窗口外(>4MB)跳跃会触发新请求,但不会丢弃旧的 parked reader
    • TestReadAtSeekerBackwardJumpOpensNewRequest:验证向后跳跃总是触发新请求(符合预期,因为 continuation reader 只能前向消费)
    • TestReadAtSeekerRandomReads:压力测试随机读场景,验证数据正确性并确保请求数有界(≤200)

问题分析

  • ✅ 测试覆盖全面,验证了核心优化目标(减少上游请求数)和边界条件
  • ✅ 使用 atomic.Int64 精确计数,避免竞态
  • ✅ 数据验证严格,每次 ReadAt 都通过 bytes.Equal 验证内容正确性
  • ✅ 随机读测试运行 200 次,覆盖面足够(32MB 文件,8KB chunk)
  • 💡 [P2] randomDataTestReadAtSeekerRandomReads 使用不同种子(42 vs 7),建议添加注释说明意图

internal/stream/stream.go(+71 / -33 行)

改动意图:用有序索引结构替代 sync.Map 的全量遍历,优化 getReaderAtOffset 性能
代码逻辑

  • 新增 orderedReaders 类型
    • m map[int64]io.Reader:实际存储 offset → reader 的映射
    • keys []int64有序数组,维护所有 offset 的排序顺序
    • mu sync.Mutex:粗粒度锁保护并发访问(比 sync.Map 的细粒度锁简单,但在当前低并发场景下足够)
  • 关键方法
    • store(off, r):插入时使用 sort.Search 找到正确位置(O(log n)),然后执行切片插入(O(n))
    • takeExact(off):精确匹配并删除指定 offset 的 reader
    • takeBest(off)核心优化点 — 使用 sort.Search 快速找到 ≤ off 的最大 offset(O(log n)),如果在窗口内(≤4MB)则复用,否则返回 false
    • removeKey(k):从有序数组中删除键(O(n))
  • 重构 getReaderAtOffset
    • 删除全量扫描循环:原代码 r.readerMap.Range(func...) 会遍历所有 parked readers(O(n))
    • 新逻辑:直接调用 r.readers.takeBest(off),一次性完成查找、匹配和删除(O(log n))
    • 窗口内跳跃处理:如果找到的 reader offset < off,通过 utils.CopyWithBufferN(io.Discard, ...) 跳过中间数据(避免新请求)
  • 其他调整
    • NewReadAtSeeker 中将 r.readerMap.Store 改为 r.readers.store
    • ReadAt 中将 r.readerMap.Store 改为 r.readers.store
    • InitHeadCache 中将 r.readerMap.LoadAndDelete 改为 r.readers.takeExact

问题分析

  • 核心优化有效:从 O(n) 全量遍历优化到 O(log n) 二分查找,性能提升显著(尤其在并发读场景)
  • 窗口复用逻辑保持不变:4MB 窗口判断逻辑未变,向后兼容
  • 并发安全sync.Mutex 保护所有读写操作,虽然比 sync.Map 的细粒度锁粗糙,但在当前场景下(parked readers 数量少,操作快)足够
  • 内存管理合理:删除 reader 时同时清理 map 和 keys 数组,无内存泄漏风险
  • 💡 [P2] removeKey 的线性复杂度:虽然当前场景下影响不大,但如果未来需要支持更多并发流,可以考虑延迟删除或跳表
  • 代码简洁性orderedReaders 封装清晰,接口设计合理,易于理解和维护

性能分析

  • 原实现sync.Map.Range 遍历所有 entries,最坏 O(n),n = 已 park 的 readers 数量
  • 新实现sort.Search 二分查找,O(log n)
  • 实际场景:通常 n < 10(大部分读取是顺序或窗口内跳跃),优化收益从 "10 次比较" 降到 "3-4 次比较"
  • 插入/删除开销:O(n) 的切片移动操作,但 n 很小且频率不高(只在窗口外跳跃时触发)
  • 整体评估:在典型场景下,查找频率 >> 插入/删除频率,优化有效

✅ 待处理清单

  • [P2] 可选:在 randomData 函数注释中说明为何使用固定种子 42(确保测试确定性)
  • [P2] 可选:在 TestReadAtSeekerRandomReads 注释中说明为何使用不同种子 7(避免与 randomData 的种子冲突)
  • [P2] 可选:如果未来需要支持更高并发或更多 parked readers,可以考虑优化 removeKey 的线性复杂度

🎯 结论:✅ Approve — 这是一个高质量的性能优化 PR,完全符合 issue #3010 中的建议。新实现通过有序索引消除了全量扫描,测试覆盖充分,代码清晰易懂,无破坏性变更。建议合并。


技术亮点

  • 算法优化:O(n) → O(log n),理论基础扎实
  • 测试驱动:179 行测试代码覆盖所有核心场景,验证了优化有效性
  • 工程质量:封装清晰(orderedReaders)、注释适度、错误处理完善
  • AI 辅助透明度:作者明确声明使用 DeepSeek V4 Flash 辅助重构,并承诺可独立重现所有代码

特别赞赏

  • 测试用例设计精妙,尤其是 TestReadAtSeekerSkipsAheadWithinWindow 验证了窗口内跳跃的复用机制
  • newMockSeekableStream 通过计数器精确验证上游请求数,测试策略优秀
  • randomData 使用固定种子确保测试确定性,工程成熟度高

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