Optimize UserAgent version validation (#864)

Optimize UserAgent version validation

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'
     }
 }
 

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

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

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);
+    }
+}

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();
+    }
 }

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.
+ * <p>
+ * 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:
+ * <p>
+ * - 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).
+ * <p>
+ * 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);
+    }
+}

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<Arguments> 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));
+    }
+}

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