AlexStocks commented on issue #2031: URL: https://github.com/apache/dubbo-go/issues/2031#issuecomment-4275035871
> > [@AlexStocks](https://github.com/AlexStocks) https://claude.ai/share/baf1073b-4359-481a-9426-a1eb491f8761 > > claude 结论很明确： > > **不要用 sync.Pool，也不要做 MergeURL(serviceURL, referenceURL) 的全局缓存。** 最终认为这两个方向都只是表面优化，或者会引入正确性问题。 > > 最后收敛到的方案是： > > **把 MergeURL 的执行时机从“每次 RPC 调用”前移到“Invoker 创建时”只做一次。** 也就是在 InvokerDelegate 初始化时预先算好 mergedURL，后续 GetURL() 直接返回这个已经合并好的结果，不再每次调用都 Clone + Merge。 > > 对话里的理由是： > > providerURL 和 consumerURL 在 invoker 生命周期内基本不变。 现在的问题本质上是 GetURL() 每次 RPC 都触发 common.MergeURL()，而 MergeURL() 内部 Clone() 每次都会分配新对象，造成大量短命对象和 GC 压力。 如果在 invoker 创建时算一次，后面直接复用，就能把热路径上的分配降到 0 allocs/op。 这个方案不需要额外的缓存失效逻辑，因为服务变更时本来就会创建新的 invoker，生命周期天然保证数据新鲜。 对话里明确否掉了两个备选方案： > > sync.Pool：只能减轻分配压力，不能真正解决 retention；而且还要求调用方正确 ReleaseURL()，改动面大，容易出错。 基于 serviceURL.String() + referenceURL.String() 的缓存：必须在其他路径补 InvalidateMergeURLCache()，否则会返回过期 merged URL；而且 URL.String() 自己也有分配成本。 改进具体实施路径： **这个分支其实已经把 MergeURL 从 RPC 热路径挪出去了。** 也就是说，Claude 对话里最后收敛出的“大方向”在当前代码里已经基本成立，不是完全没做。 证据有两处： * 批量通知路径里，refreshAllInvokers 已经明确写了“Originally it will Merge URL many times, now we just execute once”，然后把 merged URL 回写到 event 里，再拿这个 event 去建 invoker。 见 registry/directory/directory.go (line 313) 到 registry/directory/directory.go (line 318) * invoker 自身的 GetURL() 只是原子读取已有指针，没有再做 merge。 见 protocol/base/base_invoker.go (line 84) 所以如果你问“还能怎么改进”，我觉得重点不是再做一版 sync.Pool 或全局 cache，而是把当前实现继续收口，减少重复 merge 和隐藏副作用。 **我建议的改进点** * 1 把单事件路径和批量路径统一成同一个“预合并”入口。 当前批量路径在 registry/directory/directory.go (line 315) 到 registry/directory/directory.go (line 318) 先 merge 再 event.Update()；但单事件路径还是走 refreshInvokers -> cacheInvokerByEvent -> cacheInvoker，在 registry/directory/directory.go (line 610) 重新 url.MergeURL(referenceUrl)。这虽然不在 RPC 热路径上，但逻辑分叉了，后面容易再回归出重复 merge。 * 2 不要让 invokerCacheKey() 带副作用。 现在这个函数名字看起来只是“算 key”，但它会在 registry/directory/directory.go (line 381) 到 registry/directory/directory.go (line 382) 里做 MergeURL + event.Update()。这会让调用方很难判断 event 什么时候被改写了。更干净的做法是抽一个 prepareMergedEvent(event) / mergedServiceURL(event) helper，所有路径都先显式调用它，再单独取 key。 * 3 cacheInvoker() 应该优先复用 event.Updated() 的结果。 现在 ServiceEvent 已经有 Updated() / Update() 机制，见 registry/event.go (line 56) 到 registry/event.go (line 66)。但 cacheInvoker() 没利用这个状态，还是无条件 merge。这里可以加一个 fast path：如果 event 已经 updated，就直接用 event.Service，不要再 merge 一次。 *4 MergeURL() 本身先 Clone() 再 ReplaceParams()，分配仍然不轻，但优先级已经下降。 当前 MergeURL() 还是以 Clone() 开头，见 common/url.go (line 841) 到 common/url.go (line 889)，而 Clone() 又走 CloneWithFilter()，见 common/url.go (line 944)。如果后面 benchmark 证明“注册中心事件风暴”场景下这里仍然有明显压力，再考虑做更细的 copy 优化；但它已经不是最值得优先动的点了。 **更合适的落地方向**： 我会建议做一个很小的重构，不改语义，只统一入口： 新增一个 prepareMergedEvent(event) helper refreshAllInvokers、invokerCacheKey、cacheInvokerByEvent 全部走它 cacheInvoker() 不再自己直接 MergeURL 补两个测试： 单事件 Notify() 路径只 merge 一次 批量 NotifyAll() 路径不会在后续 key/cache 阶段再次 merge 这样收益很实际： 现有修复思路保持不变 单事件和批量事件行为一致 后续不会再有人在“取 key”或“缓存 invoker”时不小心把 merge 放回去 代码可读性会比现在好很多 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
