-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix performance regression. Do not escape request path in echo.ServeH… #1799
Conversation
…TTP. Make sure that path is not double escaped in rewrite/proxy middleware.
Codecov Report
@@ Coverage Diff @@
## master #1799 +/- ##
==========================================
+ Coverage 89.56% 89.67% +0.10%
==========================================
Files 32 32
Lines 2646 2674 +28
==========================================
+ Hits 2370 2398 +28
Misses 178 178
Partials 98 98
Continue to review full report at Codecov.
|
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.
fix #1777 performance regression. Do not escape request path in echo.ServeHTTP. Make sure that path is not double escaped in rewrite/proxy middleware.
did 3 benchmarks
Results:
before versus after performance problem was commited
x@x:~/code/echo$ benchstat before_problem.out before_fix.out name old time/op new time/op delta EchoStaticRoutes-6 19.9µs ± 2% 28.2µs ± 1% +41.84% (p=0.000 n=8+9) EchoStaticRoutesMisses-6 22.6µs ±13% 28.4µs ± 1% +25.61% (p=0.000 n=10+8) EchoGitHubAPI-6 32.7µs ± 2% 56.7µs ± 1% +73.19% (p=0.000 n=8+8) EchoGitHubAPIMisses-6 33.1µs ± 1% 56.7µs ± 1% +71.52% (p=0.000 n=8+9) EchoParseAPI-6 2.52µs ± 5% 3.89µs ± 2% +54.44% (p=0.000 n=10+10)
commit on master before problem was fixed versus after fix branch
x@x:~/code/echo$ benchstat before_fix.out after_fix.out name old time/op new time/op delta EchoStaticRoutes-6 28.2µs ± 1% 17.7µs ± 9% -37.14% (p=0.000 n=9+10) EchoStaticRoutesMisses-6 28.4µs ± 1% 17.3µs ± 2% -39.04% (p=0.000 n=8+9) EchoGitHubAPI-6 56.7µs ± 1% 32.2µs ± 2% -43.23% (p=0.000 n=8+10) EchoGitHubAPIMisses-6 56.7µs ± 1% 32.8µs ± 2% -42.22% (p=0.000 n=9+8) EchoParseAPI-6 3.89µs ± 2% 2.14µs ± 3% -44.99% (p=0.000 n=10+9)
before problem was commits versus after fix
NB: these numbers include also other changes we have done to router in master
x@x:~/code/echo$ benchstat before_problem.out after_fix.out name old time/op new time/op delta EchoStaticRoutes-6 19.9µs ± 2% 17.7µs ± 9% -10.83% (p=0.000 n=8+10) EchoStaticRoutesMisses-6 22.6µs ±13% 17.3µs ± 2% -23.43% (p=0.000 n=10+9) EchoGitHubAPI-6 32.7µs ± 2% 32.2µs ± 2% -1.67% (p=0.008 n=8+10) EchoGitHubAPIMisses-6 33.1µs ± 1% 32.8µs ± 2% -0.90% (p=0.021 n=8+8) EchoParseAPI-6 2.52µs ± 5% 2.14µs ± 3% -15.04% (p=0.000 n=10+9)
I added similar benchmark tests as router has but this time we use full echo stack
e.ServeHTTP
NB: I think
Rewrite
andProxy
has somewhat fundamental flaw by usingmap
for rewrite rules. Map does not have guaranteed iteration order so overlapping rules are not executed in expected order