Merged
Conversation
Contributor
Author
Owner
|
LGTM。看起来蛮solid的。 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Summary
先说说这个问题是怎么被注意到的:
一开始我想先找一个边界清楚、收益能量化、而且 review 成本不会太高的点做掉。
看
EventStorage的时候,很快就发现:事件列表分页、角色筛选、宗门筛选、长短期记忆相关查询,基本都要走这里。只要这里有性能瓶颈,那全局都会受影响。然后顺着代码往下看,问题就出来了。
get_events()这种方法会先查出一页events,这一步本身没什么问题。问题在后面。每条事件在组装成Event对象的时候,还会各自再去查一次:event_avatarsevent_sects也就是说,旧链路实际上是“先查一页主表,再按页内每条事件回两次关联表”。这就是很典型的 N+1,而且还是双倍的。
拿
limit=100来说,旧逻辑理论上就是:event_avatars100 次event_sects100 次合起来刚好
201次 SQL。看到这里时,我基本就觉得这个点值得单独提一个 PR 了。
排查过程本身倒没有什么玄学,就是顺着调用链老老实实往下看。
先看
get_events、get_major_events_by_avatar这些入口,确认它们都是“先拿一批 row,再逐条组装事件对象”的模式。然后再看_row_to_event(),确认它在组装单条事件时会额外查询event_avatars和event_sects。最后再用 SQLite trace callback 把 SQL 条数数出来,看和代码推导是不是对得上。旧逻辑的核心长这样:
而
_row_to_event()里面会做这两次回表:页内有多少条事件,这两类查询就跟着放大多少次。
而且受影响的不只是
get_events()。一起会踩到这套模式的还有:get_major_events_by_avatarget_minor_events_by_avatarget_major_events_betweenget_minor_events_between所以这就是
EventStorage内部一个重复出现的查询模式问题,而不是某一个接口的小问题。改动本身反而挺朴素的。
这次没有改 API,没有改 schema,也没有借机去拆大文件。我只做了一件事:把“逐条组装时逐条回表”改成“这一页先把关联数据一次拉出来,再统一组装”。
现在的流程是:
event_idevent_avatarsevent_sectsevent_id分组后统一组装Event内部新加了 3 个 helper:
_load_avatar_map_for_events_load_sect_map_for_events_build_events_from_rows然后把下面这些读取路径接到了新的批量组装逻辑上:
get_eventsget_major_events_by_avatarget_minor_events_by_avatarget_major_events_betweenget_minor_events_between另外
_row_to_event()的 fallback 语义我保留了。也就是说,如果别的地方还是单独拿一条 row 来组装事件,它仍然按原来的方式工作。这一点我觉得很重要,因为这能把这次改动的影响面压在EventStorage内部,不会莫名其妙把别的调用方一起拖下水。benchmark 这块,我先拿真实库做确认。
如果连真实存档上都看不出这个问题,那这个 PR 的优先级其实就没那么高。
这次用的是我自己在服务器上跑了半个小时产出的数据库:
save_20260404_1347_events.db它当前的数据量不算夸张,但足够说明问题:
events = 320event_avatars = 257event_sects = 105在这份真实库上,我对比了两种模式:
_row_to_event()装载结果如下:
all_events limit=100avatar_filter limit=50sect_filter limit=50major_events_by_avatar limit=10这一步已经足够说明问题是真实存在的。
然后我开始伪造数据库看看能不能更深入证明问题的影响规模。
主要是想回答两个更实际的问题:
这里没有用那种“一个热点 key 命中整库”的极端模型,因为那个模型太容易把主查询本身的扫描成本也一起放大,最后很难看清到底是谁在拖后腿。
后面把数据做成了更接近真实筛选负载的分布:
1k / 10k / 100kavatar_hotsect=1伪造数据的关键生成逻辑大概是这样:
benchmark 的跑法很直接:同一份数据库上,分别跑当前实现和旧实现模拟,然后统计 SQL 数和平均耗时。
结果如下。
1k:all_events limit=100avatar_filter limit=50sect_filter limit=50major_events_by_avatar limit=1010k:all_events limit=100avatar_filter limit=50sect_filter limit=50major_events_by_avatar limit=10100k:all_events limit=100avatar_filter limit=50sect_filter limit=50major_events_by_avatar limit=10看完这些数字之后,可以得出判断了。
get_events这条主链路的收益非常稳。这个几乎不用多解释了,不管是真实库还是1k / 10k / 100k,SQL 数都稳定从线性增长压到了常数级3,而且耗时也明显更好。这部分就是这次 PR 最核心的价值。avatar_filter和sect_filter也都是真实受益的。在中小规模下提升很直接;到了100k,墙钟时间优势开始收窄,但 SQL 数的下降依然很明显。这说明这次去掉的 N+1 是真问题,不是统计噪音。major_events_by_avatar这条路径稍微有点不同。它的 N+1 同样被去掉了,SQL 数也确实从21降到了3,但在100k规模下,耗时已经不再明显占优。我不觉得这是坏消息,反而说明这条路径下一步该优化的点已经变了。现在更像是event_observations这条主查询本身在吃掉主要成本,而不是页内事件组装还在拖后腿。换句话说,这次优化把第一层已经确认的热点处理掉之后,下一层真正的瓶颈也跟着露出来了。
测试这边,除了 benchmark,我还补了针对查询效率的回归测试,主要就是防止后面有人不小心又把它改回逐条回表。
本次执行结果:
pytest tests/test_event_storage.py tests/test_api_events.py74 passed至此我决定停下来了,至于为什么:
如果只看
100k的 benchmark,当然还能继续往下挖,尤其是event_observations的筛选、排序和索引命中,后面大概率还有空间。但我不想把这件事继续塞进同一个 PR 里,原因也很简单。第一,正常项目使用场景里,短期内其实不太容易碰到
100k级别的事件量。第二,这次 PR 的目标已经很明确了,就是先把已经确认存在的 N+1 去掉,而且真实库和伪造库都已经把收益说明白了。
第三,如果继续把下一层 observation 查询优化也叠进来,这个 PR 就会开始同时回答两类问题,review 会变重,结论也会变得没那么干净。
所以这次先收住:
如果后面真的出现这类负载需求,完全可以再提一个独立 PR,专门看
event_observations这条查询链路。看看大家后面愿不愿意继续讨论这个问题了。好的,已经很晚了,就写到这里。如果后续有什么问题也可以继续讨论。这个项目真的很好玩,希望大家也能长久坚持下去吧~
Test Plan
pytest tests/test_event_storage.py tests/test_api_events.pysave_20260404_1347_events.db上对比当前实现与旧逐条装载模式1k / 10k / 100k的选择性命中伪造数据库,统计 SQL 数和平均耗时