Alanxtl commented on code in PR #3402:
URL: https://github.com/apache/dubbo-go/pull/3402#discussion_r3401417659
##########
common/url_test.go:
##########
@@ -1507,3 +1507,82 @@ func TestCloneThreadSafe(t *testing.T) {
wg.Wait()
}
+
+// --- Benchmarks for IsEquals and ToMap ---
+
+func BenchmarkIsEquals_SmallParams(b *testing.B) {
+ u1, _ :=
NewURL("dubbo://127.0.0.1:20000/com.test.Service?key1=value1&key2=value2")
+ u2, _ :=
NewURL("dubbo://127.0.0.1:20000/com.test.Service?key1=value1&key2=value2")
+ b.ResetTimer()
+ for i := 0; i < b.N; i++ {
+ IsEquals(u1, u2)
+ }
+}
+
+func BenchmarkIsEquals_LargeParams(b *testing.B) {
+ base1 := "dubbo://127.0.0.1:20000/com.test.Service?"
+ base2 := "dubbo://127.0.0.1:20000/com.test.Service?"
+ for i := 0; i < 50; i++ {
+ kv := "key" + strconv.Itoa(i) + "=val" + strconv.Itoa(i)
+ if i > 0 {
+ base1 += "&"
+ base2 += "&"
+ }
+ base1 += kv
+ base2 += kv
+ }
+ u1, _ := NewURL(base1)
+ u2, _ := NewURL(base2)
+ b.ResetTimer()
+ for i := 0; i < b.N; i++ {
+ IsEquals(u1, u2)
+ }
+}
+
+func BenchmarkIsEquals_WithExcludes(b *testing.B) {
+ u1, _ :=
NewURL("dubbo://127.0.0.1:20000/com.test.Service?key1=value1&key2=different&key3=value3")
+ u2, _ :=
NewURL("dubbo://127.0.0.1:20000/com.test.Service?key1=value1&key2=other&key3=value3")
+ b.ResetTimer()
+ for i := 0; i < b.N; i++ {
+ IsEquals(u1, u2, "key2")
+ }
+}
+
+func BenchmarkToMap_SmallParams(b *testing.B) {
+ u, _ :=
NewURL("dubbo://127.0.0.1:20000/com.test.Service?key1=value1&key2=value2")
+ u.Username = "user"
+ u.Password = "pass"
+ b.ResetTimer()
+ for i := 0; i < b.N; i++ {
+ _ = u.ToMap()
+ }
+}
+
+func BenchmarkToMap_LargeParams(b *testing.B) {
+ base := "dubbo://127.0.0.1:20000/com.test.Service?"
+ for i := 0; i < 50; i++ {
+ if i > 0 {
+ base += "&"
+ }
+ base += "key" + strconv.Itoa(i) + "=val" + strconv.Itoa(i)
+ }
+ u, _ := NewURL(base)
+ u.Username = "user"
+ u.Password = "pass"
+ b.ResetTimer()
+ for i := 0; i < b.N; i++ {
+ _ = u.ToMap()
+ }
+}
+
+func BenchmarkToMap_EmptyParams(b *testing.B) {
+ u := &URL{
+ Protocol: "dubbo",
+ Location: "127.0.0.1:20000",
+ Path: "/com.test.Service",
+ }
+ b.ResetTimer()
+ for i := 0; i < b.N; i++ {
+ _ = u.ToMap()
+ }
+}
Review Comment:
不用自己写benchmark 直接复用已有的benchmark
##########
common/url.go:
##########
@@ -1019,23 +1038,76 @@ func IsEquals(left *URL, right *URL, excludes
...string) bool {
return false
}
- leftMap := left.ToMap()
- rightMap := right.ToMap()
- for _, exclude := range excludes {
- delete(leftMap, exclude)
- delete(rightMap, exclude)
+ // Build a small lookup set for excluded keys to avoid repeated linear
scans
+ // and to avoid materializing two full maps just for comparison.
+ var excludeSet map[string]struct{}
+ if len(excludes) > 0 {
+ excludeSet = make(map[string]struct{}, len(excludes))
+ for _, k := range excludes {
+ excludeSet[k] = struct{}{}
+ }
}
- if len(leftMap) != len(rightMap) {
- return false
+ isExcluded := func(key string) bool {
+ if excludeSet == nil {
+ return false
+ }
+ _, ok := excludeSet[key]
+ return ok
}
Review Comment:
This rewrite seems more complicated than necessary, and it also changes the
original comparison semantics.
The old implementation compares the result of ToMap(), so excluding only
"port" should still compare "host", and excluding only "host" should still
compare "port". In the new logic, when left.Location != right.Location and only
one of host/port is excluded, the code falls through to param comparison, but
it never compares the remaining non-excluded part of Location.
For example, these should not be equal when only "port" is excluded:
left.Location = "10.0.0.1:20880"
right.Location = "10.0.0.2:20880"
IsEquals(left, right, "port")
Besides that, the code still materializes leftParams, so the optimization is
not as simple as "avoid materializing maps"; it only avoids building two full
ToMap() results. I suggest either keeping the original map-based comparison for
correctness, or splitting host/port explicitly and comparing only the
non-excluded fields.
##########
common/url.go:
##########
@@ -1019,23 +1038,76 @@ func IsEquals(left *URL, right *URL, excludes
...string) bool {
return false
}
- leftMap := left.ToMap()
- rightMap := right.ToMap()
- for _, exclude := range excludes {
- delete(leftMap, exclude)
- delete(rightMap, exclude)
+ // Build a small lookup set for excluded keys to avoid repeated linear
scans
+ // and to avoid materializing two full maps just for comparison.
+ var excludeSet map[string]struct{}
+ if len(excludes) > 0 {
+ excludeSet = make(map[string]struct{}, len(excludes))
+ for _, k := range excludes {
+ excludeSet[k] = struct{}{}
+ }
}
- if len(leftMap) != len(rightMap) {
- return false
+ isExcluded := func(key string) bool {
+ if excludeSet == nil {
+ return false
+ }
+ _, ok := excludeSet[key]
+ return ok
}
- for lk, lv := range leftMap {
- if rv, ok := rightMap[lk]; !ok {
+ // Compare scalar fields directly.
+ if left.Protocol != right.Protocol && !isExcluded(PROTOCOL) {
+ return false
+ }
+ if left.Username != right.Username && !isExcluded("username") {
+ return false
+ }
+ if left.Password != right.Password && !isExcluded("password") {
+ return false
+ }
+ if left.Path != right.Path && !isExcluded("path") {
+ return false
+ }
+ if left.Location != right.Location {
+ if isExcluded("host") && isExcluded("port") {
+ // Both excluded, skip Location comparison
+ } else if isExcluded("host") || isExcluded("port") {
+ // Only one side of host:port excluded — fall through
to param comparison
+ } else {
return false
- } else if lv != rv {
+ }
+ }
Review Comment:
`IsEquals` 在只 exclude `host` 或 `port` 时会漏比较另一半 location
PR 里当 `left.Location != right.Location` 且只排除 `host` 或 `port`
其中一个时，代码直接“fall through”，但后面只比较 params，不再比较未被排除的 host/port。
你试一下`Location: 10.0.0.1:20880` vs `10.0.0.2:20880`，exclude `port` 后 PR 返回
`true`，base 返回 `false`。
##########
common/url.go:
##########
@@ -839,24 +864,18 @@ func (c *URL) ToMap() map[string]string {
paramsMap["password"] = c.Password
}
if c.Location != "" {
- paramsMap["host"] = strings.Split(c.Location, ":")[0]
- var port string
- if strings.Contains(c.Location, ":") {
- port = strings.Split(c.Location, ":")[1]
+ // Single-pass split avoids repeated string scanning.
+ parts := strings.Split(c.Location, ":")
+ paramsMap["host"] = parts[0]
+ if len(parts) > 1 {
+ paramsMap["port"] = parts[len(parts)-1]
} else {
- port = "0"
+ paramsMap["port"] = "0"
}
Review Comment:
`ToMap` 的多地址 port 解析语义被改了
原逻辑对 `127.0.0.1:2181,127.0.0.1:2182,127.0.0.1:2183` 的 `port` 是
`2181,127.0.0.1` 确实不合理，
PR 改成取最后一个 `:` 后的 `2183`。也不合理
这里改一下，然后在pr简介里面说明
##########
common/url.go:
##########
@@ -820,7 +820,32 @@ func (c *URL) SetParams(m url.Values) {
// ToMap transfer URL to Map
func (c *URL) ToMap() map[string]string {
- paramsMap := make(map[string]string)
+ c.paramsLock.RLock()
+ paramCount := len(c.params)
+ c.paramsLock.RUnlock()
+
+ fixedCount := 0
+ if c.Protocol != "" {
+ fixedCount++
+ }
+ if c.Username != "" {
+ fixedCount++
+ }
+ if c.Password != "" {
+ fixedCount++
+ }
+ if c.Location != "" {
+ fixedCount += 2 // host + port
+ }
+ if c.Path != "" {
+ fixedCount++
+ }
+
+ if paramCount == 0 && fixedCount == 0 {
+ return nil
+ }
+
+ paramsMap := make(map[string]string, paramCount+fixedCount)
Review Comment:
- `paramCount` 单独加锁读一次，后面 `RangeParams` 又加锁遍历一次，多了一次锁操作。
- 这不是一致性快照：两次锁之间 params 可能变，容量估算可能不准。
- `fixedCount` 只是容量估算，不影响逻辑；估多了也只是多分一点内存。
- 如果 params 里本来就有 `protocol`、`host`、`port` 这些 key，后面的固定字段会覆盖它们，`fixedCount`
仍然按新增 key 算，容量也会估大。
- 性能收益主要来自预分配，但可读性代价有点高。
建议就是保留容量估算，去掉无意义初始化，并确保 `protocol/location` 行为不变。
--
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]
