fix performance regression. Do not escape request path in echo.ServeH…

[aldas]

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

# before problem was introduced go test -run="-" -bench="BenchmarkEcho.*" -count 10 > before_problem.out # on current master go test -run="-" -bench="BenchmarkEcho.*" -count 10 > before_fix.out # on fix branch go test -run="-" -bench="BenchmarkEcho.*" -count 10 > after_fix.out 

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

func benchmarkEchoRoutes(b *testing.B, routes []*Route) { e := New() req := httptest.NewRequest("GET", "/", nil) u := req.URL w := httptest.NewRecorder() b.ReportAllocs() // Add routes for _, route := range routes { e.Add(route.Method, route.Path, func(c Context) error { return nil	})	} // Find routes b.ResetTimer() for i := 0; i < b.N; i++ { for _, route := range routes { req.Method = route.Method u.Path = route.Path e.ServeHTTP(w, req)	}	} } func BenchmarkEchoStaticRoutes(b *testing.B) { benchmarkEchoRoutes(b, staticRoutes) } func BenchmarkEchoStaticRoutesMisses(b *testing.B) { benchmarkEchoRoutes(b, staticRoutes) } func BenchmarkEchoGitHubAPI(b *testing.B) { benchmarkEchoRoutes(b, gitHubAPI) } func BenchmarkEchoGitHubAPIMisses(b *testing.B) { benchmarkEchoRoutes(b, gitHubAPI) } func BenchmarkEchoParseAPI(b *testing.B) { benchmarkEchoRoutes(b, parseAPI) }

NB: I think Rewrite and Proxy has somewhat fundamental flaw by using map for rewrite rules. Map does not have guaranteed iteration order so overlapping rules are not executed in expected order

[codecov]

Codecov Report

Merging #1799 (16e13ca) into master (6f9b71c) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@ 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 

Impacted Files	Coverage Δ
echo.go	91.64% <100.00%> (+0.10%)	⬆️
middleware/middleware.go	100.00% <100.00%> (ø)
middleware/proxy_1_11.go	100.00% <0.00%> (ø)
router.go	96.04% <0.00%> (+0.16%)	⬆️

---

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 6f9b71c...16e13ca. Read the comment docs.

[lammel]

Added a test case to verify behaviour for #1798.
Approved, great work @aldas