Cache parsed URIs throughout Dialogue

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

@@ -0,0 +1,9 @@
+type: improvement
+improvement:
+  description: Parsing String to URI can be expensive in terms of CPU and allocations
+    for high throughput services. This generalizes the caching previously added to
+    DnsSupport in https://github.com/palantir/dialogue/pull/2398 to also cover ApacheHttpClientBlockingChannel
+    requests and HttpsProxyDefaultRoutePlanner proxy parsing to leverage a shared
+    parsed URI cache.
+  links:
+  - https://github.com/palantir/dialogue/pull/2432

dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientChannels.java

@@ -474,7 +474,7 @@ public CloseableClient build() {
             String name = Preconditions.checkNotNull(clientName, "Client name is required");
             Preconditions.checkArgument(
                     !conf.fallbackToCommonNameVerification(), "fallback-to-common-name-verification is not supported");
-            Preconditions.checkArgument(!conf.meshProxy().isPresent(), "Mesh proxy is not supported");
+            Preconditions.checkArgument(conf.meshProxy().isEmpty(), "Mesh proxy is not supported");
 
             Timeout socketTimeout = getSocketTimeout(conf, name);
 

dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/HttpsProxyDefaultRoutePlanner.java

@@ -17,11 +17,12 @@
 package com.palantir.dialogue.hc5;
 
 import com.palantir.conjure.java.client.config.HttpsProxies;
+import com.palantir.dialogue.core.Uris;
+import com.palantir.dialogue.core.Uris.MaybeUri;
 import java.net.InetSocketAddress;
 import java.net.Proxy;
 import java.net.ProxySelector;
 import java.net.URI;
-import java.net.URISyntaxException;
 import java.util.List;
 import javax.annotation.CheckForNull;
 import org.apache.hc.client5.http.impl.routing.DefaultRoutePlanner;
@@ -49,12 +50,6 @@ final class HttpsProxyDefaultRoutePlanner extends DefaultRoutePlanner {
     @Override
     @CheckForNull
     public HttpHost determineProxy(final HttpHost target, final HttpContext _context) throws HttpException {
-        final URI targetUri;
-        try {
-            targetUri = new URI(target.toURI());
-        } catch (final URISyntaxException ex) {
-            throw new HttpException("Cannot convert host to URI: " + target, ex);
-        }
         ProxySelector proxySelectorInstance = this.proxySelector;
         if (proxySelectorInstance == null) {
             proxySelectorInstance = ProxySelector.getDefault();
@@ -63,20 +58,25 @@ public HttpHost determineProxy(final HttpHost target, final HttpContext _context
             // The proxy selector can be "unset", so we must be able to deal with a null selector
             return null;
         }
-        final List<Proxy> proxies = proxySelectorInstance.select(targetUri);
-        final Proxy p = chooseProxy(proxies);
-        HttpHost result = null;
-        if (p.type() == Proxy.Type.HTTP) {
+        Proxy proxy = chooseProxy(proxySelectorInstance.select(parseTargetUri(target)));
+        if (Proxy.Type.HTTP.equals(proxy.type())) {
             // convert the socket address to an HttpHost
-            if (!(p.address() instanceof InetSocketAddress)) {
-                throw new HttpException("Unable to handle non-Inet proxy address: " + p.address());
+            if (proxy.address() instanceof InetSocketAddress isa) {
+                String scheme = HttpsProxies.isHttps(proxy) ? "https" : "http";
+                return new HttpHost(scheme, isa.getAddress(), isa.getHostString(), isa.getPort());
             }
-            final InetSocketAddress isa = (InetSocketAddress) p.address();
-            String scheme = HttpsProxies.isHttps(p) ? "https" : "http";
-            result = new HttpHost(scheme, isa.getAddress(), isa.getHostString(), isa.getPort());
+            throw new HttpException("Unable to handle non-Inet proxy address: " + proxy.address());
         }
 
-        return result;
+        return null;
+    }
+
+    private static URI parseTargetUri(HttpHost target) throws HttpException {
+        MaybeUri maybeUri = Uris.tryParse(target.toString());
+        if (maybeUri.isSuccessful()) {
+            return maybeUri.uriOrThrow();
+        }
+        throw new HttpException("Cannot convert host to URI: " + target, maybeUri.exception());
     }
 
     private Proxy chooseProxy(final List<Proxy> proxies) {
@@ -98,7 +98,7 @@ private Proxy chooseProxy(final List<Proxy> proxies) {
         }
         if (result == null) {
             // @@@ log as warning or info that only a socks proxy is available?
-            // result can only be null if all proxies are socks proxies
+            // result can only be null if all proxies are socks proxies.
             // socks proxies are not handled on the route planning level
             result = Proxy.NO_PROXY;
         }

dialogue-clients/src/main/java/com/palantir/dialogue/clients/DialogueDnsResolutionWorker.java

@@ -25,8 +25,9 @@
 import com.google.common.collect.SetMultimap;
 import com.google.common.collect.Sets;
 import com.google.errorprone.annotations.concurrent.GuardedBy;
-import com.palantir.dialogue.clients.DnsSupport.MaybeUri;
 import com.palantir.dialogue.core.DialogueDnsResolver;
+import com.palantir.dialogue.core.Uris;
+import com.palantir.dialogue.core.Uris.MaybeUri;
 import com.palantir.logsafe.Safe;
 import com.palantir.logsafe.SafeArg;
 import com.palantir.logsafe.Unsafe;
@@ -102,7 +103,7 @@ private synchronized void doUpdate(@Nullable INPUT updatedInputState) {
             long start = System.nanoTime();
             ImmutableSet<String> allHosts = spec.extractUris(inputState)
                     .filter(Objects::nonNull)
-                    .map(DnsSupport::tryParseUri)
+                    .map(Uris::tryParse)
                     .filter(MaybeUri::isSuccessful)
                     .map(MaybeUri::uri)
                     .filter(Objects::nonNull)

dialogue-clients/src/main/java/com/palantir/dialogue/clients/DnsPollingSpec.java

@@ -83,7 +83,8 @@ public String kind() {
 
             @Override
             public Stream<String> extractUris(Optional<ServiceConfiguration> input) {
-                return input.stream().flatMap(item -> item.uris().stream());
+                return input.map(serviceConfiguration -> serviceConfiguration.uris().stream())
+                        .orElseGet(Stream::empty);
             }
 
             @Override

dialogue-clients/src/main/java/com/palantir/dialogue/clients/DnsSupport.java

@@ -18,9 +18,6 @@
 
 import com.codahale.metrics.Counter;
 import com.codahale.metrics.Timer;
-import com.github.benmanes.caffeine.cache.Caffeine;
-import com.github.benmanes.caffeine.cache.LoadingCache;
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Suppliers;
 import com.google.common.collect.Collections2;
 import com.google.common.collect.ImmutableList;
@@ -32,19 +29,17 @@
 import com.palantir.dialogue.core.DialogueDnsResolver;
 import com.palantir.dialogue.core.DialogueExecutors;
 import com.palantir.dialogue.core.TargetUri;
-import com.palantir.logsafe.Preconditions;
+import com.palantir.dialogue.core.Uris;
 import com.palantir.logsafe.Safe;
 import com.palantir.logsafe.SafeArg;
 import com.palantir.logsafe.Unsafe;
 import com.palantir.logsafe.UnsafeArg;
-import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
 import com.palantir.logsafe.logger.SafeLogger;
 import com.palantir.logsafe.logger.SafeLoggerFactory;
 import com.palantir.refreshable.Disposable;
 import com.palantir.refreshable.Refreshable;
 import com.palantir.refreshable.SettableRefreshable;
 import com.palantir.tritium.metrics.MetricRegistries;
-import com.palantir.tritium.metrics.caffeine.CacheStats;
 import com.palantir.tritium.metrics.registry.SharedTaggedMetricRegistries;
 import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;
 import java.lang.ref.Cleaner;
@@ -90,24 +85,6 @@ final class DnsSupport {
                             .build(),
                     SCHEDULER_NAME)));
 
-    /**
-     * Shared cache of string to parsed URI. This avoids excessive allocation overhead when parsing repeated targets.
-     */
-    private static final LoadingCache<String, MaybeUri> uriCache = CacheStats.of(
-                    SharedTaggedMetricRegistries.getSingleton(), "dialogue-uri")
-            .register(stats -> Caffeine.newBuilder()
-                    .maximumWeight(100_000)
-                    .<String, MaybeUri>weigher((key, _value) -> key.length())
-                    .expireAfterAccess(Duration.ofMinutes(1))
-                    .softValues()
-                    .recordStats(stats)
-                    .build(DnsSupport::tryParseUri));
-
-    @VisibleForTesting
-    static void invalidateCaches() {
-        uriCache.invalidateAll();
-    }
-
     /** Identical to the overload, but using the {@link #sharedScheduler}. */
     static <I> Refreshable<DnsResolutionResults<I>> pollForChanges(
             boolean dnsNodeDiscovery,
@@ -208,23 +185,11 @@ private static boolean usesProxy(ProxySelector proxySelector, URI uri) {
         }
     }
 
-    @Unsafe
-    static MaybeUri tryParseUri(@Unsafe String uriString) {
-        try {
-            return MaybeUri.success(new URI(uriString));
-        } catch (Exception e) {
-            log.debug("Failed to parse URI", e);
-            return MaybeUri.failure(
-                    new SafeIllegalArgumentException("Failed to parse URI", e, UnsafeArg.of("uri", uriString)));
-        }
-    }
-
     @Unsafe
     @Nullable
     private static URI tryParseUri(TaggedMetricRegistry metrics, @Safe String serviceName, @Unsafe String uri) {
         try {
-            MaybeUri maybeUri = uriCache.get(uri);
-            URI result = maybeUri.uriOrThrow();
+            URI result = Uris.tryParse(uri).uriOrThrow();
             if (result.getHost() == null) {
                 log.error(
                         "Failed to correctly parse URI {} for service {} due to null host component. "
@@ -275,27 +240,4 @@ public void run() {
             }
         }
     }
-
-    @Unsafe
-    record MaybeUri(@Nullable URI uri, @Nullable SafeIllegalArgumentException exception) {
-        static @Unsafe DnsSupport.MaybeUri success(URI uri) {
-            return new MaybeUri(uri, null);
-        }
-
-        static @Unsafe DnsSupport.MaybeUri failure(SafeIllegalArgumentException exception) {
-            return new MaybeUri(null, exception);
-        }
-
-        boolean isSuccessful() {
-            return uri() != null;
-        }
-
-        @Unsafe
-        URI uriOrThrow() {
-            if (exception() != null) {
-                throw exception();
-            }
-            return Preconditions.checkNotNull(uri(), "uri");
-        }
-    }
 }

dialogue-clients/src/test/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolverTest.java

@@ -86,7 +86,7 @@ void unknown_host() {
 
         assertThat(result).isEmpty();
         ClientDnsMetrics metrics = ClientDnsMetrics.of(registry);
-        assertThat(metrics.failure("EAI_NONAME").getCount()).isEqualTo(1);
+        assertThat(metrics.failure("EAI_NONAME").getCount()).isOne();
     }
 
     @Test
@@ -99,12 +99,12 @@ void unknown_host_from_cache() {
         ImmutableSet<InetAddress> result = resolver.resolve(badHost);
 
         assertThat(result).isEmpty();
-        assertThat(metrics.failure("EAI_NONAME").getCount()).isEqualTo(1);
+        assertThat(metrics.failure("EAI_NONAME").getCount()).isOne();
 
         // should resolve from cache
         ImmutableSet<InetAddress> result2 = resolver.resolve(badHost);
         assertThat(result2).isEmpty();
-        assertThat(metrics.failure("CACHED").getCount()).isEqualTo(1);
+        assertThat(metrics.failure("CACHED").getCount()).isOne();
     }
 
     private static ImmutableSet<InetAddress> resolve(String hostname) {

dialogue-core/build.gradle

@@ -7,20 +7,21 @@ dependencies {
     api 'com.palantir.conjure.java.runtime:client-config'
     implementation project(':dialogue-futures')
     implementation 'com.github.ben-manes.caffeine:caffeine'
+    implementation 'com.google.code.findbugs:jsr305'
+    implementation 'com.google.errorprone:error_prone_annotations'
     implementation 'com.google.guava:guava'
+    implementation 'com.palantir.conjure.java.api:errors'
+    implementation 'com.palantir.conjure.java.api:service-config'
+    implementation 'com.palantir.refreshable:refreshable'
     implementation 'com.palantir.safe-logging:logger'
     implementation 'com.palantir.safe-logging:preconditions'
     implementation 'com.palantir.safe-logging:safe-logging'
-    implementation 'com.palantir.tracing:tracing'
-    implementation 'io.dropwizard.metrics:metrics-core'
     implementation 'com.palantir.safethreadlocalrandom:safe-thread-local-random'
-    implementation 'com.palantir.tritium:tritium-metrics'
-    implementation 'com.google.code.findbugs:jsr305'
-    implementation 'com.google.errorprone:error_prone_annotations'
-    implementation 'com.palantir.conjure.java.api:errors'
-    implementation 'com.palantir.conjure.java.api:service-config'
+    implementation 'com.palantir.tracing:tracing'
     implementation 'com.palantir.tracing:tracing-api'
-    implementation 'com.palantir.refreshable:refreshable'
+    implementation 'com.palantir.tritium:tritium-caffeine'
+    implementation 'com.palantir.tritium:tritium-metrics'
+    implementation 'io.dropwizard.metrics:metrics-core'
 
     testImplementation 'com.palantir.tracing:tracing-test-utils'
     testImplementation 'com.palantir.safe-logging:preconditions-assertj'

dialogue-core/src/main/java/com/palantir/dialogue/core/TargetUri.java

@@ -84,12 +84,10 @@ public int hashCode() {
         return result;
     }
 
-    /** Note that this does not retain service-mesh prefixes. */
     public static TargetUri of(String uri) {
         return new TargetUri(uri, Optional.empty());
     }
 
-    /** Note that this does not retain service-mesh prefixes. */
     public static TargetUri of(String uri, @Nullable InetAddress resolvedAddress) {
         return new TargetUri(uri, Optional.ofNullable(resolvedAddress));
     }