Clojure

Clojure resolves to wrong deftype classes when AOT compiling or reloading

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Critical Critical
  • Resolution: Unresolved
  • Affects Version/s: Release 1.3, Release 1.4, Release 1.5, Release 1.6, Release 1.7
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Patch:
    Code and Test
  • Approval:
    Vetted

Description

Compiling a class via `deftype` during AOT compilation gives different results for the different constructors. These hashes should be identical.

user=> (binding [*compile-files* true] (eval '(deftype Abc [])))
user.Abc
user=> (hash Abc)
16446700
user=> (hash (class (->Abc)))
31966239 ;; should be 16446700

This also means that whenever there's a stale AOT compiled deftype class in the classpath, that class will be used rather then the JIT compiled one, breaking repl interaction.

Another demonstration of this classloader issue (from CLJ-1495) when reloading deftypes (no AOT) :

user> (defrecord Foo [bar])
user.Foo
user> (= (->Foo 42) #user.Foo{:bar 42}) ;;expect this to evaluate to true
true
user> (defrecord Foo [bar])
user.Foo
user> (= (->Foo 42) #user.Foo{:bar 42}) ;;expect this to evaluate to true also -- but it doesn't!
false
user>

This bug also affects AOT compilation of multimethods that dispatch on a class, this affected core.match for years see http://dev.clojure.org/jira/browse/MATCH-86, http://dev.clojure.org/jira/browse/MATCH-98. David had to work-around this issue by using a bunch of protocols instead of multimethods.

Cause of the bug: currently clojure uses Class.forName to resolve a class from a class name, which ignores the class cache from DynamicClassLoader thus reloading deftypes or mixing AOT compilation at the repl with deftypes breaks, resolving to the wrong class.

Approach: the current patch (CLJ-979-v7.patch) addresses this issue in multiple ways:

  • it makes RT.classForName/classForNameNonLoading look in the class cache before delegating to Class/forName if the current classloader is not a DynamicClassLoader (this incidentally addresses also CLJ-1457)
  • it makes clojure use RT.classForName/classForNameNonLoading instead of Class/forName
  • it overrides Classloader/loadClass so that it's class cache aware – this method is used by the jvm to load classes
  • it changes gen-interface to always emit an in-memory interface along the [optional] in disk interface so that the in-memory class is always updated.
  1. CLJ-979.patch
    11/Dec/14 1:40 PM
    8 kB
    Nicola Mometto
  2. clj-979-symptoms.patch
    29/Mar/14 2:27 PM
    2 kB
    Ambrose Bonnaire-Sergeant
  3. CLJ-979-v2.patch
    12/Dec/14 6:06 AM
    9 kB
    Nicola Mometto
  4. CLJ-979-v3.patch
    12/Dec/14 9:38 AM
    9 kB
    Nicola Mometto
  5. CLJ-979-v4.patch
    12/Dec/14 10:09 AM
    9 kB
    Nicola Mometto
  6. CLJ-979-v5.patch
    12/Dec/14 1:07 PM
    10 kB
    Nicola Mometto
  7. CLJ-979-v6.patch
    15/Dec/14 5:43 PM
    10 kB
    Nicola Mometto
  8. CLJ-979-v7.patch
    16/Dec/14 12:50 PM
    10 kB
    Nicola Mometto

Activity

Hide
Scott Lowe added a comment -

I can't reproduce this under Clojure 1.3 or 1.4, and Leiningen 1.7.1 on either Java 1.7.0-jdk7u4-b21 OpenJDK 64-Bit or Java 1.6.0_31 Java HotSpot 64-Bit. OS is Mac OS X 10.7.

Edmund, how are you running this AOT code? I wrapped your code in a main function and built an uberjar from it.

Show
Scott Lowe added a comment - I can't reproduce this under Clojure 1.3 or 1.4, and Leiningen 1.7.1 on either Java 1.7.0-jdk7u4-b21 OpenJDK 64-Bit or Java 1.6.0_31 Java HotSpot 64-Bit. OS is Mac OS X 10.7. Edmund, how are you running this AOT code? I wrapped your code in a main function and built an uberjar from it.
Hide
Edmund Jackson added a comment -

Hi Scott,

Interesting.

I have two use cases
1. AOT compile and call from repl.
My steps: git clone, lein compile, lein repl, (use 'aots.death), (in-ns 'aots.death), (= (class (Dontwork. nil)) (class (map->Dontwork {:a 1}))) => false

2. My original use case, which I've minimised here, is an AOT ns, producing a genclass that is called instantiated from other Java (no main). This produces the same error. I will produce an example of this and post it too.

Show
Edmund Jackson added a comment - Hi Scott, Interesting. I have two use cases 1. AOT compile and call from repl. My steps: git clone, lein compile, lein repl, (use 'aots.death), (in-ns 'aots.death), (= (class (Dontwork. nil)) (class (map->Dontwork {:a 1}))) => false 2. My original use case, which I've minimised here, is an AOT ns, producing a genclass that is called instantiated from other Java (no main). This produces the same error. I will produce an example of this and post it too.
Hide
Edmund Jackson added a comment -

Hi Scott,

Here is an example of it failing in the interop case: https://github.com/ejackson/aotquestion2
The steps I'm following to compile this all up are

git clone git@github.com:ejackson/aotquestion2.git
cd aotquestion2/cljside/
lein uberjar
lein install
cd ../javaside/
mvn package
java -jar ./target/aotquestion-1.0-SNAPSHOT.jar

and it dies with this:

Exception in thread "main" java.lang.ClassCastException: cljside.core.Dontwork cannot be cast to cljside.core.Dontwork
at cljside.MyClass.makeDontwork(Unknown Source)
at aotquestion.App.main(App.java:8)

The error message is really confusing (to me, anyway), but I think its the same root problem as for the REPL case.

What do you see when you run the above ?

Show
Edmund Jackson added a comment - Hi Scott, Here is an example of it failing in the interop case: https://github.com/ejackson/aotquestion2 The steps I'm following to compile this all up are git clone git@github.com:ejackson/aotquestion2.git cd aotquestion2/cljside/ lein uberjar lein install cd ../javaside/ mvn package java -jar ./target/aotquestion-1.0-SNAPSHOT.jar and it dies with this: Exception in thread "main" java.lang.ClassCastException: cljside.core.Dontwork cannot be cast to cljside.core.Dontwork at cljside.MyClass.makeDontwork(Unknown Source) at aotquestion.App.main(App.java:8) The error message is really confusing (to me, anyway), but I think its the same root problem as for the REPL case. What do you see when you run the above ?
Hide
Scott Lowe added a comment -

Ah, yes, looks like my initial attempt to reproduce was too simplistic. I used your second git repo, and can now confirm that it's failing for me with the same error.

Show
Scott Lowe added a comment - Ah, yes, looks like my initial attempt to reproduce was too simplistic. I used your second git repo, and can now confirm that it's failing for me with the same error.
Hide
Scott Lowe added a comment -

I looked into this a little further and the AOT generated code looks correct, in the sense that both code paths appear to be returning the same type.

However, I wonder if this is really a ClassLoader issue, whereby two definitions of the same class are being loaded at different times, because that would cause the x.y.Class cannot be cast to x.y.Class exception that we're seeing here.

Show
Scott Lowe added a comment - I looked into this a little further and the AOT generated code looks correct, in the sense that both code paths appear to be returning the same type. However, I wonder if this is really a ClassLoader issue, whereby two definitions of the same class are being loaded at different times, because that would cause the x.y.Class cannot be cast to x.y.Class exception that we're seeing here.
Hide
Steve Miner added a comment -

This could be related to CLJ-1157 which deals with a ClassLoader issue with AOT compiled code.

Show
Steve Miner added a comment - This could be related to CLJ-1157 which deals with a ClassLoader issue with AOT compiled code.
Hide
Ambrose Bonnaire-Sergeant added a comment -

I've tried this patch attached to CLJ-1157 and it did not solve this issue.

Show
Ambrose Bonnaire-Sergeant added a comment - I've tried this patch attached to CLJ-1157 and it did not solve this issue.
Hide
Ambrose Bonnaire-Sergeant added a comment -

This bug seems to be rooted in different behaviour for do/let under compilation. Attached a patch showing these symptoms in the hope it helps people find the cause.

Show
Ambrose Bonnaire-Sergeant added a comment - This bug seems to be rooted in different behaviour for do/let under compilation. Attached a patch showing these symptoms in the hope it helps people find the cause.
Hide
Peter Taoussanis added a comment -

Just a quick note to confirm that this still seems to be around as of Clojure 1.7.0-alpha2. Don't have any useful input on possible solutions, sorry.

Show
Peter Taoussanis added a comment - Just a quick note to confirm that this still seems to be around as of Clojure 1.7.0-alpha2. Don't have any useful input on possible solutions, sorry.
Hide
Alex Miller added a comment -

Duplicates - CLJ-1495, CLJ-1132

Show
Alex Miller added a comment - Duplicates - CLJ-1495, CLJ-1132
Hide
Nicola Mometto added a comment -

The attached patch fixes the classloader issues by routing RT.classForName & variants through the DynamicClassLoader class cache before invoking Class.forName

Show
Nicola Mometto added a comment - The attached patch fixes the classloader issues by routing RT.classForName & variants through the DynamicClassLoader class cache before invoking Class.forName
Hide
Nicola Mometto added a comment -

Re-adding triaged status added by Alex Miller that got accidentaly nuked by a race-condition between my edits to the ticket description and Alex's ones

Show
Nicola Mometto added a comment - Re-adding triaged status added by Alex Miller that got accidentaly nuked by a race-condition between my edits to the ticket description and Alex's ones
Hide
Nicola Mometto added a comment - - edited

0001-CLJ-979-make-clojure-resolve-to-the-correct-Class-in-v2.patch is the same as 0001-CLJ-979-make-clojure-resolve-to-the-correct-Class-in.patch except it unconditionally looks for classes in the class cache of DynamicClassLoader, even if baseLoader() is not a DynamicClassLoader.
This fixes the bug of CLJ-1457 but might just be a workaround

Show
Nicola Mometto added a comment - - edited 0001-CLJ-979-make-clojure-resolve-to-the-correct-Class-in-v2.patch is the same as 0001-CLJ-979-make-clojure-resolve-to-the-correct-Class-in.patch except it unconditionally looks for classes in the class cache of DynamicClassLoader, even if baseLoader() is not a DynamicClassLoader. This fixes the bug of CLJ-1457 but might just be a workaround
Hide
Michael Blume added a comment - - edited

Current patch blows up my Clojure build

https://gist.github.com/MichaelBlume/aa26fc715cbbdf711290

Show
Michael Blume added a comment - - edited Current patch blows up my Clojure build https://gist.github.com/MichaelBlume/aa26fc715cbbdf711290
Hide
Nicola Mometto added a comment -

Michael: the current patch builds clojure fine for me, I'll try to reproduce. Which jvm version are you using?

Show
Nicola Mometto added a comment - Michael: the current patch builds clojure fine for me, I'll try to reproduce. Which jvm version are you using?
Hide
Michael Blume added a comment -

[14:24][michael.blume@tcc-michael-4:~/workspace/clojure((0fc43db...))]$ java -version
java version "1.8.0_25"
Java(TM) SE Runtime Environment (build 1.8.0_25-b17)
Java HotSpot(TM) 64-Bit Server VM (build 25.25-b02, mixed mode)
[14:24][michael.blume@tcc-michael-4:~/workspace/clojure((0fc43db...))]$ mvn -version
Apache Maven 3.2.3 (33f8c3e1027c3ddde99d3cdebad2656a31e8fdf4; 2014-08-11T13:58:10-07:00)
Maven home: /usr/local/Cellar/maven/3.2.3/libexec
Java version: 1.8.0_25, vendor: Oracle Corporation
Java home: /Library/Java/JavaVirtualMachines/jdk1.8.0_25.jdk/Contents/Home/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "10.10.1", arch: "x86_64", family: "mac"

build was after I applied the patch to the current master branch of the clojure github repo

Show
Michael Blume added a comment - [14:24][michael.blume@tcc-michael-4:~/workspace/clojure((0fc43db...))]$ java -version java version "1.8.0_25" Java(TM) SE Runtime Environment (build 1.8.0_25-b17) Java HotSpot(TM) 64-Bit Server VM (build 25.25-b02, mixed mode) [14:24][michael.blume@tcc-michael-4:~/workspace/clojure((0fc43db...))]$ mvn -version Apache Maven 3.2.3 (33f8c3e1027c3ddde99d3cdebad2656a31e8fdf4; 2014-08-11T13:58:10-07:00) Maven home: /usr/local/Cellar/maven/3.2.3/libexec Java version: 1.8.0_25, vendor: Oracle Corporation Java home: /Library/Java/JavaVirtualMachines/jdk1.8.0_25.jdk/Contents/Home/jre Default locale: en_US, platform encoding: UTF-8 OS name: "mac os x", version: "10.10.1", arch: "x86_64", family: "mac" build was after I applied the patch to the current master branch of the clojure github repo
Hide
Andy Fingerhut added a comment -

I am seeing a similar compilation error as Michael Blume, with both JDK 1.7 and 1.8 on Mac OS X 10.9.5.

By accident I found that if I take latest Clojure master and do 'mvn package', then apply the patch CLJ-979.patch dated Dec 11 2014, then do 'mvn package' again without 'mvn clean', it compiles with no errors. If I do 'mvn clean' then 'mvn package' in a patched tree, I get the error every time I've tried.

Show
Andy Fingerhut added a comment - I am seeing a similar compilation error as Michael Blume, with both JDK 1.7 and 1.8 on Mac OS X 10.9.5. By accident I found that if I take latest Clojure master and do 'mvn package', then apply the patch CLJ-979.patch dated Dec 11 2014, then do 'mvn package' again without 'mvn clean', it compiles with no errors. If I do 'mvn clean' then 'mvn package' in a patched tree, I get the error every time I've tried.
Hide
Nicola Mometto added a comment -

The updated patch fixes the LinkageError Andy and Michael were getting.

Andy, Michael, can you confirm?

Show
Nicola Mometto added a comment - The updated patch fixes the LinkageError Andy and Michael were getting. Andy, Michael, can you confirm?
Hide
Nicola Mometto added a comment -

Added more testcases to new patch

Show
Nicola Mometto added a comment - Added more testcases to new patch
Hide
Nicola Mometto added a comment -

Cleaned up the patch from whitespace changes

Show
Nicola Mometto added a comment - Cleaned up the patch from whitespace changes
Hide
Andy Fingerhut added a comment -

I tried latest Clojure master plus patch CLJ-979-v4.patch, dated 12 Dec 2014, with Mac OS X 10.9.5 + JDK7, and Ubuntu Linux 14.04 with JDKs 6 through 9, and 'mvn clean' followed by 'mvn package' built and passed tests successfully with all of them.

I did notice that some files were created in the test directory that were not cleaned up by the end of the test, which you can use 'git status .' to see. Not sure if that is considered a bad thing for Clojure tests.

Show
Andy Fingerhut added a comment - I tried latest Clojure master plus patch CLJ-979-v4.patch, dated 12 Dec 2014, with Mac OS X 10.9.5 + JDK7, and Ubuntu Linux 14.04 with JDKs 6 through 9, and 'mvn clean' followed by 'mvn package' built and passed tests successfully with all of them. I did notice that some files were created in the test directory that were not cleaned up by the end of the test, which you can use 'git status .' to see. Not sure if that is considered a bad thing for Clojure tests.
Hide
Nicola Mometto added a comment -

Thanks Andy, I've updated the patch and it now should remove all temporary classes created by the test.
It's probably not the best way to do it but I couldn't figure out how to do it another way.

Show
Nicola Mometto added a comment - Thanks Andy, I've updated the patch and it now should remove all temporary classes created by the test. It's probably not the best way to do it but I couldn't figure out how to do it another way.
Hide
Michael Blume added a comment -

Yep, looks good to me =)

Show
Michael Blume added a comment - Yep, looks good to me =)
Hide
Alex Miller added a comment -

Thanks first to Nicola for all his work so far on this!

Some feedback:
1) While the ticket itself isn't bad, I would really like to focus the title and description on a crisp statement of the real problem now that we understand it more. I'd like help on making sure we capture that correctly - how is this for a title: "Uses of Class.forName() miss classes in DynamicClassLoader cache?" ?

Similarly, the description should focus on the problem and show some examples. The defrecord one is good. The first example works for me before the patch and fails after?

2) The crux of this whole thing is the change in loading order in DCL.loadClass() - changing this is a big deal. We really need broader testing with things likely to be affected - off the top of my head: Immutant, Pomegranate, Leiningen, or anything else that monkeys with classloader stuff. Maybe something with Eclipse/OSGi if there is something testable out there.

3) DynamicClassLoader comments:
a) loadClass(String name) - I believe this is identical to the super impl, so can be removed.
b) findClass(String name) - now that we are hijacking loadClass(), I'm not sure it's even necessary to implement this or to call super.findClass() - if you get to the super.findClass(), I would expect that to always throw CNFE. Potentially, this method could even be removed (but this might do bad things if there are subclasses of DCL out there in the wild).
c) loadClass(String name, ...) - instead of calling findClass() and using the CNFE for control flow, you could just directly call findInMemoryClass(), then use a returned null as the decision factor. Again, this is possibly bad if there are DCL subclasses, so I'm on the fence about it.

4) Is the change in gen-interface something that should be a separate ticket? Seems like it could be separable.

5) I don't like the test changes with respect to set up and cleanup. The build already supports compiling a subset of test ns'es (like clojure.test-clojure.genclass.examples). I'd prefer to use that existing mechanism if at all possible. Check the build.xml for the hard-coded ns list.

6) What are the performance implications? I'm not expecting they are significant but we just made a bunch of changes for compilation performance and I'd hate to undo them all. Could findInMemoryClass be smarter about avoiding checks that can't succeed (avoiding "java.*" for example?).

Show
Alex Miller added a comment - Thanks first to Nicola for all his work so far on this! Some feedback: 1) While the ticket itself isn't bad, I would really like to focus the title and description on a crisp statement of the real problem now that we understand it more. I'd like help on making sure we capture that correctly - how is this for a title: "Uses of Class.forName() miss classes in DynamicClassLoader cache?" ? Similarly, the description should focus on the problem and show some examples. The defrecord one is good. The first example works for me before the patch and fails after? 2) The crux of this whole thing is the change in loading order in DCL.loadClass() - changing this is a big deal. We really need broader testing with things likely to be affected - off the top of my head: Immutant, Pomegranate, Leiningen, or anything else that monkeys with classloader stuff. Maybe something with Eclipse/OSGi if there is something testable out there. 3) DynamicClassLoader comments: a) loadClass(String name) - I believe this is identical to the super impl, so can be removed. b) findClass(String name) - now that we are hijacking loadClass(), I'm not sure it's even necessary to implement this or to call super.findClass() - if you get to the super.findClass(), I would expect that to always throw CNFE. Potentially, this method could even be removed (but this might do bad things if there are subclasses of DCL out there in the wild). c) loadClass(String name, ...) - instead of calling findClass() and using the CNFE for control flow, you could just directly call findInMemoryClass(), then use a returned null as the decision factor. Again, this is possibly bad if there are DCL subclasses, so I'm on the fence about it. 4) Is the change in gen-interface something that should be a separate ticket? Seems like it could be separable. 5) I don't like the test changes with respect to set up and cleanup. The build already supports compiling a subset of test ns'es (like clojure.test-clojure.genclass.examples). I'd prefer to use that existing mechanism if at all possible. Check the build.xml for the hard-coded ns list. 6) What are the performance implications? I'm not expecting they are significant but we just made a bunch of changes for compilation performance and I'd hate to undo them all. Could findInMemoryClass be smarter about avoiding checks that can't succeed (avoiding "java.*" for example?).
Hide
Nicola Mometto added a comment - - edited

1) It's not really about Class.forName() specifically, it's about DynamicClassLoader not being class cache aware in the loadClass method. The JVM uses the classloader loadClass method for resolving all kind of class usages,
including but not limited to Class.forName() (i.e. when loading some bytecode containing a "new" instruction, that class reference will be resolved via a call to loadClass)
I'll try to make the documentation a bit more clear, the first example is an exhibit of the bugged behaviour, the two calls should output the same hash.

2,4) So, there are 3 approaches to how DynamicClassLoader could go at it:

  • Prefer in-disk classes over in-memory classes, roughly the current approach (sometimes it will pick the in-memory class over the in-disk one causing weird bugs like Foo. and ->Foo constructing different classes), has the
    negative effect of breaking interaction between AOT compilation and JIT loading, which has created all sorts of troubles with redefinig deftypes/defprotocols in repls while having stale classfiles in disk.
  • Always pick the most-updated class, this has the advangate of being always correct but has several disadvantages that make it inpracticable in my opinion: we'd have to keep track of the timestamp in which a dynamic class
    is defined, and make the loadClass implementation such that if there a class is both in-memory and in-disk, it compares the timestamps and select the most updated one. This would complicate the implementation a lot and we'd
    likely have to pay a substantial performance hit.
  • Prefer in-memory classes over in-disk classes, the approach proposed in the current patches. It has the advantage of being almost always correct, make repl interaction & jit/aot mixing work fine and the implementation is
    mostly straightforward. The downside is that in cases like gen-class where an AOT class can actually be the most updated version, the in-memory version will be used. In clojure all the forms that do bytecode emission either
    only do AOT compilation or do AOT compilation on demand and always load the class in memory, except gen-interface that doesn't load the class in memory if it's being AOT compiled. Changing its semantics to behave like the
    other jit/aot compiling forms (deftype/defrecord/reify) is the only way to make this approach work so I don't think this should go in another ticket.

5) I don't like the previous testing strategy either but couldn't figure out a better way. Thanks for the pointer on the already in-place infrastructure, I'll check it out and update the patch

In the meantime I've uploaded a new patch addressing 3 and 6. Specifically:
3) I removed the unnecessary loadClass(String) arity, I've made loadClass(String, boolean) use findInMemoryClass(String) directly rather than relying on findClass(String) since nowhere in the documentation it guarantess that
findClass will be used by loadClass. However I've left the findClass(String) implementation in place in case there's code out there that relies on this.
6) I haven't done any serious testing but I haven't noticed any significant difference in compile times for any of my tools.* contrib libraries with the current patch. Filtering "java.*" class names before the inMemory check
didn't seem to produce any difference so it's not included in the updated patch. However I'll probably include an alternative patch with that filtering to do more performance testings and see if it can actually help a bit.

All this said, I'm afraid that I won't have time to personally do an in-depth benchmarking & cross project testing of this patch. I've been spending almost all the free time I had in the past weeks working through a bunch of tickets (mostly this one) but now because of school and other commitments I can't promise I will be able to do anything more than maintaining the current patch & answering to any questions about the bug. Any help in moving this ticket further would be appreciated, in particular to address points 2 and 6.

Show
Nicola Mometto added a comment - - edited 1) It's not really about Class.forName() specifically, it's about DynamicClassLoader not being class cache aware in the loadClass method. The JVM uses the classloader loadClass method for resolving all kind of class usages, including but not limited to Class.forName() (i.e. when loading some bytecode containing a "new" instruction, that class reference will be resolved via a call to loadClass) I'll try to make the documentation a bit more clear, the first example is an exhibit of the bugged behaviour, the two calls should output the same hash. 2,4) So, there are 3 approaches to how DynamicClassLoader could go at it:
  • Prefer in-disk classes over in-memory classes, roughly the current approach (sometimes it will pick the in-memory class over the in-disk one causing weird bugs like Foo. and ->Foo constructing different classes), has the negative effect of breaking interaction between AOT compilation and JIT loading, which has created all sorts of troubles with redefinig deftypes/defprotocols in repls while having stale classfiles in disk.
  • Always pick the most-updated class, this has the advangate of being always correct but has several disadvantages that make it inpracticable in my opinion: we'd have to keep track of the timestamp in which a dynamic class is defined, and make the loadClass implementation such that if there a class is both in-memory and in-disk, it compares the timestamps and select the most updated one. This would complicate the implementation a lot and we'd likely have to pay a substantial performance hit.
  • Prefer in-memory classes over in-disk classes, the approach proposed in the current patches. It has the advantage of being almost always correct, make repl interaction & jit/aot mixing work fine and the implementation is mostly straightforward. The downside is that in cases like gen-class where an AOT class can actually be the most updated version, the in-memory version will be used. In clojure all the forms that do bytecode emission either only do AOT compilation or do AOT compilation on demand and always load the class in memory, except gen-interface that doesn't load the class in memory if it's being AOT compiled. Changing its semantics to behave like the other jit/aot compiling forms (deftype/defrecord/reify) is the only way to make this approach work so I don't think this should go in another ticket.
5) I don't like the previous testing strategy either but couldn't figure out a better way. Thanks for the pointer on the already in-place infrastructure, I'll check it out and update the patch In the meantime I've uploaded a new patch addressing 3 and 6. Specifically: 3) I removed the unnecessary loadClass(String) arity, I've made loadClass(String, boolean) use findInMemoryClass(String) directly rather than relying on findClass(String) since nowhere in the documentation it guarantess that findClass will be used by loadClass. However I've left the findClass(String) implementation in place in case there's code out there that relies on this. 6) I haven't done any serious testing but I haven't noticed any significant difference in compile times for any of my tools.* contrib libraries with the current patch. Filtering "java.*" class names before the inMemory check didn't seem to produce any difference so it's not included in the updated patch. However I'll probably include an alternative patch with that filtering to do more performance testings and see if it can actually help a bit. All this said, I'm afraid that I won't have time to personally do an in-depth benchmarking & cross project testing of this patch. I've been spending almost all the free time I had in the past weeks working through a bunch of tickets (mostly this one) but now because of school and other commitments I can't promise I will be able to do anything more than maintaining the current patch & answering to any questions about the bug. Any help in moving this ticket further would be appreciated, in particular to address points 2 and 6.
Hide
Alex Miller added a comment -

Thanks Nicola. I'll certainly take over sheparding the bug and appeal to the greater community for help in broad testing when I think we're ready for that.

Show
Alex Miller added a comment - Thanks Nicola. I'll certainly take over sheparding the bug and appeal to the greater community for help in broad testing when I think we're ready for that.
Hide
Nicola Mometto added a comment -

Updated patch with better tests, addressing Alex Miller's comments.

Show
Nicola Mometto added a comment - Updated patch with better tests, addressing Alex Miller's comments.

People

Vote (13)
Watch (8)

Dates

  • Created:
    Updated: