Replace copier.Copy with optimized manual deep copy in URL.Clone (#2678)
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.Copywith manual implementation inURL.Clone(). - Removed
github.com/jinzhu/copierimport fromurl.goand dependency fromgo.modviago mod tidy. - Added
TestSubURLCopyto verify deep-copy behavior. - Updated
BenchmarkCloneto 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
@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.
have changed the target branch from main to develop.
@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
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.
Have you pull the newest upsteam/develop branch? CI error still exists.
@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 ?
Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code