From 3e9e2136208bceb07e5bfc8eec5e4acacc8d1780 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Wed, 20 Jul 2022 14:40:34 -0400 Subject: [PATCH 1/4] Increase UserAgents.format buffer size --- .../palantir/conjure/java/api/config/service/UserAgents.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 065ec758e..7e287f127 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 @@ -46,7 +46,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 +59,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()); } From 5fae91d2b7552d436de60bd250eaffe7fb93c58b Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Thu, 21 Jul 2022 17:18:42 -0400 Subject: [PATCH 2/4] Optimize UserAgents.isValidName Use a hand-rolled implementation to validate user agent name to avoid allocation and regex overhead. --- .../config/service/UserAgentsBenchmark.java | 52 +++++++++++++++++++ .../java/api/config/service/UserAgents.java | 23 +++++++- .../api/config/service/UserAgentTest.java | 36 +++++++++++++ 3 files changed, 109 insertions(+), 2 deletions(-) create mode 100644 service-config/src/jmh/java/com/palantir/conjure/java/api/config/service/UserAgentsBenchmark.java diff --git a/service-config/src/jmh/java/com/palantir/conjure/java/api/config/service/UserAgentsBenchmark.java b/service-config/src/jmh/java/com/palantir/conjure/java/api/config/service/UserAgentsBenchmark.java new file mode 100644 index 000000000..67eb29405 --- /dev/null +++ b/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(); + } +} 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 7e287f127..e148194d2 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 @@ -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 = @@ -143,7 +145,24 @@ private static Map 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 (!Character.isAlphabetic(ch)) { + return false; + } + + for (int i = 1; i < name.length(); i++) { + ch = name.charAt(i); + if (!Character.isAlphabetic(ch) && !Character.isDigit(ch) && ch != '-') { + return false; + } + } + + return true; } static boolean isValidNodeId(String instanceId) { diff --git a/service-config/src/test/java/com/palantir/conjure/java/api/config/service/UserAgentTest.java b/service-config/src/test/java/com/palantir/conjure/java/api/config/service/UserAgentTest.java index 120a379c0..4c38cacc4 100644 --- a/service-config/src/test/java/com/palantir/conjure/java/api/config/service/UserAgentTest.java +++ b/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); + } } From e495b02c07b747f69bca8e7b699e839c9d233759 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Thu, 21 Jul 2022 21:39:51 +0000 Subject: [PATCH 3/4] Add generated changelog entries --- changelog/@unreleased/pr-868.v2.yml | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 changelog/@unreleased/pr-868.v2.yml diff --git a/changelog/@unreleased/pr-868.v2.yml b/changelog/@unreleased/pr-868.v2.yml new file mode 100644 index 000000000..8445c7848 --- /dev/null +++ b/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 From 781372cd19892fe957a645efed485eb9f675fc4f Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Thu, 21 Jul 2022 17:53:26 -0400 Subject: [PATCH 4/4] Simplify ASCII character comparisons --- .../conjure/java/api/config/service/UserAgents.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) 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 e148194d2..40a4bfc7e 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 @@ -151,13 +151,13 @@ static boolean isValidName(String name) { return false; } char ch = name.charAt(0); - if (!Character.isAlphabetic(ch)) { + if (!isAlpha(ch)) { return false; } for (int i = 1; i < name.length(); i++) { ch = name.charAt(i); - if (!Character.isAlphabetic(ch) && !Character.isDigit(ch) && ch != '-') { + if (!isAlpha(ch) && !isNumeric(ch) && ch != '-') { return false; } } @@ -165,6 +165,14 @@ static boolean isValidName(String name) { 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) { return NODE_REGEX.matcher(instanceId).matches(); }