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

Reduce memory usage for ClassLoaderHasClassesNamedMatcher #7866

Merged
merged 2 commits into from
Mar 17, 2023
Merged

Reduce memory usage for ClassLoaderHasClassesNamedMatcher #7866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb929f5
Reduce memory usage for ClassLoaderHasClassesNamedMatcher
laurit Feb 21, 2023
153b713
Disable matcher cache for running muzzle
laurit Feb 22, 2023
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
3 changes: 0 additions & 3 deletions ...ugins/src/main/kotlin/io/opentelemetry/javaagent/muzzle/matcher/MuzzleGradlePluginUtil.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,6 @@ class MuzzleGradlePluginUtil {

val matcherClass = agentClassLoader.loadClass("io.opentelemetry.javaagent.tooling.muzzle.ClassLoaderMatcher")

// We cannot reference Mismatch class directly here, because we are loaded from a different
// class loader.

// We cannot reference Mismatch class directly here, because we are loaded from a different
// class loader.
val allMismatches = matcherClass
Expand Down
6 changes: 3 additions & 3 deletions ...ain/java/io/opentelemetry/javaagent/bootstrap/internal/ClassLoaderMatcherCacheHolder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,19 @@
public final class ClassLoaderMatcherCacheHolder {

@GuardedBy("allCaches")
private static final List<Cache<ClassLoader, Boolean>> allCaches = new ArrayList<>();
private static final List<Cache<ClassLoader, ?>> allCaches = new ArrayList<>();

private ClassLoaderMatcherCacheHolder() {}

public static void addCache(Cache<ClassLoader, Boolean> cache) {
public static void addCache(Cache<ClassLoader, ?> cache) {
synchronized (allCaches) {
allCaches.add(cache);
}
}

public static void invalidateAllCachesForClassLoader(ClassLoader loader) {
synchronized (allCaches) {
for (Cache<ClassLoader, Boolean> cache : allCaches) {
for (Cache<ClassLoader, ?> cache : allCaches) {
cache.remove(loader);
}
}
Expand Down
60 changes: 55 additions & 5 deletions .../java/io/opentelemetry/javaagent/extension/matcher/ClassLoaderHasClassesNamedMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,29 @@
import io.opentelemetry.instrumentation.api.internal.cache.Cache;
import io.opentelemetry.javaagent.bootstrap.internal.ClassLoaderMatcherCacheHolder;
import io.opentelemetry.javaagent.bootstrap.internal.InClassLoaderMatcher;
import java.util.BitSet;
import java.util.List;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.atomic.AtomicInteger;
import net.bytebuddy.matcher.ElementMatcher;

class ClassLoaderHasClassesNamedMatcher extends ElementMatcher.Junction.AbstractBase<ClassLoader> {

private final Cache<ClassLoader, Boolean> cache = Cache.weak();
// caching is disabled for build time muzzle checks
// this field is set via reflection from ClassLoaderMatcher
static boolean useCache = true;
private static final AtomicInteger counter = new AtomicInteger();

private final String[] resources;
private final int index = counter.getAndIncrement();
Copy link
Member

Choose a reason for hiding this comment

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

@laurit can I suggest you add a bit more documentation in this class? I think the interaction between the index and the bitset are interesting, but not obvious.


ClassLoaderHasClassesNamedMatcher(String... classNames) {
resources = classNames;
for (int i = 0; i < resources.length; i++) {
resources[i] = resources[i].replace(".", "/") + ".class";
}
ClassLoaderMatcherCacheHolder.addCache(cache);
if (useCache) {
Manager.INSTANCE.add(this);
}
}

@Override
Expand All @@ -30,10 +39,14 @@ public boolean matches(ClassLoader cl) {
// Can't match the bootstrap class loader.
return false;
}
return cache.computeIfAbsent(cl, this::hasResources);
if (useCache) {
return Manager.INSTANCE.match(this, cl);
} else {
return hasResources(cl, resources);
}
}

private boolean hasResources(ClassLoader cl) {
private static boolean hasResources(ClassLoader cl, String... resources) {
boolean priorValue = InClassLoaderMatcher.getAndSet(true);
try {
for (String resource : resources) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a bug in this for loop, if there is a single resource class that is not managed by the particular ClassLoader, then this will return marking that ClassLoader as never containing any of the instrumentation classes. It may be worth it to think about what if an instrumentation exposes two classes and one of those is managed by the particular ClassLoader, wouldn't you want to mark that ClassLoader as matching?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent is to match only when all the resources are present. See javadoc in

opentelemetry-java-instrumentation/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/matcher/AgentElementMatchers.java

Lines 83 to 94 in 4621e74

/**
* Matches a class loader that contains all classes that are passed as the {@code classNames}
* parameter. Does not match the bootstrap classpath. Don't use this matcher with classes expected
* to be on the bootstrap.
*
* <p>In the event no class names are passed at all, the matcher will always return {@code true}.
*
* @param classNames list of class names to match.
*/
public static ElementMatcher.Junction<ClassLoader> hasClassesNamed(String... classNames) {
return new ClassLoaderHasClassesNamedMatcher(classNames);
}

If you need to matched based on either class A or B being present you can write something like hasClassesNamed(A).or(hasClassesNamed(B)).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @laurit, the all-or-nothing matching makes sense

Expand All @@ -46,4 +59,41 @@ private boolean hasResources(ClassLoader cl) {
}
return true;
}

Copy link
Contributor

@robododge robododge Mar 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is a step in the right direction.

My interpretation: (Please confirm if I'm interpreting this correctly)
So the new Manger is holding a list of all matchers, which correspond to N instrumentations. For each matcher a BitSet of size M will eventually expand out. That is N bitsets of length M being created. Previously, N instrumentations were each creating M cache entries, one entry for every ClassLoader instance encountered. Each entry in the WeakCache creates a weak key, so previously the total weak keys created amounted to N x M. Now, the a single weak key is created for each instrumenation. This is a very good improvement. During working on PR #7710, new instances of the Janino classloader were created in a runaway fashion inside my legacy application, counting for millions of classloader-to-boolean weakkeys. Here, we will end up with just a fixed list of very long BitSets if a classloader storm happens.

My past situation I ended up with 5Mil weak keys and I had about 112 instrumentations active.

Previous
Trace 5Mil classloaders took 5Mil Boolean instances ~ 40MB x 112instumentations ~ 4.5GB (but my heap dumps only consumed 150MB since JDK 8 reuses Boolean instances)

Now
Track 5Mil ClassLoaders with BitSet will be 78KB per instrumentation ~ 78KB x 112 ~ 8Mb MUCH BETTER

(Based my BitSet calculation on internals of a bit set modeled with array of Long[]. 5Mil/64 = 78k Longs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion you analysis is flawed. It is based on the need to keep track of 5M class loaders, but that situation only arose when cache keys were not removed due to a bug. Given such bug the code in this pr will eventually also fail, it will just take a bit longer.

Here, we will end up with just a fixed list of very long BitSets if a classloader storm happens.

The size of the bit set is based on the count of the matchers not class loaders. This size is deterministic, I think it was a bit over 200. There is a bit set for each class loader.

Trace 5Mil classloaders took 5Mil Boolean instances ~ 40MB x 112instumentations ~ 4.5GB (but my heap dumps only consumed 150MB since JDK 8 reuses Boolean instances)

A new Boolean instance is created only when you call new Boolean(true). Boxing conversion calls Boolean valueOf(boolean b) (didn't verify this) which does not create new instances. I wouldn't expect these Boolean values to cause problems. My guess would be that a large consumer would be the array that backs the map. If you have 5M keys then the backing map would have 8M elements. And even large would be the map keys. Minimum size of a java object is like 16 bytes, with 5M keys that alone would be 80M. In my opinion it is better to build a small demo app that replicates the situation and let Eclipse memory analyzer compute the retained size.

private static class Manager {
private static final BitSet EMPTY = new BitSet(0);
static final Manager INSTANCE = new Manager();
private final List<ClassLoaderHasClassesNamedMatcher> matchers = new CopyOnWriteArrayList<>();
private final Cache<ClassLoader, BitSet> enabled = Cache.weak();
private volatile boolean matchCalled = false;

Manager() {
ClassLoaderMatcherCacheHolder.addCache(enabled);
}

void add(ClassLoaderHasClassesNamedMatcher matcher) {
if (matchCalled) {
throw new IllegalStateException("All matchers should be create before match is called");
}
matchers.add(matcher);
}

boolean match(ClassLoaderHasClassesNamedMatcher matcher, ClassLoader cl) {
matchCalled = true;
BitSet set = enabled.get(cl);
if (set == null) {
set = new BitSet(counter.get());
for (ClassLoaderHasClassesNamedMatcher m : matchers) {
if (hasResources(cl, m.resources)) {
set.set(m.index);
}
}
enabled.put(cl, set.isEmpty() ? EMPTY : set);
enabled.put(cl, set);
laurit marked this conversation as resolved.
Show resolved Hide resolved
laurit marked this conversation as resolved.
Show resolved Hide resolved
} else if (set.isEmpty()) {
return false;
}
return set.get(matcher.index);
}
}
}
17 changes: 17 additions & 0 deletions muzzle/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ClassLoaderMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
import io.opentelemetry.javaagent.tooling.HelperInjector;
import java.lang.reflect.Field;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -32,6 +33,8 @@ public class ClassLoaderMatcher {
*/
public static Map<String, List<Mismatch>> matchesAll(
ClassLoader classLoader, boolean injectHelpers, Set<String> excludedInstrumentationNames) {
disableMatcherCache();

Map<String, List<Mismatch>> result = new HashMap<>();
ServiceLoader.load(InstrumentationModule.class, ClassLoaderMatcher.class.getClassLoader())
.forEach(
Expand Down Expand Up @@ -101,5 +104,19 @@ private static List<Mismatch> checkHelperInjection(
return mismatches;
}

private static void disableMatcherCache() {
try {
Class<?> matcherClass =
Class.forName(
"io.opentelemetry.javaagent.extension.matcher.ClassLoaderHasClassesNamedMatcher");
Field field = matcherClass.getDeclaredField("useCache");
field.setAccessible(true);
field.setBoolean(null, false);
} catch (Exception exception) {
throw new IllegalStateException(
"Failed to disable cache for ClassLoaderHasClassesNamedMatcher", exception);
}
}

private ClassLoaderMatcher() {}
}