Skip to content
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

Improve sliceValidateError.Error performance #2765

Merged
merged 1 commit into from Jun 28, 2021

Conversation

zihengCat
Copy link
Contributor

@zihengCat zihengCat commented Jun 25, 2021

Improve sliceValidateError.Error performance using switch and strings.Builder.

  • Avoiding []string memory allocation.
  • Using strings.Builder instead of strings.Join, one loop cycle is enough (strings.Join runs a loop internally).
  • Add more test cases.

@codecov
Copy link

codecov bot commented Jun 25, 2021

Codecov Report

Merging #2765 (4cb798f) into master (1d0f938) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 4cb798f differs from pull request most recent head 77a8f5e. Consider uploading reports for the commit 77a8f5e to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2765   +/-   ##
=======================================
  Coverage   98.70%   98.70%           
=======================================
  Files          41       41           
  Lines        2077     2085    +8     
=======================================
+ Hits         2050     2058    +8     
  Misses         15       15           
  Partials       12       12           
Impacted Files Coverage Δ
binding/default_validator.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d0f938...77a8f5e. Read the comment docs.

@zihengCat zihengCat force-pushed the improve-errmsgs branch 5 times, most recently from d0b97e2 to df0d20e Compare June 25, 2021 16:43
daheige
daheige previously approved these changes Jun 26, 2021
Copy link
Contributor

@daheige daheige left a comment

Choose a reason for hiding this comment

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

LGTM

@zihengCat
Copy link
Contributor Author

@appleboy Hi, take a review :)

@appleboy
Copy link
Member

@zihengCat Can you also add a benchmark report?

@appleboy appleboy added this to the v1.8 milestone Jun 28, 2021
@zihengCat
Copy link
Contributor Author

@appleboy

Benchmark Report

Benchmark tests code placed in default_validator_benchmark_test.go.

Command: go test -benchtime=5s -benchmem -bench=BenchmarkSliceValidateError

Before

goos: linux
goarch: amd64
pkg: github.com/gin-gonic/gin/binding
cpu: Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
BenchmarkSliceValidateError-2         175496         35167 ns/op        8976 B/op        309 allocs/op
PASS
ok      github.com/gin-gonic/gin/binding    6.538s

New

goos: linux
goarch: amd64
pkg: github.com/gin-gonic/gin/binding
cpu: Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
BenchmarkSliceValidateError-2         225392         27070 ns/op        5272 B/op        209 allocs/op
PASS
ok      github.com/gin-gonic/gin/binding    6.396s

….Builder

fix missing nil pointer check

use simpler switch case

add missing tests

use for-loop instead of range

add benchmark test codes
@zihengCat zihengCat changed the title Improve sliceValidateError.Error performance Improve sliceValidateError.Error performance Jun 28, 2021
@appleboy appleboy requested a review from thinkerou June 28, 2021 13:29
Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

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

lgtm

@thinkerou thinkerou merged commit e3ee01d into gin-gonic:master Jun 28, 2021
@zihengCat zihengCat deleted the improve-errmsgs branch June 28, 2021 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants