-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
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