Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RC] Use Guava instead of Caffeine caches for Android #2483

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ allprojects {
details.because "The error_prone_annotations dependency must be low to avoid forcing consumers to use newer releases"
}
}
resolutionStrategy {
force 'com.google.guava:guava:33.4.0-android'
}
}
}

Expand All @@ -84,6 +87,7 @@ subprojects {

tasks.withType(JavaCompile) {
options.compilerArgs += ['-Werror']
options.errorprone.enabled = false
}

plugins.withId('com.palantir.baseline-error-prone', {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

import com.google.auto.common.AnnotationMirrors;
import com.google.auto.common.MoreElements;
import com.google.common.base.Predicates;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableSet;
import com.google.errorprone.annotations.CompileTimeConstant;
Expand Down Expand Up @@ -146,7 +145,7 @@ private JavaFile generateDialogueServiceFactory(Element annotatedInterface, Coll

Preconditions.checkArgument(
maybeEndpoints.stream()
.filter(Predicates.not(Optional::isPresent))
.filter(Optional::isEmpty)
.collect(Collectors.toList())
.isEmpty(),
"Failed validation");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ static ClassicHttpRequest createRequest(BaseUrl baseUrl, Endpoint endpoint, Requ
.setPath(getPath(target));

// Fill headers
request.headerParams().forEach(builder::addHeader);
request.headerParams().entries().forEach(e -> builder.addHeader(e.getKey(), e.getValue()));

if (request.body().isPresent()) {
Preconditions.checkArgument(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ final class ScheduledIdleConnectionEvictor {
.setDaemon(true)
.build(),
EXECUTOR_NAME)),
EXECUTOR_NAME));
EXECUTOR_NAME))::get;

static ScheduledFuture<?> schedule(ConnPoolControl<?> connectionManager, Duration delayBetweenChecks) {
return schedule(connectionManager, delayBetweenChecks, sharedScheduler.get());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public final class BlockingChannelAdapter {
.setNameFormat("dialogue-blocking-channel-%d")
.setDaemon(true)
.build(),
"dialogue-blocking-channel"))));
"dialogue-blocking-channel"))))::get;

public static Channel of(BlockingChannel blockingChannel) {
return of(blockingChannel, blockingExecutor.get());
Expand Down
2 changes: 0 additions & 2 deletions dialogue-clients/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ dependencies {

implementation project(':dialogue-apache-hc5-client')
implementation project(':dialogue-serde')
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'
Expand All @@ -21,7 +20,6 @@ dependencies {
implementation 'com.palantir.safe-logging:logger'
implementation 'com.palantir.safe-logging:preconditions'
implementation 'com.palantir.safe-logging:safe-logging'
implementation 'com.palantir.tritium:tritium-caffeine'
implementation 'com.palantir.tritium:tritium-registry'
implementation 'com.palantir.tritium:tritium-metrics'
implementation 'io.dropwizard.metrics:metrics-core'
Expand Down
19 changes: 0 additions & 19 deletions dialogue-clients/metrics.md
Original file line number Diff line number Diff line change
Expand Up @@ -134,25 +134,6 @@ Instrumentation for the ROUND_ROBIN node selection strategy (currently implement
Metrics produced instrumented Jackson components.
- `json.parser.string.length` tagged `format` (histogram): Histogram describing the length of strings parsed from input.

## Tritium Caffeine

`com.palantir.tritium:tritium-caffeine`

### cache
Cache statistic metrics
- `cache.request` (meter): Count of cache requests
- `cache`
- `result` values (`hit`,`miss`)
- `cache.load` (timer): Count of successful cache loads
- `cache`
- `result` values (`success`,`failure`)
- `cache.eviction` tagged `cache`, `cause` (meter): Count of evicted entries
- `cache.eviction.weight` tagged `cache`, `cause` (meter): Count of evicted weights
- `cache.estimated.size` tagged `cache` (gauge): Approximate number of entries in this cache
- `cache.weighted.size` tagged `cache` (gauge): Approximate accumulated weight of entries in this cache
- `cache.maximum.size` tagged `cache` (gauge): Maximum number of cache entries cache can hold if limited
- `cache.stats.disabled` tagged `cache` (meter): Meter marked when `CaffeineCacheStats.registerCache` is called on a cache that does not record stats using `caffeineBuilder.recordStats()`.

## Tritium Metrics

`com.palantir.tritium:tritium-metrics`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
package com.palantir.dialogue.clients;

import com.codahale.metrics.Meter;
import com.github.benmanes.caffeine.cache.Cache;
import com.github.benmanes.caffeine.cache.Caffeine;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import com.google.common.collect.ImmutableSet;
import com.palantir.dialogue.clients.ClientDnsMetrics.Lookup_Result;
import com.palantir.dialogue.core.DialogueDnsResolver;
Expand All @@ -37,11 +37,12 @@ final class CachingFallbackDnsResolver implements DialogueDnsResolver {
private final Meter lookupFallback;
private final Meter lookupFailure;

@SuppressWarnings("checkstyle:IllegalType")
private final Cache<String, ImmutableSet<InetAddress>> fallbackCache;

CachingFallbackDnsResolver(DialogueDnsResolver delegate, TaggedMetricRegistry registry) {
this.delegate = delegate;
this.fallbackCache = Caffeine.newBuilder()
this.fallbackCache = CacheBuilder.newBuilder()
.maximumSize(1000)
.expireAfterWrite(Duration.ofMinutes(10))
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@

package com.palantir.dialogue.clients;

import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.LoadingCache;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.palantir.conjure.java.api.config.service.ServiceConfiguration;
import com.palantir.conjure.java.client.config.ClientConfiguration;
import com.palantir.dialogue.core.DialogueChannel;
Expand Down Expand Up @@ -69,11 +70,18 @@ final class ChannelCache {
*/
private final Map<String, ApacheCacheEntry> apacheCache = new ConcurrentHashMap<>();

private final LoadingCache<ChannelCacheKey, DialogueChannel> channelCache = Caffeine.newBuilder()
@SuppressWarnings("checkstyle:IllegalType")
private final LoadingCache<ChannelCacheKey, DialogueChannel> channelCache = CacheBuilder.newBuilder()
.maximumSize(MAX_CACHED_CHANNELS)
// Avoid holding onto old targets, which is now more common as we bind to resolved IP addresses
.weakValues()
.build(this::createNonLiveReloadingChannel);
.build(new CacheLoader<>() {
@Override
public DialogueChannel load(ChannelCacheKey key) {
return createNonLiveReloadingChannel(key);
}
});

private final int instanceNumber;

private ChannelCache() {
Expand Down Expand Up @@ -118,7 +126,7 @@ DialogueChannel getNonReloadingChannel(
@Safe String channelName,
Optional<OverrideHostIndex> overrideHostIndex) {
if (log.isWarnEnabled()) {
long estimatedSize = channelCache.estimatedSize();
long estimatedSize = channelCache.size();
if (estimatedSize >= MAX_CACHED_CHANNELS * 0.75) {
log.warn(
"channelCache nearing capacity - possible bug? {} {} {}",
Expand All @@ -128,7 +136,7 @@ DialogueChannel getNonReloadingChannel(
}
}

return channelCache.get(ImmutableChannelCacheKey.builder()
return channelCache.getUnchecked(ImmutableChannelCacheKey.builder()
.from(reloadingParams)
.blockingExecutor(reloadingParams.blockingExecutor())
.serviceConf(serviceConf)
Expand Down Expand Up @@ -249,7 +257,7 @@ public String toString() {
+ ", apacheCache.size=" + apacheCache.size()
// Channel names are safe-loggable
+ ", apacheCache=" + apacheCache.keySet()
+ ", channelCache.size=" + channelCache.estimatedSize() + "/" + MAX_CACHED_CHANNELS
+ ", channelCache.size=" + channelCache.size() + "/" + MAX_CACHED_CHANNELS
+ ", channelCache="
// Channel names are safe-loggable
+ channelCache.asMap().keySet().stream()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,11 @@

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.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.collect.Collections2;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
Expand All @@ -44,7 +45,6 @@
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;
Expand Down Expand Up @@ -88,20 +88,24 @@ final class DnsSupport {
.setNameFormat(SCHEDULER_NAME + "-%d")
.setDaemon(true)
.build(),
SCHEDULER_NAME)));
SCHEDULER_NAME)))::get;

/**
* 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")
Comment on lines -96 to -97
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've dropped this instrumentation since we don't have a 1-1 replacement using the same timeseries names for guava caches. If this unblocks android stuff and we move forward, we can look into adding some guava cache utility functionality.

.register(stats -> Caffeine.newBuilder()
.maximumWeight(100_000)
.<String, MaybeUri>weigher((key, _value) -> key.length())
.expireAfterAccess(Duration.ofMinutes(1))
.softValues()
.recordStats(stats)
.build(DnsSupport::tryParseUri));
@SuppressWarnings("checkstyle:IllegalType")
private static final LoadingCache<String, MaybeUri> uriCache = CacheBuilder.newBuilder()
.maximumWeight(100_000)
.<String, MaybeUri>weigher((key, _value) -> key.length())
.expireAfterAccess(Duration.ofMinutes(1))
.softValues()
.recordStats()
.build(new CacheLoader<>() {
@Override
public MaybeUri load(String key) {
return DnsSupport.tryParseUri(key);
}
});

@VisibleForTesting
static void invalidateCaches() {
Expand Down Expand Up @@ -223,7 +227,7 @@ static MaybeUri tryParseUri(@Unsafe String uriString) {
@Nullable
private static URI tryParseUri(TaggedMetricRegistry metrics, @Safe String serviceName, @Unsafe String uri) {
try {
MaybeUri maybeUri = uriCache.get(uri);
MaybeUri maybeUri = uriCache.getUnchecked(uri);
URI result = maybeUri.uriOrThrow();
if (result.getHost() == null) {
log.error(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ public <T> T sticky(Class<T> clientInterface) {

@Override
public StickyChannelSession session() {
Supplier<Channel> channelSupplier = Suppliers.memoize(this::getStickyChannel);
Supplier<Channel> channelSupplier = Suppliers.memoize(this::getStickyChannel)::get;
return new StickyChannelSession() {
@Override
public Channel getStickyChannel() {
Expand Down Expand Up @@ -578,7 +578,7 @@ private static final class LazilyMappedRefreshable<T, U> implements Supplier<U>
private final Supplier<Refreshable<U>> delegate;

LazilyMappedRefreshable(Refreshable<T> refreshable, Function<? super T, U> function) {
delegate = Suppliers.memoize(() -> refreshable.map(function));
delegate = Suppliers.memoize(() -> refreshable.map(function))::get;
}

@Override
Expand Down
1 change: 0 additions & 1 deletion dialogue-core/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ dependencies {
api project(':dialogue-target')
api 'com.palantir.conjure.java.runtime:client-config'
implementation project(':dialogue-futures')
implementation 'com.github.ben-manes.caffeine:caffeine'
implementation 'com.google.guava:guava'
implementation 'com.palantir.safe-logging:logger'
implementation 'com.palantir.safe-logging:preconditions'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

package com.palantir.dialogue.core;

import com.github.benmanes.caffeine.cache.Ticker;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Ticker;
import com.google.common.collect.ImmutableList;
import com.google.common.math.IntMath;
import com.google.common.util.concurrent.ListenableFuture;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
package com.palantir.dialogue.core;

import com.codahale.metrics.Meter;
import com.github.benmanes.caffeine.cache.Ticker;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Ticker;
import com.google.common.collect.ImmutableList;
import com.google.common.primitives.Ints;
import com.google.common.util.concurrent.FutureCallback;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ private BaseUrl(DefaultUrlBuilder builder) {
public URL render(Endpoint endpoint, Request request) {
DefaultUrlBuilder url = builder.newBuilder();
endpoint.renderPath(request.pathParameters(), url);
request.queryParams().forEach(url::queryParam);
request.queryParams().entries().forEach(e -> url.queryParam(e.getKey(), e.getValue()));
return url.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@

package com.palantir.dialogue.core;

import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.LoadingCache;
import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.util.concurrent.ListenableFuture;
import com.palantir.dialogue.Channel;
import com.palantir.dialogue.Endpoint;
Expand All @@ -27,14 +28,20 @@

final class ChannelToEndpointChannel implements Channel {

@SuppressWarnings("checkstyle:IllegalType")
private final LoadingCache<Endpoint, Channel> cache;

ChannelToEndpointChannel(Function<Endpoint, Channel> loader) {
this.cache = Caffeine.newBuilder().weakKeys().maximumSize(10_000).build(loader::apply);
this.cache = CacheBuilder.newBuilder().weakKeys().maximumSize(10_000).build(new CacheLoader<>() {
@Override
public Channel load(Endpoint key) {
return loader.apply(key);
}
});
}

@Override
public ListenableFuture<Response> execute(Endpoint endpoint, Request request) {
return cache.get(endpoint).execute(endpoint, request);
return cache.getUnchecked(endpoint).execute(endpoint, request);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

package com.palantir.dialogue.core;

import com.github.benmanes.caffeine.cache.Ticker;
import com.google.common.base.Ticker;
import com.palantir.conjure.java.client.config.ClientConfiguration;
import com.palantir.conjure.java.client.config.ClientConfiguration.ClientQoS;
import com.palantir.logsafe.DoNotLog;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@
package com.palantir.dialogue.core;

import com.codahale.metrics.Meter;
import com.github.benmanes.caffeine.cache.Cache;
import com.github.benmanes.caffeine.cache.Caffeine;
import com.google.common.base.Suppliers;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import com.google.common.util.concurrent.FutureCallback;
import com.google.common.util.concurrent.ListenableFuture;
import com.palantir.dialogue.Endpoint;
Expand Down Expand Up @@ -47,8 +47,10 @@
final class DeprecationWarningChannel implements EndpointChannel {
private static final SafeLogger log = SafeLoggerFactory.get(DeprecationWarningChannel.class);
// Static cache avoids poor interactions with the jaxrs shim and consumers which recreate clients too aggressively.
@SuppressWarnings("checkstyle:IllegalType")
private static final Cache<LoggingRateLimiterKey, Object> loggingRateLimiter =
Caffeine.newBuilder().expireAfterWrite(Duration.ofMinutes(1)).build();
CacheBuilder.newBuilder().expireAfterWrite(Duration.ofMinutes(1)).build();

private static final Object SENTINEL = new Object();

private final EndpointChannel delegate;
Expand Down Expand Up @@ -76,7 +78,7 @@ public ListenableFuture<Response> execute(Request request) {

private FutureCallback<Response> createCallback(String channelName, Endpoint endpoint) {
// lazily create meter metric name only if deprecated endpoint is accessed
Supplier<Meter> meterSupplier = Suppliers.memoize(() -> metrics.deprecations(endpoint.serviceName()));
Supplier<Meter> meterSupplier = Suppliers.memoize(() -> metrics.deprecations(endpoint.serviceName()))::get;
return DialogueFutures.onSuccess(response -> {
if (response == null) {
return;
Expand Down
Loading