groovy
  1. groovy
  2. GROOVY-3294

Improved class loading for AST transformations

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.6-rc-1, 1.6-rc-2
    • Fix Version/s: 1.6-rc-3, 1.7-beta-1
    • Component/s: Compiler
    • Labels:
      None
    • Patch Submitted:
      Yes
    • Number of attachments :
      3

      Description

      Currently, groovyc loads AST transformations with the same class loader that is also responsible for loading compile dependencies. This can lead to problems for compiler clients that provide a custom class loader for loading compile dependencies, as such a class loader might not be able to meet the requirements for loading AST transformations. The most prominent example is GMaven, which uses a custom class loader to prevent classpath pollution without having to fork groovyc. Unfortunately this also means that GMaven cannot compile any programs that make use of AST transformations.
      This patch allows for compiler clients to optionally provide a separate class loader for loading AST transformations, thereby decoupling the class loading of AST transformations from that of compile dependencies. This makes it possible for compiler clients such as GMaven (and possibly Gradle) to support AST transformations, and paves the way for further class loading improvements in future versions of groovyc.
      The patch is fairly small and tries to keep changes to the existing codebase to an absolute minimum. Apart from applying moderate changes to two transform-related classes, it only adds a few lines of code to class CompilationUnit. Existing compiler clients will see no change in behavior. Please consider this patch for inclusion in Groovy 1.6, or a 1.6 maintenance release. Also attached is the corresponding GMaven patch. Any feedback is welcome.

      1. improved_transform_class_loading_gmaven.patch
        1 kB
        Peter Niederwieser
      2. improved_transform_class_loading.patch
        10 kB
        Peter Niederwieser
      3. transform_class_loading_fixed.patch
        9 kB
        Peter Niederwieser

        Issue Links

          Activity

          Guillaume Laforge made changes -
          Field Original Value New Value
          Fix Version/s 1.6.1 [ 14852 ]
          Guillaume Laforge made changes -
          Fix Version/s 1.7-beta-1 [ 14014 ]
          Hide
          Guillaume Laforge added a comment -

          Isn't it related to a previous issue you had opened?

          Show
          Guillaume Laforge added a comment - Isn't it related to a previous issue you had opened?
          Jason Dillon made changes -
          Link This issue relates to MGROOVY-174 [ MGROOVY-174 ]
          Hide
          Peter Niederwieser added a comment -

          In a sense yes, but this patch has a much smaller scope: the only thing it does is to allow compiler clients to optionally provide a custom class loader for loading AST transformations, just as they have always been able to provide a custom class loader for loading compile dependencies. As such it is a very natural extension of groovyc that both fixes immediate problems and simplifies future efforts such as GROOVY-3169, all without changing the class loading behavior of existing compiler clients.

          Show
          Peter Niederwieser added a comment - In a sense yes, but this patch has a much smaller scope: the only thing it does is to allow compiler clients to optionally provide a custom class loader for loading AST transformations, just as they have always been able to provide a custom class loader for loading compile dependencies. As such it is a very natural extension of groovyc that both fixes immediate problems and simplifies future efforts such as GROOVY-3169 , all without changing the class loading behavior of existing compiler clients.
          Hide
          blackdrag blackdrag added a comment -

          there are some small adjustments to the patch I would like to see. SourceUnits may have a different ClassLoader then the CompileUnit or CompilationUnit. So I would like to see the transform loader in the same class.. Also I would probably name it to compilerExtensionLoader or better only extensionLoader. Also per default the laoder should be the same as what getClassLoader produces... these x==null?y:z checks I don't like very much.Then of course ASTTransformationCollectorCodeVisitor should not get the loader in the constructor, but request it from source as it does with the normal classloader.

          Show
          blackdrag blackdrag added a comment - there are some small adjustments to the patch I would like to see. SourceUnits may have a different ClassLoader then the CompileUnit or CompilationUnit. So I would like to see the transform loader in the same class.. Also I would probably name it to compilerExtensionLoader or better only extensionLoader. Also per default the laoder should be the same as what getClassLoader produces... these x==null?y:z checks I don't like very much.Then of course ASTTransformationCollectorCodeVisitor should not get the loader in the constructor, but request it from source as it does with the normal classloader.
          Hide
          Peter Niederwieser added a comment -

          > SourceUnits may have a different ClassLoader then the CompileUnit or CompilationUnit. So I would like to see the transform loader in the same class.
          OK, so I should move transformLoader into ProcessingUnit and add another constructor there? Note that without further changes, initialization of transformLoader has to occur at construction time, because loading of global transforms is triggered from within CompilationUnit's constructor.

          > Also I would probably name it to compilerExtensionLoader or better only extensionLoader.
          I can do that, but "extension" already has an established meaning in the JVM (extension classpath, extension class loader).

          > Also per default the loader should be the same as what getClassLoader produces... these x==null?y:z checks I don't like very much.
          Neither do I, but the problem is that the regular class loader can also be set with ProcessingUnit.setClassLoader(), and I tried to support this form of initialization at least for local transforms. If that's not necessary then I can simplify the code.

          Show
          Peter Niederwieser added a comment - > SourceUnits may have a different ClassLoader then the CompileUnit or CompilationUnit. So I would like to see the transform loader in the same class. OK, so I should move transformLoader into ProcessingUnit and add another constructor there? Note that without further changes, initialization of transformLoader has to occur at construction time, because loading of global transforms is triggered from within CompilationUnit's constructor. > Also I would probably name it to compilerExtensionLoader or better only extensionLoader. I can do that, but "extension" already has an established meaning in the JVM (extension classpath, extension class loader). > Also per default the loader should be the same as what getClassLoader produces... these x==null?y:z checks I don't like very much. Neither do I, but the problem is that the regular class loader can also be set with ProcessingUnit.setClassLoader(), and I tried to support this form of initialization at least for local transforms. If that's not necessary then I can simplify the code.
          Hide
          blackdrag blackdrag added a comment -

          the transforms loaded in the CompilationUnit are global ones. Of course CompilationUnit will still get the loader and that loader should be used to load the global transforms. IMHO ASTTransformationVisitor.addPhaseOperations should get that loader for this. As for the name "extensionLoader", you are right... then we should stick with compilerExtensionLoader.. or with astTransformationLoader.

          A for ProcessingUnit.setClassLoader()... so you mean if someone uses null for the ast transformation loader, then it should use whatever is used for the normal classloader even if that is changed...hmm.... hadn't thought about that aspect.... But I don' think it is needed, better have a more simple code.

          Show
          blackdrag blackdrag added a comment - the transforms loaded in the CompilationUnit are global ones. Of course CompilationUnit will still get the loader and that loader should be used to load the global transforms. IMHO ASTTransformationVisitor.addPhaseOperations should get that loader for this. As for the name "extensionLoader", you are right... then we should stick with compilerExtensionLoader.. or with astTransformationLoader. A for ProcessingUnit.setClassLoader()... so you mean if someone uses null for the ast transformation loader, then it should use whatever is used for the normal classloader even if that is changed...hmm.... hadn't thought about that aspect.... But I don' think it is needed, better have a more simple code.
          Hide
          Peter Niederwieser added a comment -

          >A for ProcessingUnit.setClassLoader()... so you mean if someone uses null for the ast transformation loader, then it should use whatever is used for the normal classloader even if that is changed
          I meant:

          def unit = new CompilationUnit()
          unit.classLoader = someClassLoader
          unit.addSource(someFile)
          unit.compile()
          

          The initialization logic for ProcessingUnit/SourceUnit/CompilationUnit looks overly complex to me, with ProcessingUnit.setClassLoader() being especially evil. For example, if you call setClassLoader() after addSource(), the SourceUnit will end up with the "wrong" class loader. Should I just pretend that setClassLoader() doesn't exist?

          Show
          Peter Niederwieser added a comment - >A for ProcessingUnit.setClassLoader()... so you mean if someone uses null for the ast transformation loader, then it should use whatever is used for the normal classloader even if that is changed I meant: def unit = new CompilationUnit() unit.classLoader = someClassLoader unit.addSource(someFile) unit.compile() The initialization logic for ProcessingUnit/SourceUnit/CompilationUnit looks overly complex to me, with ProcessingUnit.setClassLoader() being especially evil. For example, if you call setClassLoader() after addSource(), the SourceUnit will end up with the "wrong" class loader. Should I just pretend that setClassLoader() doesn't exist?
          Hide
          Peter Niederwieser added a comment -

          One more thing that troubles me is that CompilationUnit.setSource(SourceUnit) always forces the CompilationUnit's class loader on the SourceUnit. Therefore, setting a class loader on the SourceUnit has no effect. Is this OK?

          Show
          Peter Niederwieser added a comment - One more thing that troubles me is that CompilationUnit.setSource(SourceUnit) always forces the CompilationUnit's class loader on the SourceUnit. Therefore, setting a class loader on the SourceUnit has no effect. Is this OK?
          Guillaume Laforge made changes -
          Fix Version/s 1.6-rc-2 [ 13832 ]
          Fix Version/s 1.6.1 [ 14852 ]
          Hide
          Guillaume Laforge added a comment -

          I'll let you apply this patch, and amend it as appropriate.

          Show
          Guillaume Laforge added a comment - I'll let you apply this patch, and amend it as appropriate.
          Guillaume Laforge made changes -
          Assignee Jochen Theodorou [ blackdrag ]
          Hide
          blackdrag blackdrag added a comment -

          I now applied the patch, but with a few changes... Firstly I removed the reflective code you added. For a transform to work properly you need access to the compiler classes, so there is no point in excluding the annotation classes for the transform in th the compiler. Also I changed transform loader so it has to be a GroovyClassLoder. The loader used for transforms might be a GroovyClassLoader, but this loader has the ability compile scripts into classes on its own. But since we are currently in a compilation this must not happen. The result would be a deadlock or a cyclic compilation.... that means a file is to be compiled, which needs another file, which then is compiled, but depends on the first file. So if the compiler spawns a new compilation process outside the current one it would have to spawn new ones again and again. So a special loadClass method is used to avoid this cycle. The compiler then identifies classes to compile itself, instead of depending on GroovyClassLoader for this.

          Of course that means the gmaven patch has to be changed to use GroovyClassLoader instead of URLClassLoader, but that shouldn't be a problem.

          Show
          blackdrag blackdrag added a comment - I now applied the patch, but with a few changes... Firstly I removed the reflective code you added. For a transform to work properly you need access to the compiler classes, so there is no point in excluding the annotation classes for the transform in th the compiler. Also I changed transform loader so it has to be a GroovyClassLoder. The loader used for transforms might be a GroovyClassLoader, but this loader has the ability compile scripts into classes on its own. But since we are currently in a compilation this must not happen. The result would be a deadlock or a cyclic compilation.... that means a file is to be compiled, which needs another file, which then is compiled, but depends on the first file. So if the compiler spawns a new compilation process outside the current one it would have to spawn new ones again and again. So a special loadClass method is used to avoid this cycle. The compiler then identifies classes to compile itself, instead of depending on GroovyClassLoader for this. Of course that means the gmaven patch has to be changed to use GroovyClassLoader instead of URLClassLoader, but that shouldn't be a problem.
          Hide
          blackdrag blackdrag added a comment -

          I close this issue as fixed now.... should we keep GROOVY-3169 open?

          Show
          blackdrag blackdrag added a comment - I close this issue as fixed now.... should we keep GROOVY-3169 open?
          blackdrag blackdrag made changes -
          Resolution Fixed [ 1 ]
          Status Open [ 1 ] Closed [ 6 ]
          Peter Niederwieser made changes -
          Status Closed [ 6 ] Reopened [ 4 ]
          Resolution Fixed [ 1 ]
          Hide
          Peter Niederwieser added a comment -

          This patch makes local transforms work in the presence of a custom class loader. It also includes a thorough unit test.

          Show
          Peter Niederwieser added a comment - This patch makes local transforms work in the presence of a custom class loader. It also includes a thorough unit test.
          Peter Niederwieser made changes -
          blackdrag blackdrag made changes -
          Fix Version/s 1.6-rc-2 [ 13832 ]
          Affects Version/s 1.6-rc-2 [ 13832 ]
          Fix Version/s 1.6.1 [ 14852 ]
          blackdrag blackdrag made changes -
          Fix Version/s 1.6 [ 14913 ]
          Fix Version/s 1.6.1 [ 14852 ]
          Hide
          blackdrag blackdrag added a comment -

          patch applied

          Show
          blackdrag blackdrag added a comment - patch applied
          blackdrag blackdrag made changes -
          Resolution Fixed [ 1 ]
          Status Reopened [ 4 ] Resolved [ 5 ]
          Paul King made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          CÚdric Champeau made changes -
          Link This issue is related to GROOVY-5044 [ GROOVY-5044 ]

            People

            • Assignee:
              blackdrag blackdrag
              Reporter:
              Peter Niederwieser
            • Votes:
              4 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: