perf(router): adjust the sequence of predicate generated inside a traditional compatible route expression

[windmgc]

Summary

This PR adjusts the sequence of HTTP fields and their predicates generated inside a traditional compatible route expression, expecting to improve routing matching performance in general situations.

Before this PR, the sequence is src_ip -> src_port -> dst_ip -> dst_port -> methods -> hosts -> paths -> headers
After this PR it becomes src_ip -> src_port -> dst_ip -> dst_port -> paths -> headers -> hosts -> methods.

The reason for this change is that the expression is evaluated from left to right in the atc-router, so a field that is most unlikely to be matched should be put into the very left of the expression as a short circuit, otherwise, when we execute the evaluation on a set of very large routes, there will be lots of unnecessary predicates executed and CPU cycles wasted. Note: this PR does not bring performance improvements for all cases, but for general cases in which the path value has more variants than the limited set of http.method.

Checklist

• The Pull Request has tests
• A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
• There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

This was discovered as part of the issue in FTI-6456.

[chronolaw]

I saw that only the order changed, no other code is touched, am I correct?

[chronolaw]

could we add a comment of sequence before get_expression(), like:

-- match priority: src_ip -> src_port -> dst_ip -> dst_port -> paths -> headers -> hosts -> methods.

[windmgc]

@chronolaw

I saw that only the order changed, no other code is touched, am I correct?

Yes that's correct

could we add a comment of sequence before get_expression(), like:

Comment added.

[chronolaw]

Suggested change

	-- match priority for http is as follows:
	-- match order for http is as follows:

[chronolaw]

Suggested change

	-- match priority for stream is as follows:
	-- match order for stream is as follows:

[chronolaw]

Turn to draft since we are still in discussion.

[ProBrian]

when we execute the evaluation on a set of very large routes, there will be lots of unnecessary predicates executed and CPU cycles wasted.

I agreed with this idea. However since the path matching may involves regex matching, which maybe more performance sensitive than the method string equal comparison, we may count on the benchmarking to decide if the short-circuit of path brings benefits(in some cases, maybe method could be short-circuited faster?)

[windmgc]

we may count on the benchmarking to decide if the short-circuit of path brings benefits(in some cases, maybe method could be short-circuited faster?)

There definitely exists some scenarios where method evaluation can short-circuit expensive path evaluation. For example, when you have tons of GET APIs but only few POST APIs and you're using regex for all route's path matching, then the POST API request cannot gain any improvements from the PR's change(it'll make them worse instead). However the GET API requests may, because the GET method is the majority of the route set.

That is why the change in this PR could not bring performance improvement for all requests under all scenarios, but for general cases in which the path value has more variants than the limited set of http.method.

[windmgc]

However I can also not define "general case" properly as different people may have different ideas, so I'm merely just based on the fact that HTTP protocol supports more value variants configured inside path rather than inside HTTP method 😄

[ProBrian]

However I can also not define "general case" properly as different people may have different ideas, so I'm merely just based on the fact that HTTP protocol supports more value variants configured inside path rather than inside HTTP method 😄

That make sense. This PR is good to me, and we could see how it benefits from the perf benchmarking report, I think the perf benchmarking will be more like a real-world user scenarios with more practical routes and traffics.