router: Imrpove router matching by caching sanitized_path in stream_info

[bryanwux]

Remove repeative work on sanitizing request headers when routing by caching sanitized_path in stream_info.

Commit Message: Additional Description: Risk Level: Testing: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] #21615 [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional API Considerations:]

[htuch]

/assign-from @envoyproxy/first-pass-reviewers

[repokitteh-read-only[bot]]

@envoyproxy/first-pass-reviewers assignee is @daixiang0

:cat:

Caused by: a https://github.com/envoyproxy/envoy/pull/23064#issuecomment-1246748129 was created by @htuch.

see: more, trace.

[zhxie]

I guess the patch only caches sanitized path (the path without path-parameters), but the performance concern mentioned in the issue is about removeQueryAndFragment?

[bryanwux]

I guess the patch only caches sanitized path (the path without path-parameters), but the performance concern mentioned in the issue is about removeQueryAndFragment?

True. I will cache the result of removeQueryAndFragment beforehand.

[bryanwux]

/retest

[repokitteh-read-only[bot]]

Retrying Azure Pipelines: Check envoy-presubmit didn't fail.

:cat:

Caused by: a https://github.com/envoyproxy/envoy/pull/23064#issuecomment-1252380031 was created by @bryanwux.

see: more, trace.

[yanavlasov]

I think the problem with this change is that if a HTTP filter changes the :path header the router will not see this change and will still route based on the original :path.

Was there a performance issue found with the finding the start of query in the existing code? I understand that theoretically it can be expensive if the :path is long. If this needs to be addressed the only viable way is to add helper methods to the RequestHeaderMap to store string_view of the :path without query that can be updated any time :path header is changed.

[yanavlasov]

/wait-any

[bryanwux]

Was there a performance issue found with the finding the start of query in the existing code?

Yes, caching the result of removeQueryAndFragment will end up with a 5x performance improvement by benchmark test. The problem is there is per-route repeative work here: https://github.com/envoyproxy/envoy/blob/f6f56150b119e58f671401fae4cf31c44f93bec0/source/common/common/matchers.cc#L133-L135

If this needs to be addressed the only viable way is to add helper methods to the RequestHeaderMap to store string_view of the :path without query that can be updated any time :path header is changed.

Like adding a new HEADER_FUNC to store the path without query in header_map.h?

[yanavlasov]

Are you talking about 5x improvement for a microbenchmark, or a full Envoy system benchmark? I'd expect this change to make almost no difference when measuring request throughput of a complete system.

I do not think this can be done with the HEADER_FUNC. This will need to be custom code that overrides the setPath method and adds additional method to access query.

[yanavlasov]

/wait-any

[github-actions[bot]]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[github-actions[bot]]

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!