Skip to content

Conversation

@styd
Copy link

@styd styd commented Jul 2, 2019

Hi, I'm thinking of removing some checking repetitions.
Although it's slightly less readable, it should perform faster as there are less comparisons for some cases.
For example, you don't have to repeatedly check if p[r] is '.' when the next byte is not the last.

@styd styd force-pushed the cleanpath-switch-case branch from 830f3c6 to ec700b0 Compare July 2, 2019 23:40
@codecov
Copy link

codecov bot commented Jul 3, 2019

Codecov Report

Merging #1968 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1968      +/-   ##
==========================================
+ Coverage   98.83%   98.83%   +<.01%     
==========================================
  Files          40       40              
  Lines        2226     2234       +8     
==========================================
+ Hits         2200     2208       +8     
  Misses         14       14              
  Partials       12       12
Impacted Files Coverage Δ
path.go 100% <100%> (ø) ⬆️

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 b8b2fad...d8cd8b6. Read the comment docs.

@styd styd force-pushed the cleanpath-switch-case branch from ec700b0 to b073cd8 Compare July 3, 2019 02:26
@appleboy
Copy link
Member

appleboy commented Sep 3, 2019

@styd Could you provide the benchmark before and after? Thanks.

@appleboy appleboy added this to the 1.5 milestone Sep 3, 2019
@styd styd force-pushed the cleanpath-switch-case branch from 116192e to 4f721a4 Compare September 4, 2019 11:53
@styd
Copy link
Author

styd commented Sep 4, 2019

I added benchmark but the result is not as I expected it would be.
The performance of my refactor was about the same or even worse once in a while.

BenchmarkCleanPath/OldCleanPath-8                1000000              2064 ns/op
BenchmarkCleanPath/NewCleanPath-8                1122319              2084 ns/op

Do you happen to know why? Did I do the benchmark wrong?

@styd
Copy link
Author

styd commented Sep 6, 2019

After I added some test data (for when a two dots sequence is not removed), the refactored cleanPath always runs slightly faster in my computer:

$ go test -bench BenchmarkCleanPath -benchtime=5s
[GIN] 2019/09/06 - 22:05:53 | 200 |       3.323µs |       192.0.2.1 | GET      /name1/api
[GIN] 2019/09/06 - 22:05:53 | 200 |       1.199µs |       192.0.2.1 | GET      /name2/api
[GIN] 2019/09/06 - 22:05:53 | 200 |       4.283µs |       192.0.2.1 | GET      /test/copy/race
goos: linux
goarch: amd64
pkg: github.com/gin-gonic/gin
BenchmarkCleanPath/OldCleanPath-8                2164497              2762 ns/op
BenchmarkCleanPath/NewCleanPath-8                2218494              2746 ns/op
PASS
ok      github.com/gin-gonic/gin        17.854s

@thinkerou thinkerou modified the milestones: 1.5, 1.6 Oct 31, 2019
@thinkerou thinkerou modified the milestones: 1.6, 1.7 Feb 28, 2020
@thinkerou thinkerou modified the milestones: 1.7, v1.8 Oct 27, 2020
@thinkerou thinkerou modified the milestones: v1.8, v1.9 Nov 21, 2021
@thinkerou thinkerou modified the milestones: v1.9, v1.10 Jan 17, 2023
@appleboy
Copy link
Member

Please fix conflicts and move to 1.11

@appleboy appleboy modified the milestones: v1.10, v1.11 Mar 21, 2024
@appleboy appleboy modified the milestones: v1.11, v1.12 Jun 21, 2025
@appleboy
Copy link
Member

appleboy commented Oct 5, 2025

Please fix the conflicts.

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.

3 participants