Copilot commented on code in PR #3402:
URL: https://github.com/apache/dubbo-go/pull/3402#discussion_r3407611122
##########
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
+ }
Review Comment:
`ToMap()` releases `paramsLock` before the early-return check, so a
concurrent `SetParam/SetParams` can add params between the unlock and the
`return nil`, causing `ToMap()` to incorrectly return nil even though params
exist. Keep the lock held until the emptiness decision is made (fixedCount can
be computed outside the lock).
##########
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
Review Comment:
When only one of `host` or `port` is excluded and `left.Location !=
right.Location`, the current code intentionally "falls through" but never
compares the non-excluded component. That can incorrectly treat URLs as equal
even though the non-excluded host/port differs.
##########
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]
Review Comment:
`ToMap()` currently uses `strings.Split(c.Location, ":")` and then takes the
last element as port. This both allocates and changes behavior vs the previous
implementation for locations containing multiple ':' (e.g. IPv6 or multiple
endpoints). If the goal is to preserve existing semantics while reducing
allocations, parse host/port without allocating and keep port as the segment
between the first and second ':' (or "0" when missing).
--
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]
