[GitHub] [openjpa] rmannibucau commented on a change in pull request #65: [OPENJPA-2817] dropping _transforming from PCClassFileTransformer and replace it by a minimal exclusion list

[GitBox <gi...@apache.org>]

rmannibucau commented on a change in pull request #65:
URL: https://github.com/apache/openjpa/pull/65#discussion_r454804045



##########
File path: openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCClassFileTransformer.java
##########
@@ -118,17 +120,44 @@ public PCClassFileTransformer(MetaDataRepository repos,
         if (className == null) {
             return null;
         }
-        // prevent re-entrant calls, which can occur if the enhancing
-        // loader is used to also load OpenJPA libraries; this is to prevent
-        // recursive enhancement attempts for internal openjpa libraries
-        if (_transforming)
-            return null;
-
-        _transforming = true;
 
         return transform0(className, redef, bytes);
     }
 
+    // very simplified flavor of
+    // @apache/tomee >
+    // container/openejb-core >
+    // org/apache/openejb/util/classloader/URLClassLoaderFirst.java#L207
+    private boolean isExcluded(final String className) {
+        if (    // api
+                className.startsWith("javax/") ||
+                className.startsWith("jakarta/") ||
+                // openjpa dependencies
+                className.startsWith("serp/") ||
+                // jvm
+                className.startsWith("java/") ||
+                className.startsWith("sun/") ||
+                className.startsWith("jdk/")) {
+            return true;
+        }
+        // it is faster to use substring when multiple tests are needed
+        if (className.startsWith("org/apache/")) {
+            final String sub = className.substring("org/apache/".length());
+            if (sub.startsWith("openjpa/") ||

Review comment:
       @struberg yes but it works cause we use build time enhancement - also probably why we didn't notice this issue earlier
   @dazey3 as mentionned please benchmark it on the different kind of mentionned apps (no deps - jpa in the container, no deps + no tempclassloader - means temploader=apploader, lot of deps, lot of deps without temp loader, a few deps, a few deps without temp loader and you do the test without the javaagent or with it for all these cases). In practise this change is *faster* than most temploader usage and most bytecode reading so the assumption it is slower is not justified in the general case.




----------------------------------------------------------------
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:
users@infra.apache.org