dubbo-go icon indicating copy to clipboard operation
dubbo-go copied to clipboard

Replace copier.Copy with optimized manual deep copy in URL.Clone (#2678)

Open Nexusrex18 opened this issue 11 months ago • 7 comments

Fixes by replacing copier.Copy with a manual deep copy in URL.Clone(), optimized with pre-allocated maps for params and attributes. This reduces memory and CPU overhead, especially for large data volumes. Key changes:

  • Replaced copier.Copy with manual implementation in URL.Clone().
  • Removed github.com/jinzhu/copier import from url.go and dependency from go.mod via go mod tidy.
  • Added TestSubURLCopy to verify deep-copy behavior.
  • Updated BenchmarkClone to test the new implementation.

Benchmarks:

  • 100 params/attributes:
    • Old (copier): 51 µs/op, 23.74 KB/op, 222 allocs/op
    • New (manual): 26 µs/op, 14.12 KB/op, 112 allocs/op
  • 1,000 params/attributes:
    • Old (copier): 428 µs/op, 228 KB/op, 1,142 allocs/op
    • New (manual): 270 µs/op, 197 KB/op, 1,010 allocs/op

The new implementation is faster (~1.6x at 1,000 entries, ~2x at 100), uses less memory (~14% at 1,000, ~40% at 100), and reduces allocations (~12% at 1,000, ~50% at 100).

Note: Unrelated test failures in rpc_service_test.go (e.g., unexported testService) were observed but pre-exist this change and are outside this PR’s scope.

Fixes: #2678

Tested with:

  • go test -v -run TestSubURLCopy: Passed
  • go test -bench=BenchmarkClone -benchmem: Confirmed performance

Nexusrex18 avatar Mar 13 '25 21:03 Nexusrex18

@No-SilverBullet @FoghostCn I updated URL.Clone() in common/url.go to use defer for lock releases and added nil checks for params and attributes as per the suggestions. Additionally, I enhanced TestSubURLCopy to test Protocol and params for thorough deep-copy validation and fixed TestCompareURLEqualFunc with SetCompareURLEqualFunc(defaultCompareURLEqual) resets to address state pollution.

Nexusrex18 avatar Mar 15 '25 10:03 Nexusrex18

have changed the target branch from main to develop.

AlexStocks avatar Mar 15 '25 12:03 AlexStocks

@No-SilverBullet @FoghostCn I updated URL.Clone() in common/url.go to use defer for lock releases and added nil checks for params and attributes as per the suggestions. Additionally, I enhanced TestSubURLCopy to test Protocol and params for thorough deep-copy validation and fixed TestCompareURLEqualFunc with SetCompareURLEqualFunc(defaultCompareURLEqual) resets to address state pollution.

Sry for the late response, LGTM, but we need to check the CI error. And can you update the new benchmark? The performance may be reduced after the lock adjustment (the lock holding time becomes longer), but for the sake of code readability, I want to know whether this performance loss is acceptable. Besides, since you added the nil check for params and attributes, It would be better to move the initialization inside the nil check to avoid unnecessary initialization. like this

if c.params != nil { newURL.params = make(url.Values, len(c.params)) // initialize here ...//copy }

No-SilverBullet avatar Mar 17 '25 15:03 No-SilverBullet

@No-SilverBullet Rebased issue-2678 onto upstream/develop for CI sync. Benchmarks: 303 µs/op (avg of 5 runs), ~1.41x faster than copier.Copy. All common/ tests pass locally (Windows & Ubuntu WSL, including -race), but Ubuntu CI fails: make: *** [Makefile:59: test] Error 1. This was fixed in #2795 by develop, but persists here.

Nexusrex18 avatar Mar 18 '25 17:03 Nexusrex18

Have you pull the newest upsteam/develop branch? CI error still exists.

No-SilverBullet avatar Mar 21 '25 10:03 No-SilverBullet

@No-SilverBullet even after fetching the latest changes , getting the same issue so should i try any other approach or recreate a new pull request with same changes ?

Nexusrex18 avatar Mar 21 '25 13:03 Nexusrex18