From 567265123b738045db4808960331812adc3a7aab Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Fri, 15 May 2020 17:07:58 +0200 Subject: [PATCH] Avoid using regex matching for static patterns 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 gh-24887 --- .../springframework/util/AntPathMatcher.java | 93 +++--- .../util/AntPathMatcherTests.java | 3 - .../util/pattern/PathMatchingBenchmark.java | 278 ++++++++++++++++++ src/checkstyle/checkstyle-suppressions.xml | 2 +- 4 files changed, 319 insertions(+), 57 deletions(-) create mode 100644 spring-web/src/jmh/java/org/springframework/web/util/pattern/PathMatchingBenchmark.java diff --git a/spring-core/src/main/java/org/springframework/util/AntPathMatcher.java b/spring-core/src/main/java/org/springframework/util/AntPathMatcher.java index 6573af5215..fe8cd7d58e 100644 --- a/spring-core/src/main/java/org/springframework/util/AntPathMatcher.java +++ b/spring-core/src/main/java/org/springframework/util/AntPathMatcher.java @@ -80,7 +80,7 @@ public class AntPathMatcher implements PathMatcher { private static final Pattern VARIABLE_PATTERN = Pattern.compile("\\{[^/]+?}"); - private static final char[] WILDCARD_CHARS = { '*', '?', '{' }; + private static final char[] WILDCARD_CHARS = {'*', '?', '{'}; private String pathSeparator; @@ -646,20 +646,15 @@ public class AntPathMatcher implements PathMatcher { private static final String DEFAULT_VARIABLE_PATTERN = "(.*)"; + private final String rawPattern; + + private final boolean caseSensitive; + + private final boolean exactMatch; + + @Nullable private final Pattern pattern; - private final String originalPattern; - - /** - * True if this matcher accepts any value (e.g. regex .*) - */ - private final boolean acceptAny; - - /** - * True if given pattern is a literal pattern, thus candidate string may be tested by strings comparison for better perf. - */ - private final boolean allowStringComparison; - private final List variableNames = new LinkedList<>(); public AntPathStringMatcher(String pattern) { @@ -667,6 +662,8 @@ public class AntPathMatcher implements PathMatcher { } public AntPathStringMatcher(String pattern, boolean caseSensitive) { + this.rawPattern = pattern; + this.caseSensitive = caseSensitive; StringBuilder patternBuilder = new StringBuilder(); Matcher matcher = GLOB_PATTERN.matcher(pattern); int end = 0; @@ -696,13 +693,17 @@ public class AntPathMatcher implements PathMatcher { } end = matcher.end(); } - patternBuilder.append(quote(pattern, end, pattern.length())); - this.pattern = (caseSensitive ? Pattern.compile(patternBuilder.toString()) : - Pattern.compile(patternBuilder.toString(), Pattern.CASE_INSENSITIVE)); - - this.originalPattern = pattern; - this.acceptAny = this.pattern.pattern().equals(DEFAULT_VARIABLE_PATTERN); - this.allowStringComparison = end == 0; + // No glob pattern was found, this is an exact String match + if (end == 0) { + this.exactMatch = true; + this.pattern = null; + } + else { + this.exactMatch = false; + patternBuilder.append(quote(pattern, end, pattern.length())); + this.pattern = (this.caseSensitive ? Pattern.compile(patternBuilder.toString()) : + Pattern.compile(patternBuilder.toString(), Pattern.CASE_INSENSITIVE)); + } } private String quote(String s, int start, int end) { @@ -717,43 +718,29 @@ public class AntPathMatcher implements PathMatcher { * @return {@code true} if the string matches against the pattern, or {@code false} otherwise. */ public boolean matchStrings(String str, @Nullable Map uriTemplateVariables) { - // Check whether current pattern accepts any value or given string is exact match - if (this.acceptAny) { - if (uriTemplateVariables != null) { - assertUrlVariablesCount(1); - uriTemplateVariables.put(this.variableNames.get(0), str); - } - return true; + if (this.exactMatch) { + return this.caseSensitive ? this.rawPattern.equals(str) : this.rawPattern.equalsIgnoreCase(str); } - else if (this.allowStringComparison && str.equals(this.originalPattern)) { - return true; - } - - Matcher matcher = this.pattern.matcher(str); - if (matcher.matches()) { - if (uriTemplateVariables != null) { - assertUrlVariablesCount(matcher.groupCount()); - for (int i = 1; i <= matcher.groupCount(); i++) { - String name = this.variableNames.get(i - 1); - String value = matcher.group(i); - uriTemplateVariables.put(name, value); + else if (this.pattern != null) { + Matcher matcher = this.pattern.matcher(str); + if (matcher.matches()) { + if (uriTemplateVariables != null) { + if (this.variableNames.size() != matcher.groupCount()) { + throw new IllegalArgumentException("The number of capturing groups in the pattern segment " + + this.pattern + " does not match the number of URI template variables it defines, " + + "which can occur if capturing groups are used in a URI template regex. " + + "Use non-capturing groups instead."); + } + for (int i = 1; i <= matcher.groupCount(); i++) { + String name = this.variableNames.get(i - 1); + String value = matcher.group(i); + uriTemplateVariables.put(name, value); + } } + return true; } - return true; - } - else { - return false; - } - } - - // SPR-8455 - private void assertUrlVariablesCount(int expected) { - if (this.variableNames.size() != expected) { - throw new IllegalArgumentException("The number of capturing groups in the pattern segment " + - this.pattern + " does not match the number of URI template variables it defines, " + - "which can occur if capturing groups are used in a URI template regex. " + - "Use non-capturing groups instead."); } + return false; } } diff --git a/spring-core/src/test/java/org/springframework/util/AntPathMatcherTests.java b/spring-core/src/test/java/org/springframework/util/AntPathMatcherTests.java index 3d81cd0fa4..a67ebf3796 100644 --- a/spring-core/src/test/java/org/springframework/util/AntPathMatcherTests.java +++ b/spring-core/src/test/java/org/springframework/util/AntPathMatcherTests.java @@ -130,9 +130,6 @@ class AntPathMatcherTests { assertThat(pathMatcher.match("", "")).isTrue(); assertThat(pathMatcher.match("/{bla}.*", "/testing.html")).isTrue(); - - // Test that sending the same pattern will not match (gh #24887) - assertThat(pathMatcher.match("/bla/{foo:[0-9]}", "/bla/{foo:[0-9]}")).isFalse(); } @Test diff --git a/spring-web/src/jmh/java/org/springframework/web/util/pattern/PathMatchingBenchmark.java b/spring-web/src/jmh/java/org/springframework/web/util/pattern/PathMatchingBenchmark.java new file mode 100644 index 0000000000..499f0a5513 --- /dev/null +++ b/spring-web/src/jmh/java/org/springframework/web/util/pattern/PathMatchingBenchmark.java @@ -0,0 +1,278 @@ +/* + * Copyright 2002-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.web.util.pattern; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Level; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.infra.Blackhole; + +import org.springframework.http.server.PathContainer; +import org.springframework.util.AntPathMatcher; + +/** + * Benchmarks for matching requests paths against path patterns in a web context. + * We're considering here the {@link org.springframework.util.AntPathMatcher} and + * {@link PathPatternParser} implementations with typical sets of patterns. + * @author Brian Clozel + */ +@BenchmarkMode(Mode.Throughput) +public class PathMatchingBenchmark { + + @State(Scope.Benchmark) + public static class AllRoutesPatternParser extends PatternParserData { + + @Setup(Level.Trial) + public void registerPatterns() { + parseRoutes(RouteGenerator.allRoutes()); + } + } + + @Benchmark + public void matchAllRoutesWithPathPatternParser(AllRoutesPatternParser data, Blackhole bh) { + for (PathContainer path : data.requestPaths) { + for (PathPattern pattern : data.patterns) { + bh.consume(pattern.matches(path)); + } + } + } + + @Benchmark + public void matchAndSortAllRoutesWithPathPatternParser(AllRoutesPatternParser data, Blackhole bh) { + for (PathContainer path : data.requestPaths) { + List matches = new ArrayList<>(); + for (PathPattern pattern : data.patterns) { + if (pattern.matches(path)) { + matches.add(pattern); + } + } + Collections.sort(matches); + bh.consume(matches); + } + } + + @State(Scope.Benchmark) + public static class StaticRoutesPatternParser extends PatternParserData { + + @Setup(Level.Trial) + public void registerPatterns() { + parseRoutes(RouteGenerator.staticRoutes()); + } + } + + @Benchmark + public void matchStaticRoutesWithPathPatternParser(StaticRoutesPatternParser data, Blackhole bh) { + for (PathContainer path : data.requestPaths) { + for (PathPattern pattern : data.patterns) { + bh.consume(pattern.matches(path)); + } + } + } + + @State(Scope.Benchmark) + public static class AllRoutesAntPathMatcher extends AntPathMatcherData { + + @Setup(Level.Trial) + public void registerPatterns() { + parseRoutes(RouteGenerator.allRoutes()); + } + } + + @Benchmark + public void matchAllRoutesWithAntPathMatcher(AllRoutesAntPathMatcher data, Blackhole bh) { + for (String path : data.requestPaths) { + for (String pattern : data.patterns) { + bh.consume(data.matcher.match(pattern, path)); + } + } + } + + @Benchmark + public void matchAndSortAllRoutesWithAntPathMatcher(AllRoutesAntPathMatcher data, Blackhole bh) { + for (String path : data.requestPaths) { + List matches = new ArrayList<>(); + for (String pattern : data.patterns) { + if (data.matcher.match(pattern, path)) { + matches.add(pattern); + } + } + matches.sort(data.matcher.getPatternComparator(path)); + bh.consume(matches); + } + } + + @State(Scope.Benchmark) + public static class StaticRoutesAntPathMatcher extends AntPathMatcherData { + + @Setup(Level.Trial) + public void registerPatterns() { + parseRoutes(RouteGenerator.staticRoutes()); + } + } + + @Benchmark + public void matchStaticRoutesWithAntPathMatcher(StaticRoutesAntPathMatcher data, Blackhole bh) { + for (String path : data.requestPaths) { + for (String pattern : data.patterns) { + bh.consume(data.matcher.match(pattern, path)); + } + } + } + + + static class PatternParserData { + + List patterns = new ArrayList<>(); + + List requestPaths = new ArrayList<>(); + + void parseRoutes(List routes) { + PathPatternParser parser = new PathPatternParser(); + routes.forEach(route -> { + this.patterns.add(parser.parse(route.pattern)); + route.matchingPaths.forEach(path -> this.requestPaths.add(PathContainer.parsePath(path))); + }); + } + + } + + static class AntPathMatcherData { + + AntPathMatcher matcher = new AntPathMatcher(); + + List patterns = new ArrayList<>(); + + List requestPaths = new ArrayList<>(); + + void parseRoutes(List routes) { + routes.forEach(route -> { + this.patterns.add(route.pattern); + this.requestPaths.addAll(route.matchingPaths); + }); + } + + } + + /** + * Route in the web application. + * Each route has a path pattern and can generate sets of matching request paths for that pattern. + */ + static class Route { + + private final String pattern; + + private final List matchingPaths; + + public Route(String pattern, String... matchingPaths) { + this.pattern = pattern; + if (matchingPaths.length > 0) { + this.matchingPaths = Arrays.asList(matchingPaths); + } + else { + this.matchingPaths = Collections.singletonList(pattern); + } + } + + public String pattern() { + return this.pattern; + } + + public Iterable matchingPaths() { + return this.matchingPaths; + } + } + + static class RouteGenerator { + + static List staticRoutes() { + return Arrays.asList( + new Route("/"), + new Route("/why-spring"), + new Route("/microservices"), + new Route("/reactive"), + new Route("/event-driven"), + new Route("/cloud"), + new Route("/web-applications"), + new Route("/serverless"), + new Route("/batch"), + new Route("/community/overview"), + new Route("/community/team"), + new Route("/community/events"), + new Route("/community/support"), + new Route("/some/other/section"), + new Route("/blog.atom") + ); + } + + static List captureRoutes() { + return Arrays.asList( + new Route("/guides"), + new Route("/guides/gs/{repositoryName}", + "/guides/gs/rest-service", "/guides/gs/scheduling-tasks", + "/guides/gs/consuming-rest", "/guides/gs/relational-data-access"), + new Route("/projects"), + new Route("/projects/{name}", + "/projects/spring-boot", "/projects/spring-framework", + "/projects/spring-data", "/projects/spring-security", "/projects/spring-cloud"), + new Route("/blog/category/{category}.atom", + "/blog/category/releases.atom", "/blog/category/engineering.atom", + "/blog/category/news.atom"), + new Route("/tools/{name}", "/tools/eclipse", "/tools/vscode"), + new Route("/team/{username}", + "/team/jhoeller", "/team/bclozel", "/team/snicoll", "/team/sdeleuze", "/team/rstoyanchev"), + new Route("/api/projects/{projectId}", + "/api/projects/spring-boot", "/api/projects/spring-framework", + "/api/projects/reactor", "/api/projects/spring-data", + "/api/projects/spring-restdocs", "/api/projects/spring-batch"), + new Route("/api/projects/{projectId}/releases/{version}", + "/api/projects/spring-boot/releases/2.3.0", "/api/projects/spring-framework/releases/5.3.0", + "/api/projects/spring-boot/releases/2.2.0", "/api/projects/spring-framework/releases/5.2.0") + ); + } + + static List regexRoute() { + return Arrays.asList( + new Route("/blog/{year:\\\\d+}/{month:\\\\d+}/{day:\\\\d+}/{slug}", + "/blog/2020/01/01/spring-boot-released", "/blog/2020/02/10/this-week-in-spring", + "/blog/2020/03/12/spring-one-conference-2020", "/blog/2020/05/17/spring-io-barcelona-2020", + "/blog/2020/05/17/spring-io-barcelona-2020", "/blog/2020/06/06/spring-cloud-release"), + new Route("/user/{name:[a-z]+}", + "/user/emily", "/user/example", "/user/spring") + ); + } + + static List allRoutes() { + List routes = new ArrayList<>(); + routes.addAll(staticRoutes()); + routes.addAll(captureRoutes()); + routes.addAll(regexRoute()); + routes.add(new Route("/static/**", "/static/image.png", "/static/style.css")); + routes.add(new Route("/**", "/notfound", "/favicon.ico")); + return routes; + } + + } +} diff --git a/src/checkstyle/checkstyle-suppressions.xml b/src/checkstyle/checkstyle-suppressions.xml index bb91954fce..1d7e1aa0cb 100644 --- a/src/checkstyle/checkstyle-suppressions.xml +++ b/src/checkstyle/checkstyle-suppressions.xml @@ -8,7 +8,7 @@ - +