perf: optimize IsEquals and ToMap to reduce allocations by JunLa-0 · Pull Request #3402 · apache/dubbo-go

JunLa-0 · 2026-06-12T03:16:31Z

Optimize URL equality comparison and map conversion to avoid unnecessary allocations. IsEquals() now compares fields directly instead of materializing two full maps via ToMap(). ToMap() pre-sizes the map and uses a single-pass split for Location parsing.

Fixes: #3394

Signed-off-by: 13927699266@163.com

Optimize URL equality comparison and map conversion to avoid unnecessary allocations. IsEquals() now compares fields directly instead of materializing two full maps via ToMap(). ToMap() pre-sizes the map and uses a single-pass split for Location parsing. Fixes: apache#3248 Signed-off-by: 温俊朗 <wenjunlang@wenjunlangdeMacBook-Air.local>

sonarqubecloud · 2026-06-12T03:18:58Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

codecov-commenter · 2026-06-12T03:24:06Z

Codecov Report

❌ Patch coverage is 77.41935% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.42%. Comparing base (e6e14fd) to head (6a5d4c4).

Files with missing lines	Patch %	Lines
common/url.go	77.41%	8 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3402      +/-   ##
==========================================
+ Coverage   52.40%   52.42%   +0.01%     
==========================================
  Files         492      492              
  Lines       37785    37826      +41     
==========================================
+ Hits        19800    19829      +29     
- Misses      16380    16387       +7     
- Partials     1605     1610       +5

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Alanxtl · 2026-06-12T03:29:11Z

the target branch should be develop

Alanxtl · 2026-06-12T03:30:14Z

@ywxzm03 help review this :)

Alanxtl · 2026-06-12T03:32:42Z

update to latest develop and run https://github.com/apache/dubbo-go/blob/develop/common/url_benchmark_test.go to see the performance shift before and after you commit, and paste the result comparison here

可以参考 #3403 的pr body写法

Alanxtl · 2026-06-12T07:09:50Z

+	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 {
+		}
+	}


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。

Alanxtl · 2026-06-12T07:11:35Z

-	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)


paramCount 单独加锁读一次，后面 RangeParams 又加锁遍历一次，多了一次锁操作。

这不是一致性快照：两次锁之间 params 可能变，容量估算可能不准。

fixedCount 只是容量估算，不影响逻辑；估多了也只是多分一点内存。

如果 params 里本来就有 protocol、host、port 这些 key，后面的固定字段会覆盖它们，fixedCount 仍然按新增 key 算，容量也会估大。

性能收益主要来自预分配，但可读性代价有点高。

建议就是保留容量估算，去掉无意义初始化，并确保 protocol/location 行为不变。

Alanxtl · 2026-06-12T07:12:16Z

 	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"
 		}


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简介里面说明

Alanxtl · 2026-06-12T07:13:56Z

+// --- 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()
+	}
+}


不用自己写benchmark 直接复用已有的benchmark 或者在benchmark文件里面添加你需要的benchmark

Alanxtl · 2026-06-12T07:18:58Z

 // IsEquals compares if two URLs equals with each other. Excludes are all parameter keys which should ignored.
 func IsEquals(left *URL, right *URL, excludes ...string) bool {
 	if (left == nil && right != nil) || (right == nil && left != nil) {
 		return false
 	}
 	if left.Ip != right.Ip || left.Port != right.Port {
 		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
 	}



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.

ywxzm03 · 2026-06-12T10:43:17Z

+
+	// Compare params directly without materializing maps.
+	// Build left params (excluding filtered keys), then verify right matches.
+	leftParams := make(map[string]string)


这里可以做个容量预估，len(left.params) 就有了

Copilot

Pull request overview

This PR targets allocation reductions in the common.URL utilities by optimizing equality checks and map conversion, aligning with issue #3394’s request to avoid materializing maps during IsEquals() and to tighten ToMap().

Changes:

Reworks IsEquals() to compare URL scalar fields + params directly, avoiding ToMap() allocations during comparisons.
Optimizes ToMap() by pre-sizing the result map and simplifying Location parsing.
Adds benchmarks for IsEquals() and ToMap() across small/large param sets and with excludes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
common/url.go	Optimizes `ToMap()` allocations and refactors `IsEquals()` to avoid map materialization.
common/url_test.go	Adds benchmarks for `IsEquals()` and `ToMap()` to validate allocation improvements.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+	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
+	}


+		// 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]


+	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


AlexStocks · 2026-06-15T01:48:10Z

+	if left.Path != right.Path && !isExcluded("path") {
+		return false
+	}
+	if left.Location != right.Location {


[P1] 这里把 host / port 的排除逻辑挂在整条 Location 上了：只传 host 或只传 port 时，left.Location != right.Location 也会直接落入“放行”分支，另一半地址差异被一并忽略。旧实现是按 ToMap() 删除单个 key 比较，两个字段的排除语义是独立的。请分别比较 host/port，或保留原来的按键删除语义。

JunLa-0 closed this Jun 12, 2026

JunLa-0 reopened this Jun 12, 2026

Alanxtl added ✏️ Feature 3.3.2 version 3.3.2 labels Jun 12, 2026

Alanxtl reviewed Jun 12, 2026

View reviewed changes

ywxzm03 reviewed Jun 12, 2026

View reviewed changes

Alanxtl self-assigned this Jun 13, 2026

AlexStocks requested a review from Copilot June 13, 2026 07:32

Copilot started reviewing on behalf of AlexStocks June 13, 2026 07:32 View session

Copilot AI reviewed Jun 13, 2026

View reviewed changes

Alanxtl changed the base branch from main to develop June 14, 2026 06:34

AlexStocks reviewed Jun 15, 2026

View reviewed changes

Conversation

JunLa-0 commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud Bot commented Jun 12, 2026

Quality Gate passed

Uh oh!

codecov-commenter commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Alanxtl commented Jun 12, 2026

Uh oh!

Alanxtl commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alanxtl commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alanxtl Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

Alanxtl Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

Alanxtl Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Alanxtl Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Alanxtl Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

ywxzm03 Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

AlexStocks Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JunLa-0 commented Jun 12, 2026 •

edited

Loading

codecov-commenter commented Jun 12, 2026 •

edited

Loading

Alanxtl commented Jun 12, 2026 •

edited

Loading

Alanxtl commented Jun 12, 2026 •

edited

Loading

Alanxtl Jun 12, 2026 •

edited

Loading

Alanxtl Jun 12, 2026 •

edited

Loading