feat: add two new tags implementation based on slice #2325
Conversation
Change-Id: Id960f6a177b0dc545b86c3725ddcbb58a15ccd8a
Change-Id: Ibb1477aafe7d62204b6cddadad0e7b970b47e584
Change-Id: I19f9c09c6a79084322c8a71edc44447dd721aada
shunjiazhu
changed the title
Change-Id: I294ed88fc4e8debdfa4f2a83548ec0547d252f68
Change-Id: Ica62373452f6aa5e82e115607dee419b4ca03329
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new slice-based implementation for tag storage as an alternative to the existing map-based approach. The implementation provides both sorted and unsorted slice variants that can be used interchangeably with the existing map implementation. This change aims to improve performance for scenarios with fewer than 100 elements, where slice operations can be faster than map operations due to Go's hashmap iteration disadvantages.
- Added slice-based tag implementations with sorted and unsorted variants
- Introduced new APIs like
LoadOrStore,Range, andRangeMutfor more complex use cases - Enhanced the existing
KeyValuesinterface with additional methods for better performance and functionality
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/models/slice.go | Core implementation of sorted and unsorted slice-based key-value storage |
| pkg/models/slice_test.go | Comprehensive test suite and benchmarks for the new slice implementations |
| pkg/models/common.go | Enhanced KeyValues interface with new methods and updated existing map implementation |
| pkg/models/factory.go | Updated factory functions to use new map wrapper with Size() and Clone() methods |
| pkg/models/metrics.go | Optimized size calculation using new Size() method instead of iterating |
| pkg/models/common_test.go | Additional tests and benchmarks for the new functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Change-Id: If88d47a59b052fd50975d0025d586d8d5b6fdcf2
shunjiazhu
force-pushed
the
feat/sorted-tags
branch
from
30ef010 to
c180149
Compare
| if values, ok := kv.values(); ok { | ||
| if v, ok := values[key]; ok { | ||
| return v, true | ||
| } | ||
| } | ||
| kv.Add(key, value) |
There was a problem hiding this comment.
这里逻辑好像有点问题。当values返回了false的时候，kv.Add里面values还是false，新的key value不会被add
There was a problem hiding this comment.
原先的Add逻辑应该也是为了保护tags未被初始化的场景，如果kv本身是nil，应该还是要走noop操作的。
但是实际上因为内部的map基本不会出现nil的场景。当kv被初始化后，kv.values()总是返回true
| } | ||
|
|
||
| func (kv *keyValuesMapImpl[TValue]) Reset() { | ||
| if kv.IsNil() { |
| func (s *strKeyValuesMapImpl) Size() int { | ||
| if s == nil || s.IsNil() { | ||
| return 0 | ||
| } | ||
| size := 0 | ||
| for k, v := range s.keyValues { | ||
| size += len(k) + len(v) | ||
| } | ||
| return size | ||
| } |
There was a problem hiding this comment.
每次调用都要重新计算，size是不是可以缓存成一个字段？
There was a problem hiding this comment.
- 加了cachedSize之后，会让之前的Add等方法都引入一次先判断旧元素是否存在+计算diff的开销，我们测试了多个插件有劣化。
- 而且之前代码中有直接操作底层map的逻辑，可能会绕过AIPI。
innerMap := tags.Iterator()
innerMap[newKey] = newValue。
Change-Id: Icfd15d2ecf3d0db911917ffc0c5e2585f31abc66
2 participants
Change-Id: Id960f6a177b0dc545b86c3725ddcbb58a15ccd8a
由于 go 的 hashmap 在遍历上有较大劣势，考虑新增基于 slice 的切片实现，插件开发者可自行在 NewTags 时选择，两种 Tags 可以互相混用。同时考虑到Tags更多复杂的场景，增加几个API，例如LoadOrStore，Range。
推荐用户使用新 API 来加速，并且不绑定特定实现，减少使用 Iterator 接口。
通过一些快路径加速，即使退化到基于二分查找， slice 在少于 100个元素的情况下，不会比 map慢。
替换效果：
在我们实际的运行环境，可降低 20%-40% cpu。同时在 processor较多时，可以提高 20% 以上吞吐。