This fixes the issue #24873
As of now, AntPathStringMatcher uses regex matching even when:
- the pattern is a constant. In such cases, it should be enough to use strings comparison
- the pattern is .*. In such cases it is always a positive match
Applying such optimizations increased PatternsRequestCondition evaluation (on my machine) from ~440 ops/ms to ~700 ops/ms
AntPathStringMatcher is used not only for URL matching, so it would be nice to double-check carefully
Comment From: bclozel
I've run these changes and variants as JMH benchmarks and I'm not seeing significant improvements (a bit better or worse than the current implementation depending on the seed data, but always within the error margin).
I guess the JVM is already optimizing those bits for us; the performance test strategy used in this PR is flawed. I'd suggest to look at JMH for a proper benchmark harness. I'm closing this PR for now. Thanks!
Comment From: encircled
I don’t think JVM is able to do such optimization. Can you provide your test?
Comment From: encircled
Alright, the same test using JMH:
@Benchmark
public void test() {
matcher.match("/something/things/foos/{foo}/bar/{id}", "/something/things/foos/test/bar/qwerty");
}
Result is 767.614 ns/op vs 344.477 ns/op. Is not it significant @bclozel ? ;) I know this is just one particular type of URL pattern tho.
Comment From: bclozel
Alright this is a very targeted improvement but an improvement still. It's a small change but in a core part of our infrastructure that tends to lead to unintended behavior change quite easily. So I'll schedule that for our next 5.3 because it's a bit late to introduce that in a bugfix release.
Thanks!
Comment From: encircled
Great, thanks!
I know it is in the core, so I'll double check it and maybe add more functional tests in the meanwhile.
Shall I also keep the new perf test and add @EnabledForTestGroups(PERFORMANCE)?
Comment From: bclozel
You can remove the performance test, as we're about to remove them anyway in #24830. I'll ping you if I need some input for contributing JMH tests but I think we should be good.
Comment From: encircled
I've fixed one more issue, should be final now