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.

Sign up for GitHub

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

Caffeine - Automatically register metrics cache impls if Micrometer is around #30826

Merged
merged 1 commit into from
Feb 4, 2023
Merged

Caffeine - Automatically register metrics cache impls if Micrometer is around #30826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 18 additions & 0 deletions ...s/caffeine/deployment/src/main/java/io/quarkus/caffeine/deployment/CaffeineProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Optional;

import org.jboss.jandex.ClassInfo;
import org.jboss.jandex.DotName;
Expand All @@ -12,8 +13,11 @@
import io.quarkus.deployment.annotations.BuildStep;
import io.quarkus.deployment.builditem.CombinedIndexBuildItem;
import io.quarkus.deployment.builditem.NativeImageFeatureBuildItem;
import io.quarkus.deployment.builditem.nativeimage.NativeImageSystemPropertyBuildItem;
import io.quarkus.deployment.builditem.nativeimage.ReflectiveClassBuildItem;
import io.quarkus.deployment.metrics.MetricsCapabilityBuildItem;
import io.quarkus.deployment.pkg.steps.NativeOrNativeSourcesBuild;
import io.quarkus.runtime.metrics.MetricsFactory;

public class CaffeineProcessor {

Expand Down Expand Up @@ -49,4 +53,18 @@ void cacheLoaders(CombinedIndexBuildItem combinedIndex, BuildProducer<Reflective
NativeImageFeatureBuildItem nativeImageFeature() {
return new NativeImageFeatureBuildItem(CacheConstructorsFeature.class);
}

@BuildStep(onlyIf = NativeOrNativeSourcesBuild.class)
NativeImageSystemPropertyBuildItem registerRecordStatsImplementationsIfMicrometerAround(
Optional<MetricsCapabilityBuildItem> metricsCapability) {
if (metricsCapability.isEmpty()) {
return null;
}
if (!metricsCapability.get().metricsSupported(MetricsFactory.MICROMETER)) {
return null;
}

return new NativeImageSystemPropertyBuildItem(CacheConstructorsFeature.REGISTER_RECORD_STATS_IMPLEMENTATIONS,
"true");
}
}
31 changes: 27 additions & 4 deletions ...ine/runtime/src/main/java/io/quarkus/caffeine/runtime/graal/CacheConstructorsFeature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@
*/
public class CacheConstructorsFeature implements Feature {

private final AtomicBoolean triggered = new AtomicBoolean(false);
public static final String REGISTER_RECORD_STATS_IMPLEMENTATIONS = "io.quarkus.caffeine.graalvm.recordStats";

/**
* To set this, add `-J-Dio.quarkus.caffeine.graalvm.diagnostics=true` to the native-image parameters
*/
private static final boolean log = Boolean.getBoolean("io.quarkus.caffeine.graalvm.diagnostics");

private final AtomicBoolean triggered = new AtomicBoolean(false);

@Override
public void beforeAnalysis(BeforeAnalysisAccess access) {
Class<?> caffeineCoreClazz = access.findClassByName("com.github.benmanes.caffeine.cache.Caffeine");
Expand All @@ -49,6 +51,12 @@ private void registerCaffeineReflections(DuringAnalysisAccess duringAnalysisAcce
for (String className : needsHavingSimpleConstructors) {
registerForReflection(className, duringAnalysisAccess);
}

if (Boolean.getBoolean(REGISTER_RECORD_STATS_IMPLEMENTATIONS)) {
for (String className : typesNeedingConstructorsRegisteredWhenRecordingStats()) {
registerForReflection(className, duringAnalysisAccess);
}
}
}

private void registerForReflection(
Expand All @@ -60,15 +68,18 @@ private void registerForReflection(
RuntimeReflection.register(z);
}

/**
* This list is not complete, but a selection of the types we expect being most useful.
* unfortunately registering all of them has been shown to have a very significant impact
* on executable sizes. See https://github.com/quarkusio/quarkus/issues/12961
*/
public static String[] typesNeedingConstructorsRegistered() {
return new String[] {
//N.B. this list is not complete, but a selection of the types we expect being most useful.
//unfortunately registering all of them has been shown to have a very significant impact
//on executable sizes. See https://github.com/quarkusio/quarkus/issues/12961
"com.github.benmanes.caffeine.cache.PDMS",
"com.github.benmanes.caffeine.cache.PSA",
"com.github.benmanes.caffeine.cache.PSMS",
"com.github.benmanes.caffeine.cache.PSW",
"com.github.benmanes.caffeine.cache.PSMW",
"com.github.benmanes.caffeine.cache.PSWMS",
"com.github.benmanes.caffeine.cache.PSWMW",
"com.github.benmanes.caffeine.cache.SILMS",
Expand All @@ -82,4 +93,16 @@ public static String[] typesNeedingConstructorsRegistered() {
};
}

public static String[] typesNeedingConstructorsRegisteredWhenRecordingStats() {
return new String[] {
"com.github.benmanes.caffeine.cache.SILSMS",
"com.github.benmanes.caffeine.cache.SSSA",
"com.github.benmanes.caffeine.cache.SSLSA",
"com.github.benmanes.caffeine.cache.SSLSMS",
"com.github.benmanes.caffeine.cache.SSSMS",
"com.github.benmanes.caffeine.cache.SSSMSA",
"com.github.benmanes.caffeine.cache.SSSMSW",
"com.github.benmanes.caffeine.cache.SSSW"
Comment on lines +98 to +105
Copy link
Member

Choose a reason for hiding this comment

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

@ben-manes Could you please confirm that this list contains all caches that should be registered for reflection when the cache metrics are enabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW, it doesn't contain all of them. But I think it contains all the ones corresponding to the ones we already have.

Copy link
Member

Choose a reason for hiding this comment

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

How did you determine which ones we need?

Copy link

Choose a reason for hiding this comment

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

If possible, you might write a unit test to assert that the bundled configurations work in native image. I don't know the class mappings offhand as it is done by logic and a test would avoid regressions if the listing became stale.

I use Guava's Sets.cartesianProduct(features) which has a 2^n growth rate, so each new feature might double the class listings. In reality that is pruned down by reusing classes if they match the required data shape, e.g. an entry with key/value/timestamp can share a single class by either expireAfterAccess, expireAfterWrite, or variable expiration configurations. Caffeine's unit tests likewise use this cartesian product trick for parameterized tests to avoid regressions as features can interact in subtle ways.

};
}
}
2 changes: 2 additions & 0 deletions integration-tests/cache/src/main/resources/application.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ quarkus.hibernate-orm.sql-load-script=import.sql

# configure the caches
quarkus.cache.caffeine."forest".expire-after-write=10M

quarkus.cache.caffeine."expensiveResourceCache".expire-after-write=10M
quarkus.cache.caffeine."expensiveResourceCache".metrics-enabled=true

io.quarkus.it.cache.SunriseRestClient/mp-rest/url=${test.url}