Improve bytebuddy class enhance for retransform classes #561

kylixs · 2023-06-23T08:44:25Z

Improve bytebuddy class enhance for retransform classes

If this is non-trivial feature, paste the links/URLs to the design doc.
Update the documentation to include this new feature.
Tests(including UT, IT, E2E) are added to verify the new feature.
If it's UI related, attach the screenshots below.
If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
Update the CHANGES log.

Add bytebuddy patch to support retransform class multiple times compatible with other javaagent without class cache.
Support for hotswap classes that are enhanced by skywalking plugins, like Spring MVC Controller, etc.

...ache/skywalking/apm/agent/core/plugin/interceptor/enhance/v2/ClassEnhancePluginDefineV2.java

.../org/apache/skywalking/apm/agent/core/plugin/interceptor/enhance/DelegateNamingResolver.java

apm-sniffer/bytebuddy-patch/pom.xml

...ebuddy-patch/src/main/java/org/apache/skywalking/apm/agent/bytebuddy/SWClassFileLocator.java

...rc/main/java/org/apache/skywalking/apm/agent/bytebuddy/SWExtractionClassFileTransformer.java

...tch/src/test/java/org/apache/skywalking/apm/agent/bytebuddy/case1/AbstractInterceptTest.java

wu-sheng · 2023-06-23T10:20:06Z

test/plugin/scenarios/retransform-class-tomcat-scenario/configuration.yml

 dependencies:
-withPlugins: apm-spring-annotation-plugin-*.jar
+withPlugins: apm-*.jar


With all plugins? Are these necessary? I prefer the explicit names of plugins.

retransform class test scenario should test with more plugins, especially with the jdk plugins, test scenario is not enough currently, may add more cases later.

We should separate them, otherwise, it would be confusing when it fails.

I think it is ok to load all plugins in this test scenario. In order to ensure the retransform function, plugins that affect retransform need to be identified in time. Retransform class is not a standalone feature and in principle all plugins need to support it.

The point is not about these two plugin tests, it is about the stability of the testing. The plugins are going to be added/removed from time to time, if you copy all, actually, you don't know which plugins exist, which may cause these tests to fail by other contributions.

I think we don't cover multiple plugins for one method case, how could that happens?

We now support multiple plugins enhancing the same class, although enhancing different methods, but if the same method/field name is generated, there will be an error.

And even yoi copied all, it doesn't verify this, because many plugins are not activated(not used) in your scenarios.

Yes, and we may need more testcases.

We now support multiple plugins enhancing the same class, although enhancing different methods, but if the same method/field name is generated, there will be an error.

Yes, but that could be error when the plugin tests run too. What your new cases cover is only re-transfer. They are verifying for different cases.

It's not that simple. Different plugin enhancers of the same class may affect each other during re-transforming.
Leave this aside for now and add test cases later if incompatibilities arise.

I think we don't cover multiple plugins for one method case, how could that happens?

We now support multiple plugins enhancing the same class, although enhancing different methods, but if the same method/field name is generated, there will be an error.

And even yoi copied all, it doesn't verify this, because many plugins are not activated(not used) in your scenarios.

Yes, and we may need more testcases.

@wu-sheng @kylixs Hello，I wonder if multiple plugins enhancing the same class has been supported?

No, it isn't. Also, because we added a new field to the target class, I don't recommend to supporting that.

wu-sheng · 2023-06-23T10:24:52Z

Could you write pseudocode about the new generated class? Including new field names and types for constructor, instance methods and static methods.

kylixs · 2023-06-23T11:39:58Z

Could you write pseudocode about the new generated class? Including new field names and types for constructor, instance methods and static methods.

SWAuxiliaryTypeNamingStrategy
Auxiliary type name pattern: <origin_class_name>$<name_trait>$auxiliary$<auxiliary_type_instance_hash>
DelegateNamingResolver
Interceptor delegate field name pattern: <name_trait>$delegate$<class_name_hash>$<plugin_define_hash>$<intercept_point_hash>
SWMethodNameTransformer
Renamed origin method pattern: <name_trait>$original$<method_name>$<method_description_hash>
SWImplementationContextFactory
Method cache value field pattern: cachedValue$<name_trait>$<origin_class_name_hash>$<field_value_hash>
Accessor method name pattern: <renamed_origin_method>$accessor$<name_trait>$<origin_class_name_hash>

 import sample.mybatis.controller.HotelController$sw$auxiliary$19cja42;
 import sample.mybatis.controller.HotelController$sw$auxiliary$p257su0;
 import sample.mybatis.domain.Hotel;
 import sample.mybatis.service.HotelService;

 @RequestMapping(value={"/hotel"})
 @RestController
 public class HotelController
 implements EnhancedInstance {
     @Autowired
     @Lazy
     private HotelService hotelService;
     private volatile Object _$EnhancedClassField_ws;

     // Interceptor delegate fields
     public static volatile /* synthetic */ InstMethodsInter sw$delegate$td03673$ain2do0$8im5jm1;
     public static volatile /* synthetic */ InstMethodsInter sw$delegate$td03673$ain2do0$edkmf61;
     public static volatile /* synthetic */ ConstructorInter sw$delegate$td03673$ain2do0$qs9unv1;
     public static volatile /* synthetic */ InstMethodsInter sw$delegate$td03673$fl4lnk1$m3ia3a2;
     public static volatile /* synthetic */ InstMethodsInter sw$delegate$td03673$fl4lnk1$sufrvp1;
     public static volatile /* synthetic */ ConstructorInter sw$delegate$td03673$fl4lnk1$cteu7s1;

     // Origin method cache value field
     private static final /* synthetic */ Method cachedValue$sw$td03673$g5sobj1;

     public HotelController() {
         this(null);
         sw$delegate$td03673$ain2do0$qs9unv1.intercept(this, new Object[0]);
     }

     private /* synthetic */ HotelController(sw.auxiliary.p257su0 p257su02) {
     }

     @GetMapping(value={"city/{cityId}"})
     public Hotel selectByCityId(@PathVariable(value="cityId") int n) {
         // call interceptor with auxiliary type and parameters and origin method object
         return (Hotel)sw$delegate$td03673$ain2do0$8im5jm1.intercept(this, new Object[]{n}, new HotelController$sw$auxiliary$19cja42(this, n), cachedValue$sw$td03673$g5sobj1);
     }

     // Renamed origin method
     private /* synthetic */ Hotel sw$origin$selectByCityId$a8458p3(int cityId) {
/*22*/         return this.hotelService.selectByCityId(cityId);
     }

     // Accessor of renamed origin method, calling from auxiliary type
     final /* synthetic */ Hotel sw$origin$selectByCityId$a8458p3$accessor$sw$td03673(int n) {
         // Calling renamed origin method
         return this.sw$origin$selectByCityId$a8458p3(n);
     }

     @Override
     public Object getSkyWalkingDynamicField() {
         return this._$EnhancedClassField_ws;
     }

     @Override
     public void setSkyWalkingDynamicField(Object object) {
         this._$EnhancedClassField_ws = object;
     }

     static {
         ClassLoader.getSystemClassLoader().loadClass("org.apache.skywalking.apm.dependencies.net.bytebuddy.dynamic.Nexus").getMethod("initialize", Class.class, Integer.TYPE).invoke(null, HotelController.class, -1072476370);
         // Method object
         cachedValue$sw$td03673$g5sobj1 = HotelController.class.getMethod("selectByCityId", Integer.TYPE);
     }
 }

Auxiliary type of Constructor :

class HotelController$sw$auxiliary$p257su0 {
}

Auxiliary type of selectByCityId method:

class HotelController$sw$auxiliary$19cja42
implements Runnable,
Callable {
    private HotelController argument0;
    private int argument1;

    public Object call() throws Exception {
        return this.argument0.sw$origin$selectByCityId$a8458p3$accessor$sw$td03673(this.argument1);
    }

    @Override
    public void run() {
        this.argument0.sw$origin$selectByCityId$a8458p3$accessor$sw$td03673(this.argument1);
    }

    HotelController$sw$auxiliary$19cja42(HotelController hotelController, int n) {
        this.argument0 = hotelController;
        this.argument1 = n;
    }
}

.../org/apache/skywalking/apm/agent/core/plugin/interceptor/enhance/DelegateNamingResolver.java

apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java

...ebuddy-patch/src/test/java/org/apache/skywalking/apm/agent/bytebuddy/SWClassFileLocator.java

...y-patch/src/main/java/org/apache/skywalking/apm/agent/bytebuddy/SWMethodNameTransformer.java

...tebuddy-patch/src/test/java/org/apache/skywalking/apm/agent/bytebuddy/ConstructorAdvice.java

wu-sheng · 2023-06-24T05:48:03Z

I think you misunderstand my point, and did it in the exactly opposite direction of my asking.
Could you recheck #561 (comment)?

kylixs · 2023-06-24T05:53:05Z

The key point is, InterceptPoint doesn't include anything really about intercepting.

Sometimes an interface is used to identify a certain type of objects, such as java.io.Serializable , java.lang.Cloneable, java.util.EventListener

kylixs · 2023-06-24T05:58:32Z

I asked for DelegateNamingResolver having multiple resolve overloads for all interceptor point type, rather than a new InterceptPoint interface.

I don't quite understand this. Can you give me an example? How do you override a method without an interface or a parent class?

This reverts commit 64b2916.

wu-sheng · 2023-06-24T06:01:52Z

Remove InterceptPoint
DelegateNamingResolver has methods for resolve#ConstructorInterceptV2Point, resolve#ConstructorInterceptPoint, resolve#InstanceMethodsInterceptV2Point, etc.(6 methods)
computeHashCode stays as default method in ConstructorInterceptV2Point, ConstructorInterceptPoint, etc.

wu-sheng · 2023-06-24T06:17:46Z

...ebuddy-patch/src/test/java/org/apache/skywalking/apm/agent/bytebuddy/SWClassFileLocator.java

+    public int getTimeoutSeconds() {
+        return timeoutSeconds;
+    }
+
+    public void setTimeoutSeconds(int timeoutSeconds) {
+        this.timeoutSeconds = timeoutSeconds;
+    }


Unused methods.

It's used to config resolve timeout, not every public method need a usage, might need it somewhere later.

This is only a test code, I think usually manual hard code is good enough?

...ebuddy-patch/src/test/java/org/apache/skywalking/apm/agent/bytebuddy/SWClassFileLocator.java

wu-sheng · 2023-06-24T06:41:48Z

Generally, this PR should be good enough. Update change logs about the new naming policy of code generation

After this PR is merged, I think you should set another PR to update the version to 9.0.0-SNAPSHOT, we are going to bump up the version number as new kernel relative codes.

Update main compatibility doc, https://skywalking.apache.org/docs/main/next/en/setup/service-agent/agent-compatibility/ > = 8.0 & > = 9.0

kylixs · 2023-06-24T06:52:14Z

To ensure that the hashCode for recreating the XxxInterceptPoint instance is the same as the previous instance, each ElementMatcher implementation class needs to implement toString(). Where should I put this hint? There seems to be no way to guarantee it.

wu-sheng · 2023-06-24T07:07:37Z

Do you want a consistent and certain hashcode for every method?

kylixs · 2023-06-24T07:12:50Z

Do you want a consistent and certain hashcode for every method?

Yes, should return a consistent and certain hashcode from DelegateNamingResolver#resolve(InterceptPoint) for same plugin define on re-transforming.

wu-sheng · 2023-06-24T07:17:06Z

Isn't your current algorithm guaranteed? hashcode is not changed once the values of involved fields are not changed.
Could you explain your concern?
Are you concern about code changed after retransfer or conflicts among methods in a same class?

kylixs · 2023-06-24T07:20:25Z

The current code is fine, and both ByteBuddy and our custom ElementMatcher implement the toString() method. I worry that in the future, new ElementMatcher subclasses might not implement the toString() method, with no mandatory constraint guarantees. If the hashcode is inconsistent, the retransform will fail.

wu-sheng · 2023-06-24T07:46:53Z

That should be able to check through UT.
Are you ware of OAP class scan? You could check whether the scaned matchers implement/override(declared) the expected methods.

kylixs · 2023-06-24T07:51:59Z

Any doc/example about OAP class scan?

wu-sheng · 2023-06-24T08:07:44Z

https://github.com/apache/skywalking/blob/master/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/annotation/AnnotationScan.java

Ref this, this is an annotation scan. You could easily do an inheritance scan(matcher implementations, and/or other class type implementations).

wu-sheng

LGTM. Thanks for making this ready fast. Please update the version in another PR.

wu-sheng · 2023-06-24T13:02:10Z

@kylixs As you have been involved into core mechanism of the agent, and did significant contributions, I am willing to invite you join the committer team. If you want to, please ping me on the slack

kylixs · 2023-06-24T13:29:57Z

That's good. I'm interested in that.

@RequestMapping

* SWAuxiliaryTypeNamingStrategy Auxiliary type name pattern: <origin_class_name>$<name_trait>$auxiliary$<auxiliary_type_instance_hash> * DelegateNamingResolver Interceptor delegate field name pattern: <name_trait>$delegate$<class_name_hash>$<plugin_define_hash>$<intercept_point_hash> * SWMethodNameTransformer Renamed origin method pattern: <name_trait>$original$<method_name>$<method_description_hash> * SWImplementationContextFactory Method cache value field pattern: cachedValue$<name_trait>$<origin_class_name_hash>$<field_value_hash> Accessor method name pattern: <renamed_origin_method>$accessor$<name_trait>$<origin_class_name_hash> Here is an example of manipulated enhanced class with new naming policies of auxiliary classes, fields, and methods ```java import sample.mybatis.controller.HotelController$sw$auxiliary$19cja42; import sample.mybatis.controller.HotelController$sw$auxiliary$p257su0; import sample.mybatis.domain.Hotel; import sample.mybatis.service.HotelService; @RequestMapping(value={"/hotel"}) @RestController public class HotelController implements EnhancedInstance { @Autowired @lazy private HotelService hotelService; private volatile Object _$EnhancedClassField_ws; // Interceptor delegate fields public static volatile /* synthetic */ InstMethodsInter sw$delegate$td03673$ain2do0$8im5jm1; public static volatile /* synthetic */ InstMethodsInter sw$delegate$td03673$ain2do0$edkmf61; public static volatile /* synthetic */ ConstructorInter sw$delegate$td03673$ain2do0$qs9unv1; public static volatile /* synthetic */ InstMethodsInter sw$delegate$td03673$fl4lnk1$m3ia3a2; public static volatile /* synthetic */ InstMethodsInter sw$delegate$td03673$fl4lnk1$sufrvp1; public static volatile /* synthetic */ ConstructorInter sw$delegate$td03673$fl4lnk1$cteu7s1; // Origin method cache value field private static final /* synthetic */ Method cachedValue$sw$td03673$g5sobj1; public HotelController() { this(null); sw$delegate$td03673$ain2do0$qs9unv1.intercept(this, new Object[0]); } private /* synthetic */ HotelController(sw.auxiliary.p257su0 p257su02) { } @GetMapping(value={"city/{cityId}"}) public Hotel selectByCityId(@PathVariable(value="cityId") int n) { // call interceptor with auxiliary type and parameters and origin method object return (Hotel)sw$delegate$td03673$ain2do0$8im5jm1.intercept(this, new Object[]{n}, new HotelController$sw$auxiliary$19cja42(this, n), cachedValue$sw$td03673$g5sobj1); } // Renamed origin method private /* synthetic */ Hotel sw$origin$selectByCityId$a8458p3(int cityId) { /*22*/ return this.hotelService.selectByCityId(cityId); } // Accessor of renamed origin method, calling from auxiliary type final /* synthetic */ Hotel sw$origin$selectByCityId$a8458p3$accessor$sw$td03673(int n) { // Calling renamed origin method return this.sw$origin$selectByCityId$a8458p3(n); } @OverRide public Object getSkyWalkingDynamicField() { return this._$EnhancedClassField_ws; } @OverRide public void setSkyWalkingDynamicField(Object object) { this._$EnhancedClassField_ws = object; } static { ClassLoader.getSystemClassLoader().loadClass("org.apache.skywalking.apm.dependencies.net.bytebuddy.dynamic.Nexus").getMethod("initialize", Class.class, Integer.TYPE).invoke(null, HotelController.class, -1072476370); // Method object cachedValue$sw$td03673$g5sobj1 = HotelController.class.getMethod("selectByCityId", Integer.TYPE); } } ``` Auxiliary type of Constructor : ```java class HotelController$sw$auxiliary$p257su0 { } ``` Auxiliary type of `selectByCityId` method: ```java class HotelController$sw$auxiliary$19cja42 implements Runnable, Callable { private HotelController argument0; private int argument1; public Object call() throws Exception { return this.argument0.sw$origin$selectByCityId$a8458p3$accessor$sw$td03673(this.argument1); } @OverRide public void run() { this.argument0.sw$origin$selectByCityId$a8458p3$accessor$sw$td03673(this.argument1); } HotelController$sw$auxiliary$19cja42(HotelController hotelController, int n) { this.argument0 = hotelController; this.argument1 = n; } } ```

Add bytebuddy-patch to support retransform classes with other agents

0554e12

kylixs mentioned this pull request Jun 23, 2023

Improve bytebuddy class enhance #521

Closed

5 tasks

kylixs added 2 commits June 23, 2023 17:08

Remove class cache relative codes

7b66985

Remove class cache relative codes 2

6528f3d

wu-sheng reviewed Jun 23, 2023

View reviewed changes

...ache/skywalking/apm/agent/core/plugin/interceptor/enhance/v2/ClassEnhancePluginDefineV2.java Outdated Show resolved Hide resolved

wu-sheng added enhancement New feature or request core labels Jun 23, 2023

wu-sheng reviewed Jun 23, 2023

View reviewed changes

.../org/apache/skywalking/apm/agent/core/plugin/interceptor/enhance/DelegateNamingResolver.java Outdated Show resolved Hide resolved

wu-sheng reviewed Jun 23, 2023

View reviewed changes

apm-sniffer/bytebuddy-patch/pom.xml Outdated Show resolved Hide resolved

wu-sheng reviewed Jun 23, 2023

View reviewed changes

apm-sniffer/bytebuddy-patch/pom.xml Outdated Show resolved Hide resolved

wu-sheng reviewed Jun 23, 2023

View reviewed changes

...ebuddy-patch/src/main/java/org/apache/skywalking/apm/agent/bytebuddy/SWClassFileLocator.java Outdated Show resolved Hide resolved

wu-sheng reviewed Jun 23, 2023

View reviewed changes

...ebuddy-patch/src/main/java/org/apache/skywalking/apm/agent/bytebuddy/SWClassFileLocator.java Outdated Show resolved Hide resolved

wu-sheng reviewed Jun 23, 2023

View reviewed changes

...rc/main/java/org/apache/skywalking/apm/agent/bytebuddy/SWExtractionClassFileTransformer.java Outdated Show resolved Hide resolved

wu-sheng reviewed Jun 23, 2023

View reviewed changes

...tch/src/test/java/org/apache/skywalking/apm/agent/bytebuddy/case1/AbstractInterceptTest.java Outdated Show resolved Hide resolved

wu-sheng reviewed Jun 23, 2023

View reviewed changes

Compute hashcode of intercept point

f57cf0f

kylixs added 3 commits June 23, 2023 18:46

polish test

66b0e41

polish code

7c3499c

polish code

ea829da

polish comments

1d4baeb

wu-sheng reviewed Jun 23, 2023

View reviewed changes

.../org/apache/skywalking/apm/agent/core/plugin/interceptor/enhance/DelegateNamingResolver.java Outdated Show resolved Hide resolved

move unused class

461ae30

wu-sheng reviewed Jun 23, 2023

View reviewed changes

apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java Outdated Show resolved Hide resolved

wu-sheng reviewed Jun 23, 2023

View reviewed changes

apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java Outdated Show resolved Hide resolved

wu-sheng reviewed Jun 23, 2023

View reviewed changes

...ebuddy-patch/src/test/java/org/apache/skywalking/apm/agent/bytebuddy/SWClassFileLocator.java Show resolved Hide resolved

move name trait to Constants

988ffe4

wu-sheng reviewed Jun 23, 2023

View reviewed changes

...y-patch/src/main/java/org/apache/skywalking/apm/agent/bytebuddy/SWMethodNameTransformer.java Outdated Show resolved Hide resolved

wu-sheng reviewed Jun 23, 2023

View reviewed changes

...tebuddy-patch/src/test/java/org/apache/skywalking/apm/agent/bytebuddy/ConstructorAdvice.java Outdated Show resolved Hide resolved

Extract InterceptPointUtil

64b2916

Revert "Extract InterceptPointUtil"

560ec88

This reverts commit 64b2916.

overload resolve method

bbabc66

wu-sheng reviewed Jun 24, 2023

View reviewed changes

...ebuddy-patch/src/test/java/org/apache/skywalking/apm/agent/bytebuddy/SWClassFileLocator.java Outdated Show resolved Hide resolved

wu-sheng reviewed Jun 24, 2023

View reviewed changes

...ebuddy-patch/src/test/java/org/apache/skywalking/apm/agent/bytebuddy/SWClassFileLocator.java Show resolved Hide resolved

polish code

ee80bfa

polish code

161cfc1

update changes doc

c92f8d8

add doc for intercept point hashcode

5ab814a

verify subclass of ElementMatcher

47bf951

wu-sheng approved these changes Jun 24, 2023

View reviewed changes

wu-sheng merged commit 267aba8 into apache:main Jun 24, 2023

lujiajing1126 mentioned this pull request Oct 27, 2023

[Bug] SkyWalking Java Agent performance degradation after upgrading to 9.0 apache/skywalking#11460

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve bytebuddy class enhance for retransform classes #561

Improve bytebuddy class enhance for retransform classes #561

kylixs commented Jun 23, 2023

wu-sheng Jun 23, 2023

kylixs Jun 23, 2023

wu-sheng Jun 23, 2023

kylixs Jun 23, 2023

wu-sheng Jun 24, 2023

kylixs Jun 24, 2023

wu-sheng Jun 24, 2023

kylixs Jun 24, 2023

wspray Sep 26, 2024

wu-sheng Sep 26, 2024

wu-sheng commented Jun 23, 2023

kylixs commented Jun 23, 2023 •

edited

Loading

wu-sheng commented Jun 24, 2023

kylixs commented Jun 24, 2023

kylixs commented Jun 24, 2023 •

edited

Loading

wu-sheng commented Jun 24, 2023

wu-sheng Jun 24, 2023

kylixs Jun 24, 2023

wu-sheng Jun 24, 2023

wu-sheng commented Jun 24, 2023

kylixs commented Jun 24, 2023 •

edited

Loading

wu-sheng commented Jun 24, 2023

kylixs commented Jun 24, 2023

wu-sheng commented Jun 24, 2023

kylixs commented Jun 24, 2023

wu-sheng commented Jun 24, 2023

kylixs commented Jun 24, 2023

wu-sheng commented Jun 24, 2023

wu-sheng left a comment

wu-sheng commented Jun 24, 2023

kylixs commented Jun 24, 2023

Improve bytebuddy class enhance for retransform classes #561

Improve bytebuddy class enhance for retransform classes #561

Conversation

kylixs commented Jun 23, 2023