Skip to content

Avoid using regex matching for static patterns #24887

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

No Sign up for GitHub

By clicking “No Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? No Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Avoid using regex matching for static patterns #24887

wants to merge 1 commit into from

Conversation

encircled
Copy link
Contributor

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

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 9, 2020
@bclozel bclozel self-assigned this Apr 9, 2020
@bclozel
Copy link
Member

bclozel commented Apr 9, 2020

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!

@bclozel bclozel closed this Apr 9, 2020
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 9, 2020
@encircled
Copy link
Contributor Author

I don’t think JVM is able to do such optimization. Can you provide your test?

@encircled
Copy link
Contributor Author

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.

@bclozel bclozel added this to the 5.3 M1 milestone Apr 10, 2020
@bclozel bclozel added type: enhancement A general enhancement and removed status: declined A suggestion or change that we don't feel we should currently apply labels Apr 10, 2020
@bclozel
Copy link
Member

bclozel commented Apr 10, 2020

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!

@bclozel bclozel reopened this Apr 10, 2020
@encircled
Copy link
Contributor Author

encircled commented Apr 10, 2020 •
edited
Loading

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)?

@bclozel
Copy link
Member

bclozel commented Apr 10, 2020

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.

@encircled encircled force-pushed the ant-path-matcher-perf branch from c6e45c9 to 221386c Compare April 12, 2020 13:31
@encircled
Copy link
Contributor Author

I've fixed one more issue, should be final now

@encircled encircled force-pushed the ant-path-matcher-perf branch from 221386c to 4d7dc2c Compare April 13, 2020 09:36
@bclozel bclozel changed the title Avoid using regex for URL matching when possible Avoid using regex matching for static patterns May 15, 2020
bclozel pushed a commit that referenced this pull request May 15, 2020
@encircled @bclozel
Avoid using regex for URL matching when possible
9bfe410 
See gh-24887
@bclozel bclozel closed this in 5672651 May 15, 2020
kenny5he pushed a commit to kenny5he/spring-framework that referenced this pull request Jun 21, 2020
@encircled @kenny5he
Avoid using regex for URL matching when possible
c866c8c 
See spring-projectsgh-24887
kenny5he pushed a commit to kenny5he/spring-framework that referenced this pull request Jun 21, 2020
@bclozel @kenny5he
Avoid using regex matching for static patterns
e19a14f 
Prior to this commit (and the previous one), the `AntPathStringMatcher`
(inner class of `AntPathmatcher`) would compile `Pattern` instances and
use regex matching even for static patterns such as `"/home"`.

This change introduces a shortcut in the string matcher algorithm to
skip the `Pattern` creation and uses `String` equality instead.
Static patterns are quite common in applications and this change can
bring performance improvements, depending on the mix of patterns
configured in the web application.

In benchmarks (added with this commit), we're seeing +20% throughput
and -40% allocation. This of course can vary depending on the number of
static patterns configured in the application.

Closes spring-projectsgh-24887
No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Reviewers
No reviews
Assignees

@bclozel bclozel

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
5.3 M1
Development

Successfully merging this pull request may close these issues.
3 participants
@encircled @bclozel @spring-projects-issues