OpenJPA
  1. OpenJPA
  2. OPENJPA-952

Utilize Sun JDK's Attach API to dynamically load the OpenJPA enhancer agent

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0
    • Fix Version/s: 2.0.0-M3
    • Component/s: kernel
    • Labels:
      None
    • Environment:
      Sun 1.6 JDK.

      Note: The Attach API is ONLY a part of the JDK, not the SDK.
    • Patch Info:
      Patch Available

      Description

      When running in a JSE environment, OpenJPA could use the Attach API to dynamically load the enhancer agent at runtime. Dynamically loading the enhancer means that an OpenJPA developer doesn't need to configure a -javaagent. Doing this would dramatically improve the out of box performance, and also improve the ease of use.

      This improvement has the following caveats:
      1.) This API is ONLY a part of the 1.6 JDK.
      2.) This API is supported by only the Sun JDK.
      3.) If the agent is loaded from the earliest OpenJPA code, the agent will be laoded when creating an EntityManager in the EntityManagerFactoryImpl. If an Entity class is loaded by the JVM before the enhancer agent is loaded, that class' byte code will not be enhanced.

      Attach API - http://java.sun.com/javase/6/docs/technotes/guides/attach/index.html

      1. OPENJPA-952.patch
        20 kB
        Rick Curtis
      2. OPENJPA-952.patch
        34 kB
        Rick Curtis
      3. OPENJPA-952.patch
        61 kB
        Rick Curtis

        Issue Links

        There are no Sub-Tasks for this issue.

          Activity

          Rick Curtis created issue -
          Hide
          Kevin Sutter added a comment -

          This sounds very promising! Thanks for diving into this. A couple of questions...

          Can you comment on or further explain your third caveat above? So, you are indicating that the "earliest OpenJPA code" is when an EM is created? Why not any earlier? For example, when creating the EMF? We still may hit the window you are describing, but it might be a little narrower.

          And, in the case of an Entity class loaded by the JVM before the agent is hooked up... You mention that the enhancement won't take place. Okay. But, what happens then? Would we fall back into the subclassing support?

          Or, could we detect this condition and possibly do the class redefinition to replace the class that was already loaded?

          Thanks,
          Kevin

          Show
          Kevin Sutter added a comment - This sounds very promising! Thanks for diving into this. A couple of questions... Can you comment on or further explain your third caveat above? So, you are indicating that the "earliest OpenJPA code" is when an EM is created? Why not any earlier? For example, when creating the EMF? We still may hit the window you are describing, but it might be a little narrower. And, in the case of an Entity class loaded by the JVM before the agent is hooked up... You mention that the enhancement won't take place. Okay. But, what happens then? Would we fall back into the subclassing support? Or, could we detect this condition and possibly do the class redefinition to replace the class that was already loaded? Thanks, Kevin
          Hide
          Rick Curtis added a comment -

          Yes I think it would be possible to move the loading of the agent into the factory ctor. It would close the window a little, but the window will still exit. I'll update the patch I'm working on.

          If an Entity class is loaded by the JVM before the agent is loaded, that class will fallback to subclassing enhancement. In the small tests that I have run on my machine, bytecode enhacement plays nicely with subclassing enhancement. I'll look into whether or not it would be possible to redefine a class that has already been loaded. I'll tell you that it doesn't look promising... see below.

          http://java.sun.com/j2se/1.5.0/docs/api/java/lang/instrument/Instrumentation.html#redefineClasses(java.lang.instrument.ClassDefinition[])
          ...
          The redefinition may change method bodies, the constant pool and attributes. The redefinition must not add, remove or rename fields or methods, change the signatures of methods, or change inheritance. These restrictions maybe be lifted in future versions.
          ...

          Show
          Rick Curtis added a comment - Yes I think it would be possible to move the loading of the agent into the factory ctor. It would close the window a little, but the window will still exit. I'll update the patch I'm working on. If an Entity class is loaded by the JVM before the agent is loaded, that class will fallback to subclassing enhancement. In the small tests that I have run on my machine, bytecode enhacement plays nicely with subclassing enhancement. I'll look into whether or not it would be possible to redefine a class that has already been loaded. I'll tell you that it doesn't look promising... see below. http://java.sun.com/j2se/1.5.0/docs/api/java/lang/instrument/Instrumentation.html#redefineClasses(java.lang.instrument.ClassDefinition[ ]) ... The redefinition may change method bodies, the constant pool and attributes. The redefinition must not add, remove or rename fields or methods, change the signatures of methods, or change inheritance. These restrictions maybe be lifted in future versions. ...
          Hide
          Rick Curtis added a comment -

          In this initial patch I purposely ignored wrapping blocks of code that require special security permission. I do so because I'm not entirely sure that it is a valid use case. I assume this will be a point of some discussion.?

          Show
          Rick Curtis added a comment - In this initial patch I purposely ignored wrapping blocks of code that require special security permission. I do so because I'm not entirely sure that it is a valid use case. I assume this will be a point of some discussion.?
          Rick Curtis made changes -
          Field Original Value New Value
          Attachment OPENJPA-952.patch [ 12401861 ]
          Hide
          Kevin Sutter added a comment -

          Rick,
          Thanks for the patch. I like the idea behind this patch and the JIRA itself, but I have a few questions and comments...

          o Testing. We need a means of testing this approach on a regular basis. One of the problems with the old subclassing support is that it wasn't exercised. We didn't know about some of the problems until users ran into them. Somehow, we need the ability to run the junit bucket or a proper subset of the bucket using this dynamic agent support. Could a new testing profile be created to make this easy to execute? Maybe it's not run with every test build, but we easily have the capability to run the bucket?

          o Documentation. As you have found out, we're short on documentation in this enhancement area. I don't see any doc updates in the patch you provided. If we put this support in place, then we need some documentation to explain this new capability. It's there automatically with Sun Java6, but is there any way to turn it off? Should there be? What about the other existing options like oopenjpa.RuntimeEnhancedClasses? Any conflicts with these option settings?

          o Javadoc. I noticed a couple of places where javadoc probably needed to be updated, like with the PCEnhancerAgent. The javadoc in this class talks about the -javaagent processing. This would be a good area to update with the information on the dynamic agent plugin. In general, don't forget about the javadoc. It's very important.

          o Java2 Security. Due to the jar file manipulation and classloading that you are attempting to do, these type of changes need to be executed with Java2 Security turned on. Albert has provided an easy way to run the bucket or an individual test with Java2 Security turned on. It's something like this (but don't quote me):

          mvn myTest -Penable-security

          o The flags and methods in PCEnhancerAgent don't synch up for me. For example, you introduced a boolean called enhancementComplete, but the accessor method is hasRun(). Since enhancementComplete seems to be set to true when preMain is first called, enhancement isn't really complete, is it? Something along the lines of resident, invoked, initialized, or something like that seems more appropriate. And, then having an accessor method that matches the boolean would be better.

          o Warning mesasge id of entities-loaded-before-em. I thought we could key off the emf creation instead of the em. Would it be better to re-name and re-word this message to just indicate that entities were accessed before the dynamic agent was configured. And, if the EMF creation is the proper step-off point, then it's okay to use that for an explanation and resolution.

          o Trace messages. It looks like you are trying to introduce some type of "eye catcher" for trace messages (ie. the ":157" in the trace message for ManagedClassSubclasser). This is not consistent with the rest of the OpenJPA trace messages. So, unless you are proposing to introduce a change to all of the Trace message processing, then I'd stick with the current conventions.

          o I see that you modified the existing logic of just warning a message when RuntimeUnenhancedClasses is set to WARN. If it's something other than WARN, then you now throw an Exception. What about a value of Supported? And, is this the right place to throw this exception? At minimum, we need more comments as to why we're changing the existing processing. I'm not clear on why an exception is proper processing at this point.

          o The indentation and general formatting doesn't seem to be consistent. In one spot, your indentation is the expected 4 spaces, while in other spaces it's all the way out to 8 spaces. Are you using space substitution for tabs? And, have you checked for the 80 column limit?

          o Nit. Check the spelling in your comments and javadoc updates. Just read through them and I think you'll find a few.

          o Some of the Trace statements in PersistenceProviderImpl need more "beef". They should be self-explanatory by reading a Trace log file without having to reference the code. For example,

          _log.trace(_name + ":468",t);

          ... wouldn't tell me enough in the log file as to why this was logged. Some additional text explaining what this dumped exception means to me would help with deciphering a log without relying on the code.

          o Can we be more helpful with the necessary runtime environment? For example, if we detect they are running with the Sun JDK 6 JRE instead of the SDK (ie. no tools directory), could we output a message that would tell them about this nice feature if they would only run with the SDK...

          o I like the getAgentJar processing for when running within Eclipse. Should make testing even easier. Thanks. But... (always a but)... Shouldn't there be a way to turn this feature off? Just in case we're debugging a particular situation, I think we need a means of turning off this dynamic support. Don't you think?

          o All external messages (non Trace) needs to go into our message files. Developers want the ability to read translated messages, much like end users.

          That's it. Thanks again for driving this JIRA. I think we really need this function and I appreciate your efforts. When you have an updated patch, I'd be happy to review it again.

          Kevin

          Show
          Kevin Sutter added a comment - Rick, Thanks for the patch. I like the idea behind this patch and the JIRA itself, but I have a few questions and comments... o Testing. We need a means of testing this approach on a regular basis. One of the problems with the old subclassing support is that it wasn't exercised. We didn't know about some of the problems until users ran into them. Somehow, we need the ability to run the junit bucket or a proper subset of the bucket using this dynamic agent support. Could a new testing profile be created to make this easy to execute? Maybe it's not run with every test build, but we easily have the capability to run the bucket? o Documentation. As you have found out, we're short on documentation in this enhancement area. I don't see any doc updates in the patch you provided. If we put this support in place, then we need some documentation to explain this new capability. It's there automatically with Sun Java6, but is there any way to turn it off? Should there be? What about the other existing options like oopenjpa.RuntimeEnhancedClasses? Any conflicts with these option settings? o Javadoc. I noticed a couple of places where javadoc probably needed to be updated, like with the PCEnhancerAgent. The javadoc in this class talks about the -javaagent processing. This would be a good area to update with the information on the dynamic agent plugin. In general, don't forget about the javadoc. It's very important. o Java2 Security. Due to the jar file manipulation and classloading that you are attempting to do, these type of changes need to be executed with Java2 Security turned on. Albert has provided an easy way to run the bucket or an individual test with Java2 Security turned on. It's something like this (but don't quote me): mvn myTest -Penable-security o The flags and methods in PCEnhancerAgent don't synch up for me. For example, you introduced a boolean called enhancementComplete, but the accessor method is hasRun(). Since enhancementComplete seems to be set to true when preMain is first called, enhancement isn't really complete, is it? Something along the lines of resident, invoked, initialized, or something like that seems more appropriate. And, then having an accessor method that matches the boolean would be better. o Warning mesasge id of entities-loaded-before-em. I thought we could key off the emf creation instead of the em. Would it be better to re-name and re-word this message to just indicate that entities were accessed before the dynamic agent was configured. And, if the EMF creation is the proper step-off point, then it's okay to use that for an explanation and resolution. o Trace messages. It looks like you are trying to introduce some type of "eye catcher" for trace messages (ie. the ":157" in the trace message for ManagedClassSubclasser). This is not consistent with the rest of the OpenJPA trace messages. So, unless you are proposing to introduce a change to all of the Trace message processing, then I'd stick with the current conventions. o I see that you modified the existing logic of just warning a message when RuntimeUnenhancedClasses is set to WARN. If it's something other than WARN, then you now throw an Exception. What about a value of Supported? And, is this the right place to throw this exception? At minimum, we need more comments as to why we're changing the existing processing. I'm not clear on why an exception is proper processing at this point. o The indentation and general formatting doesn't seem to be consistent. In one spot, your indentation is the expected 4 spaces, while in other spaces it's all the way out to 8 spaces. Are you using space substitution for tabs? And, have you checked for the 80 column limit? o Nit. Check the spelling in your comments and javadoc updates. Just read through them and I think you'll find a few. o Some of the Trace statements in PersistenceProviderImpl need more "beef". They should be self-explanatory by reading a Trace log file without having to reference the code. For example, _log.trace(_name + ":468",t); ... wouldn't tell me enough in the log file as to why this was logged. Some additional text explaining what this dumped exception means to me would help with deciphering a log without relying on the code. o Can we be more helpful with the necessary runtime environment? For example, if we detect they are running with the Sun JDK 6 JRE instead of the SDK (ie. no tools directory), could we output a message that would tell them about this nice feature if they would only run with the SDK... o I like the getAgentJar processing for when running within Eclipse. Should make testing even easier. Thanks. But... (always a but)... Shouldn't there be a way to turn this feature off? Just in case we're debugging a particular situation, I think we need a means of turning off this dynamic support. Don't you think? o All external messages (non Trace) needs to go into our message files. Developers want the ability to read translated messages, much like end users. That's it. Thanks again for driving this JIRA. I think we really need this function and I appreciate your efforts. When you have an updated patch, I'd be happy to review it again. Kevin
          Hide
          Rick Curtis added a comment -

          Kevin -

          Thanks for all the comments! The latest patch should have addressed all of your comments. The only comment I'd like to address directly is,

          Question: I like the getAgentJar processing for when running within Eclipse. Should make testing even easier. Thanks. But... (always a but)... Shouldn't there be a way to turn this feature off? Just in case we're debugging a particular situation, I think we need a means of turning off this dynamic support. Don't you think?

          Answer: That processing is out of necessity, not a feature. To dynamically load an agent, a jar file is needed that has an Agent-class defined. Under normal circumstances the PersistenceProviderImpl is loaded from the OpenJPA jar and we can load the agent with that jar. When running a test the OpenJPA jar doesn't exist and the only way to get around this would be to setup your test to run off a compiled OpenJPA jar.

          To run the news test suite associated with this patch the following steps need to be followed:

          • Compile OpenJPA with the 1.5 JDK and make sure to skip the tests. (-DskipTests=true ... We need to skip the tests so that the build time enhancer doesn't run)
          • Switch to the Sun 1.6 JDK.
          • Run mvn test -P test-dynamic-enhancer,test-derby (or which ever DB you want to run with, I've only tried derby)

          Notes:

          • See openjpa-persistence-jdbc -> org.apache.openjpa.persistence.enhance.DynamicEnhancementSuite for a detailed list of the tests that are run.
          • By specifying mvn test -P test-dynamic-enhancer,test-derby -DdynamicTest=org.apache.openjpa.... allows you to run a single test with the dynamic enhancer.

          -Rick

          Show
          Rick Curtis added a comment - Kevin - Thanks for all the comments! The latest patch should have addressed all of your comments. The only comment I'd like to address directly is, Question: I like the getAgentJar processing for when running within Eclipse. Should make testing even easier. Thanks. But... (always a but)... Shouldn't there be a way to turn this feature off? Just in case we're debugging a particular situation, I think we need a means of turning off this dynamic support. Don't you think? Answer: That processing is out of necessity, not a feature. To dynamically load an agent, a jar file is needed that has an Agent-class defined. Under normal circumstances the PersistenceProviderImpl is loaded from the OpenJPA jar and we can load the agent with that jar. When running a test the OpenJPA jar doesn't exist and the only way to get around this would be to setup your test to run off a compiled OpenJPA jar. To run the news test suite associated with this patch the following steps need to be followed: Compile OpenJPA with the 1.5 JDK and make sure to skip the tests. (-DskipTests=true ... We need to skip the tests so that the build time enhancer doesn't run) Switch to the Sun 1.6 JDK. Run mvn test -P test-dynamic-enhancer,test-derby (or which ever DB you want to run with, I've only tried derby) Notes: See openjpa-persistence-jdbc -> org.apache.openjpa.persistence.enhance.DynamicEnhancementSuite for a detailed list of the tests that are run. By specifying mvn test -P test-dynamic-enhancer,test-derby -DdynamicTest=org.apache.openjpa.... allows you to run a single test with the dynamic enhancer. -Rick
          Rick Curtis made changes -
          Attachment OPENJPA-952.patch [ 12404363 ]
          Hide
          Milosz Tylenda added a comment -

          Don't we have some of this functionality already in the InstrumentationFactory class? If so, we could try to reuse it.

          Show
          Milosz Tylenda added a comment - Don't we have some of this functionality already in the InstrumentationFactory class? If so, we could try to reuse it.
          Hide
          Michael Dick added a comment -

          Reviewing Rick's patch.

          Show
          Michael Dick added a comment - Reviewing Rick's patch.
          Michael Dick made changes -
          Original Estimate 0h [ 0 ]
          Remaining Estimate 0h [ 0 ]
          Assignee Michael Dick [ mikedd ]
          Hide
          Michael Dick added a comment -

          I've spoken with Rick a bit and he's looking into reusing some of the code from InstrumentationFactory (thanks for pointing it out Milosz).

          Show
          Michael Dick added a comment - I've spoken with Rick a bit and he's looking into reusing some of the code from InstrumentationFactory (thanks for pointing it out Milosz).
          Hide
          Rick Curtis added a comment -

          Milosz -

          You are correct that some of this functionality already exists in the InstrumentationFactory... not sure why I didn't use that in the first place. I'm in the process of re-working my patch. I spoke with Mike and he is going to take a look at it once I post it.

          -Rick

          Show
          Rick Curtis added a comment - Milosz - You are correct that some of this functionality already exists in the InstrumentationFactory... not sure why I didn't use that in the first place. I'm in the process of re-working my patch. I spoke with Mike and he is going to take a look at it once I post it. -Rick
          Hide
          Rick Curtis added a comment -

          Giving this one another shot.

          • Refactored much of the code (as Miloz commented, there was some commonality between what I was doing and the InstrumentationFactory).
          • Previously I mentioned that the code needed to be compiled with JDK1.5, that is no longer true.
          • The previous patch didn't have the test suite that I added.
          • To run the entire test suite:
            openjpa-parent>mvn test -P test-dynamic-enhancer,test-derby -DfailIfNoTests=false
          • The test suite is composed of:
          • All tests that failed when running with subclassing.
          • All tests in the following openjpa-persistence-jdbc packages
          • org.apache.openjpa.persistence.enhance
          • org.apache.openjpa.persistence.relations
          • org.apache.openjpa.persistence.simple
          • org.apache.openjpa.persistence.identity
          • org.apache.openjpa.persistence.annotations
          • To run a single test with the dynamic enhancer:
            openjpa-parent>mvn test -P test-dynamic-enhancer,test-derby -DfailIfNoTests=false -DdynamicTest=org.apache.openjpa...
          Show
          Rick Curtis added a comment - Giving this one another shot. Refactored much of the code (as Miloz commented, there was some commonality between what I was doing and the InstrumentationFactory). Previously I mentioned that the code needed to be compiled with JDK1.5, that is no longer true. The previous patch didn't have the test suite that I added. To run the entire test suite: openjpa-parent>mvn test -P test-dynamic-enhancer,test-derby -DfailIfNoTests=false The test suite is composed of: All tests that failed when running with subclassing. All tests in the following openjpa-persistence-jdbc packages org.apache.openjpa.persistence.enhance org.apache.openjpa.persistence.relations org.apache.openjpa.persistence.simple org.apache.openjpa.persistence.identity org.apache.openjpa.persistence.annotations To run a single test with the dynamic enhancer: openjpa-parent>mvn test -P test-dynamic-enhancer,test-derby -DfailIfNoTests=false -DdynamicTest=org.apache.openjpa...
          Rick Curtis made changes -
          Attachment OPENJPA-952.patch [ 12408585 ]
          Donald Woods made changes -
          Patch Info [Patch Available]
          Michael Dick committed 780086 (18 files)
          Reviews: none

          OPENJPA-952. Committing patch from Rick Curtis

          openjpa trunk
          Hide
          Michael Dick added a comment -

          Thanks for the patch Rick. Fixed in 2.0.0 but could be merged to 1.3.x if there's sufficient demand.

          Show
          Michael Dick added a comment - Thanks for the patch Rick. Fixed in 2.0.0 but could be merged to 1.3.x if there's sufficient demand.
          Michael Dick made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s 2.0.0 [ 12313483 ]
          Resolution Fixed [ 1 ]
          Michael Dick made changes -
          Link This issue blocks OPENJPA-1125 [ OPENJPA-1125 ]
          Michael Dick made changes -
          Fix Version/s 2.0.0 [ 12314019 ]
          Fix Version/s 2.0.0-M2 [ 12313483 ]
          Affects Version/s 2.0.0 [ 12314019 ]
          Affects Version/s 2.0.0-M2 [ 12313483 ]
          Donald Woods made changes -
          Fix Version/s 2.0.0-M3 [ 12314148 ]
          Fix Version/s 2.0.0 [ 12314019 ]
          Donald Woods made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Rick Curtis made changes -
          Link This issue is related to OPENJPA-1734 [ OPENJPA-1734 ]
          Jason Pyeron made changes -
          Link This issue relates to OPENJPA-2036 [ OPENJPA-2036 ]
          Gavin made changes -
          Link This issue blocks OPENJPA-1125 [ OPENJPA-1125 ]
          Gavin made changes -
          Link This issue is depended upon by OPENJPA-1125 [ OPENJPA-1125 ]

            People

            • Assignee:
              Michael Dick
              Reporter:
              Rick Curtis
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development