Activity

Attached is a patch that externalizes and moves the ASM lib to the latest version (4.1), which has support for invokeDynamic if a brave soul wants to emit that instruction. Heed fogus's caveats [3].

ASM in this patch is not re-rooted, but an external Maven dependency. It does have the disadvantage of possibly conflicting with other dependents of ASM, but this is the same approach is used by other JVM langs.

Ghadi, out of curiosity, if you do AOT compilation of some Clojure class files, does it produce byte-for-byte identical .class files after your changes vs. before your changes? If so, that would be pretty high assurance of no problems. If not, it would be good to understand what changed.

Andy Fingerhut
added a comment - 11/Mar/13 3:43 PM Ghadi, out of curiosity, if you do AOT compilation of some Clojure class files, does it produce byte-for-byte identical .class files after your changes vs. before your changes? If so, that would be pretty high assurance of no problems. If not, it would be good to understand what changed.

Alex Miller
added a comment - 06/Sep/13 12:47 PM Attached asm41.patch that updates the re-rooted ASM to 4.1. This is an alternative to Ghadi's patch in terms of package choice but should be the same otherwise.

Also, I looked at the .class files emitted before and after the patch for Clojure itself - 97% of ~3k .class files were identical. I looked through a random sampling from the 100 or so that differed and found that they were mostly from

Those all seemed innocuous. My opinion is that as far as I can tell from the Clojure code base itself, ASM 4.1 is ok to go in, however it would of course be interesting to hear the opinion of a diverse set of users in the community.

Alex Miller
added a comment - 06/Sep/13 2:47 PM Also, I looked at the .class files emitted before and after the patch for Clojure itself - 97% of ~3k .class files were identical. I looked through a random sampling from the 100 or so that differed and found that they were mostly from
1) ordering changes in constant pool (constants being swapped)
2) ordering changes in loads in method bytecode
3) changes in calls into asm where things changed from classes to interface calls (invokevirtual vs invokeinterface, etc)
Those all seemed innocuous. My opinion is that as far as I can tell from the Clojure code base itself, ASM 4.1 is ok to go in, however it would of course be interesting to hear the opinion of a diverse set of users in the community.

I think it's a good idea to update this sooner rather than later. Invokedynamic opens up some interesting code generation options, and it's better to make a change like this earlier rather than later so that the kinks can get discovered and worked out (if there are any).

Mike Anderson
added a comment - 06/Sep/13 8:42 PM I think it's a good idea to update this sooner rather than later. Invokedynamic opens up some interesting code generation options, and it's better to make a change like this earlier rather than later so that the kinks can get discovered and worked out (if there are any).

In what way does it fail to apply? Do you mean that it has messages in the output like '1610 lines add whitespace errors' when you use the command below?

git am -s --keep-cr --ignore-whitespace < asm41.patch

If so, that is because there are trailing carriage returns (in addition to line feeds) at the end of many of the lines that are added, probably because the files that Alex copied from have those characters in them. If you want them removed, I can do that and make another patch.

Andy Fingerhut
added a comment - 04/Oct/13 9:12 AM In what way does it fail to apply? Do you mean that it has messages in the output like '1610 lines add whitespace errors' when you use the command below?
git am -s --keep-cr --ignore-whitespace < asm41.patch
If so, that is because there are trailing carriage returns (in addition to line feeds) at the end of many of the lines that are added, probably because the files that Alex copied from have those characters in them. If you want them removed, I can do that and make another patch.

asm41-cleaned.patch is identical to asm41.patch, except all added lines have had trailing whitespace removed, including any carriage returns that were present in the original patch. It applies cleanly to latest master with no warnings or errors, as of Oct 4 2013, and passes all tests.

Andy Fingerhut
added a comment - 04/Oct/13 10:30 AM asm41-cleaned.patch is identical to asm41.patch, except all added lines have had trailing whitespace removed, including any carriage returns that were present in the original patch. It applies cleanly to latest master with no warnings or errors, as of Oct 4 2013, and passes all tests.

Nov 24, 2013 update: Now that all of the patches mentioned in my earlier message below have been applied to master, the latest master compiles and passes all tests cleanly with JDK8 early access b116 (currently the latest version). However, the same unit test named compare-reflect-and-asm in reflect.clj consistently fails, throwing an exception during the call that uses the AsmReflector to get info for a class. All other tests pass.

There are no test failures with this patch for Mac+Apple JDK 1.6, or Linux with any of OpenJDK 1.6, Oracle 1.6 or 1.7, so it appears specific to this patch and JDK 1.8.

Original Oct 4, 2013 message:
This is probably more trouble to bring up than it is worth, but I will mention it anyway, since I've been trying out builds of Clojure with JDK8 early access versions. The behavior may be due to bugs in JDK8 for all I know – I have not tracked them down to their root cause.

The latest Clojure master as of Oct 4 2013 builds with warnings, and fails one unit test, with JDK8 early access b109. I think the unit test failure is fixed by patch clj-1246-fix-type-reflect-exception-patch-v1.txt on CLJ-1246. The compilation warnings are fixed by patch clj-1264-1.txt on CLJ-1264.

If I apply those two patches, JDK8 b109 builds Clojure and passes tests fine, although it gives a warning that 1.5 is obsolete, and says it is ignoring that option.

If I apply those two patches, then clj-1268.patch on CLJ-1268, JDK8 b109 builds Clojure and passes tests fine, and it eliminates the warning about 1.5. It still gives the warning "warning: [options] bootstrap class path not set in conjunction with -source 1.6", but earlier current Clojure master and JDK7 gives the same warning except with version 1.5, and that has never been a problem that I can tell.

If I apply those three patches, then asm41ws.patch, JDK8 b109 consistently fails to pass the tests, in particular on of the reflector tests.

Like I said, maybe this is a "wait and see" problem, but given the scope of the ASM patch, I wanted to give some advance warning of a possible future problem.

Andy Fingerhut
added a comment - 04/Oct/13 12:03 PM - edited Nov 24, 2013 update: Now that all of the patches mentioned in my earlier message below have been applied to master, the latest master compiles and passes all tests cleanly with JDK8 early access b116 (currently the latest version). However, the same unit test named compare-reflect-and-asm in reflect.clj consistently fails, throwing an exception during the call that uses the AsmReflector to get info for a class. All other tests pass.
There are no test failures with this patch for Mac+Apple JDK 1.6, or Linux with any of OpenJDK 1.6, Oracle 1.6 or 1.7, so it appears specific to this patch and JDK 1.8.
Original Oct 4, 2013 message:
This is probably more trouble to bring up than it is worth, but I will mention it anyway, since I've been trying out builds of Clojure with JDK8 early access versions. The behavior may be due to bugs in JDK8 for all I know – I have not tracked them down to their root cause.
The latest Clojure master as of Oct 4 2013 builds with warnings, and fails one unit test, with JDK8 early access b109. I think the unit test failure is fixed by patch clj-1246-fix-type-reflect-exception-patch-v1.txt on CLJ-1246. The compilation warnings are fixed by patch clj-1264-1.txt on CLJ-1264.
If I apply those two patches, JDK8 b109 builds Clojure and passes tests fine, although it gives a warning that 1.5 is obsolete, and says it is ignoring that option.
If I apply those two patches, then clj-1268.patch on CLJ-1268, JDK8 b109 builds Clojure and passes tests fine, and it eliminates the warning about 1.5. It still gives the warning "warning: [options] bootstrap class path not set in conjunction with -source 1.6", but earlier current Clojure master and JDK7 gives the same warning except with version 1.5, and that has never been a problem that I can tell.
If I apply those three patches, then asm41ws.patch, JDK8 b109 consistently fails to pass the tests, in particular on of the reflector tests.
Like I said, maybe this is a "wait and see" problem, but given the scope of the ASM patch, I wanted to give some advance warning of a possible future problem.

I verified that Clojure builds and its tests pass. I also tried building and testing a few contrib libraries with the new Clojure JAR, all of which passed. Hudson will test with all contrib libraries when this patch is committed to Git master.

Stuart Sierra
added a comment - 04/Oct/13 1:10 PM Screened successfully now that I'm using the correct git am command.
I verified that Clojure builds and its tests pass. I also tried building and testing a few contrib libraries with the new Clojure JAR, all of which passed. Hudson will test with all contrib libraries when this patch is committed to Git master.

The change was motivated by refactoring in ASM. In ASM 3, ClassVisitor is an interface. In ASM 4, ClassVisitor is an abstract class which we must subclass. I will try to get a feel for any perf difference.

Alex Miller
added a comment - 22/Nov/13 11:36 AM The change was motivated by refactoring in ASM. In ASM 3, ClassVisitor is an interface. In ASM 4, ClassVisitor is an abstract class which we must subclass. I will try to get a feel for any perf difference.

The AsmReflector is an alternate reflector provided for the clojure.reflect functionality, as such I do not believe it's used in Clojure itself or likely to be a significant problem even if there were performance issues.

Alex Miller
added a comment - 23/Nov/13 11:30 PM The AsmReflector is an alternate reflector provided for the clojure.reflect functionality, as such I do not believe it's used in Clojure itself or likely to be a significant problem even if there were performance issues.
I ran some command line tests (no lein) using criterium:

which showed a 2% worse performance for this case.
Based on the necessity of the change due to the ASM refactoring, the minimal performance difference, and the relatively non-critical path of this code I am marking Screened again.