<< Back to previous view

[CLJ-713] Upgrade ASM to a more current version Created: 11/Jan/11  Updated: 11/Jan/14  Resolved: 11/Jan/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.6

Type: Enhancement Priority: Critical
Reporter: Aaron Bedra Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File asm41.patch     Text File asm41ws.patch     Text File asm-split.txt     File clj-713-2.diff    
Patch: Code
Approval: Ok


Upgrade ASM to latest version.
See design page for more info: http://dev.clojure.org/display/design/ASM

  • Move to the latest stable ASM (4.1)
  • Still re-rooted it under clojure.asm
  • Subset ASM to leave out unnecessary packages as before
  • Investigate stack frame support (will be needed in the future)

Patch: clj-713-2.diff

Screened by: Stuart Sierra

Comment by Ghadi Shayban [ 10/Mar/13 6:34 AM ]

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.

Can classloaders mitigate that problem?

[1] Recent post on clojure-dev, oblivious of [2] https://groups.google.com/d/msg/clojure-dev/iX-ZFPk_zyE/Es3mDOdG3bYJ
[2] https://groups.google.com/d/msg/clojure-dev/ahJXBT1vG68/_fEEM6Lc7LcJ
[3] http://blog.fogus.me/2011/10/14/why-clojure-doesnt-need-invokedynamic-but-it-might-be-nice/

Comment by Ghadi Shayban [ 11/Mar/13 10:41 AM ]

Ran some of Andy Fingerhut's expression microbenchmarks on -master with the patch. There is no difference between performance compared to 1.5.1.

Comment by Andy Fingerhut [ 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.

Comment by Alex Miller [ 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.

Comment by Alex Miller [ 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.

Comment by Mike Anderson [ 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).

Comment by Stuart Sierra [ 04/Oct/13 9:02 AM ]

Screening failed. asm41.patch does not apply to master at commit 469005f139d733ae1ca492bbe453b0b343606d3a

Comment by Andy Fingerhut [ 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.

Comment by Andy Fingerhut [ 04/Oct/13 9:12 AM ]

Oh, but except for those warning/error messages, the patch applies cleanly to the latest Clojure master.

Comment by Andy Fingerhut [ 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.

Comment by Alex Miller [ 04/Oct/13 11:05 AM ]

Updated with a new patch that address the whitespace failures.

Comment by Alex Miller [ 04/Oct/13 11:06 AM ]

sorry Andy! I didn't see your update!

Comment by Andy Fingerhut [ 04/Oct/13 11:37 AM ]

No problem. Better 2 people fixing the problem than 0 I will remove my patch since it is identical to yours.

Comment by Andy Fingerhut [ 04/Oct/13 12:03 PM ]

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.

Comment by Stuart Sierra [ 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.

Comment by Andy Fingerhut [ 25/Oct/13 12:21 PM ]

I've not looked into details yet, but all current patches fail to apply cleanly as of Oct 25, 2013 latest Clojure master.

Comment by Alex Miller [ 25/Oct/13 12:36 PM ]

I am updating - it's due to the addition of visitAnnotation in the other patch.

Comment by Alex Miller [ 25/Oct/13 12:58 PM ]

Updated patch to apply to master as of Oct 25, 2013.

Comment by Rich Hickey [ 22/Nov/13 10:46 AM ]

Is there a reason why we had to move from reify to proxy of ClassVisitor in AsmReflector? Any perf difference?

Comment by Alex Miller [ 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.

Comment by Alex Miller [ 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:

(use 'criterium.core)
(use 'clojure.reflect)
(def a (clojure.reflect.AsmReflector. (.getContextClassLoader (Thread/currentThread))))
(bench (do-reflect a java.lang.String))

with the following results:

user=> (bench (do-reflect a java.lang.String))
Evaluation count : 76500 in 60 samples of 1275 calls.
             Execution time mean : 793.155510 µs          // <- here
    Execution time std-deviation : 6.806142 µs
   Execution time lower quantile : 783.297262 µs ( 2.5%)
   Execution time upper quantile : 809.091007 µs (97.5%)
                   Overhead used : 1.562019 ns

Found 3 outliers in 60 samples (5.0000 %)
	low-severe	 3 (5.0000 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers

user=> (bench (do-reflect a java.lang.String))
Evaluation count : 73860 in 60 samples of 1231 calls.
             Execution time mean : 810.945774 µs           // <- here
    Execution time std-deviation : 7.719422 µs
   Execution time lower quantile : 797.452517 µs ( 2.5%)
   Execution time upper quantile : 827.689093 µs (97.5%)
                   Overhead used : 1.586383 ns

Found 3 outliers in 60 samples (5.0000 %)
	low-severe	 3 (5.0000 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers

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.

Generated at Tue Jan 16 17:50:03 CST 2018 using JIRA 4.4#649-r158309.