Add limit on ClassLoader WeakReference cache #7698
Conversation
tylerbenson
force-pushed
the
tyler/weak-bounded
branch
2 times, most recently
from
cd13582
to
726fe30
Compare
## Problem Having many WeakReferences can cause problems in an application. To reduce the number, we have a cache lookup to allow us to reuse those WeakReference instances. This cache is currently unlimited in size, and only reduced by cleaning references to GC'd ClassLoaders. Most applications have a limited number of persistent ClassLoaders. Some applications generate them dynamically. This is where the cache can grow excessively. ## Solution Add a size limit to the cache so that less commonly used loaders are evicted. Note: This could cause a common entry to be evicted which may result in a less performant lookup (because identity comparison won't match for subsequently generated WeakReferences). This is considered an acceptable tradeoff to avoid excessive memory use.
These are optimizations where caching is just an optimization and invalidation can be recalculated. I didn't change the cache in `HelperInjector` as that seems to have more functionality around it.
tylerbenson
force-pushed
the
tyler/weak-bounded
branch
from
524eb08
to
ff7c102
Compare
|
@opentelemetrybot update |
|
I measured heap usage for Surprisingly the bound cache used in this pr does not result in smaller memory usage. As far as I can tell it is because 41,296
|
On a run without the javaagent or with my cache limit fixes, the assertion passes. When run on main, the test fails because the heap size difference is over 200MB.
tylerbenson
force-pushed
the
tyler/weak-bounded
branch
from
0e53cd8
to
25c69f4
Compare
|
@trask per your request, I created a test that runs with the full javaagent. When run on main the test fails, when run with this branch or without the javaagent the test passes. (Notice, I @laurit I'm not familiar with Liferay. How many classloaders would you say it generates/uses? If the classloader usage is spread out more it probably isn't as problematic. This PR is explicitly trying to handle dynamically generated classloaders. |
|
@tylerbenson Junit 5 has a different annotation for disabling tests, see https://junit.org/junit5/docs/current/user-guide/#writing-tests-disabling My understanding is that when using one shared map what increases when class loaders are added is the array backing that map. Creating class loaders will eventually exhaust the heap and get gcd. That will prevent that map from growing further. As class loaders are much larger than what is in that map I doubt that you can observe that map growing too large even if you don't explicitly limit its size. |
See #7698 This is an attempt to reduce memory usage for `ClassLoaderHasClassesNamedMatcher`. Instead of having each matcher keep a `Map<ClassLoader, Boolean>` we can have one `Map<ClassLoader, BitSet>` where each matcher uses one bit in the `BitSet`. Alternatively `Map<ClassLoader, Set<ClassLoaderHasClassesNamedMatcher>>` where set contains matchers that match for given class loader would also work well because these matchers usually don't match so we can expect to have only a few elements in the set.
# Conflicts: # javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/matcher/ClassLoaderHasClassesNamedMatcher.java
|
@laurit Thanks for the pointer about The reason that tracking and keeping |
|
@laurit I ran the included test on |
| @@ -39,7 +39,7 @@ class MuzzleMatcher implements AgentBuilder.RawMatcher { | |||
| private final InstrumentationModule instrumentationModule; | |||
| private final Level muzzleLogLevel; | |||
| private final AtomicBoolean initialized = new AtomicBoolean(false); | |||
| private final Cache<ClassLoader, Boolean> matchCache = Cache.weak(); | |||
| private final Cache<ClassLoader, Boolean> matchCache = Cache.weakBounded(25); | |||
There was a problem hiding this comment.
@laurit do you think your change should be applied to this matcher too?
Problem
Having many WeakReferences can cause problems in an application. To reduce the number, we have a cache lookup to allow us to reuse those WeakReference instances. This cache is currently unlimited in size, and only reduced by cleaning references to GC'd ClassLoaders. Most applications have a limited number of persistent ClassLoaders. Some applications generate them dynamically. This is where the cache can grow excessively.
Solution
Add a size limit to the cache so that less commonly used loaders are evicted.
Note: This could cause a common entry to be evicted which may result in a less performant lookup (because identity comparison won't match for subsequently generated WeakReferences). This is considered an acceptable tradeoff to avoid excessive memory use.
Related: #7678