[GitHub] [cxf] dufoli opened a new pull request #721: [cxf-8340] add precompiled class for Graalvm native support

classic Classic list List threaded Threaded
215 messages Options
1234 ... 11
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] dufoli opened a new pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox

dufoli opened a new pull request #721:
URL: https://github.com/apache/cxf/pull/721


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] dufoli commented on pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox

dufoli commented on pull request #721:
URL: https://github.com/apache/cxf/pull/721#issuecomment-724624482


   hello, can I have some review about this PR. I know that I open asmHelper to external class but goal is to speed up performance of boot time of cxf by removing class generation.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

reta commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r520969844



##########
File path: rt/databinding/jaxb/src/main/java/org/apache/cxf/jaxb/JAXBContextInitializer.java
##########
@@ -559,44 +559,47 @@ private static Object createTypeReference(QName n, Class<?> cls) {
     private Object createFactory(Class<?> cls, Constructor<?> contructor) {
         String newClassName = cls.getName() + "Factory";
         ASMHelper helper = new ASMHelper();
-        ClassWriter cw = helper.createClassWriter();
-        MethodVisitor mv;
-
-        cw.visit(Opcodes.V1_6, Opcodes.ACC_PUBLIC + Opcodes.ACC_SUPER,
-                 ASMHelper.periodToSlashes(newClassName), null, "java/lang/Object", null);
-
-        cw.visitSource(cls.getSimpleName() + "Factory" + ".java", null);
+        Class<?> factoryClass = helper.findClass(newClassName, cls);

Review comment:
       I think we have to clearly distinguish the case when factory class exists and does not (`createFactory`). Could you please extract the logic of detecting the presence of factory class into dedicated method and modify `addClass` method to try to find the factory class first and fallback to `createFactory` if needed?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

reta commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r520970218



##########
File path: core/src/main/java/org/apache/cxf/common/util/ASMHelper.java
##########
@@ -342,7 +342,10 @@ public ClassWriter createClassWriter() {
         TypeHelperClassLoader loader = getTypeHelperClassLoader(l);
         return loader.lookupDefinedClass(className);
     }
-
+    public static void addExternalClass(String className, ClassLoader l, Class<?> cls) {

Review comment:
       How these methods are supposed to be used?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

reta commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r520971208



##########
File path: core/src/main/java/org/apache/cxf/common/jaxb/JAXBUtils.java
##########
@@ -1346,7 +1346,11 @@ private static Object createEclipseNamespaceMapper(Class<?> mcls, Map<String, St
             + ("RI".equals(postFix) ? "" : "internal/")
             + "bind/marshaller/NamespacePrefixMapper";
         String postFixedName = "org/apache/cxf/jaxb/NamespaceMapper" + postFix;
+        Class<?> cls = helper.findClass(className, ref);

Review comment:
       Same as for `createFactory`, it would be better to distinguish the case when suitable factory exists vs creating a new one.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

reta commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r520969844



##########
File path: rt/databinding/jaxb/src/main/java/org/apache/cxf/jaxb/JAXBContextInitializer.java
##########
@@ -559,44 +559,47 @@ private static Object createTypeReference(QName n, Class<?> cls) {
     private Object createFactory(Class<?> cls, Constructor<?> contructor) {
         String newClassName = cls.getName() + "Factory";
         ASMHelper helper = new ASMHelper();
-        ClassWriter cw = helper.createClassWriter();
-        MethodVisitor mv;
-
-        cw.visit(Opcodes.V1_6, Opcodes.ACC_PUBLIC + Opcodes.ACC_SUPER,
-                 ASMHelper.periodToSlashes(newClassName), null, "java/lang/Object", null);
-
-        cw.visitSource(cls.getSimpleName() + "Factory" + ".java", null);
+        Class<?> factoryClass = helper.findClass(newClassName, cls);

Review comment:
       I think we have to clearly distinguish the case when factory class exists and does not (`createFactory`). Could you please extract the logic of detecting the presence of factory class into dedicated method and modify `addClass` method to try to find the factory class first and fallback to `createFactory` if needed?
   
   We should also ensure that the factory class is suitable and could be used accordingly.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] dufoli commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

dufoli commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r521169509



##########
File path: core/src/main/java/org/apache/cxf/common/util/ASMHelper.java
##########
@@ -342,7 +342,10 @@ public ClassWriter createClassWriter() {
         TypeHelperClassLoader loader = getTypeHelperClassLoader(l);
         return loader.lookupDefinedClass(className);
     }
-
+    public static void addExternalClass(String className, ClassLoader l, Class<?> cls) {

Review comment:
       inside quarkiverse-cxf inside a a servlet
   https://github.com/quarkiverse/quarkiverse-cxf/blob/master/runtime/src/main/java/io/quarkiverse/cxf/CXFQuarkusServlet.java#L50
   or
   https://github.com/quarkiverse/quarkiverse-cxf/blob/master/runtime/src/main/java/io/quarkiverse/cxf/CxfClientProducer.java#L19
   
   I can add
   ASMHelper.addExternalClass('io.quarkiverse.test.FruitFactory', Class.forName('io.quarkiverse.test.FruitFactory'));
   here it is just for factory but I will provide the full list of generated class wrapperHelper (for request and response), packageInfo, customException with faultInfo field, NamespacePrefixMapper




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] dufoli commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

dufoli commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r521209060



##########
File path: core/src/main/java/org/apache/cxf/common/util/ASMHelper.java
##########
@@ -342,7 +342,10 @@ public ClassWriter createClassWriter() {
         TypeHelperClassLoader loader = getTypeHelperClassLoader(l);
         return loader.lookupDefinedClass(className);
     }
-
+    public static void addExternalClass(String className, ClassLoader l, Class<?> cls) {

Review comment:
       augmentor will produce class during build time and send class generated through recorder to runtime.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] rmannibucau commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

rmannibucau commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r521212171



##########
File path: core/src/main/java/org/apache/cxf/common/util/ASMHelper.java
##########
@@ -342,7 +342,10 @@ public ClassWriter createClassWriter() {
         TypeHelperClassLoader loader = getTypeHelperClassLoader(l);
         return loader.lookupDefinedClass(className);
     }
-
+    public static void addExternalClass(String className, ClassLoader l, Class<?> cls) {

Review comment:
       Is it possible to not make it quarkus specific? Here is my proposal:
   
   1. Make ASMHelper a SPI set as cxf bus extension
   2. Provide current default impls
   3. Have a a) generate proxy at build time mojo AND b) a load first proxy (before generating it) strategy (note that it makes asm useless and must not trigger asm classloading to work).
   
   3 is close to https://github.com/apache/openwebbeans/blob/1f83552e74c7966fc1009a3a7417900e7ce42b32/webbeans-impl/src/main/java/org/apache/webbeans/service/ClassLoaderProxyService.java#L85 for runtime and https://github.com/apache/openwebbeans/blob/1f83552e74c7966fc1009a3a7417900e7ce42b32/webbeans-impl/src/main/java/org/apache/webbeans/service/ClassLoaderProxyService.java#L62 for build time (and dump the proxies as .class).
   
   This way the code is not quarkus specific and benefits all applications in JVM and native mode.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] rmannibucau commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

rmannibucau commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r521212171



##########
File path: core/src/main/java/org/apache/cxf/common/util/ASMHelper.java
##########
@@ -342,7 +342,10 @@ public ClassWriter createClassWriter() {
         TypeHelperClassLoader loader = getTypeHelperClassLoader(l);
         return loader.lookupDefinedClass(className);
     }
-
+    public static void addExternalClass(String className, ClassLoader l, Class<?> cls) {

Review comment:
       Is it possible to not make it quarkus specific? Here is my proposal:
   
   1. Make ASMHelper a SPI set as cxf bus extension - to support versions in a pluggable way and not hardcode 10 versions of asm + reflection
   2. Provide current default impls
   3. Have a a) generate proxy at build time mojo AND b) a load first proxy (before generating it) strategy (note that it makes asm useless and must not trigger asm classloading to work).
   
   3 is close to https://github.com/apache/openwebbeans/blob/1f83552e74c7966fc1009a3a7417900e7ce42b32/webbeans-impl/src/main/java/org/apache/webbeans/service/ClassLoaderProxyService.java#L85 for runtime and https://github.com/apache/openwebbeans/blob/1f83552e74c7966fc1009a3a7417900e7ce42b32/webbeans-impl/src/main/java/org/apache/webbeans/service/ClassLoaderProxyService.java#L62 for build time (and dump the proxies as .class).
   
   This way the code is not quarkus specific and benefits all applications in JVM and native mode.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] dufoli commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

dufoli commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r521267800



##########
File path: core/src/main/java/org/apache/cxf/common/util/ASMHelper.java
##########
@@ -342,7 +342,10 @@ public ClassWriter createClassWriter() {
         TypeHelperClassLoader loader = getTypeHelperClassLoader(l);
         return loader.lookupDefinedClass(className);
     }
-
+    public static void addExternalClass(String className, ClassLoader l, Class<?> cls) {

Review comment:
       I do not get the point that this is quarkus specific
   1. indeed it is not quarkus specific but graal specific because can be used by spring boot too. Or any system which can provide class directly to avoid class generation. Anyway, It was the other proposition in the jira. But I found that less intrusive. The issue if I do so, I will have to do it for more class (wrapper helper, exception,...) which are generated. That s why I choose this solution.
   
   2.  If I understand correctly, I create a class factory proxy static which provide creator which return Class<T> and if not in Map<String, Class<?>> . and each time, AsmHelper is used, I add this ProxyClassFactory in between. And in my code I just call ProxyClassFactory.getProxies().put(...);
   
   Sorry, I do not know what you mean by "SPI set as cxf bus extension" ?
   Do you mean, adding interface and implementation in bus-extensions.txt and adding interface for ASMHelper and inherit ASMHelper an implemtnation for each asm version in getASMClass ?
   It seems a big refactoring and not related to the proxy need for quarkus ?
   I agree that static class is not a good solution for ASMHelper and it is better to have factory pattern but I need a little help on that to have more explanation to respect the cxf pattern (bus-extensions.txt and factory). Do you have any document / guide ?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] rmannibucau commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

rmannibucau commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r521310494



##########
File path: core/src/main/java/org/apache/cxf/common/util/ASMHelper.java
##########
@@ -342,7 +342,10 @@ public ClassWriter createClassWriter() {
         TypeHelperClassLoader loader = getTypeHelperClassLoader(l);
         return loader.lookupDefinedClass(className);
     }
-
+    public static void addExternalClass(String className, ClassLoader l, Class<?> cls) {

Review comment:
       @dufoli:
   
   1. it is quarkus specific until you have the generator in cxf codebase (which is not the case in the pr) so it is quarkus specific ;). In other words, current PR is a quarkus extension point because it is feature incomplete for all other env and there is no real reason for that - don't get me wrong, I understand you got stucked and solve your issue but I think to integrate mainstream code it should go a bit further otherwise there is no benefit to get it in cxf land.
   2. you actually don't need this map, you just do a classloader.loadClass (take care it is not the tccl but cxf classloader, can be in bus extensions). Only small thing to take care is to not do it at runtime but only deploy time but it should already be the case (or with a lazy init pattern but only once). In quarkus land you just generate the class at build time and put it in runtime classes (as other proxies).
   3. the spi point is to avoid to have to handle ow2.asm, xbean.asm7, xbean.asm8, xbean.asm9 etc.
   4. to not make it global it must be in cxf bus extensions IMHO (as ClassUnwrapper for example). Each time a class is generated, it has a bus not far so it should be easy to read from there.
   
   hope it makes sense
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] dufoli commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

dufoli commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r521452805



##########
File path: core/src/main/java/org/apache/cxf/common/util/ASMHelper.java
##########
@@ -342,7 +342,10 @@ public ClassWriter createClassWriter() {
         TypeHelperClassLoader loader = getTypeHelperClassLoader(l);
         return loader.lookupDefinedClass(className);
     }
-
+    public static void addExternalClass(String className, ClassLoader l, Class<?> cls) {

Review comment:
       @rmannibucau  thanks for response ! ;-)
   1. hmm, I think, I got your point. indeed, you mean that I need to use cxf parsing system to get serviceInfo and generate all classes without loading servlet or client stuff. The issue is that quarkus use yandex to parse classes and gizmo lib to generate code during build time. I am not sure, I can reuse the cxf system to parse wsdl, and annoted classe and generate serviceInfo in order to generate all classes. Is it doable ? Because I need to inject particular classWriter for that.
   Is it your point ? I have think to a such system at the beginning but it was a nightmare to understand how to use cxf for that. Because it is not a usual case.
   2. So you mean that I inject my own classLoader which contain my classes in order that classLoader see the class I have generated ?
   I do not get you point fully but I will try to code what I understand and I will let you review the code and give feedback.
   
   
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] rmannibucau commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

rmannibucau commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r521458109



##########
File path: core/src/main/java/org/apache/cxf/common/util/ASMHelper.java
##########
@@ -342,7 +342,10 @@ public ClassWriter createClassWriter() {
         TypeHelperClassLoader loader = getTypeHelperClassLoader(l);
         return loader.lookupDefinedClass(className);
     }
-
+    public static void addExternalClass(String className, ClassLoader l, Class<?> cls) {

Review comment:
       @dufoli
   
   1. jandex or not does not change much things, at the end you must register the generated class in graal so a loadClass will cover both cases, only the generation phase will be different but CXF must get one impl for this feature to make sense in CXF itself IMHO - and it shouldnt be that hard to do a mojo generating proxies.
   2. you don't need any custom classloader since classes are generated and dumped (physically or not) at build time in the target/classes of your project so you have them at runtime and just need to read them at run time.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] dufoli commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

dufoli commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r521634019



##########
File path: core/src/main/java/org/apache/cxf/common/util/ASMHelper.java
##########
@@ -342,7 +342,10 @@ public ClassWriter createClassWriter() {
         TypeHelperClassLoader loader = getTypeHelperClassLoader(l);
         return loader.lookupDefinedClass(className);
     }
-
+    public static void addExternalClass(String className, ClassLoader l, Class<?> cls) {

Review comment:
       TODO:
   For point 1)
   - I create ASMHelper as interface with public
   - rename ASMHelper class to ASMHelperImpl and make it inherite of interface.
   - create subclass of ASMHelperImpl  for each version of ASM ? (not sure of this part )
   - create class Proxy implements ASMHelper which do nothing but just load class from classLoader. For this part it is not clear because className is send through a visite on ClassWriter. So it mean I need a NoOpClassWriter wich just store className ?
   - add in bus-extenstions.txt a line:
   org.apache.cxf.common.util.ASMHelper$TypeHelperClassLoader:org.apache.cxf.common.util.ASMHelper$:true
   
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] dufoli commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

dufoli commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r521635360



##########
File path: core/src/main/java/org/apache/cxf/common/util/ASMHelper.java
##########
@@ -342,7 +342,10 @@ public ClassWriter createClassWriter() {
         TypeHelperClassLoader loader = getTypeHelperClassLoader(l);
         return loader.lookupDefinedClass(className);
     }
-
+    public static void addExternalClass(String className, ClassLoader l, Class<?> cls) {

Review comment:
       just to know if I am on right direction... @rmannibucau can you just check what I provide.
   But I need to understand some things which seems odd to me.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] rmannibucau commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

rmannibucau commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r521638575



##########
File path: core/src/main/java/org/apache/cxf/common/util/ASMHelper.java
##########
@@ -342,7 +342,10 @@ public ClassWriter createClassWriter() {
         TypeHelperClassLoader loader = getTypeHelperClassLoader(l);
         return loader.lookupDefinedClass(className);
     }
-
+    public static void addExternalClass(String className, ClassLoader l, Class<?> cls) {

Review comment:
       Hmm,
   
   Think it mixes two things, sorry if I was not clear about this point.
   Until now we were generating proxies with asm, that was it.
   Now we will be able to load proxies without using asm at all so we must split loading and generation IMO and only trigger generation if not existing so it means 2 independent services: GeneratedClassLoader maybe and ClassGenerator (backend by ASM).
   Splitting it avoids to have the generation API in build time generation case.
   It also means CXF does not need the proxy code anymore since asmX integration can now belong to another module (we can create a cxf-asm-compat module for migration purposes maybe and only keep ow2 integration code in cxf-core and register it only if asm is available and ClassGenerator not registered in the bus already).
   About the registration: I wouldn't register it by default but use something like: if (not bus has ClassGenerator) tryLoadOw2AsmClassGenerator().
   
   Hope it makes sense.
   In all case I think it goes in the very right direction, thanks a lot for the effort!




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] dufoli commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

dufoli commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r521654591



##########
File path: core/src/main/java/org/apache/cxf/common/util/ASMHelper.java
##########
@@ -342,7 +342,10 @@ public ClassWriter createClassWriter() {
         TypeHelperClassLoader loader = getTypeHelperClassLoader(l);
         return loader.lookupDefinedClass(className);
     }
-
+    public static void addExternalClass(String className, ClassLoader l, Class<?> cls) {

Review comment:
       indeed best is just to provide me pseudoCode of usage you expect. Because, you are talking about SPI for ASMHelper and ClassLoader so I am bit lost? I think we need a factory for both (because I have static class)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] rmannibucau commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

rmannibucau commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r521667736



##########
File path: core/src/main/java/org/apache/cxf/common/util/ASMHelper.java
##########
@@ -342,7 +342,10 @@ public ClassWriter createClassWriter() {
         TypeHelperClassLoader loader = getTypeHelperClassLoader(l);
         return loader.lookupDefinedClass(className);
     }
-
+    public static void addExternalClass(String className, ClassLoader l, Class<?> cls) {

Review comment:
       Idea is to split the way to get the class - findClass (Generated Class Loader or finder, name was not very great since ambiguous with ClassLoader) - and the way to generate a class (all asm impls).
   All static calls must become deprecated imho.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] dufoli commented on a change in pull request #721: [cxf-8340] add precompiled class for Graalvm native support

GitBox
In reply to this post by GitBox

dufoli commented on a change in pull request #721:
URL: https://github.com/apache/cxf/pull/721#discussion_r521654591



##########
File path: core/src/main/java/org/apache/cxf/common/util/ASMHelper.java
##########
@@ -342,7 +342,10 @@ public ClassWriter createClassWriter() {
         TypeHelperClassLoader loader = getTypeHelperClassLoader(l);
         return loader.lookupDefinedClass(className);
     }
-
+    public static void addExternalClass(String className, ClassLoader l, Class<?> cls) {

Review comment:
       indeed best is just to provide me pseudoCode of usage you expect. Because, you are talking about SPI for ASMHelper and ClassLoader so I am bit lost? I think we need a factory for both (because I hate static class)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


1234 ... 11