[#10858] Add InterceptorHolder instead of InterceptorRegistry

pinpoint-apm · Apr 9, 2024 · 86f8f20 · 86f8f20
1 parent 244286c
commit 86f8f20
Show file tree

Hide file tree

Showing 32 changed files with 1,083 additions and 296 deletions.
diff --git a/agent-module/agent/src/main/resources/profiles/local/pinpoint.config b/agent-module/agent/src/main/resources/profiles/local/pinpoint.config
@@ -32,6 +32,9 @@ profiler.application.namespace=
 
 profiler.interceptorregistry.size=65536
 
+# InterceptorHolder improves transform performance.
+profiler.interceptor.holder.enable=true
+
 # Manually override jvm vendor name (Oracle, IBM, OpenJDK, etc)
 # You probably won't ever need to set this value.
 profiler.jvm.vendor.name=

diff --git a/agent-module/agent/src/main/resources/profiles/release/pinpoint.config b/agent-module/agent/src/main/resources/profiles/release/pinpoint.config
@@ -31,6 +31,9 @@ profiler.application.namespace=
 
 profiler.interceptorregistry.size=8192
 
+# InterceptorHolder improves transform performance.
+profiler.interceptor.holder.enable=true
+
 # Manually override jvm vendor name (Oracle, IBM, OpenJDK, etc)
 # You probably won't ever need to set this value.
 profiler.jvm.vendor.name=

diff --git a/.../navercorp/pinpoint/bootstrap/interceptor/registry/DefaultInterceptorRegistryAdaptor.java b/.../navercorp/pinpoint/bootstrap/interceptor/registry/DefaultInterceptorRegistryAdaptor.java
@@ -30,21 +30,29 @@ public DefaultInterceptorRegistryAdaptor(int maxRegistrySize) {
         this.index = new WeakAtomicReferenceArray<Interceptor>(maxRegistrySize, Interceptor.class);
     }
 
-
     @Override
     public int addInterceptor(Interceptor interceptor) {
         if (interceptor == null) {
             return -1;
         }
 
-        final int newId = nextId();
-        if (newId >= registrySize) {
-            throw new IndexOutOfBoundsException("Interceptor registry size exceeded. Check the \"profiler.interceptorregistry.size\" setting. size=" + index.length() + " id=" + id);
-        }
+        final int newId = checkMaxSize(nextId());
         index.set(newId, interceptor);
         return newId;
     }
 
+    @Override
+    public int addInterceptor() {
+        return checkMaxSize(nextId());
+    }
+
+    private int checkMaxSize(int id) {
+        if (id >= registrySize) {
+            throw new IndexOutOfBoundsException("Interceptor registry size exceeded. Check the \"profiler.interceptorregistry.size\" setting. size=" + index.length() + " id=" + id);
+        }
+        return id;
+    }
+
     private int nextId() {
         return id.getAndIncrement();
     }

diff --git a/...main/java/com/navercorp/pinpoint/bootstrap/interceptor/registry/EmptyRegistryAdaptor.java b/...main/java/com/navercorp/pinpoint/bootstrap/interceptor/registry/EmptyRegistryAdaptor.java
@@ -32,6 +32,11 @@ public EmptyRegistryAdaptor() {
     }
 
 
+    @Override
+    public int addInterceptor() {
+        return 0;
+    }
+
     @Override
     public int addInterceptor(Interceptor interceptor) {
         return -1;

diff --git a/.../main/java/com/navercorp/pinpoint/bootstrap/interceptor/registry/InterceptorRegistry.java b/.../main/java/com/navercorp/pinpoint/bootstrap/interceptor/registry/InterceptorRegistry.java
@@ -32,6 +32,8 @@ public final class InterceptorRegistry {
     private static final AtomicReference<InterceptorRegistryAdaptor> REGISTRY =
             new AtomicReference<>(EmptyRegistryAdaptor.EMPTY);
 
+    private static boolean interceptorHolderEnable = true;
+
     public static void bind(final InterceptorRegistryAdaptor interceptorRegistryAdaptor, final Object lock) {
         Objects.requireNonNull(interceptorRegistryAdaptor, "interceptorRegistryAdaptor");
 
@@ -53,4 +55,12 @@ public static void unbind(final Object lock) {
     public static Interceptor getInterceptor(int key) {
         return REGISTRY.get().getInterceptor(key);
     }
+
+    public static void setInterceptorHolderEnable(boolean enable) {
+        interceptorHolderEnable = enable;
+    }
+
+    public static boolean isInterceptorHolderEnable() {
+        return interceptorHolderEnable;
+    }
 }
diff --git a/...ava/com/navercorp/pinpoint/bootstrap/interceptor/registry/InterceptorRegistryAdaptor.java b/...ava/com/navercorp/pinpoint/bootstrap/interceptor/registry/InterceptorRegistryAdaptor.java
@@ -7,7 +7,10 @@
  * @author emeroad
  */
 public interface InterceptorRegistryAdaptor {
+    int addInterceptor();
+
     int addInterceptor(Interceptor interceptor);
+
     Interceptor getInterceptor(int key);
 
     void clear();

diff --git a/...a/com/navercorp/pinpoint/bootstrap/interceptor/DefaultInterceptorRegistryAdaptorTest.java b/...a/com/navercorp/pinpoint/bootstrap/interceptor/DefaultInterceptorRegistryAdaptorTest.java
@@ -58,4 +58,10 @@ public void addSimpleInterceptor() {
 
         Assertions.assertSame(mock, find);
     }
+
+    @Test
+    public void addInterceptor() {
+        InterceptorRegistryAdaptor registry = new DefaultInterceptorRegistryAdaptor();
+        int key = registry.addInterceptor();
+    }
 }
diff --git a/...l-jdk8/src/test/java/com/navercorp/test/pinpoint/jdk8/interfaces/MethodInterfaceTest.java b/...l-jdk8/src/test/java/com/navercorp/test/pinpoint/jdk8/interfaces/MethodInterfaceTest.java
@@ -17,22 +17,21 @@
 package com.navercorp.test.pinpoint.jdk8.interfaces;
 
 import com.navercorp.pinpoint.bootstrap.instrument.InstrumentContext;
+import com.navercorp.pinpoint.bootstrap.instrument.InstrumentException;
+import com.navercorp.pinpoint.bootstrap.interceptor.Interceptor;
 import com.navercorp.pinpoint.profiler.instrument.ASMClass;
 import com.navercorp.pinpoint.profiler.instrument.ASMClassNodeAdapter;
 import com.navercorp.pinpoint.profiler.instrument.ASMClassWriter;
 import com.navercorp.pinpoint.profiler.instrument.ASMFieldNodeAdapter;
+import com.navercorp.pinpoint.profiler.instrument.ASMInterceptorHolder;
 import com.navercorp.pinpoint.profiler.instrument.ASMMethodNodeAdapter;
 import com.navercorp.pinpoint.profiler.instrument.EngineComponent;
 import com.navercorp.pinpoint.profiler.instrument.interceptor.InterceptorDefinition;
 import com.navercorp.pinpoint.profiler.instrument.interceptor.InterceptorDefinitionFactory;
-import com.navercorp.pinpoint.profiler.interceptor.registry.DefaultInterceptorRegistryBinder;
-import com.navercorp.pinpoint.profiler.interceptor.registry.InterceptorRegistryBinder;
 import com.navercorp.pinpoint.profiler.util.JavaAssistUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
-import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.Assertions;
-import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 import org.objectweb.asm.ClassReader;
 import org.objectweb.asm.ClassWriter;
@@ -47,6 +46,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.mock;
@@ -55,28 +55,18 @@
  * @author jaehong.kim
  */
 public class MethodInterfaceTest {
-    private final static InterceptorRegistryBinder interceptorRegistryBinder = new DefaultInterceptorRegistryBinder();
     private final static InstrumentContext pluginContext = mock(InstrumentContext.class);
+    private AtomicInteger interceptorIdCounter = new AtomicInteger();
 
     private final Logger logger = LogManager.getLogger(this.getClass());
 
-    @BeforeAll
-    public static void beforeClass() {
-        interceptorRegistryBinder.bind();
-    }
-
-    @AfterAll
-    public static void afterClass() {
-        interceptorRegistryBinder.unbind();
-    }
-
     @Test
     public void addInterceptor() throws Exception {
         final String targetInterfaceName = "com.navercorp.test.pinpoint.jdk8.interfaces.MethodInterface";
         final String targetClassName = "com.navercorp.test.pinpoint.jdk8.interfaces.MethodInterfaceClass";
         logger.debug("Add interceptor interface={}, class={}", targetInterfaceName, targetClassName);
 
-        final int interceptorId = interceptorRegistryBinder.getInterceptorRegistryAdaptor().addInterceptor(new SimpleInterceptor());
+        final Interceptor interceptor = new SimpleInterceptor();
         final InterceptorDefinition interceptorDefinition = new InterceptorDefinitionFactory().createInterceptorDefinition(SimpleInterceptor.class);
         final List<String> methodNameList = Arrays.asList("currentTimeMillis", "foo");
         TestClassLoader classLoader = new TestClassLoader();
@@ -100,6 +90,13 @@ public void handle(ClassNode classNode) {
                     if (!methodNameList.contains(methodNode.name)) {
                         continue;
                     }
+                    int interceptorId = interceptorIdCounter.incrementAndGet();
+                    try {
+                        ASMInterceptorHolder.create(interceptorId, classLoader, interceptor);
+                    } catch (InstrumentException e) {
+                        throw new RuntimeException(e);
+                    }
+
                     methodNodeAdapter.addBeforeInterceptor(interceptorId, interceptorDefinition, 99);
                     logger.debug("Add before interceptor in method={}", methodNode.name);
                     methodNodeAdapter.addAfterInterceptor(interceptorId, interceptorDefinition, 99);

diff --git a/...a/com/navercorp/pinpoint/profiler/context/provider/InterceptorRegistryBinderProvider.java b/...a/com/navercorp/pinpoint/profiler/context/provider/InterceptorRegistryBinderProvider.java
@@ -28,22 +28,27 @@
 /**
  * @author Woonduk Kang(emeroad)
  */
-public class InterceptorRegistryBinderProvider implements Provider<InterceptorRegistryBinder>  {
+public class InterceptorRegistryBinderProvider implements Provider<InterceptorRegistryBinder> {
 
     private final InterceptorRegistryBinder interceptorRegistryBinder;
 
     @Inject
     public InterceptorRegistryBinderProvider(InstrumentConfig instrumentConfig) {
-        this(getInterceptorRegistrySize(instrumentConfig));
+        this(getInterceptorRegistrySize(instrumentConfig), getInterceptorHolderEnable(instrumentConfig));
     }
 
     private static int getInterceptorRegistrySize(InstrumentConfig instrumentConfig) {
         Objects.requireNonNull(instrumentConfig, "instrumentConfig");
         return instrumentConfig.getInterceptorRegistrySize();
     }
 
-    public InterceptorRegistryBinderProvider(int interceptorSize) {
-        this.interceptorRegistryBinder = new DefaultInterceptorRegistryBinder(interceptorSize);
+    private static boolean getInterceptorHolderEnable(InstrumentConfig instrumentConfig) {
+        Objects.requireNonNull(instrumentConfig, "instrumentConfig");
+        return instrumentConfig.getInterceptorHolderEnable();
+    }
+
+    public InterceptorRegistryBinderProvider(int interceptorSize, boolean interceptorHolderEnable) {
+        this.interceptorRegistryBinder = new DefaultInterceptorRegistryBinder(interceptorSize, interceptorHolderEnable);
     }
 
     @Override

diff --git a/agent-module/profiler/src/main/java/com/navercorp/pinpoint/profiler/instrument/ASMClass.java b/agent-module/profiler/src/main/java/com/navercorp/pinpoint/profiler/instrument/ASMClass.java
@@ -431,7 +431,7 @@ private int addInterceptor0(TargetFilter annotation, Class<? extends Interceptor
         Objects.requireNonNull(filterTypeName, "type of @TargetFilter");
 
         ObjectBinderFactory objectBinderFactory = engineComponent.getObjectBinderFactory();
-        final InterceptorArgumentProvider interceptorArgumentProvider = objectBinderFactory.newInterceptorArgumentProvider(this);
+        final InterceptorArgumentProvider interceptorArgumentProvider = objectBinderFactory.newInterceptorArgumentProvider();
         final AutoBindingObjectFactory filterFactory = objectBinderFactory.newAutoBindingObjectFactory(pluginContext, classNode.getClassLoader(), interceptorArgumentProvider);
         final ObjectFactory objectFactory = ObjectFactory.byConstructor(filterTypeName, (Object[]) annotation.constructorArguments());
         final MethodFilter filter = (MethodFilter) filterFactory.createInstance(objectFactory);