From 9d7682f7764e1176a0ee770214754e0051b5c546 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Fri, 21 Feb 2020 12:22:51 -0500 Subject: [PATCH 1/9] Fix hashcode calculation in TypeCacheKey Using potentially very large number for a mod is probably not very effective --- .../datadog/trace/agent/tooling/DDCachingPoolStrategy.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java index 1d4cccdb35..960feb0aa1 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java @@ -89,7 +89,7 @@ public class DDCachingPoolStrategy implements PoolStrategy { loaderRefCache.put(classLoader, loaderRef); } - int loaderHash = classLoader.hashCode(); + final int loaderHash = classLoader.hashCode(); return createCachingTypePool(loaderHash, loaderRef, classFileLocator); } @@ -140,7 +140,7 @@ public class DDCachingPoolStrategy implements PoolStrategy { this.loaderRef = loaderRef; this.className = className; - hashCode = (int) (31 * this.loaderHash) ^ className.hashCode(); + hashCode = 31 * this.loaderHash + className.hashCode(); } @Override From f736c425ffebef3cd02d25eb702108802120cfcf Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Fri, 21 Feb 2020 12:23:41 -0500 Subject: [PATCH 2/9] TypeCacheKey are different if hash codes are different And this is very easy to check --- .../agent/tooling/DDCachingPoolStrategy.java | 28 +++++++++++++------ 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java index 960feb0aa1..ca80c9711d 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java @@ -150,11 +150,15 @@ public class DDCachingPoolStrategy implements PoolStrategy { @Override public boolean equals(final Object obj) { - if (!(obj instanceof TypeCacheKey)) return false; + if (!(obj instanceof TypeCacheKey)) { + return false; + } - TypeCacheKey that = (TypeCacheKey) obj; + final TypeCacheKey that = (TypeCacheKey) obj; - if (loaderHash != that.loaderHash) return false; + if (hashCode != that.hashCode) { + return false; + } // Fastpath loaderRef equivalence -- works because of WeakReference cache used // Also covers the bootstrap null loaderRef case @@ -172,11 +176,15 @@ public class DDCachingPoolStrategy implements PoolStrategy { // In this case, it is fine because that means the ClassLoader is no // longer live, so the entries will never match anyway and will fall // out of the cache. - ClassLoader thisLoader = loaderRef.get(); - if (thisLoader == null) return false; + final ClassLoader thisLoader = loaderRef.get(); + if (thisLoader == null) { + return false; + } - ClassLoader thatLoader = that.loaderRef.get(); - if (thatLoader == null) return false; + final ClassLoader thatLoader = that.loaderRef.get(); + if (thatLoader == null) { + return false; + } return (thisLoader == thatLoader); } else { @@ -205,9 +213,11 @@ public class DDCachingPoolStrategy implements PoolStrategy { @Override public TypePool.Resolution find(final String className) { - TypePool.Resolution existingResolution = + final TypePool.Resolution existingResolution = sharedResolutionCache.getIfPresent(new TypeCacheKey(loaderHash, loaderRef, className)); - if (existingResolution != null) return existingResolution; + if (existingResolution != null) { + return existingResolution; + } if (OBJECT_NAME.equals(className)) { return OBJECT_RESOLUTION; From d4c6d86e6d74c575ed71433d9c955715266e01c6 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Fri, 21 Feb 2020 12:24:09 -0500 Subject: [PATCH 3/9] Do not use zero for hashcode Seems like this may have odd sideeffects --- .../java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java index ca80c9711d..7e8540cd66 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java @@ -42,7 +42,7 @@ public class DDCachingPoolStrategy implements PoolStrategy { static final int LOADER_CAPACITY = 64; static final int TYPE_CAPACITY = 64; - static final int BOOTSTRAP_HASH = 0; + static final int BOOTSTRAP_HASH = 7236344; // Just a random number /** * Cache of recent ClassLoader WeakReferences; used to... From 00c268e6d8acf642f0810f25a3d9e64b7f639543 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Fri, 21 Feb 2020 12:24:53 -0500 Subject: [PATCH 4/9] Add equivalence check to TypeCacheKey --- .../datadog/trace/agent/tooling/DDCachingPoolStrategy.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java index 7e8540cd66..a598eddbee 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/DDCachingPoolStrategy.java @@ -154,6 +154,10 @@ public class DDCachingPoolStrategy implements PoolStrategy { return false; } + if (this == obj) { + return true; + } + final TypeCacheKey that = (TypeCacheKey) obj; if (hashCode != that.hashCode) { From 31b5652d104545a44d781666c4ae07acc0f87f17 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 25 Feb 2020 10:19:47 +0100 Subject: [PATCH 5/9] Remove reference check from TypeCacheKey --- .../trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java index 759fee809a..4d83ccc6be 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java @@ -154,10 +154,6 @@ public class DDCachingPoolStrategy implements PoolStrategy { return false; } - if (this == obj) { - return true; - } - final TypeCacheKey that = (TypeCacheKey) obj; if (hashCode != that.hashCode) { From aefcc477cbb8058849e1f8234c77dfe9936964d3 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 25 Feb 2020 11:58:18 +0100 Subject: [PATCH 6/9] Simplify TypePoolCacheKey equals We have to check string equivalence regardless of classloader state --- .../tooling/bytebuddy/DDCachingPoolStrategy.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java index 4d83ccc6be..64ec4f8870 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java @@ -160,12 +160,13 @@ public class DDCachingPoolStrategy implements PoolStrategy { return false; } - // Fastpath loaderRef equivalence -- works because of WeakReference cache used - // Also covers the bootstrap null loaderRef case - if (loaderRef == that.loaderRef) { - // still need to check name - return className.equals(that.className); - } else if (className.equals(that.className)) { + if (className.equals(that.className)) { + // Fastpath loaderRef equivalence -- works because of WeakReference cache used + // Also covers the bootstrap null loaderRef case + if (loaderRef == that.loaderRef) { + return true; + } + // need to perform a deeper loader check -- requires calling Reference.get // which can strengthen the Reference, so deliberately done last From 50793e524485e6a3b48136d3f4f4e852e76236b9 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 25 Feb 2020 14:32:35 +0100 Subject: [PATCH 7/9] Make sure that same classloaders get same weak ref --- .../tooling/bytebuddy/DDCachingPoolStrategy.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java index 64ec4f8870..94cde8d482 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java @@ -82,11 +82,14 @@ public class DDCachingPoolStrategy implements PoolStrategy { return createCachingTypePool(bootstrapCacheProvider, classFileLocator); } - WeakReference loaderRef = loaderRefCache.getIfPresent(classLoader); + WeakReference loaderRef; + synchronized (loaderRefCache) { + loaderRef = loaderRefCache.getIfPresent(classLoader); - if (loaderRef == null) { - loaderRef = new WeakReference<>(classLoader); - loaderRefCache.put(classLoader, loaderRef); + if (loaderRef == null) { + loaderRef = new WeakReference<>(classLoader); + loaderRefCache.put(classLoader, loaderRef); + } } final int loaderHash = classLoader.hashCode(); From 82dd2aa1b3970695901e72fb8eed7199179ed882 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 25 Feb 2020 21:47:42 +0100 Subject: [PATCH 8/9] Revert "Make sure that same classloaders get same weak ref" This reverts commit 50793e524485e6a3b48136d3f4f4e852e76236b9. --- .../tooling/bytebuddy/DDCachingPoolStrategy.java | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java index 94cde8d482..64ec4f8870 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java @@ -82,14 +82,11 @@ public class DDCachingPoolStrategy implements PoolStrategy { return createCachingTypePool(bootstrapCacheProvider, classFileLocator); } - WeakReference loaderRef; - synchronized (loaderRefCache) { - loaderRef = loaderRefCache.getIfPresent(classLoader); + WeakReference loaderRef = loaderRefCache.getIfPresent(classLoader); - if (loaderRef == null) { - loaderRef = new WeakReference<>(classLoader); - loaderRefCache.put(classLoader, loaderRef); - } + if (loaderRef == null) { + loaderRef = new WeakReference<>(classLoader); + loaderRefCache.put(classLoader, loaderRef); } final int loaderHash = classLoader.hashCode(); From 96f74d0fef36dd689ae4dd745bdf3b3be6a1dc60 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 25 Feb 2020 21:53:12 +0100 Subject: [PATCH 9/9] Compare loader hashes --- .../trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java index 64ec4f8870..9040089283 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/DDCachingPoolStrategy.java @@ -156,7 +156,7 @@ public class DDCachingPoolStrategy implements PoolStrategy { final TypeCacheKey that = (TypeCacheKey) obj; - if (hashCode != that.hashCode) { + if (loaderHash != that.loaderHash) { return false; }