Performance regression with echo 4.2.0

[stffabi]

Issue Description

We have just updated to echo 4.2.0 and have seen a relatively high performance regression with that release. With bisecting
it seems like commit 2d79ff3 has introduced a perfomance penalty of about 30-40%.

The following benchstat has been generated with https://github.com/vishr/web-framework-benchmark comparing commit
cf2fcad to 2d79ff3.

name             old time/op    new time/op    delta
EchoStatic-8       29.5µs ± 2%    38.1µs ± 2%  +29.26%  (p=0.000 n=9+8)
EchoGitHubAPI-8    46.2µs ± 1%    62.8µs ± 1%  +35.93%  (p=0.000 n=8+9)
EchoGplusAPI-8     2.29µs ± 4%    3.23µs ± 1%  +41.25%  (p=0.000 n=9+9)
EchoParseAPI-8     3.99µs ± 1%    5.67µs ± 1%  +42.00%  (p=0.000 n=9+9)

Steps to reproduce

Run https://github.com/vishr/web-framework-benchmark benchmark with commit 2d79ff3 and cf2fcad

Version/commit

4.2.0 first commit with regression 2d79ff3

[lammel]

Just ran the benchmarks of tags v4.1.17 and v4.2.0 against each other and the impace is different here.
Seems like this is the reason why it was not detected (not sure if we had already setup benchstat at the time of merging though).

Result of runnung 10 times and compare using benchstat bench-v4.1.17.txt bench-v4.2.0.txt

name                    old time/op    new time/op    delta
RouterStaticRoutes-16     18.2µs ± 1%    23.3µs ± 4%   +28.10%  (p=0.000 n=8+10)
RouterGitHubAPI-16        42.0µs ± 5%    45.7µs ± 3%    +8.87%  (p=0.000 n=10+8)
RouterParseAPI-16         5.94µs ±10%    2.64µs ±11%   -55.64%  (p=0.000 n=10+10)
RouterGooglePlusAPI-16    11.0µs ± 4%     1.7µs ± 4%   -84.63%  (p=0.000 n=10+10)

name                    old alloc/op   new alloc/op   delta
RouterStaticRoutes-16      0.00B          0.00B           ~     (all equal)
RouterGitHubAPI-16         2.00B ± 0%     0.00B       -100.00%  (p=0.000 n=10+10)
RouterParseAPI-16          0.00B          0.00B           ~     (all equal)
RouterGooglePlusAPI-16     0.00B          0.00B           ~     (all equal)

name                    old allocs/op  new allocs/op  delta
RouterStaticRoutes-16       0.00           0.00           ~     (all equal)
RouterGitHubAPI-16          0.00           0.00           ~     (all equal)
RouterParseAPI-16           0.00           0.00           ~     (all equal)
RouterGooglePlusAPI-16      0.00           0.00           ~     (all equal)

When I do the same using web-framework-benchmark I get similar results as you showed before.

[lammel]

Seems like I was the one doing a perf review of PR #1628 and also wrote that I did not see a performance regression..

[stffabi]

AFAIK the benchmark tests in the echo repo only test the performance of the router itself by directly calling the find method on it, whereas the web-framework-benchmark benchmarks the whole echo stack.

Seems like the regression has been introduced between the other parts and the find call on the router and not in the router itself.

[lammel]

PR #1628 introduced r.URL.EscapedPath() which sanitizes the request URL on every request. The focus here was correctness for proxying.

So this PR needs to be revisited if this is required for every request or could be limited to proxying.
We can probably find a way to only escape if required (although internally EscapedPath() does that by scanning through the url and deciding if escaping is needed).

[stffabi]

@lammel Isn't PR #1628 even a breaking change for route finding?

Assuming a user has registered the following route:

e.Any("/test whitespace/", myHandler)

If the user enters the URL localhost:1323/test%20whitespace/ in his browser, with echo 4.1.17 this route is found because .Path == "/test whitespace/". Whereas after having upgraded to 4.2.0 the route will be searched with /test%20whitespace/and the route is not found anymore.

[lammel]

You are probably right, es the paths added to the router would also need to use EscapedPath() when adding the path if we keep doing it that way. (adding routes is not relevant for performance)

I still think we should limit the implications of path escaping to appropriate places (e.g. proxy as the other PR originally stated).
Like restoring the previously used GetPath() helper function and identify the spots to use EscapedPath for proxying.
All headers where URLs are added (like Location for redirect) may also be affected.