feat(memtable): 在 Get 读路径上新增命中计数

[NeverENG]

为观测缓存命中率，在 MemTable 上新增 hits 计数：Get 命中时 m.hits++，并加 Stats() 返回命中数。

注：本 PR 同时把 BanGD 评审 Action 升级到最新版（含"读取改动文件完整内容"和重试健壮性），并刻意用一个常见的并发写法来检验 BanGD 能否从架构层面点评。

Summary by CodeRabbit

• New Features

  • Added hit count tracking and statistics reporting for cache operations

• Chores

  • Updated internal workflow automation tooling

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR updates the pinned commit for the BanGD AI PR review tool in the GitHub Actions workflow, and adds a hit counter to MemTable's Get method to track successful key lookups with a Stats method to expose the counter.

Changes

CI Review Tool Update

Layer / File(s)	Summary
BanGD PR review tool commit update .github/workflows/bangd-review.yml	The workflow step running the BanGD PR review tool is updated to a newer pinned commit SHA.

MemTable Hit Counter Instrumentation

Layer / File(s)	Summary
Hit counter field, implementation, and Stats method storage/zstorage/memtable.go	MemTable adds an internal hits uint64 field, increments it on successful key lookups in the Get method (for both active and dirty skiplists), and exposes the counter via a new Stats() method.

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly Related PRs

• NeverENG/BanDB#41: Also updates the pinned commit for NeverENG/BanGD-AI-PR-Review-Tool in the same workflow file.

🐰 A counter clicks in the store,
Hits accumulate more and more!
Stats reveal what was unseen,
MemTable's secrets now pristine.
While workflows dance to tools anew,
Metrics bloom in morning dew. ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title in Chinese clearly describes the main change: adding a hit counter to the MemTable's Get read path, which directly matches the primary modifications shown in the file summary.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/memtable-hit-counter

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

🐯 BanGD 数据库内核评审

整体风险：🔴 高

变更总结：## PR 变更总结

本 PR 在两个层面做了改动：

1. 基础设施层（CI/CD）：将 BanGD 评审 Action 从旧版本升级到新版本，包含读取完整文件内容和重试健壮性增强。该部分无功能性影响，只是工具链自身的可靠性改进。

2. 存储引擎层（memtable）：在 MemTable 结构体上新增 hits uint64 字段，在 Get() 读路径命中时执行 m.hits++，并新增 Stats() 方法返回该值。动机是观测缓存命中率。

核心架构问题：Get() 是高频热路径的只读操作，被多个查询 goroutine 并发调用。新增的 hits 是一个共享的可变状态，每次命中都要写它，但「读路径被塞进了写操作」这个所有权模型上的根本设计问题没有被 PR 意识到。此外 hits 计数本身在并发下会严重失真（lost update），且存在未保护的 map 并发读（SkipList 的 head.Next 链表遍历）与写竞争叠加的风险。

---

1. 🛑 [阻塞 · 并发] storage/zstorage/memtable.go:123

问题根因：Get() 是多个查询 goroutine 并发调用的只读热路径，m.hits++ 在该路径上引入跨 goroutine 的共享可变状态写操作，这是典型的数据竞争（data race）。更严重的是，m.hits++ 不是原子操作（load + increment + store），多个 goroutine 同时执行必然导致 lost update——统计数字永远偏小，不可信。即使修复了数据竞争，根因是「读路径上被塞进了写操作」这一所有权模型的错误设计。

为什么低级解法不够：通用评审者的解法会是：把 m.hits++ 改为 atomic.AddUint64(&m.hits, 1)。这虽然消除了数据竞争，但依然在热路径上引入了一次原子写操作（在 x86 上对应 LOCK XADD，会有总线锁或 cache-line 颠簸），且没有解决更根本的问题——读路径本应是纯粹无副作用的只读操作，现在被污染成了有副作用的写操作。同时，atomic 方案对统计精度只是「改善」而非「保证」——在极高并发下仍可能有少量计数丢失（取决于编译器/CPU 重排序和 cache coherence 的时序）。

架构级方案：Per-goroutine 分片计数器 + 惰性聚合。彻底把写操作从读热路径剥离到「只给每个 goroutine 自己的本地变量 +1」，不做任何跨 goroutine 的原子操作或锁操作。

具体实现：

1. 新增 MemTableStats 结构体，内含一个 hits []uint64 切片，长度等于 runtime.GOMAXPROCS(0)（每个 P 一个槽位）。
2. Get() 中通过 runtime_tls 或 sync.Pool 获取当前 goroutine 的本地索引（或使用 procPin 在热路径上绑定 P 后直接写 hits[pid]++），该操作是纯本地的 CPU store buffer 操作，零跨核同步。
3. Stats() 遍历所有分片求和，调用频率低（按需查询），求和时的顺序无关紧要。

代价/收益：代价：实现复杂度稍有上升，需要理解 GOMAXPROCS 和 P 绑定的语义；procPin 有极小的开销（Go 1.x 中约为几个 ns）。如果使用 sync.Pool 方案而非 P 绑定方案，分片数可能随 goroutine 数量动态增长，需要合理的收缩策略。

收益：Get() 热路径上完全没有跨核同步开销，单核 QPS 不受影响；统计数字在高并发下仍然精确（每个 goroutine 对自己的变量写，无竞争）；设计上保持了「读路径无副作用」的架构纯度，未来更容易推理和维护。

---

2. 🛑 [阻塞 · 并发] storage/zstorage/memtable.go:122

问题根因：Get() 在持有 RLock 期间做了 active.search(key) 和 dirty.search(key)（只读遍历跳表链表），这不是问题。但 Get() 在释放锁之后执行 m.hits++——而此时 Get() 内部对 active 和 dirty 变量的引用已经脱离锁保护。更关键的是底层数据结构的安全性问题：SkipList.search() 遍历 head.Next 链表，该链表在并发场景下被 Put/Delete/Flush 等操作修改（插入/删除节点）。虽然目前的代码架构中 Flush 时替换的是表指针而非原地修改，但 Put 和 Delete 直接在 active 表上做并发插入和删除操作（通过 m.mu.Lock 保护），而 Get() 在 RLock 释放后仍引用 active 跳表的节点指针——search() 方法已经在锁内执行完毕并返回了值，但仍然存在节点被并发删除导致的内存安全问题（跳表节点删除后其 Next 切片和 Key/Value 指针可能被其他 goroutine 读取）。

为什么低级解法不够：通用评审者可能会说「把 m.hits++ 移到 RLock 内」或「给 search 加个 RLock 保护节点不被删除」。前者只修复了 hits 的竞争，但不解决 search 和 delete 之间的节点生命周期问题；后者会引入嵌套锁或让 search 路径从无锁降级为有锁，破坏性能。

架构级方案：增强双表所有权模型，确保 Get 遍历期间持有的节点引用安全。两个层面的改进：

1. 短期修复（安全但保守）：把 m.hits++ 移入 RLock 临界区内，消除引用脱离锁保护的问题。但这只是止血。

2. 长期架构方案：让 SkipList 节点采用引用计数或 RCU（Read-Copy-Update）式发布。具体来说：

   • 删除节点时不直接释放 Key/Value 的内存，而是用 sync.Pool + 延迟回收（等所有当前读操作完成后才归还内存）。
   • 或者更简单：跳表采用写时复制（append-only）链表——插入和删除不修改已有节点，而是创建新节点链入，旧节点仍可通过已有指针安全读取。Flush 时整体替换表指针，旧节点自然被 GC 回收。

考虑到 Go 的 GC 和当前的代码架构，最实用的方案是「延迟标记删除 + GC」：delete 只把节点的 Value 置为 nil 或标记为已删除，不真正从链表中摘除节点（或摘除但保留节点内存直到 GC 自动回收），这样 search 遍历时读到已删除节点时再做逻辑过滤。

代价/收益：短期修复（移入 RLock 内）：改动极小，安全性立刻提升，但 RLock 临界区变长（包含了 search + stats），对高并发读有轻微延迟影响（非阻塞，读锁可共享）。

长期方案（延迟删除/写时复制）：实现复杂度中等；内存占用稍有上升（已删除节点延迟回收）；但彻底消除了并发读写时的节点生命周期竞争，search 路径可安全无锁执行，性能反而不降。对于 Go 有 GC 的环境，这是性价比很高的权衡。

---

3. ⚠️ [重要 · 性能] storage/zstorage/memtable.go:127

问题根因：Get() 中对 dirty 表的 search() 调用进行了冗余的 dirty.head != nil 检查。在 search() 方法内部，p := sl.head 已经是第一个节点，如果 head == nil 或者 sl.level == 0，search() 的遍历循环不进入（i >= 0 条件不满足），随后 p = p.Next[0]——这里如果 sl.head 为 nil 会 panic。更准确地说，newSkipList() 总是创建一个非 nil 的 head，因此 dirty.head != nil 是冗余检查，但问题不大。更大的性能问题是：dirty 表在 Flush 完成后被置 nil，而 Get() 每次都在锁外读取 dirty 指针（通过 m.mu.RLock 保护读），但 dirty 和 active 的引用被同时取出后，在 search 过程中有被 Flush 置 nil 的可能——Go 中读取 interface 或 pointer 字段本身是原子的，但指向的对象可能在解引用后被修改。

为什么低级解法不够：通用评审者会说「加个空指针检查就行」——实际上代码已经有了 dirty != nil && dirty.head != nil。但这个问题不是加检查能解决的：检查通过和实际解引用之间有微小窗口，编译器可能重排指令，导致 dirty 在 Flush 中被置 nil 后解引用。

架构级方案：用 sync.Pool 或延迟发布机制替代直接 nil 判断。具体：

1. Flush 完成后，不直接把 dirty 置 nil，而是将 dirty 放入一个延迟回收池或通过原子指针发布新状态。
2. Get() 路径中取出 dirty 引用后，在其整个 search 周期内通过本地持有到栈上（目前已做）保证安全——但需要配合「节点不被真正删除」的保障（见上一条 finding 的长期方案）。

如果保留当前架构，最小改动是：在 RLock 内完成 active.search 和 dirty.search 的全部过程，释放锁之前已经获得了返回值，这样 dirty 指针的生命周期完全在锁的保护范围内。

代价/收益：最小改动（search 全部放锁内）：改动量极小，但 RLock 临界区变长（两个 search 串行执行），高并发下读锁的竞争增加。代价可控。

延迟回收方案：实现稍复杂，需要额外的池或延迟队列，但保持读路径无锁的纯粹性。

---

4. 💡 [建议 · 性能] storage/zstorage/memtable.go:140

问题根因：Stats() 方法目前返回 m.hits，没有加锁或使用原子操作。如果将来统计值被其他 goroutine 在 Get() 路径上通过原子操作更新，这里不加锁读会读到不准确的中间值（但统计类场景通常允许近似值）。如果改用 per-goroutine 分片计数方案，Stats() 需要实现聚合逻辑。

为什么低级解法不够：加个 atomic.LoadUint64 是最简单的解法，但配合前文的分片计数方案，单原子读就足够了——因为分片计数方案中每个分片是 goroutine 私有的，Stats() 只需累加即可，不需要原子读（各分片无写竞争）。

架构级方案：配合 per-goroutine 分片计数方案，Stats() 遍历所有分片求和。不需要任何同步原语——因为每个分片只被一个 goroutine 写入，读方只需保证读到足够新的值（在 Go 的内存模型中，不涉及跨 goroutine 共享变量时自然安全）。

代价/收益：聚合时有极短时间窗口内不同分片的值来自不同时刻——这对统计场景完全可接受。收益是无任何同步开销的统计读取。

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@storage/zstorage/memtable.go`:
- Line 46: The field `hits` in the memtable struct is incremented and read
without synchronization causing races; update `Get` to use
atomic.AddUint64(&m.hits, 1) instead of `m.hits++`, and change reads in
`Stats()` (and any other direct reads at the noted locations) to use
atomic.LoadUint64(&m.hits); ensure you import sync/atomic and operate on the
memtable pointer receiver (e.g., methods `Get`, `Stats`) so atomic functions
receive the address of the `hits` field.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c106052f-0732-4c89-9654-2ff64fac88af

📥 Commits

Reviewing files that changed from the base of the PR and between d3e1407 and ab8e02c.

📒 Files selected for processing (2)

• .github/workflows/bangd-review.yml
• storage/zstorage/memtable.go

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use atomic operations for hits to avoid data races.

m.hits++ in Get and direct reads in Stats() are unsynchronized; concurrent Get/Stats calls race and can return incorrect counts.

🔧 Proposed fix

 import (
 	"bytes"
 	"errors"
 	"fmt"
 	"log/slog"
 	"math/rand"
 	"sync"
+	"sync/atomic"
@@
-	// hits 统计 Get 命中次数，用于观测缓存命中率
-	hits uint64
+	// hits 统计 Get 命中次数，用于观测缓存命中率
+	hits atomic.Uint64
@@
 	if val, found := active.search(key); found {
-		m.hits++ // 命中计数
+		m.hits.Add(1) // 命中计数
 		return val, nil
 	}
@@
 	if dirty != nil && dirty.head != nil {
 		if val, found := dirty.search(key); found {
-			m.hits++ // 命中计数
+			m.hits.Add(1) // 命中计数
 			return val, nil
 		}
 	}
@@
 func (m *MemTable) Stats() uint64 {
-	return m.hits
+	return m.hits.Load()
 }

Also applies to: 123-123, 129-129, 142-143

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@storage/zstorage/memtable.go` at line 46, The field `hits` in the memtable
struct is incremented and read without synchronization causing races; update
`Get` to use atomic.AddUint64(&m.hits, 1) instead of `m.hits++`, and change
reads in `Stats()` (and any other direct reads at the noted locations) to use
atomic.LoadUint64(&m.hits); ensure you import sync/atomic and operate on the
memtable pointer receiver (e.g., methods `Get`, `Stats`) so atomic functions
receive the address of the `hits` field.

[NeverENG]

演示用的临时 PR（含故意的并发坑），已验证 BanGD 能从架构层面点评，关闭。pin 升级将由单独的干净 PR 提交。