-
Notifications
You must be signed in to change notification settings - Fork 953
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
base: develop
Are you sure you want to change the base?
Conversation
common/url.go
Outdated
Methods: make([]string, len(c.Methods)), | ||
} | ||
copy(newURL.Methods, c.Methods) | ||
newURL.params = make(url.Values, len(c.params)) |
There was a problem hiding this comment.
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)), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@No-SilverBullet @FoghostCn |
have changed the target branch from main to develop. |
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
|
@No-SilverBullet |
Have you pull the newest upsteam/develop branch? CI error still exists. |
Signed-off-by: Nexusrex18 <[email protected]>
@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 ? |
|
Fixes by replacing
copier.Copy
with a manual deep copy inURL.Clone()
, optimized with pre-allocated maps forparams
andattributes
. This reduces memory and CPU overhead, especially for large data volumes.Key changes:
copier.Copy
with manual implementation inURL.Clone()
.g.yxqyang.asia/jinzhu/copier
import fromurl.go
and dependency fromgo.mod
viago mod tidy
.TestSubURLCopy
to verify deep-copy behavior.BenchmarkClone
to test the new implementation.Benchmarks:
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., unexportedtestService
) were observed but pre-exist this change and are outside this PR’s scope.Fixes: #2678
Tested with:
go test -v -run TestSubURLCopy
: Passedgo test -bench=BenchmarkClone -benchmem
: Confirmed performance