Weaver should return null instead of the original bytes for unwoven classes #277
Comments
|
Hello. Thanks for the report. What you say makes sense at first glance, but I would have to check if any consumers of transformed classes might expect non-null results for whatever reasons. Because I am unfamiliar with CDS archives, please provide a full, minimal example with exact steps to reproduce the problem. I want to see for myself what you are doing. Please also explain why you are doing it. I wonder why AspectJ firstly does not skip JDK classes when weaving and secondly tries to instrument code that is not visible to its class loader. There are too many open questions without a reproducer. Currently, I would not even know how to write a regression test for it. Besides, is there any connection between this issue and #278? It strikes me as a strange coincidence, that two issues concerning class-loading are opened within just 10 minutes. |
|
AppCdsDemo-1.0-SNAPSHOT-zip.zip |
|
By the way, we use CDS to improve the startup speed of the application, and most of us use AspectJ for monitoring and tracking. That's why we meet this situation. |
For reference, this is what I see on the console: And this is the written error dump: |
|
One more question: Do you have a working counter-example with a Java agent transforming your demo class, but returning null for the others? I.e. a proof of concept that in that case, it actually works as expected? Also with source code, please. |
|
Oh sorry, there is a typo in the command, I've fixed it on the comment above. |
|
I have never used CDS before, so please also tell me how to run the application after creating the archive. After I changed Is that what you would expect? And did I call the program correctly, again with Decompiling your demo class yields: package com.vip.demo;
import java.lang.management.GarbageCollectorMXBean;
import java.lang.management.ManagementFactory;
import java.util.List;
public class Demo {
public Demo() {
}
public static void main(String[] args) {
List<GarbageCollectorMXBean> garbageCollectorMXBeans = ManagementFactory.getGarbageCollectorMXBeans();
}
}That is not much. A more meaningful reproducer would be helpful. I do not want to do all the work from scratch that you obviously have done already. |
|
That's it, I met the same core dump error like yours and I've try another version of aspectjweaver but it was failed too. |
|
Hi, after generating .jsa file, it means CDS dumping works, and you can the same command to run the application again, and JVM will use .jsa file to load application classes. You can add JVM option And here are some information about CDS (the first one would be the quick start): |
|
Hm, what is the point of creating those optimised files for faster start-up, if then the load-time weaver needs to do its work again? I would have expected you want to create such an archive, because when starting the application again the classes are woven already. Is there any performance gain in doing it like you described? |
|
BTW, java agents can in principle also instrument JDK bootstrap classes, albeit with certain structural limitations. AspectJ happens not to do that, but it does not mean that it never will in the future. What would you expect to happen in such a case when running the java agent again on the CDS archive? Sorry for the many questions, but I do not want to blindly change some code, but also understand what the whole purpose is. That has an influence on both my motivation and the prioritisation of this issue. As the saying goes, if you want to pitch an idea or make someone do something for you, you need to be able to sell it. Let us not forget, that I am doing this work for free, unless you decide to hire me or otherwise sponsor the change. |
I think it is a very good question I can understand your position very well. As CDS is a new feature that few people use. It will inevitably be risky to modify the originally stable code in order to support it. Appreciate for your reply again. Best wishes! |
|
I am working on this, because independently of CDS it makes sense to return
The latter option might be right, if you use production aspects that always should be in place when running the application. If your aspects are rather used temporarily, e.g. for debugging or tracing, and should not be in the byte code permanently, the former option might is better. A hybrid solution would be CTW and aspects with activation flag, e.g. using An important open question for me as someone who has never used CDS before, which I kindly request you to answer: Is there a way to dump classes transformed by a Java agent into a CDS file? If there is such a way and you are using it, please let me know and explain how to activate it, so I can test it. JDK-8213813 (Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping) does not explain anything about it. |
kriegaex
changed the title
OK, I think I have found out. My experimental findings, both as a note to myself and information to the audience: When creating the archive with the agent on the command line, i.e. when using The dump file is also much smaller than when doing the same with the agent on the classpath, in my test case 2.4M without aspectjweaver on the CP versus 6.9M with. Example: See? No more Now let us run the application again with the identical command line: Please note above:
Now, let us go one step further and remove the Very interesting! There is no more weaver message, but still the aspect does its job, because obviously the application class has been archived in its woven state. I.e., the effect is similar to a "poor man's compiler-time weaving". The same log output we would probably see, if we would just build using compile-time weaving and creating the CDS archive while running the woven application with just |
This change makes sense independently of #277, but also enables using - cp "my.jar;aspectjweaver.jar" -XX:+AllowArchivingWithJavaAgent -javaagent:aspectjweaver.jar while creating a CDS archive. Afterward, the application can be run in its woven state from the CDS archive even without '-javaagent', because the byte code was archived in its woven state ("poor man's AJC"). See #277 (comment) for details. Fixes #277. Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
This change makes sense independently of #277, but also enables using - cp "my.jar;aspectjweaver.jar" -XX:+AllowArchivingWithJavaAgent -javaagent:aspectjweaver.jar while creating a CDS archive. Afterward, the application can be run in its woven state from the CDS archive even without '-javaagent', because the byte code was archived in its woven state ("poor man's AJC"). See #277 (comment) for details. Fixes #277. Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
|
Good morning kriegaex, I'm so excited that you are so interested in this problem. I was confused by |
|
@KimmingLau, yes, CDS seems to need a bit of assistance there when creating the archive. For simple weaving, this is unnecessary, but obviously CDS disregards (or rather cannot find) agent classes, if it cannot find them on the CP additionally. According to the JDK-8213813 description and the warning "[cds] This archive was created with AllowArchivingWithJavaAgent. It should be used for testing purposes only and should not be used in a production environment", running the CDS creator with an agent is not meant to modify byte code for production, i.e. the missing support for adding agent JAR classes to the CP automatically probably cannot be termed a bug. The OpenJDK team would probably reject it, even though it would be convenient to have this automatism. However, I can imagine cases in which the classes are not wanted in the CDS archive, which is why I guess the opt-in solution is acceptable. |
This change makes sense independently of #277, but also enables using - cp "my.jar;aspectjweaver.jar" -XX:+AllowArchivingWithJavaAgent -javaagent:aspectjweaver.jar while creating a CDS archive. Afterward, the application can be run in its woven state from the CDS archive even without '-javaagent', because the byte code was archived in its woven state ("poor man's AJC"). See #277 (comment) for details. Fixes #277. Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
This change makes sense independently of #277, but also enables using - cp "my.jar;aspectjweaver.jar" -XX:+AllowArchivingWithJavaAgent -javaagent:aspectjweaver.jar while creating a CDS archive. Afterward, the application can be run in its woven state from the CDS archive even without '-javaagent', because the byte code was archived in its woven state ("poor man's AJC"). See #277 (comment) for details. Fixes #277. Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
This change makes sense independently of #277, but also enables using - cp "my.jar;aspectjweaver.jar" -XX:+AllowArchivingWithJavaAgent -javaagent:aspectjweaver.jar while creating a CDS archive. Afterward, the application can be run in its woven state from the CDS archive even without '-javaagent', because the byte code was archived in its woven state ("poor man's AJC"). See #277 (comment) for details. Fixes #277. Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
Relates to #277. Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
|
@KimmingLau, the problem should be fixed. Please try the latest <repositories>
<repository>
<id>ossrh-snapshots</id>
<url>https://oss.sonatype.org/content/repositories/snapshots</url>
<releases>
<enabled>false</enabled>
</releases>
<snapshots>
<enabled>true</enabled>
<updatePolicy>always</updatePolicy>
</snapshots>
</repository>
</repositories>BTW, the snapshot also contains an improvement which lets you use LTW without |
|
Another note for the record: While creating CDS archives with |
|
I created a minimal reproducer as a Maven project with a very simple agent, not AspectJ: Just run As a workaround, edit the line containing <argument>-javaagent:${project.basedir}/../cds-javaagent-agent/target/cds-javaagent-agent-1.0-SNAPSHOT.jar=XXXreturnNull</argument>in file cds-javaagent-test/pom.xml, removing the "XXX". Then, the agent will return BTW, the problem does not occur with any program that runs with the agent, but specifically if the program contains The reproducer serves as the input for a new OpenJDK bug I just created. It has the internal review ID 9076545. As soon as the bug is accepted and visible in the public JDK bug database, I will add a link here. |
|
Yes, because |
|
See https://bugs.openjdk.org/browse/JDK-8325536. Relates to #277. Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
We try to experience dynamic AppCDS which is a new feature from JDK19 AppCDS with LTW javaagent.
But we found that jdk core classes cannot be load from the default archive, because when Aj realize a class is loaded by boostrap classloader, it will return the original bytes. This will cause the JVM to think that the class has been modified by ClassFileTransformer, and do not load the class from default CDS archive.
According to ClassFileTransformer javadoc , it refer to that return null if no transform is performed. So I think when a class isn't been weaved, Aj should return null instead of the original bytes.
The text was updated successfully, but these errors were encountered: