Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Nexusrex18
Copy link
Contributor

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

common/url.go Outdated
Methods: make([]string, len(c.Methods)),
}
copy(newURL.Methods, c.Methods)
newURL.params = make(url.Values, len(c.params))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put this line of code in the newURL initialization above, more concise

common/url.go Outdated
Path: c.Path,
Username: c.Username,
Password: c.Password,
Methods: make([]string, len(c.Methods)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to -> Methods: make([]string, len(c.Methods)) , and remove line843 copy, more concise

Copy link
Contributor

@FoghostCn FoghostCn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Nexusrex18
Copy link
Contributor Author

Nexusrex18 commented Mar 15, 2025

@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.

@AlexStocks AlexStocks changed the base branch from main to develop March 15, 2025 12:54
@AlexStocks
Copy link
Contributor

have changed the target branch from main to develop.

@No-SilverBullet
Copy link
Member

No-SilverBullet commented Mar 17, 2025

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

@Nexusrex18
Copy link
Contributor Author

Nexusrex18 commented Mar 18, 2025

@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.

@No-SilverBullet
Copy link
Member

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

@Nexusrex18
Copy link
Contributor Author

@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 ?

Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Url.Clone
4 participants