diff --git a/build.gradle b/build.gradle index 0e70700b..4649e89a 100644 --- a/build.gradle +++ b/build.gradle @@ -12,6 +12,7 @@ buildscript { classpath 'com.palantir.gradle.revapi:gradle-revapi:1.7.0' classpath 'com.palantir.baseline:gradle-baseline-java:4.147.0' classpath 'com.palantir.gradle.consistentversions:gradle-consistent-versions:2.11.0' + classpath 'me.champeau.jmh:jmh-gradle-plugin:0.6.6' } } diff --git a/changelog/@unreleased/pr-864.v2.yml b/changelog/@unreleased/pr-864.v2.yml new file mode 100644 index 00000000..f0800bef --- /dev/null +++ b/changelog/@unreleased/pr-864.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Optimize UserAgent version validation + links: + - https://github.com/palantir/conjure-java-runtime-api/pull/864 diff --git a/service-config/build.gradle b/service-config/build.gradle index dceb91a3..8aaeef83 100644 --- a/service-config/build.gradle +++ b/service-config/build.gradle @@ -2,6 +2,7 @@ apply plugin: "org.inferred.processors" apply plugin: 'com.palantir.external-publish-jar' apply plugin: 'com.palantir.revapi' +apply plugin: 'me.champeau.jmh' dependencies { api project(":ssl-config") diff --git a/service-config/src/jmh/java/com/palantir/conjure/java/api/config/service/VersionParserBenchmark.java b/service-config/src/jmh/java/com/palantir/conjure/java/api/config/service/VersionParserBenchmark.java new file mode 100644 index 00000000..57bde7a3 --- /dev/null +++ b/service-config/src/jmh/java/com/palantir/conjure/java/api/config/service/VersionParserBenchmark.java @@ -0,0 +1,57 @@ +/* + * (c) Copyright 2022 Palantir Technologies Inc. All rights reserved. + * + * 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 + * + * http://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 com.palantir.conjure.java.api.config.service; + +import java.util.concurrent.TimeUnit; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; + +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.MICROSECONDS) +@Warmup(iterations = 3, time = 1, timeUnit = TimeUnit.SECONDS) +@Measurement(iterations = 3, time = 1, timeUnit = TimeUnit.SECONDS) +@Fork(1) +@State(Scope.Benchmark) +@SuppressWarnings("designforextension") +public class VersionParserBenchmark { + + @Param({"123.456.789", "12.34.56.78-rc9-0-gabc"}) + private String version; + + @Benchmark + public boolean parser() { + return VersionParser.countNumericDotGroups(version) >= 2; + } + + @Benchmark + public boolean regex() { + return UserAgents.versionMatchesRegex(version); + } + + @Benchmark + public boolean isValid() { + return UserAgents.isValidVersion(version); + } +} diff --git a/service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgents.java b/service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgents.java index e397ea75..065ec758 100644 --- a/service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgents.java +++ b/service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgents.java @@ -149,11 +149,17 @@ static boolean isValidNodeId(String instanceId) { } static boolean isValidVersion(String version) { - if (VERSION_REGEX.matcher(version).matches()) { + if (VersionParser.countNumericDotGroups(version) >= 2 // fast path for numeric & dot only version numbers + || versionMatchesRegex(version)) { return true; } log.warn("Encountered invalid user agent version '{}'", SafeArg.of("version", version)); return false; } + + // visible for benchmarking + static boolean versionMatchesRegex(String version) { + return VERSION_REGEX.matcher(version).matches(); + } } diff --git a/service-config/src/main/java/com/palantir/conjure/java/api/config/service/VersionParser.java b/service-config/src/main/java/com/palantir/conjure/java/api/config/service/VersionParser.java new file mode 100644 index 00000000..be3951fa --- /dev/null +++ b/service-config/src/main/java/com/palantir/conjure/java/api/config/service/VersionParser.java @@ -0,0 +1,133 @@ +/* + * (c) Copyright 2022 Palantir Technologies Inc. All rights reserved. + * + * 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 + * + * http://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 com.palantir.conjure.java.api.config.service; + +import com.google.errorprone.annotations.Immutable; + +/** + * An optimized version parser that supports version numbers with up to four segments, + * covering the most common {@link com.palantir.sls.versions.OrderableSlsVersion} and + * browser user-agent version strings. + *

+ * We're using parser combinator-style ideas here, where each parser function + * {@link #number(String, int)} accepts an index into our source string and returns two values: + *

+ * - an updated index into the string representing how many characters were parsed + * - a value that was actually parsed out of the string (if the parser was able to parse the string), otherwise a + * clear signal that the parser failed (we use {@link Integer#MIN_VALUE} for this). + *

+ * We bit-pack these two integer values into a single long using {@link #ok(int, int)} and {@link #fail(int)} functions + * because primitive longs live on the stack and don't impact GC. + */ +@Immutable +final class VersionParser { + private VersionParser() {} + + /** Returns the count of version number groups parsed if a valid version string, or -1 otherwise. */ + public static int countNumericDotGroups(String string) { + long state = 0; + for (int i = 1; getIndex(state) < string.length(); i++) { + state = number(string, getIndex(state)); + if (failed(state)) { + return -1; + } + + state = literalDot(string, getIndex(state)); + if (failed(state)) { + if (getIndex(state) < string.length()) { + return -1; // reject due to trailing stuff + } + return i; // no more dots + } + } + + // reject due to trailing stuff + return -1; + } + + private static final long INT_MASK = (1L << 32) - 1; + private static final int PARSE_FAILED = Integer.MIN_VALUE; + + static long number(String string, int startIndex) { + int next = startIndex; + int len = string.length(); + while (next < len) { + int codepoint = string.codePointAt(next); + if (Character.isDigit(codepoint)) { + next += 1; + } else { + break; + } + } + if (next == startIndex) { + return fail(startIndex); + } else if (next == startIndex + 1) { + return ok(next, Character.digit(string.codePointAt(startIndex), 10)); + } else { + try { + int result = Integer.parseUnsignedInt(string, startIndex, next, 10); + if (result < 0) { + // i.e. we overflowed the int + return fail(startIndex); + } + return ok(next, result); + } catch (NumberFormatException e) { + if (e.getMessage() != null && e.getMessage().endsWith("exceeds range of unsigned int.")) { + return fail(startIndex); + } else { + throw e; + } + } + } + } + + static long literalDot(String string, int startIndex) { + if (startIndex < string.length() && string.codePointAt(startIndex) == '.') { + return ok(startIndex + 1, 0); + } else { + return fail(startIndex); + } + } + + /** + * We are bit-packing two integers into a single long. The 'index' occupies half of the bits and the 'result' + * occupies the other half. + */ + static long ok(int index, int result) { + return ((long) index) << 32 | (result & INT_MASK); + } + + static long fail(int index) { + return ((long) index) << 32 | (PARSE_FAILED & INT_MASK); + } + + static boolean isOk(long state) { + return getResult(state) != PARSE_FAILED; + } + + static boolean failed(long state) { + return !isOk(state); + } + + static int getResult(long state) { + return (int) (state & INT_MASK); + } + + static int getIndex(long state) { + return (int) (state >>> 32); + } +} diff --git a/service-config/src/test/java/com/palantir/conjure/java/api/config/service/VersionParserTest.java b/service-config/src/test/java/com/palantir/conjure/java/api/config/service/VersionParserTest.java new file mode 100644 index 00000000..3fdf21fe --- /dev/null +++ b/service-config/src/test/java/com/palantir/conjure/java/api/config/service/VersionParserTest.java @@ -0,0 +1,54 @@ +/* + * (c) Copyright 2022 Palantir Technologies Inc. All rights reserved. + * + * 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 + * + * http://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 com.palantir.conjure.java.api.config.service; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.stream.Stream; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +class VersionParserTest { + + @ParameterizedTest + @MethodSource("versions") + void validVersions(String input, int result, boolean isValid) { + assertThat(VersionParser.countNumericDotGroups(input)).isEqualTo(result); + assertThat(UserAgents.isValidVersion(input)).isEqualTo(isValid); + } + + private static Stream versions() { + return Stream.of( + Arguments.of("", -1, false), + Arguments.of(" ", -1, false), + Arguments.of("bad", -1, false), + Arguments.of("1", 1, true), + Arguments.of("1.2", 2, true), + Arguments.of("1.2.3", 3, true), + Arguments.of("123.456.789", 3, true), + Arguments.of("1.2.3.4", 4, true), + Arguments.of("123.45.6.78", 4, true), + Arguments.of("1.2.3-rc4", -1, true), + Arguments.of("123.45.6.78-rc90", -1, true), + Arguments.of("1.2.3-4-gabc", -1, true), + Arguments.of("1.2.3.4-rc5-6-gabc", -1, true), + Arguments.of("123.456.789-rc0", -1, true), + Arguments.of("1.2.3-4", -1, false), + Arguments.of("0-0-0", -1, false)); + } +} diff --git a/versions.props b/versions.props index b120764a..00ab5ed7 100644 --- a/versions.props +++ b/versions.props @@ -8,3 +8,4 @@ org.assertj:assertj-core = 3.23.1 org.immutables:* = 2.8.8 org.junit.jupiter:* = 5.8.2 org.mockito:mockito-core = 4.6.1 +org.openjdk.jmh:* = 1.35