Clojure

Upgrade ASM to a more current version

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: Release 1.6
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code
  • Approval:
    Ok

Description

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

  1. clj-713-2.diff
    25/Oct/13 1:00 PM
    1.15 MB
    Alex Miller
  2. asm-split.txt
    10/Mar/13 6:34 AM
    521 kB
    Ghadi Shayban
  3. asm41ws.patch
    04/Oct/13 11:05 AM
    1.15 MB
    Alex Miller
  4. asm41.patch
    06/Sep/13 12:47 PM
    1.15 MB
    Alex Miller

Activity

Hide
Ghadi Shayban added a comment -

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/

Show
Ghadi Shayban added a comment - 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/
Hide
Ghadi Shayban added a comment -

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

Show
Ghadi Shayban added a comment - Ran some of Andy Fingerhut's expression microbenchmarks on -master with the patch. There is no difference between performance compared to 1.5.1.
Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Alex Miller added a comment -

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.

Show
Alex Miller added a comment - 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.
Hide
Alex Miller added a comment -

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.

Show
Alex Miller added a comment - 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.
Hide
Mike Anderson added a comment -

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).

Show
Mike Anderson added a comment - 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).
Hide
Stuart Sierra added a comment -

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

Show
Stuart Sierra added a comment - Screening failed. asm41.patch does not apply to master at commit 469005f139d733ae1ca492bbe453b0b343606d3a
Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Andy Fingerhut added a comment -

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

Show
Andy Fingerhut added a comment - Oh, but except for those warning/error messages, the patch applies cleanly to the latest Clojure master.
Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Alex Miller added a comment -

Updated with a new patch that address the whitespace failures.

Show
Alex Miller added a comment - Updated with a new patch that address the whitespace failures.
Hide
Alex Miller added a comment -

sorry Andy! I didn't see your update!

Show
Alex Miller added a comment - sorry Andy! I didn't see your update!
Hide
Andy Fingerhut added a comment -

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

Show
Andy Fingerhut added a comment - No problem. Better 2 people fixing the problem than 0 I will remove my patch since it is identical to yours.
Hide
Andy Fingerhut added a comment - - 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.

Show
Andy Fingerhut added a comment - - 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.
Hide
Stuart Sierra added a comment -

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.

Show
Stuart Sierra added a comment - 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.
Hide
Andy Fingerhut added a comment -

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

Show
Andy Fingerhut added a comment - I've not looked into details yet, but all current patches fail to apply cleanly as of Oct 25, 2013 latest Clojure master.
Hide
Alex Miller added a comment -

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

Show
Alex Miller added a comment - I am updating - it's due to the addition of visitAnnotation in the other patch.
Hide
Alex Miller added a comment -

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

Show
Alex Miller added a comment - Updated patch to apply to master as of Oct 25, 2013.
Hide
Rich Hickey added a comment -

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

Show
Rich Hickey added a comment - Is there a reason why we had to move from reify to proxy of ClassVisitor in AsmReflector? Any perf difference?
Hide
Alex Miller added a comment -

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.

Show
Alex Miller added a comment - 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.
Hide
Alex Miller added a comment -

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:

Before:
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


After:
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.

Show
Alex Miller added a comment - 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:
Before:
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


After:
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.

People

Vote (0)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: