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/ApacheHttpClientBlockingChannel.java

@@ -33,7 +33,9 @@
 import com.palantir.logsafe.Preconditions;
 import com.palantir.logsafe.SafeArg;
 import com.palantir.logsafe.SafeLoggable;
+import com.palantir.logsafe.UnsafeArg;
 import com.palantir.logsafe.exceptions.SafeExceptions;
+import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
 import com.palantir.logsafe.exceptions.SafeRuntimeException;
 import com.palantir.logsafe.logger.SafeLogger;
 import com.palantir.logsafe.logger.SafeLoggerFactory;
@@ -45,6 +47,7 @@
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.net.InetAddress;
+import java.net.URISyntaxException;
 import java.net.URL;
 import java.util.Arrays;
 import java.util.Collections;
@@ -67,6 +70,7 @@
 import org.apache.hc.core5.http.NoHttpResponseException;
 import org.apache.hc.core5.http.io.support.ClassicRequestBuilder;
 import org.apache.hc.core5.http.message.BasicHeader;
+import org.apache.hc.core5.net.URIAuthority;
 
 final class ApacheHttpClientBlockingChannel implements BlockingChannel {
     private static final SafeLogger log = SafeLoggerFactory.get(ApacheHttpClientBlockingChannel.class);
@@ -100,8 +104,11 @@ final class ApacheHttpClientBlockingChannel implements BlockingChannel {
     public Response execute(Endpoint endpoint, Request request) throws IOException {
         // Create base request given the URL
         URL target = baseUrl.render(endpoint, request);
-        ClassicRequestBuilder builder =
-                ClassicRequestBuilder.create(endpoint.httpMethod().name()).setUri(target.toString());
+        ClassicRequestBuilder builder = ClassicRequestBuilder.create(
+                        endpoint.httpMethod().name())
+                .setScheme(target.getProtocol())
+                .setAuthority(parseAuthority(target))
+                .setPath(target.getFile());
 
         // Fill headers
         request.headerParams().forEach(builder::addHeader);
@@ -167,6 +174,14 @@ public Response execute(Endpoint endpoint, Request request) throws IOException {
         }
     }
 
+    private static URIAuthority parseAuthority(URL url) {
+        try {
+            return URIAuthority.create(url.getAuthority());
+        } catch (URISyntaxException e) {
+            throw new SafeIllegalArgumentException("Invalid URI authority", e, UnsafeArg.of("url", url));
+        }
+    }
+
     private Arg<?>[] failureDiagnosticArgs(Endpoint endpoint, Request request, long startTimeNanos) {
         long durationMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTimeNanos);
         return new Arg<?>[] {
@@ -315,7 +330,7 @@ public int code() {
         public ListMultimap<String, String> headers() {
             if (headers == null) {
                 ListMultimap<String, String> tmpHeaders = MultimapBuilder.treeKeys(String.CASE_INSENSITIVE_ORDER)
-                        .arrayListValues()
+                        .arrayListValues(1)
                         .build();
                 Iterator<Header> headerIterator = response.headerIterator();
                 while (headerIterator.hasNext()) {

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,8 @@ 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);
-        }
+        final URI targetUri = parseTargetUri(target);
+
         ProxySelector proxySelectorInstance = this.proxySelector;
         if (proxySelectorInstance == null) {
             proxySelectorInstance = ProxySelector.getDefault();
@@ -79,6 +76,15 @@ public HttpHost determineProxy(final HttpHost target, final HttpContext _context
         return result;
     }
 
+    private static URI parseTargetUri(HttpHost target) throws HttpException {
+        MaybeUri maybeUri = Uris.tryParse(target.toString());
+        if (maybeUri.isSuccessful()) {
+            return maybeUri.uriOrThrow();
+        } else {
+            throw new HttpException("Cannot convert host to URI: " + target, maybeUri.exception());
+        }
+    }
+
     private Proxy chooseProxy(final List<Proxy> proxies) {
         Proxy result = null;
         // check the list for one we can use

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;
@@ -103,7 +104,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)
                     .filter(Predicate.not(MaybeUri::isMeshMode))
                     .map(MaybeUri::uri)

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,
@@ -151,16 +128,6 @@ static <I> Refreshable<DnsResolutionResults<I>> pollForChanges(
         return dnsResolutionResult;
     }
 
-    /**
-     * This prefix may reconfigure several aspects of the client to work better in a world where requests are routed
-     * through a service mesh like istio/envoy.
-     */
-    private static final String MESH_PREFIX = "mesh-";
-
-    static boolean isMeshMode(String uri) {
-        return uri.startsWith(MESH_PREFIX);
-    }
-
     @SuppressWarnings("checkstyle:CyclomaticComplexity")
     static ImmutableList<TargetUri> getTargetUris(
             @Safe String serviceNameForLogging,
@@ -181,7 +148,7 @@ static ImmutableList<TargetUri> getTargetUris(
             // are considered equivalent.
             // When a proxy is used, pre-resolved IP addresses have no impact. In many cases the
             // proxy handles DNS resolution.
-            if (resolvedHosts.isEmpty() || DnsSupport.isMeshMode(uri) || usesProxy(proxySelector, parsed)) {
+            if (resolvedHosts.isEmpty() || Uris.isMeshMode(uri) || usesProxy(proxySelector, parsed)) {
                 targetUris.add(TargetUri.of(uri));
             } else {
                 String host = parsed.getHost();
@@ -220,23 +187,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. "
@@ -287,31 +242,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;
-        }
-
-        boolean isMeshMode() {
-            return uri() != null && DnsSupport.isMeshMode(uri().getScheme());
-        }
-
-        @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) {