Optimize UserAgents

changelog/@unreleased/pr-868.v2.yml

@@ -0,0 +1,11 @@
+type: improvement
+improvement:
+  description: |-
+    Optimize UserAgents
+
+    Optimize UserAgents.isValidName to use a hand-rolled implementation to validate user agent name to avoid
+    allocation and regex overhead.
+
+    Increase default UserAgents.format string builder buffer size and ensure capacity as we append user agents.
+  links:
+  - https://github.com/palantir/conjure-java-runtime-api/pull/868

service-config/src/jmh/java/com/palantir/conjure/java/api/config/service/UserAgentsBenchmark.java

@@ -0,0 +1,52 @@
+/*
+ * (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 = 2, time = 1, timeUnit = TimeUnit.SECONDS)
+@Measurement(iterations = 3, time = 1, timeUnit = TimeUnit.SECONDS)
+@Fork(1)
+@State(Scope.Benchmark)
+@SuppressWarnings("designforextension")
+public class UserAgentsBenchmark {
+
+    @Param({"service", "valid-service-name-123"})
+    private String name;
+
+    @Benchmark
+    public boolean parser() {
+        return UserAgents.isValidName(name);
+    }
+
+    @Benchmark
+    public boolean regex() {
+        return UserAgents.NAME_REGEX.matcher(name).matches();
+    }
+}

service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgents.java

@@ -34,7 +34,9 @@ public final class UserAgents {
 
     private static final SafeLogger log = SafeLoggerFactory.get(UserAgents.class);
 
-    private static final Pattern NAME_REGEX = Pattern.compile("[a-zA-Z][a-zA-Z0-9\\-]*");
+    // performance note: isValidName uses hand-rolled parser to validate name effectively matches NAME_REGEX
+    // visible for testing compatibility
+    static final Pattern NAME_REGEX = Pattern.compile("[a-zA-Z][a-zA-Z0-9\\-]*");
     private static final Pattern LENIENT_VERSION_REGEX = Pattern.compile("[0-9a-z.-]+");
     private static final Pattern NODE_REGEX = Pattern.compile("[a-zA-Z0-9][a-zA-Z0-9.\\-]*");
     private static final Pattern VERSION_REGEX =
@@ -46,7 +48,7 @@ private UserAgents() {}
 
     /** Returns the canonical string format for the given {@link UserAgent}. */
     public static String format(UserAgent userAgent) {
-        StringBuilder formatted = new StringBuilder();
+        StringBuilder formatted = new StringBuilder(64); // preallocate larger buffer for longer agents
         formatSimpleAgent(userAgent.primary(), formatted);
         if (userAgent.nodeId().isPresent()) {
             formatted.append(" (nodeId:").append(userAgent.nodeId().get()).append(')');
@@ -59,6 +61,8 @@ public static String format(UserAgent userAgent) {
     }
 
     private static void formatSimpleAgent(UserAgent.Agent agent, StringBuilder output) {
+        output.ensureCapacity(
+                output.length() + 1 + agent.name().length() + agent.version().length());
         output.append(agent.name()).append('/').append(agent.version());
     }
 
@@ -141,7 +145,32 @@ private static Map<String, String> parseComments(String commentsString) {
     }
 
     static boolean isValidName(String name) {
-        return NAME_REGEX.matcher(name).matches();
+        // hand rolled implementation of NAME_REGEX.matcher(name).matches() to avoid allocations
+        // "[a-zA-Z][a-zA-Z0-9\\-]*"
+        if (name == null || name.isEmpty()) {
+            return false;
+        }
+        char ch = name.charAt(0);
+        if (!isAlpha(ch)) {
+            return false;
+        }
+
+        for (int i = 1; i < name.length(); i++) {
+            ch = name.charAt(i);
+            if (!isAlpha(ch) && !isNumeric(ch) && ch != '-') {
+                return false;
+            }
+        }
+
+        return true;
+    }
+
+    private static boolean isAlpha(char ch) {
+        return ('a' <= ch && ch <= 'z') || ('A' <= ch && ch <= 'Z');
+    }
+
+    private static boolean isNumeric(char ch) {
+        return '0' <= ch && ch <= '9';
     }
 
     static boolean isValidNodeId(String instanceId) {

service-config/src/test/java/com/palantir/conjure/java/api/config/service/UserAgentTest.java

@@ -168,4 +168,40 @@ public void tryParse_parsesWithBestEffort() {
         assertThat(UserAgents.format(UserAgents.tryParse("serviceA/1.2.3 bogus|1.2.3 foo bar (boom)")))
                 .isEqualTo("serviceA/1.2.3");
     }
+
+    @Test
+    public void valid_names() {
+        assertThat("a").satisfies(UserAgentTest::isValidName);
+        assertThat("a1").satisfies(UserAgentTest::isValidName);
+        assertThat("foo").satisfies(UserAgentTest::isValidName);
+        assertThat("foo-bar").satisfies(UserAgentTest::isValidName);
+        assertThat("foo-bar123").satisfies(UserAgentTest::isValidName);
+    }
+
+    @Test
+    public void invalid_names() {
+        assertThat((String) null).satisfies(UserAgentTest::isNotValidName);
+        assertThat("").satisfies(UserAgentTest::isNotValidName);
+        assertThat(" ").satisfies(UserAgentTest::isNotValidName);
+        assertThat("1").satisfies(UserAgentTest::isNotValidName);
+        assertThat("1a").satisfies(UserAgentTest::isNotValidName);
+        assertThat("1.0").satisfies(UserAgentTest::isNotValidName);
+        assertThat("service name").satisfies(UserAgentTest::isNotValidName);
+        assertThat("service.name").satisfies(UserAgentTest::isNotValidName);
+        assertThat("service_name").satisfies(UserAgentTest::isNotValidName);
+    }
+
+    private static void isValidName(String name) {
+        assertThat(UserAgents.isValidName(name)).isTrue();
+        assertThat(name)
+                .describedAs("Name should match regex, inconsistent with isValidName for '%s'", name)
+                .matches(UserAgents.NAME_REGEX);
+    }
+
+    private static void isNotValidName(String name) {
+        assertThat(UserAgents.isValidName(name)).isFalse();
+        assertThat(name)
+                .describedAs("Name should not match regex, inconsistent with isValidName for '%s'", name)
+                .doesNotMatch(UserAgents.NAME_REGEX);
+    }
 }