<< Back to previous view

[CLJ-1472] The locking macro fails bytecode verification on ART runtime Created: 23/Jul/14  Updated: 01/Aug/14

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

Type: Defect Priority: Major
Reporter: Adam Clements Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Android ART runtime


Attachments: Text File 0001-Move-monitor-enter-outside-try-block.patch    
Patch: Code
Approval: Triaged

 Description   

Android ART runs compile time verification on bytecode and was failing on any usage of the locking macro. Examination of the bytecode as compared to a java synchronized block shows up a number of differences:
https://gist.github.com/AdamClements/2ae6c4919964b71eb470

Having the monitor-enter inside the try block seems wrong to me, as surely if the lock fails to be acquired, it shouldn't be released with monitor-exit. Moving the monitor enter outside the try block seems to have resolved the issue and android no longer complains about usages of locking and all clojure tests still pass.

Java's generated code goes further and catches any exceptions generated by the monitor-exit itself and retries indefinitely (I believe the logic is that then at least your deadlock is in the right place, and not next time something else attempts to acquire a lock on the same object). I don't think that this can be replicated in clojure without getting down to the bytecode emitting level though and it doesn't seem to be an issue for the ART verifier.



 Comments   
Comment by Adam Clements [ 24/Jul/14 11:17 AM ]

After using this a little more, I've found that moving this outside the try block breaks nREPL.

Looking at the bytecode, the monitorenter for the locking in clojure.tools.nrepl.middleware.session/session-out and in a few other places ends up in an entirely different method definition and we now get a JVM IllegalMonitorStateException as well as an ART verification error for this function.

Comment by Andy Fingerhut [ 01/Aug/14 9:08 PM ]

Adam, I cannot comment on whether your patch is of interest or not, but it is true that no patch will be committed to Clojure if the author has not signed a Contributor Agreement, which can now be done on-line at http://clojure.org/contributing





[CLJ-1467] Implement Comparable in PersistentList Created: 17/Jul/14  Updated: 17/Jul/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Pascal Germroth Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

PersistentVector implements Comparable already.






[CLJ-1459] records should support transient Created: 05/Jul/14  Updated: 06/Jul/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord


 Description   

user=> (defrecord R [a])
user.R
user=> (transient (->R nil))
ClassCastException user.R cannot be cast to clojure.lang.IEditableCollection clojure.core/transient (core.clj:3060)






[CLJ-1456] The compiler ignores too few or too many arguments to throw Created: 30/Jun/14  Updated: 30/Jun/14

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

Type: Enhancement Priority: Major
Reporter: Alf Kristian Støyle Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

Attachments: Text File 0001-CLJ-1456-counting-forms-to-catch-malformed-throw-for.patch     Text File 0001-CLJ-1456-counting-forms-to-catch-malformed-throw-for.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The compiler does not fail on "malformed" throw forms:

user=> (defn foo [] (throw))
#'user/foo

user=> (foo)
NullPointerException   user/foo (NO_SOURCE_FILE:1)

user=> (defn bar [] (throw Exception baz))
#'user/bar

user=> (bar)
ClassCastException java.lang.Class cannot be cast to java.lang.Throwable  user/bar (NO_SOURCE_FILE:1)

; This one works, but ignored-symbol, should probably not be ignored
user=> (defn quux [] (throw (Exception. "Works!") ignored-symbol))
#'user/quux

user=> (quux)
Exception Works!  user/quux (NO_SOURCE_FILE:1)

The compiler can easily avoid these by counting forms.



 Comments   
Comment by Alf Kristian Støyle [ 30/Jun/14 11:56 AM ]

Not sure how to create a test for the attached patch. Will happily do so if anyone has a suggestion.

Comment by Alex Miller [ 30/Jun/14 12:23 PM ]

Re testing, I think the examples you give are good - you should add tests to test/clojure/test_clojure/compilation.clj that eval the form and expect compilation errors. I'm sure you can find similar examples.

Comment by Alf Kristian Støyle [ 30/Jun/14 2:01 PM ]

Newest patch also contains a few tests.





[CLJ-1453] Most Iterator implementations do not correctly implement next failing to throw the required NoSuchElementException Created: 24/Jun/14  Updated: 15/Jul/14

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

Type: Defect Priority: Major
Reporter: Meikel Brandmeyer Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: interop

Attachments: Text File 0001-Fix-iterator-implementations-to-throw-NSEE-when-exha.patch     Text File 0001-Throw-NSEE-in-gvec-iterator.patch    
Patch: Code
Approval: Triaged

 Description   

Most implementations of Iterators for Clojure's collections do not implement the next method correctly. In case the iterator is exhausted the methods fail with some case dependent error, but not with the NoSuchElementException as required by the official Java contract for that method. This causes problems when working with Java libraries relying on that behaviour.

Issue encountered in real world code using http://pipes.tinkerpop.com.

To reproduce:

(-> [] .iterator .next)

This throws a NPE instead of NSEE.

(doto (.iterator [1 2]) .next .next .next)

This throws an ArrayIndexOutOfBoundsException instead of NSEE.

The attached patch fixes the methods by adding a check for hasNext before actually trying to provide the next element. If there is no next element the correct exception is thrown.

Up-to-date patch file: 0001-Fix-iterator-implementations-to-throw-NSEE-when-exha.patch



 Comments   
Comment by Tim McCormack [ 15/Jul/14 9:56 PM ]

To establish a baseline, this piece of code checks all the iterators I could find within Clojure 1.6.0 and makes sure they throw an appropriate exception:

iterator-checker.clj
(defn next-failure
  []
  (let [ok (atom [])]
    (doseq [[tp v]
            (sorted-map
             :vec-0 []
             :vec-n [1 2 3]
             :vec-start (subvec [1 2 3 4] 0 1)
             :vec-end (subvec [1 2 3 4] 3 4)
             :vec-ls-0 (.listIterator [])
             :vec-ls-n (.listIterator [1 2 3])
             :vec-start-ls (.listIterator (subvec [1 2 3 4] 0 1))
             :vec-end-ls (.listIterator (subvec [1 2 3 4] 3 4))
             :seq ()
             :list-n '(1 2 3)
             :set-hash-0 (hash-set)
             :set-hash-n (hash-set 1 2 3)
             :set-sor-0 (sorted-set)
             :set-sor-n (sorted-set 1 2 3)
             :map-arr-0 (array-map)
             :map-arr-n (array-map 1 2, 3 4)
             :map-hash-0 (hash-map)
             :map-hash-n (hash-map 1 2, 3 4)
             :map-sor-n (sorted-map)
             :map-sor-n (sorted-map 1 2, 3 4)
             :map-sor-ks-0 (.keys (sorted-map))
             :map-sor-ks-n (.keys (sorted-map 1 2, 3 4))
             :map-sor-vs-0 (.vals (sorted-map))
             :map-sor-vs-n (.vals (sorted-map 1 2, 3 4))
             :map-sor-rev-0 (.reverseIterator (sorted-map))
             :map-sor-rev-n (.reverseIterator (sorted-map 1 2, 3 4))
             :map-ks-0 (.keySet {})
             :map-ks-n (.keySet {1 2, 3 4})
             :map-vs-0 (.values {})
             :map-vs-n (.values {1 2, 3 4})
             :gvec-int-0 (vector-of :long)
             :gvec-int-n (vector-of :long 1 2 3))]
      (let [it (if (instance? java.util.Iterator v)
                 v
                 (.iterator v))]
        (when-not it
          (println "Null iterator:" tp))
        (try (dotimes [_ 50]
               (.next it))
             (catch java.util.NoSuchElementException nsee
               (swap! ok conj tp))
             (catch Throwable t
               (println tp "threw" (class t))))))
    (println "OK:" @ok)))

The output as of current Clojure master (201a0dd970) is:

:gvec-int-0 threw java.lang.IndexOutOfBoundsException
:gvec-int-n threw java.lang.IndexOutOfBoundsException
:map-arr-0 threw java.lang.ArrayIndexOutOfBoundsException
:map-arr-n threw java.lang.ArrayIndexOutOfBoundsException
:map-hash-0 threw java.lang.ArrayIndexOutOfBoundsException
:map-ks-0 threw java.lang.ArrayIndexOutOfBoundsException
:map-ks-n threw java.lang.ArrayIndexOutOfBoundsException
:map-sor-ks-0 threw java.util.EmptyStackException
:map-sor-ks-n threw java.util.EmptyStackException
:map-sor-n threw java.util.EmptyStackException
:map-sor-rev-0 threw java.util.EmptyStackException
:map-sor-rev-n threw java.util.EmptyStackException
:map-sor-vs-0 threw java.util.EmptyStackException
:map-sor-vs-n threw java.util.EmptyStackException
:map-vs-0 threw java.lang.ArrayIndexOutOfBoundsException
:map-vs-n threw java.lang.ArrayIndexOutOfBoundsException
:vec-0 threw java.lang.NullPointerException
:vec-end threw java.lang.ArrayIndexOutOfBoundsException
:vec-end-ls threw java.lang.IndexOutOfBoundsException
:vec-ls-0 threw java.lang.IndexOutOfBoundsException
:vec-ls-n threw java.lang.IndexOutOfBoundsException
:vec-n threw java.lang.ArrayIndexOutOfBoundsException
:vec-start threw java.lang.ArrayIndexOutOfBoundsException
:vec-start-ls threw java.lang.IndexOutOfBoundsException
OK: [:list-n :map-hash-n :seq :set-hash-0 :set-hash-n :set-sor-0 :set-sor-n]
Comment by Tim McCormack [ 15/Jul/14 9:57 PM ]

0001-Fix-iterator-implementations-to-throw-NSEE-when-exha.patch missed one thing: clojure.gvec. With the patch in place, my checker outputs the following:

:gvec-int-0 threw java.lang.IndexOutOfBoundsException
:gvec-int-n threw java.lang.IndexOutOfBoundsException
OK: [:list-n :map-arr-0 :map-arr-n :map-hash-0 :map-hash-n :map-ks-0 :map-ks-n :map-sor-ks-0 :map-sor-ks-n :map-sor-n :map-sor-rev-0 :map-sor-rev-n :map-sor-vs-0 :map-sor-vs-n :map-vs-0 :map-vs-n :seq :set-hash-0 :set-hash-n :set-sor-0 :set-sor-n :vec-0 :vec-end :vec-end-ls :vec-ls-0 :vec-ls-n :vec-n :vec-start :vec-start-ls]

That should be a quick fix.

Comment by Michał Marczyk [ 15/Jul/14 10:01 PM ]

CLJ-1416 includes a fix for gvec's iterator impls (and some other improvements to interop).

Comment by Tim McCormack [ 15/Jul/14 10:17 PM ]

Attaching a fix for the gvec iterator. Combined with the existing patch, this fixes all broken iterators that I could find.





[CLJ-1446] (def v) with no init supplied destroys #'v metadata Created: 13/Jun/14  Updated: 13/Jun/14

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

Type: Defect Priority: Major
Reporter: Nahuel Greco Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

(def a) destroys #'a metadata, check this:

(def ^:mykey a 1)

(meta #'a)              ;; ok, :mykey is present

(let [v (def a)]
   [(meta v)            ;; NO :mykey present, metadata destroyed
    (identical? v #'a)  ;; true, we are talking of the same var
   ])

(meta #'a)              ;; NO :mykey present

If this is not a bug but a "feature", then we have at least two problems:

1- The def special form documentation doesn't state this behaviour at all, it needs to be clarified. With the current documentation it seems as doing a def with no init supplied will not make any side-effect at all, and this is not true for the var metadata.

2- defmulti uses this form to lookup the var and check if it already binds to a MultiFn, if that is the case then defmulti does nothing... but it really does something, defmulti will destroy the original var metadata in the (supposedly non-destructive) check. This is the involved defmulti fragment:

(let [v# (def ~mm-name)]
  (when-not (and (.hasRoot v#) (instance? clojure.lang.MultiFn (deref v#)))
   ...


 Comments   
Comment by Alex Miller [ 13/Jun/14 4:14 PM ]

I think this is mostly a dupe of CLJ-1148 but I'll leave it as it states the specific problem more precisely.

Comment by Nahuel Greco [ 13/Jun/14 7:35 PM ]

Alex Miller: It seems CLJ-1148 is an special case where this problem shows, but the patches in CLJ-1148 only fixes the issues for defonce, not generally for def, not for defmulti and not clarifies this behaviour in the def special form documentation.





[CLJ-1435] 'numerator and 'denominator fail to handle integral values (i.e. N/1) Created: 30/May/14  Updated: 30/May/14

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

Type: Defect Priority: Major
Reporter: Aaron Brooks Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   

Because ratio values reduce to lowest terms and, for integral values where the lowest term is N/1, are auto-converted to BigInts (and formerly Longs), the current behavior of clojure.core/numerator and clojure.core/denominator yield unexpected results.

user=> (numerator 1/3)
1
user=> (numerator (+ 1/3 2/3))

ClassCastException clojure.lang.BigInt cannot be cast to clojure.lang.Ratio  clojure.core/numerator (core.clj:3306)
user=> (denominator 1/3)
3
user=> (denominator (+ 1/3 2/3))

ClassCastException clojure.lang.BigInt cannot be cast to clojure.lang.Ratio  clojure.core/denominator (core.clj:3314)
user=>

The auto-conversion to Longs is not really the problem in my mind. I'd like to see numerator return the original value when presented with a BigInt and denominator always return 1 when presented with a BigInt. It seems reasonable to request the same for Longs.

If desired, I'd be happy to produce a patch.



 Comments   
Comment by Andy Fingerhut [ 30/May/14 6:35 PM ]

I don't know the official stance on this ticket, but will add some notes.

Aaron, numerator and denominator are pretty clearly documented to work on Ratio types only.

It is pretty easy to write my-numerator and my-denominator that work exactly as you wish, checking for the type of arg and using numerator, denominator for Ratio types, and doing whatever you think is correct for other numeric types.

Comment by Aaron Brooks [ 30/May/14 7:44 PM ]

I'm aware that they are documented as such. Part of my point is that you can be working entirely with Ratio types and, via arithmetic operations between them, sometimes wind up with a non-Ratio number unexpectedly.

Also consider:

user=> (numerator 2/1)
ClassCastException java.lang.Long cannot be cast to clojure.lang.Ratio  clojure.core/numerator (core.clj:3238)

You're then left either implementing a try/catch correction or always checking the type before using numerator or denominator which is a loss in performance.

The patch I have in mind is creating a protocol, extended to Ratio, BigInt and Long which calls the appropriate method (Ratios) or returns either the given number or 1 (numerator/denominator) for the integral types. I expect this to maintain the current level of performance in the cases where it works and behave properly in the cases currently not handled.





[CLJ-1431] Switch from MurmurHash3 to SipHash to prevent DoS collision attack (hash flooding) Created: 25/May/14  Updated: 26/May/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5, Release 1.6
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: James Thornton Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: security


 Description   

Clojure is using Murmur3 throughout:
https://github.com/clojure/clojure/commit/dff9600387b962f16fc78e6477e10e34651fd366

DJB, Jean-Philippe Aumasson, and Martin Boßlet have shown that Murmur3 is not resilient against hash collision attacks:
http://www.ocert.org/advisories/ocert-2012-001.html
https://131002.net/siphash/

"Hash-flooding DoS reloaded: attacks and defenses" talk by DJB, Jean-Philippe Aumasson, and Martin Boßlet
http://media.ccc.de/browse/congress/2012/29c3-5152-en-hashflooding_dos_reloaded_h264.html

"Breaking Murmur: Hash-flooding DoS Reloaded"
http://emboss.github.io/blog/2012/12/14/breaking-murmur-hash-flooding-dos-reloaded/

Python, Ruby, JRuby, Haskell, Rust, Perl, Redis... have all switched to SipHash
https://en.wikipedia.org/wiki/SipHash

Last year Google dropped CityHash from Guava and replaced it with SipHash
https://code.google.com/p/guava-libraries/issues/detail?id=1232

SipHash Guava Implementation
https://code.google.com/p/guava-libraries/source/browse/guava/src/com/google/common/hash/SipHashFunction.java

SipHash Java reference implementation
https://github.com/emboss/siphash-java/blob/master/src/main/java/com/github/emboss/siphash/SipHash.java



 Comments   
Comment by Alex Miller [ 26/May/14 12:56 AM ]

Thanks, we've talked about this issue and some possible things we could do, but didn't have a ticket for it yet.

Comment by Alex Miller [ 26/May/14 1:08 AM ]

While the Java 7 approach relied on (attempting) to properly seed hash maps with string hash codes, that was all dropped in Java 8, which addressed DoS collision hash attacks by instead improving the data structure to switch from linear collisions to a red/black tree (log-time) for collisions. It's possible a similar approach could work in Clojure as well.

One workaround that could be used now is to wrap map keys in a custom type that implements IHashEq and implements an alternate hash function.





[CLJ-1429] Cache unknown multimethod value default dispatch Created: 22/May/14  Updated: 27/Jun/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: performance

Attachments: Text File clj-1429.patch    
Patch: Code
Approval: Screened

 Description   

Multimethods maintain a cache from dispatch value (result of the dispatch function) to dispatch method. If the dispatch value does not find a match in the available methods, it falls through to a lookup using the default dispatch value and returns that method. This default dispatch case is NOT recorded in the cache. This means that every case that falls through to the default case incurs a scan of the methodTable (and the class inheritance checks that involves).

Perf test:

(defmulti mm class)
(defmethod mm String [s] s)
(defmethod mm Long [l] l)
(defmethod mm :default [v] v)

(defn perf [reps size]
  (let [data (take size (cycle ["abc" 5 :k]))]
    (dotimes [_ reps]
      (time (doall (map mm data))))))

And results:

;; Without patch:
user=> (perf 5 100000)
"Elapsed time: 1301.262 msecs"
"Elapsed time: 928.888 msecs"
"Elapsed time: 942.905 msecs"
"Elapsed time: 858.513 msecs"
"Elapsed time: 832.314 msecs"

;; With patch:
user=> (perf 5 100000)
"Elapsed time: 134.169 msecs"
"Elapsed time: 28.859 msecs"
"Elapsed time: 45.452 msecs"
"Elapsed time: 13.189 msecs"
"Elapsed time: 13.42 msecs"

Attached patch caches the mapping of unknown value -> default dispatch method and significantly improves the performance for this case.

Patch: clj-1429.patch
Screened by: Stu






[CLJ-1424] Feature Expressions Created: 15/May/14  Updated: 19/May/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File clojure-feature-expressions.diff    
Patch: Code and Test

 Description   

Feature expressions based directly on Common Lisp.

Implementation based on Roman's patch for CLJS-27.

#+ #- and or not
are supported. Unreadable tagged literals are suppressed through the *suppress-read* dynamic var. For example, in with *features* being #{:clojure}, which is the default, the following should read :foo

#+clojurescript #js {:one :two} :foo

Should *suppress-read* override *read-eval*?

TRDR-14 is a sister patch for tools.reader, which can be used by clojurescript



 Comments   
Comment by Jozef Wagner [ 16/May/14 2:19 AM ]

Has there been a decision that CL syntax is going to be used? Related discussion can be found at design page, google groups discussion and another discussion.

Comment by Alex Miller [ 16/May/14 8:34 AM ]

No, no decisions on anything yet.

Comment by Ghadi Shayban [ 19/May/14 7:25 PM ]

Just to echo a comment from TRDR-14:

This is WIP and just one approach for feature expressions. There seem to be at least two couple diverging approaches emerging from the various discussion (Brandon Bloom's idea of read-time splicing being the other.)

In any case having all Clojure platforms be ready for the change is probably essential. Also backwards compatibility of feature expr code to Clojure 1.6 and below is also not trivial.





[CLJ-1406] Libs are blindly added into loaded-libs even if an error occurs during loading Created: 17/Apr/14  Updated: 17/Apr/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6
Fix Version/s: None

Type: Defect Priority: Major
Reporter: OHTA Shogo Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-modify-clojure.core-load-lib-so-that-it-removes-the-.patch    
Patch: Code

 Description   

Suppose you have a lib that causes some errors during loading, like the following:

(ns broken-lib)

(} ; this line will cause a reader error

And then, if you require the lib, it would be added into loaded-libs in spite of the reader error, which makes require succeed silently after that.

user=> (contains? (loaded-libs) 'broken-lib)
false
user=> (require 'broken-lib)
CompilerException java.lang.RuntimeException: Unmatched delimiter: }, compiling:(broken_lib.clj:3:3) 
user=> (contains? (loaded-libs) 'broken-lib)
true
user=> (require 'broken-lib)
nil
user=>

Cause:
The patch for CLJ-1116 made the ns macro blindly add the lib being defined into loaded-libs even if an error occurs during loading.

Approach:
Modify clojure.core/load-lib so that it removes the lib from loaded-libs on error.



 Comments   
Comment by Alex Miller [ 17/Apr/14 9:07 AM ]

This patch seems somewhat removed from the cause - is there some way to instead prevent the lib from being added to loaded-libs in the first place?

Comment by OHTA Shogo [ 17/Apr/14 9:21 AM ]

To do so, I think we need to revert CLJ-1116.





[CLJ-1399] missing field munging when recreating deftypes serialized in to byte code Created: 02/Apr/14  Updated: 02/Apr/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: File clj-1399.diff    
Patch: Code

 Description   

to embed deftypes in the bytecode the compiler emits the value of each field, then emits a call to the deftypes underlying class's constructor.

to get a list of fields the compiler calls .getBasis.

the getBasis fields are the "clojure" level field names of the deftype, which the actual "jvm" level field names have been munged (replacing - with _, etc), so the compiler tries to generate code to set values on non-existent fields

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L4579

https://www.refheap.com/70731

you can work around this by using field names that don't require munging. a solution might be just calling munge in the emission of field sets of ITypes



 Comments   
Comment by Kevin Downey [ 02/Apr/14 4:26 PM ]

reproducing case

$ rlwrap java -server -Xmx1G -Xms1G -jar /Users/hiredman/src/clojure/target/clojure-1.6.0-master-SNAPSHOT.jar
Clojure 1.6.0-master-SNAPSHOT
user=> (deftype Foo [hello-world])
user.Foo
user=> (alter-var-root #'default-data-readers assoc 'foo (fn [x] (Foo. x)))
{foo #<user$eval6$fn__7 user$eval6$fn__7@2f953efd>, inst #'clojure.instant/read-instant-date, uuid #'clojure.uuid/default-uuid-reader}
user=> #foo "1"
CompilerException java.lang.IllegalArgumentException: No matching field found: hello-world for class user.Foo, compiling:(NO_SOURCE_PATH:0:0)
user=>
Comment by Kevin Downey [ 02/Apr/14 4:39 PM ]

this patch fixes the issue on the latest master for me

Comment by Chas Emerick [ 02/Apr/14 4:57 PM ]

FWIW, this was precipitated by real experience (I think I created the refheap paste). The workaround is easy (don't use dashes in field names of deftypes you want to return from data reader functions), but I wouldn't expect anyone to guess that that wasn't already oversensitized to munging edge cases.





[CLJ-1388] equality bug on records created with nested calls to map->record Created: 18/Mar/14  Updated: 23/May/14

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord

Attachments: Text File 0001-FIX-CLJ-1388.patch     Text File CLJ-1388.patch     Text File CLJ-1388-record-equality-and-map-record-factory.patch     Text File CLJ-1388v2.patch    
Patch: Code and Test
Approval: Screened

 Description   

Depending on the type of the map passed to a record map constructor, records will not correctly compare for equality:

user> (defrecord a []) 
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2)  ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2)  ;; expected => {:a 1}
#user.a{:a 1}

Cause: The type of the map passed into the map constructor leaks into the __extmap, affecting equality comparison of the record. This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

Approach: Clean the extmap before putting it into the record constructor.

Patch: CLJ-1388-record-equality-and-map-record-factory.patch

Screened by: Alex Miller



 Comments   
Comment by Steve Miner [ 28/Apr/14 3:06 PM ]

The proposed patch 0001-FIX-CLJ-1388.patch makes every map->Record method pay to copy the argument map every time. However, according to my tests, the problem only occurs with records without any fields. So it should be sufficient to generate the (into {} m#) case only when `fields` is empty. [Update: this is wrong, explained below.]

Comment by Steve Miner [ 28/Apr/14 3:10 PM ]

It would be better to fix the problem in the Java Record/create method, but I couldn't figure out how that worked. On the other hand, this bug seems like a fairly rare edge case so I think my patch is acceptable.

Comment by Alex Miller [ 28/Apr/14 3:23 PM ]

Moving out of Screened due to new patch

Comment by Nicola Mometto [ 28/Apr/14 3:35 PM ]

Steve, the problem doesn't occur with records without any fields, your testing was reporting that only because you are only using one record type.

Here's an example that returns true with my patch, but still returns false with yours.

user=> (defrecord a [a])
user.a
user=> (defrecord b [b])
user.b
user=> (def x1 (map->a {:a 1 :b 2}))
#'user/x1
user=> (def x2 (map->a (map->b {:a 1 :b 2})))
#'user/x2
user=> x1
#user.a{:a 1, :b 2}
user=> x2
#user.a{:a 1, :b 2}
user=> (= x1 x2)
false
user=> (.__extmap x2)
#user.b{:b 2}
Comment by Nicola Mometto [ 28/Apr/14 3:37 PM ]

It should also be noted that the overhead of copying the record map is probably insignificant.

Comment by Nicola Mometto [ 28/Apr/14 3:42 PM ]

I also thought at first to fix the problem either on the /create method or on the 3-arity ctor but given that:

  • a fix there would involve messing with the bytecode emitted and thus would be harder to implement than this simple 1-line patch
  • neither the /create method nor the 3-arity ctor is documented and thus should be considered implementation details

I think patching the map->record function is the best way to go.

Comment by Steve Miner [ 28/Apr/14 3:56 PM ]

Nicola, thanks for the correction. I missed the case with multiple records. I withdrew my patch. I'd still like to find a more finely tuned patch, but first I'll have to improve my tests as you demonstrated.

Comment by Steve Miner [ 29/Apr/14 10:17 AM ]

Attached CLJ-1388-record-equality-and-map-record-factory.patch that checks arg to map->Record for MapEquivalence, uses (into {} m#) when necessary. This makes equiv test work correctly with records as the argument (and other map-like values). Added tests with variety of args to map->Record.

Comment by Steve Miner [ 29/Apr/14 10:46 AM ]

A few comments about the new patch... I think the basic issue is a bad interaction between = for records and the generated Record/create method. Everything works when the interal __extmap is a regular map (MapEquivalence), but it fails if __extmap is another record. I think that's because of equiv calling = on the __extmap's.

The user expects to create a new record using the value of another record because it's just like a map. However, = on records respects the record type so it's not = to a map.

The general work-around is to use (into {} x) on the argument to the map->Record. To meet the user's expectation, that `into` call can be incorporated into the map->Record. But I didn't like the defensive copy as most of the time it's unnecessary – the argument is typically a regular map. The `into` work-around is only necessary if the arg is not a MapEquivalence.

There might be a better way to fix the Record/create method but I couldn't figure it out.

Comment by Nicola Mometto [ 29/Apr/14 1:52 PM ]

Steve's last comment made me realize that the root of the problem is on the record .equiv method, where the extmaps are compared via `=`

This new patch (CLJ-1388.patch) addresses this issue by comparing the extmaps with Utils/equiv rather than `=`, which compares maps in a type-indipendent way.

There's still a case where we need recreate the given map, that is when the given map is not an IPersistentMap but simply a java.util.Map.

Steve, my new patch incorporates my fix and your tests, I modified your patch to include only the tests (that were really comprehensive) since I figured it's fair to keep your authorship on those, let me know if that's a problem with you.

Comment by Steve Miner [ 29/Apr/14 2:10 PM ]

Whatever works for you regarding the tests is fine by me.

Comment by Alex Miller [ 30/Apr/14 12:07 AM ]

It seems weird to me that a record should ever contain another record as its extmap. We should be considering the performance aspect but I'm concerned that not locking down extmap more just invites other weirder problems later.

In CLJ-1388.patch, you mention Utils/equiv in your comment but the patch calls Utils/equals - which did you mean?

Also, that patch currently checks if m# is an IPersistentMap - I can't imagine what case we would want to allow where a valid m# is NOT an IPersistentMap?

Comment by Nicola Mometto [ 30/Apr/14 4:15 AM ]

Alex, the Utils/equiv in my comment is wrong (it's easy to confuse between equiv/equals, sorry), Utils/equals in the patch is the right method to use since it compares in a type agnostic way.

Since __extmap is an implementation detail and is only used internally by defrecord for its methods, I don't think it's going to be a problem whether it's a record or a regular clojure map. (Clojure only requires it to be an IPersistentMap)

Regarding the check for m# being an IPersistentMap, Steve in his tests had a case where the map->record ctor was invoked with a java.util.Map, I went to look into the docs for defrecord and it only mentions that the argument to map->record has to be a "map", it doesn't specify that it has to be a clojure map/IPersistentMap, so it seemed right to allow for java maps too and wrap them in IPersistentMaps internally.

Comment by Steve Miner [ 30/Apr/14 8:27 AM ]

My test with java.util.Map was an extension of the idea that anything map-like could be used to initialize a record. That might be a bridge too far, but my patch was testing for MapEquivalence to handle records so it made sense to allow j.u.Map, etc. With Nicola's latest patch, it's probably unnecessary to support non-IPersistentMaps so map->Record doesn't actually need to change.

Comment by Nicola Mometto [ 30/Apr/14 3:57 PM ]

CLJ-1388v2.patch is like CLJ-1388.patch except it doesn't copy non IPersistentMaps in a clojure map.

To summarize, here's the status of the different patches for this ticket:

  • 0001-FIX-CLJ-1388.patch copies the argument of map->record in a clojure map via `(into {} m#)`, be it already a clojure map, a record, or a java.util.Map
  • CLJ-1388-record-equality-and-map-record-factory.patch adopts the same approach except it only copies the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.MapEquivalence
  • CLJ-1388.patch fixes the issue by changing the function that compares __extmaps from `=` (type aware) to `clojure.lang.Utils/equals` (type agnostic), this patch also copies the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.IPersistentMap
  • CLJ-1388v2.patch is the same as CLJ-1388.patch except it doesn't copy the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.IPersitentMap, thus map->record will not work with bare java.util.Maps (which is the behaviour it has already)
Comment by Alex Miller [ 05/May/14 1:59 PM ]

Are these patches all still in play? Having 4 active patches does not help move a ticket forward.

Can someone re-summarize at this point what questions exist?

Comment by Nicola Mometto [ 06/May/14 5:26 AM ]

0001-FIX-CLJ-1388.patch should be superseded by the other 3 patches since they solve the same problem in a more performant way.

To pick between the other patches, we need to chose which approach to go with.
Patches CLJ-1388.patch and CLJ-1388v2.patch fix the issue in the equiv method of the defrecord, patch CLJ-1388-record-equality-and-map-record-factory.patch fixes the issue in the map->record ctor by converting maps that don't implement MapEquivalence to a clojure map.

I'd go with either CLJ-1388.patch or CLJ-1388v2.patch since they both avoid copying alltoghether in the cases where map->record currently works, while CLJ-1388-record-equality-and-map-record-factory.patch needs to copy the arg into a map if the arg is a custom IPersistentMap or a record.

To pick between CLJ-1388.patch or CLJ-1388v2.patch we need to decide whether or not the current behaviour of map->record to require strictly an IPersistentMap is the way to go: if we decide that it's ok to pass non IPersitentMap maps like java.util.Map to map->record then pick CLJ-1388.patch otherwise CLJ-1388v2.patch

Comment by Alex Miller [ 06/May/14 10:22 AM ]

From brief conversation with Rich, we should not allow arbitrary map types in __extmap so would prefer to force a clean map and rely on standard equality checking. I think CLJ-1388-record-equality-and-map-record-factory.patch is the preferred path based on that, which still seems like it should avoid copying in nearly all common cases.

Comment by Alex Miller [ 23/May/14 11:19 AM ]

Screened specifically CLJ-1388-record-equality-and-map-record-factory.patch - use map as is if it supports MapEquivalence (and can thus be compared under a map) and otherwise dump into a clojure map.





[CLJ-1384] clojure.core/set should use transients Created: 15/Mar/14  Updated: 27/Jun/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: performance

Attachments: Text File CLJ-1384-p1.patch     Text File CLJ-1384-p2.patch     File set-bench.tar    
Patch: Code
Approval: Screened

 Description   

CLJ-1384-p2 uses transients for both create and createWithCheck. This is consistent with the current implementation for map.

clojure.core/vec calls (more or less) PersistentVector.create(...), which uses a transient vector to build up the result.

clojure.core/set on the other hand, calls PersistentHashSet.create(...), which repeatedly calls .cons on a PersistentHashSet, with all the associated speed/GC issues.

Operation count now w/transients
set 5 1.771924 µs 1.295637 µs
into 5 1.407925 µs 1.402995 µs
set 1000000 2.499264 s 1.196653 s
into 1000000 0.977555 s 1.006951 s

Patch: CLJ-1384-p2.patch
Screened by: Stu



 Comments   
Comment by Gary Fredericks [ 15/Mar/14 10:13 PM ]

PersistentHashSet has six methods for creating sets – one for each combination of {with check, without check} and {array (varargs), List, ISeq}. Each of them does not use transients but could.

I believe clojure.core/set only depends on the (without check, ISeq) version.

Should all of these be changed? Three of them? One of them?

Comment by Andy Fingerhut [ 15/Mar/14 10:21 PM ]

I believe that the 'with check' versions are only intended to be used when reading set literals in Clojure source code, and give an error if there are duplicate elements. If you find examples where those set creation functions are called in other situations, I would be interested to learn about them to find out where my misunderstanding lies, or whether that is a problem with the current code.

If the belief above is correct, I would suggest not changing the 'with check' versions, since their speed isn't as critical.

Comment by Gary Fredericks [ 15/Mar/14 10:23 PM ]

Thanks Andy, I'll submit a patch that changes the three non-checked methods.

Comment by Gary Fredericks [ 15/Mar/14 10:46 PM ]

Attached CLJ-1384-p1.patch, which updates the three non-check create methods.

I also added benchmarks. It's about 2-3 times faster for large collections.

Comment by Ambrose Bonnaire-Sergeant [ 11/Apr/14 11:15 AM ]

Added benchmark suite (set-bench.tar).

FWIW results are similar to gfrederick's on my machine:

Clojure 1.6

Small collections (5 elements)

set averages 1.220601 µs
into averages 1.597991 µs

Large collections (1,000,000 elements)

set averages 2.429066 sec
into averages 1.006249 sec

After transients

Small collections (5 elements)

set averages 999.248325 ns
into averages 1.162889 µs

Large collections (1,000,000 elements)

set averages 1.003792 sec
into averages 889.993185 ms

Add full output to the tar.

Comment by Ghadi Shayban [ 11/Apr/14 11:35 AM ]

CLJ-1192 is related to this, but and Andy seems to be indicating the use of reduce as the means to better performance there.

Comment by Gary Fredericks [ 11/Apr/14 11:41 AM ]

Oh that's a good point about reduce. The difference should only apply to chunked seqs, right? It's worth noting that the benchmarks above used range which creates chunked seqs, so that might be why into looks faster on the large collections?

So this change only makes set act like vec; I think whether either/both of them should use reduce is a different question.





[CLJ-1381] Improve support for extending protocols to primitive arrays Created: 13/Mar/14  Updated: 13/Mar/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: protocols


 Description   

It is possible to extend protocols to primitive arrays but specifying the class for the type is a little tricky:

(defprotocol P (p [_]))
(extend-protocol P (Class/forName "[B") (p [_] "bytes"))
(p (byte-array 0))   ;; => "bytes"

However, things go bad if you try to do more than one of these:

(extend-protocol P 
  (Class/forName "[B") (p [_] "bytes")
  (Class/forName "[I") (p [_] "ints"))
CompilerException java.lang.UnsupportedOperationException: nth not supported on this type: Character, compiling:(NO_SOURCE_PATH:1:1)
	clojure.lang.Compiler.analyze (Compiler.java:6380)
	clojure.lang.Compiler.analyze (Compiler.java:6322)
	clojure.lang.Compiler$MapExpr.parse (Compiler.java:2879)
	clojure.lang.Compiler.analyze (Compiler.java:6369)
	clojure.lang.Compiler.analyze (Compiler.java:6322)
	clojure.lang.Compiler$InvokeExpr.parse (Compiler.java:3624)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6562)
	clojure.lang.Compiler.analyze (Compiler.java:6361)
	clojure.lang.Compiler.analyze (Compiler.java:6322)
	clojure.lang.Compiler$BodyExpr$Parser.parse (Compiler.java:5708)
	clojure.lang.Compiler$FnMethod.parse (Compiler.java:5139)
	clojure.lang.Compiler$FnExpr.parse (Compiler.java:3751)
Caused by:
UnsupportedOperationException nth not supported on this type: Character
	clojure.lang.RT.nthFrom (RT.java:857)
	clojure.lang.RT.nth (RT.java:807)
	clojure.core/emit-hinted-impl/hint--5951/fn--5953 (core_deftype.clj:758)
	clojure.core/map/fn--4207 (core.clj:2487)
	clojure.lang.LazySeq.sval (LazySeq.java:42)
	clojure.lang.LazySeq.seq (LazySeq.java:60)
	clojure.lang.RT.seq (RT.java:484)
	clojure.lang.RT.countFrom (RT.java:537)
	clojure.lang.RT.count (RT.java:530)
	clojure.lang.Cons.count (Cons.java:49)
	clojure.lang.Compiler.analyze (Compiler.java:6352)
	clojure.lang.Compiler.analyze (Compiler.java:6322)

The code in {parse-impls} is seeing the second {(Class/forName "[I")} as a function, not as a new type. One workaround for this is to only extend the protocol to one type at a time.

It would be even better (moving into enhancement area) if there was a syntax here to specify primitive array types - we already have the syntax of {bytes, ints, longs}, etc for type hints and those seem perfectly good to me.






[CLJ-1378] Hints don't work with #() form of function Created: 11/Mar/14  Updated: 10/Jun/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Roy Varghese Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: interop, typehints

Attachments: File clj-1378.diff     File clj-1378-v2.diff    
Patch: Code
Approval: Vetted

 Description   
;; WORKS
(deftest test-add-job
  (let [pool (java.util.concurrent.Executors/newFixedThreadPool 10)
        counter 50000
        f (fn [])]
    (dotimes [i counter]
      (.submit pool ^Runnable  f ))
    (Thread/sleep 1000)
    (is (= counter (count (all-jobs))))))

;; FAILS
(deftest test-add-job
  (let [pool (java.util.concurrent.Executors/newFixedThreadPool 10)
        counter 50000]
    (dotimes [i counter]
      (.submit pool ^Runnable  #()))
    (Thread/sleep 1000)
    (is (= counter (count (all-jobs))))))

Caused by: java.lang.IllegalArgumentException: More than one matching method found: submit
	at clojure.lang.Compiler.getMatchingParams(Compiler.java:2380)
	at clojure.lang.Compiler$InstanceMethodExpr.<init>(Compiler.java:1412)
	at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:952)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6560)
	... 87 more


 Comments   
Comment by Jozef Wagner [ 12/Mar/14 4:03 AM ]

Functions do have metadata, but Compiler does not look in them for type hints.

user=> (with-meta #() {:foo :bar})
#<clojure.lang.AFunction$1@779325ee>

When compiler is determining which native method to use, it matches method signature with classes of given args. There is a getJavaClass() method in Compiler.java which returns a class for given expression. Vars expressions and local bindings use :tag metadata to override this class, but most other expressions don't. Compiler parses #() into a FnExpr, which always return AFunction as its class.

Most of time this approach is OK, as AFunction implements Runnable and Callable so there is no need for type hint. However, in this particular case, there are overrides for both Runnable and Callable, and as AFunction can be either of them, the expression is ambiguous.

Comment by Jozef Wagner [ 12/Mar/14 4:17 AM ]

Patch added, following expression will now run without error

(.submit (java.util.concurrent.Executors/newCachedThreadPool) ^Runnable #())
Comment by Alex Miller [ 12/Mar/14 9:34 AM ]

Could you add a test to the patch?

Comment by Jozef Wagner [ 12/Mar/14 2:53 PM ]

Attached patch clj-1378-v2.diff which contains both fix and test.





[CLJ-1376] Initialize internal maps to more efficient version Created: 11/Mar/14  Updated: 11/Mar/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: performance


 Description   

In reviewing some hashing stuff, I noticed that there are many places internal to Clojure that use maps initialized with PersistentHashMap.EMPTY. Many of these maps are likely to have a small number of entries such that a PersistentArrayMap might be more efficient.

These are the candidates:

src/jvm/clojure/lang/ARef.java
19:private volatile IPersistentMap watches = PersistentHashMap.EMPTY;

src/jvm/clojure/lang/Compiler.java
3009:				IPersistentMap m = PersistentHashMap.EMPTY;
3819:					       KEYWORDS, PersistentHashMap.EMPTY,
3820:					       VARS, PersistentHashMap.EMPTY,
3964:	IPersistentMap closes = PersistentHashMap.EMPTY;
3977:	IPersistentMap keywords = PersistentHashMap.EMPTY;
3978:	IPersistentMap vars = PersistentHashMap.EMPTY;
5121:                            ,CLEAR_SITES, PersistentHashMap.EMPTY
7259:			       KEYWORDS, PersistentHashMap.EMPTY,
7260:			       VARS, PersistentHashMap.EMPTY
7418:			IPersistentMap opts = PersistentHashMap.EMPTY;
7475:			IPersistentMap fmap = PersistentHashMap.EMPTY;
7522:					       KEYWORDS, PersistentHashMap.EMPTY,
7523:					       VARS, PersistentHashMap.EMPTY,
7912:                            ,CLEAR_SITES, PersistentHashMap.EMPTY

src/jvm/clojure/lang/LispReader.java
755:					RT.map(GENSYM_ENV, PersistentHashMap.EMPTY));

src/jvm/clojure/lang/MultiFn.java
39:	this.methodTable = PersistentHashMap.EMPTY;
41:	this.preferTable = PersistentHashMap.EMPTY;
49:		methodTable = methodCache = preferTable = PersistentHashMap.EMPTY;

src/jvm/clojure/lang/Var.java
48:	final static Frame TOP = new Frame(PersistentHashMap.EMPTY, null);
175:	setMeta(PersistentHashMap.EMPTY);
341:	IPersistentMap ret = PersistentHashMap.EMPTY;

Approach: Two possible approaches - initialize to PersistentArrayMap.EMPTY or call RT.map(). The latter requires function invocation so is slightly slower, but has the benefit of localizing map construction into a single place.






[CLJ-1373] LazySeq should utilize cached hash from its underlying seq. Created: 09/Mar/14  Updated: 20/Mar/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, performance
Environment:

1.6.0 master SNAPSHOT


Attachments: File clj-1373.diff    
Patch: Code
Approval: Triaged

 Description   

Even if underlying seq contains a cached hash, LazySeq computes it every time.

user=> (def a (range 100000))
#'user/a
user=> (time (hash a))
"Elapsed time: 46.904273 msecs"
375952610
user=> (time (hash a)) ;; RECOMPUTE
"Elapsed time: 10.879098 msecs"
375952610
user=> (def b (seq a))
#'user/b
user=> (time (hash b))
"Elapsed time: 10.572522 msecs"
375952610
user=> (time (hash b)) ;; CACHED HASH
"Elapsed time: 0.024927 msecs"
375952610
user=> (def c (lazy-seq b))
#'user/c
user=> (time (hash c))
"Elapsed time: 12.207651 msecs"
375952610
user=> (time (hash c))
"Elapsed time: 10.995798 msecs"
375952610


 Comments   
Comment by Jozef Wagner [ 09/Mar/14 9:20 AM ]

Added patch which checks if underlying seq implements IHashEq and if yes, uses that hash instead of recomputing.





[CLJ-1372] Inconsistent hash with java collections Created: 09/Mar/14  Updated: 15/May/14

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

Type: Defect Priority: Major
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: collections, interop
Environment:

1.6.0 master


Attachments: Text File 0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M-alternative.patch     Text File 0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch     Text File 0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M-substring.patch     Text File 0005-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch     Text File 0006-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch     Text File 0007-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch     File clj-1372-2.diff     File clj-1372.diff    
Patch: Code and Test
Approval: Triaged

 Description   

c.c/hash always use hashCode for java collections, which is incompatible when comparing with Clojure collections, which use Murmur3.

user=> (== (hash (java.util.ArrayList. [1 2 3])) (hash [1 2 3]))
false
user=> (= (java.util.ArrayList. [1 2 3]) [1 2 3])
true

One way to fix it is to add a special case in Util/hasheq for java.util.Collections, as it is now for Strings.

Link to a discussion of this topic in the Clojure group: https://groups.google.com/forum/#!topic/clojure/dQhdwZsyIEw



 Comments   
Comment by Jozef Wagner [ 09/Mar/14 8:02 AM ]

Same problem for maps, so hasheq should have a special case for java.util.Map too.

Comment by Jozef Wagner [ 09/Mar/14 9:21 AM ]

Added patch with fix for j.u. Map, Set and List.

Comment by Andy Fingerhut [ 09/Mar/14 6:02 PM ]

Add patch clj-1372-2.diff that is identical to Jozef Wagner's clj-1372.diff, except it also adds some new tests that fail without his changes, and pass with them.

Comment by Alex Miller [ 10/Mar/14 9:31 AM ]

I think the contract on equiv/hasheq is more narrowly scoped than this and only applies if both collections are IPersistentCollection. In other words, I don't think this is wanted or required.

Note that the Java .equals/.hashCode contract is maintained here - these collections will compare as .equals() and do have the same .hashCode().

Comment by Jozef Wagner [ 10/Mar/14 9:38 AM ]

Without the patch the following statement is not valid: "If two objects are equal with c.c/=, than their hash returned by c.c/hash is the same number". We can say that this is valid only iff both objects are 'clojure' objects, but this goes against clojures interop principles (interop is easy, fast, no surprises).

Comment by Jozef Wagner [ 10/Mar/14 9:54 AM ]

Manifestation of this bug

user=> (assoc (hash-map [1 2 3] :foo) (java.util.ArrayList. [1 2 3]) :bar)
{[1 2 3] :bar, [1 2 3] :foo}
user=> (get (hash-map [1 2 3] :foo) (java.util.ArrayList. [1 2 3]))
nil
Comment by Alex Miller [ 10/Mar/14 10:58 AM ]

I agree that would be a nice thing to say without qualification.

There is a real cost to adding more branches in hasheq - adding those collection checks affects every hasheq. Running a full Clojure build, I see the following set of classes with >100 occurences where this happens (note that exactly 0 of these are the Java collections - this case doesn't exist in the Clojure build itself):

clojure.lang.Var 107001502
java.lang.Class 2651389
java.lang.Character 2076322 
java.util.UUID 435235 
java.util.Date 430956
clojure.lang.Compiler$LocalBinding 116830
java.lang.Boolean 112361
java.util.regex.Pattern 325

We'd be adding 4 more instanceof checks in the path of every one of those hasheqs. This would also likely blow any JVM inlining.

Rich says "all bets should be off for hasheq/equiv of non-values" where Java collections obviously fall into the class of "non-values".

Comment by Andy Fingerhut [ 10/Mar/14 11:04 AM ]

Would a doc patch be considered? Say one that modified the doc of clojure.core/hash to include a phrase indicating that it is only promised to be consistent with clojure.core/= for immutable values? It could even perhaps mention that Floats are out, too: see CLJ-1036

Comment by Alex Miller [ 10/Mar/14 12:00 PM ]

I think it would be preferred to do any detailed docs about hash at http://clojure.org/data_structures rather than in the docstring. Although the docstring on hash probably could use an update and a pointer to the web site after the latest changes.

Comment by Jozef Wagner [ 10/Mar/14 12:14 PM ]

Neverthless it is a breaking change from 1.5, and it should be mentioned in changelog. What still bugs me is that c.c/= is supported in such cases but the c.c/hash is not. If supporting c.c/hash is expensive, isn't it better to drop support for c.c/= in such cases? It will eliminate surprises such as:

user=> (apply distinct? (hash-set [1 2 3] (java.util.Collections/unmodifiableList [1 2 3])))
false
Comment by Alex Miller [ 10/Mar/14 2:05 PM ]

I'm not sure it's a "breaking" change if something not considered to be guaranteed changes. But I take your point.

I don't think it's feasible to drop = support for Clojure and Java collections - that seems important and useful. And if it were free to do so, I would like to be able to say without qualification that if equiv=true, then hasheq is the same.

It's unclear to me that the examples listed on this ticket are actually real problems people are likely to encounter. The main users of hasheq are hash map and hash set. So to manifest, you would need to be putting a mixture of Clojure and Java collections into one of those, in particular a mixture of collections that compare as equal.

Still thinking about it.

Comment by Jozef Wagner [ 10/Mar/14 3:27 PM ]

Sorry for spamming but there may be another option, to not fallback into hashCode in hasheq, but to instead throw in cases where hasheq is requested for non-values. This will lead to a cleaner separation of hash types. Of course it will prevent putting non-values into hash-set.

Comment by Alex Miller [ 10/Mar/14 3:34 PM ]

There is no simple check for "valueness" though?

Comment by Andy Fingerhut [ 10/Mar/14 3:37 PM ]

An idea, for what it might be worth: Add one test for instance of java.util.Collection in Util.hasheq method instead of 3 separate tests for Set, List, and Map. It doesn't cover Map.Entry.

Comment by Alex Miller [ 10/Mar/14 3:38 PM ]

Map doesn't extend Collection either.

Comment by Stuart Halloway [ 11/Mar/14 10:44 AM ]

I think this needs more consideration and should not hold up 1.6.

Comment by Andy Fingerhut [ 20/Mar/14 2:01 PM ]

Both patches clj-1372.diff and clj-1372-2.diff fail to apply cleanly as of latest Clojure master on Mar 20 2014. They did apply cleanly before the Mar 19 2014 commit, I believe, and the only issue appears to be a changed line of diff context. Given the discussion about whether such a change is desired, it sounds like more thought is needed before deciding what change should be made, if any.

Comment by Mike Anderson [ 11/May/14 2:31 PM ]

This is a pretty bad defect. It absolutely needs to be fixed. It's not really about whether using a mix of Clojure and Java collections is a likely use case or not (it probably isn't...), it's about providing consistent guarantees that people can rely upon.

For example, now I'm really unsure about whether some of the library functions I have that use sets or maps are broken or not. I'd be particularly worried about anything that implements object caches / memoisation / interning based on hashed values. Such code may now have some really nasty subtle defects.

Since they are library functions, I can't guarantee what kind of objects are passed in so the code has to work with all possible inputs (either that or I need to write a clear docstring and throw an exception if the input is not supported).

Comment by Michał Marczyk [ 12/May/14 11:29 PM ]

This patch (0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch) makes hasheq consistent with = for java.util.{List,Map,Map.Entry,Set}. Additionally it extends the special treatment of String (return hasheq of hashCode) to all types not otherwise handled (see below for a comment on this).

It is also available here:

https://github.com/michalmarczyk/clojure/tree/alien-hasheq-2

An earlier version is available here:

https://github.com/michalmarczyk/clojure/tree/alien-hasheq

If I understand correctly, what needs to be benchmarked is primarily the "dispatch time" for clojure.lang.Util/hasheq given a Clojure type. So, I ran a Criterium benchmark repeatedly hashing the same persistent hash map, on the theory that this will measure just the dispatch time on IHashEq instances. I then ran a separate benchmark hashing a PHM, a string and a long and adding up the results with unchecked-add. Hopefully this is a good start; I've no doubt additional benchmarks would be useful.

The results are somewhat surprising to me: hasheq on PHM is actually slightly faster in this benchmark on my build than on 1.6.0; the "add three hasheqs" benchmark is slightly faster on 1.6.0.

;;; 1.6.0

;;; NB. j.u.HM benchmark irrelevant
user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (clojure.lang.Util/hasheq phm)) (c/bench (clojure.lang.Util/hasheq juhm)))
WARNING: Final GC required 1.24405836928592 % of runtime
Evaluation count : 5549560980 in 60 samples of 92492683 calls.
             Execution time mean : 9.229881 ns
    Execution time std-deviation : 0.156716 ns
   Execution time lower quantile : 8.985994 ns ( 2.5%)
   Execution time upper quantile : 9.574039 ns (97.5%)
                   Overhead used : 1.741068 ns

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 6.2652 % Variance is slightly inflated by outliers
Evaluation count : 35647680 in 60 samples of 594128 calls.
             Execution time mean : 1.695145 µs
    Execution time std-deviation : 20.186554 ns
   Execution time lower quantile : 1.670049 µs ( 2.5%)
   Execution time upper quantile : 1.740329 µs (97.5%)
                   Overhead used : 1.741068 ns

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (unchecked-add (clojure.lang.Util/hasheq phm) (unchecked-add (clojure.lang.Util/hasheq "foo") (clojure.lang.Util/hasheq 123)))))
WARNING: Final GC required 1.028614538339401 % of runtime
Evaluation count : 1029948300 in 60 samples of 17165805 calls.
             Execution time mean : 56.797488 ns
    Execution time std-deviation : 0.732221 ns
   Execution time lower quantile : 55.856731 ns ( 2.5%)
   Execution time upper quantile : 58.469940 ns (97.5%)
                   Overhead used : 1.836671 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil

;;; patch applied

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] (assert (= (hash phm) (hash juhm))) (c/bench (clojure.lang.Util/hasheq phm)) (c/bench (clojure.lang.Util/hasheq juhm)))
Evaluation count : 5537698680 in 60 samples of 92294978 calls.
             Execution time mean : 8.973200 ns
    Execution time std-deviation : 0.157079 ns
   Execution time lower quantile : 8.733544 ns ( 2.5%)
   Execution time upper quantile : 9.289350 ns (97.5%)
                   Overhead used : 1.744772 ns
Evaluation count : 2481600 in 60 samples of 41360 calls.
             Execution time mean : 24.287800 µs
    Execution time std-deviation : 288.124326 ns
   Execution time lower quantile : 23.856445 µs ( 2.5%)
   Execution time upper quantile : 24.774097 µs (97.5%)
                   Overhead used : 1.744772 ns
nil

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (unchecked-add (clojure.lang.Util/hasheq phm) (unchecked-add (clojure.lang.Util/hasheq "foo") (clojure.lang.Util/hasheq 123)))))
WARNING: Final GC required 1.298136122909759 % of runtime
Evaluation count : 954751500 in 60 samples of 15912525 calls.
             Execution time mean : 61.681794 ns
    Execution time std-deviation : 0.712110 ns
   Execution time lower quantile : 60.622003 ns ( 2.5%)
   Execution time upper quantile : 62.904801 ns (97.5%)
                   Overhead used : 1.744772 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil

As a side note, the earlier version of the patch available on the other branch doesn't have a separate branch for String. This made hasheq faster for objects implementing IHashEq, but slowed down the "three hashes" benchmark roughly by a factor of 2.

Comment by Alex Miller [ 12/May/14 11:39 PM ]

Just for clarity, please refer to patches attached here by name so as time goes on we don't have to correlate attachment time with comment time.

I'm not particularly worried about the cost of things that implement IHashEq as they should be unaffected other than potential inlining issues. I am curious about the cost of hasheq for objects that fall through to the end of the cases and pay the cost for all of the checks. The list farther up in the comments is a good place to start - things like Class, Character, and Var (which could possibly be addressed in Var).

Comment by Michał Marczyk [ 12/May/14 11:47 PM ]

Good point, I've edited the above comment to include the patch name.

Thanks for the benchmarking suggestions – I'll post some new results in ~6 minutes.

Comment by Michał Marczyk [ 13/May/14 12:18 AM ]

First, for completeness, here's a new patch (0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M-alternative.patch) which doesn't do the extra murmuring for types not otherwise handled. It's slower for the single PHM case; see below for details. Also, here's the branch on GitHub:

https://github.com/michalmarczyk/clojure/tree/alien-hasheq-3

As for the new results, the perf hit is quite large, I'm afraid:

;;; with patch (murmur hashCode for default version)
user=> (let [class-instance java.lang.String character-instance \a var-instance #'hash] (c/bench (clojure.lang.Util/hasheq class-instance)) (c/bench (clojure.lang.Util/hasheq character-instance)) (c/bench (clojure.lang.Util/hasheq var-instance)))
WARNING: Final GC required 1.409118084170768 % of runtime
Evaluation count : 655363680 in 60 samples of 10922728 calls.
             Execution time mean : 96.459888 ns
    Execution time std-deviation : 1.019817 ns
   Execution time lower quantile : 95.079086 ns ( 2.5%)
   Execution time upper quantile : 98.684168 ns (97.5%)
                   Overhead used : 1.708347 ns
Evaluation count : 675919140 in 60 samples of 11265319 calls.
             Execution time mean : 88.965959 ns
    Execution time std-deviation : 0.825226 ns
   Execution time lower quantile : 87.817159 ns ( 2.5%)
   Execution time upper quantile : 90.755688 ns (97.5%)
                   Overhead used : 1.708347 ns
Evaluation count : 574987680 in 60 samples of 9583128 calls.
             Execution time mean : 103.881498 ns
    Execution time std-deviation : 1.103615 ns
   Execution time lower quantile : 102.257474 ns ( 2.5%)
   Execution time upper quantile : 106.071144 ns (97.5%)
                   Overhead used : 1.708347 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil

;;; 1.6.0
user=> (let [class-instance java.lang.String character-instance \a var-instance #'hash] (c/bench (clojure.lang.Util/hasheq class-instance)) (c/bench (clojure.lang.Util/hasheq character-instance)) (c/bench (clojure.lang.Util/hasheq var-instance)))
WARNING: Final GC required 1.3353133083866688 % of runtime
Evaluation count : 1829305260 in 60 samples of 30488421 calls.
             Execution time mean : 34.205701 ns
    Execution time std-deviation : 0.379106 ns
   Execution time lower quantile : 33.680636 ns ( 2.5%)
   Execution time upper quantile : 34.990138 ns (97.5%)
                   Overhead used : 1.718257 ns

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 1 (1.6667 %)
	low-mild	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
Evaluation count : 1858100340 in 60 samples of 30968339 calls.
             Execution time mean : 30.401309 ns
    Execution time std-deviation : 0.213878 ns
   Execution time lower quantile : 30.095976 ns ( 2.5%)
   Execution time upper quantile : 30.871497 ns (97.5%)
                   Overhead used : 1.718257 ns
Evaluation count : 1592932200 in 60 samples of 26548870 calls.
             Execution time mean : 36.292934 ns
    Execution time std-deviation : 0.333512 ns
   Execution time lower quantile : 35.795063 ns ( 2.5%)
   Execution time upper quantile : 36.918183 ns (97.5%)
                   Overhead used : 1.718257 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil

One PHM and Class/Character/Var results with the new patch (no extra murmur step in the default case):

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (unchecked-add (clojure.lang.Util/hasheq phm) (unchecked-add (clojure.lang.Util/hasheq "foo") (clojure.lang.Util/hasheq 123)))))
WARNING: Final GC required 1.258952964663877 % of runtime
Evaluation count : 1007768460 in 60 samples of 16796141 calls.
             Execution time mean : 58.195608 ns
    Execution time std-deviation : 0.482804 ns
   Execution time lower quantile : 57.655857 ns ( 2.5%)
   Execution time upper quantile : 59.154655 ns (97.5%)
                   Overhead used : 1.567532 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil
user=> (let [class-instance java.lang.String character-instance \a var-instance #'hash] (c/bench (clojure.lang.Util/hasheq class-instance)) (c/bench (clojure.lang.Util/hasheq character-instance)) (c/bench (clojure.lang.Util/hasheq var-instance)))
Evaluation count : 647944080 in 60 samples of 10799068 calls.
             Execution time mean : 91.275863 ns
    Execution time std-deviation : 0.659943 ns
   Execution time lower quantile : 90.330980 ns ( 2.5%)
   Execution time upper quantile : 92.711120 ns (97.5%)
                   Overhead used : 1.567532 ns
Evaluation count : 699506160 in 60 samples of 11658436 calls.
             Execution time mean : 84.564131 ns
    Execution time std-deviation : 0.517071 ns
   Execution time lower quantile : 83.765607 ns ( 2.5%)
   Execution time upper quantile : 85.569206 ns (97.5%)
                   Overhead used : 1.567532 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
Evaluation count : 594919980 in 60 samples of 9915333 calls.
             Execution time mean : 100.336792 ns
    Execution time std-deviation : 0.811312 ns
   Execution time lower quantile : 99.313490 ns ( 2.5%)
   Execution time upper quantile : 102.167675 ns (97.5%)
                   Overhead used : 1.567532 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
nil
Comment by Michał Marczyk [ 13/May/14 1:05 AM ]

Here's a new patch (0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M-substring.patch) that takes the outrageous approach of replacing the Iterable/Map/Entry test with a .startsWith("java.util.") on the class name. (I experimented with .getClass().getPackage(), but the performance of that was terrible.) The branch is here:

https://github.com/michalmarczyk/clojure/tree/alien-hasheq-4

Hash perf on the "fall-through" cases with this patch seems to be very good:

user=> (let [class-instance java.lang.String character-instance \a var-instance #'hash] (c/bench (clojure.lang.Util/hasheq class-instance)) (c/bench (clojure.lang.Util/hasheq character-instance)) (c/bench (clojure.lang.Util/hasheq var-instance)))
WARNING: Final GC required 1.31690036780011 % of runtime
Evaluation count : 1661453640 in 60 samples of 27690894 calls.
             Execution time mean : 35.099750 ns
    Execution time std-deviation : 0.422800 ns
   Execution time lower quantile : 34.454839 ns ( 2.5%)
   Execution time upper quantile : 35.953584 ns (97.5%)
                   Overhead used : 1.556642 ns
Evaluation count : 1630167600 in 60 samples of 27169460 calls.
             Execution time mean : 35.487409 ns
    Execution time std-deviation : 0.309872 ns
   Execution time lower quantile : 35.083030 ns ( 2.5%)
   Execution time upper quantile : 36.190015 ns (97.5%)
                   Overhead used : 1.556642 ns

Found 4 outliers in 60 samples (6.6667 %)
	low-severe	 3 (5.0000 %)
	low-mild	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
Evaluation count : 1440434700 in 60 samples of 24007245 calls.
             Execution time mean : 40.894457 ns
    Execution time std-deviation : 0.529510 ns
   Execution time lower quantile : 40.055991 ns ( 2.5%)
   Execution time upper quantile : 41.990985 ns (97.5%)
                   Overhead used : 1.556642 ns
nil
Comment by Michał Marczyk [ 13/May/14 1:28 AM ]

The new patch (...-substring.patch) returns hashCode for java.util.** classes other than List, Map, Map.Entry and Set, of course, so no behaviour change there.

Here are the benchmarks for repeated PHM lookups (slightly slower than 1.6.0 apparently, though within 1 ns) and the "add three hasheqs" benchmark (66 ns with patch vs. 57 ns without):

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] (assert (= (hash phm) (hash juhm))) (c/bench (clojure.lang.Util/hasheq phm)) (c/bench (clojure.lang.Util/hasheq juhm)))
Evaluation count : 5183841240 in 60 samples of 86397354 calls.
             Execution time mean : 10.076893 ns
    Execution time std-deviation : 0.182592 ns
   Execution time lower quantile : 9.838456 ns ( 2.5%)
   Execution time upper quantile : 10.481086 ns (97.5%)
                   Overhead used : 1.565749 ns
Evaluation count : 3090420 in 60 samples of 51507 calls.
             Execution time mean : 19.596627 µs
    Execution time std-deviation : 224.380257 ns
   Execution time lower quantile : 19.288347 µs ( 2.5%)
   Execution time upper quantile : 20.085620 µs (97.5%)
                   Overhead used : 1.565749 ns
nil

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (unchecked-add (clojure.lang.Util/hasheq phm) (unchecked-add (clojure.lang.Util/hasheq "foo") (clojure.lang.Util/hasheq 123)))))
WARNING: Final GC required 1.418253438197936 % of runtime
Evaluation count : 879210900 in 60 samples of 14653515 calls.
             Execution time mean : 66.939309 ns
    Execution time std-deviation : 0.747984 ns
   Execution time lower quantile : 65.667310 ns ( 2.5%)
   Execution time upper quantile : 68.155046 ns (97.5%)
                   Overhead used : 1.724002 ns
nil

It is important to note that I have obtained the no-patch result for the "three hasheqs" benchmarks on a fresh JVM when benchmarking 1.6.0, so that's also how I repeated the benchmark with the patch applied. Hashing many different types changes the results noticeably – presumably HotSpot backs off from some optimizations after seeing several different types passed in to hasheq?

Comment by Michał Marczyk [ 13/May/14 8:04 AM ]

Here's a new patch (0005-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch) that introduces a new isAlien static method that checks for instanceof Map/Map.Entry/Iterable and uses this method to test for "alien collection".

Initial benchmarking results are promising:

;;; "fall-through" benchmark
user=> (let [class-instance java.lang.String character-instance \a var-instance #'hash] (c/bench (clojure.lang.Util/hasheq class-instance)) (c/bench (clojure.lang.Util/hasheq character-instance)) (c/bench (clojure.lang.Util/hasheq var-instance)))
WARNING: Final GC required 1.258979068087473 % of runtime
Evaluation count : 1598432100 in 60 samples of 26640535 calls.
             Execution time mean : 36.358882 ns
    Execution time std-deviation : 0.566925 ns
   Execution time lower quantile : 35.718889 ns ( 2.5%)
   Execution time upper quantile : 37.414722 ns (97.5%)
                   Overhead used : 1.823120 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
Evaluation count : 1626362460 in 60 samples of 27106041 calls.
             Execution time mean : 35.426993 ns
    Execution time std-deviation : 0.294517 ns
   Execution time lower quantile : 35.047064 ns ( 2.5%)
   Execution time upper quantile : 36.058667 ns (97.5%)
                   Overhead used : 1.823120 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
Evaluation count : 1461423180 in 60 samples of 24357053 calls.
             Execution time mean : 39.541873 ns
    Execution time std-deviation : 0.423707 ns
   Execution time lower quantile : 38.943560 ns ( 2.5%)
   Execution time upper quantile : 40.499433 ns (97.5%)
                   Overhead used : 1.823120 ns

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil

;;; "three hasheqs" benchmark
user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (unchecked-add (clojure.lang.Util/hasheq phm) (unchecked-add (clojure.lang.Util/hasheq "foo") (clojure.lang.Util/hasheq 123)))))
WARNING: Final GC required 1.5536755331464491 % of runtime
Evaluation count : 820376460 in 60 samples of 13672941 calls.
             Execution time mean : 71.999365 ns
    Execution time std-deviation : 0.746588 ns
   Execution time lower quantile : 70.869739 ns ( 2.5%)
   Execution time upper quantile : 73.565908 ns (97.5%)
                   Overhead used : 1.738155 ns

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil
Comment by Michał Marczyk [ 13/May/14 8:28 AM ]

Ah, I left out the repeated phm hasheq lookup + hasheq of a java.util.HashMap instance pair of benchmarks from the above – here it is for completeness (no surprises though):

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] (assert (= (hash phm) (hash juhm))) (c/bench (clojure.lang.Util/hasheq phm)) (c/bench (clojure.lang.Util/hasheq juhm)))
WARNING: Final GC required 1.260853406580491 % of runtime
Evaluation count : 5369135760 in 60 samples of 89485596 calls.
             Execution time mean : 10.380464 ns
    Execution time std-deviation : 3.407284 ns
   Execution time lower quantile : 9.510624 ns ( 2.5%)
   Execution time upper quantile : 11.461485 ns (97.5%)
                   Overhead used : 1.566301 ns

Found 5 outliers in 60 samples (8.3333 %)
	low-severe	 3 (5.0000 %)
	low-mild	 2 (3.3333 %)
 Variance from outliers : 96.4408 % Variance is severely inflated by outliers
Evaluation count : 3078180 in 60 samples of 51303 calls.
             Execution time mean : 19.717981 µs
    Execution time std-deviation : 209.896848 ns
   Execution time lower quantile : 19.401811 µs ( 2.5%)
   Execution time upper quantile : 20.180163 µs (97.5%)
                   Overhead used : 1.566301 ns

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil
Comment by Alex Miller [ 13/May/14 9:17 AM ]

Please don't submit any patches that change hashcode for anything other than making Java collections match Clojure collections - any other change is out of scope of this ticket.

In general, I would prefer just the execution time mean report for the moment rather than everything - the full criterium output makes these comments much harder to read and compare.

Comment by Alex Miller [ 13/May/14 9:33 AM ]

Could I get a summary of approaches, and a timing of 1.6.0 vs each patch for a consistent set of tests - say time of hash for Long, PHM, juHM, Class, and the "three hasheqs" test?

Comment by Rich Hickey [ 13/May/14 9:47 AM ]

"Hashing many different types changes the results noticeably – presumably HotSpot backs off from some optimizations after seeing several different types passed in to hasheq?"

Right - if your benchmarks do not treat this site as megamorphic you will get all sorts of distorted results.

Comment by Michał Marczyk [ 14/May/14 3:15 AM ]

Ok, I have what I think is an improved microbenchmark for this: xor of hasheqs for a long, a double, a string, a class, a character and a PHM (single instance, so it'll be a hash lookup). The results are not very encouraging.

Single form including the require to make it convenient to run; also bundled is a j.u.HashMap (128 entries) hasheq benchmark:

(do
  (require '[criterium.core :as c])
  (let [l    41235125123
        d    123.456
        s    "asdf;lkjh"
        k    BigInteger
        c    \S
        phm  (apply hash-map (interleave (range 128) (range 128)))
        juhm (java.util.HashMap. phm)
        f    (fn f []
               (-> (clojure.lang.Util/hasheq l)
                   (bit-xor (clojure.lang.Util/hasheq d))
                   (bit-xor (clojure.lang.Util/hasheq s))
                   (bit-xor (clojure.lang.Util/hasheq k))
                   (bit-xor (clojure.lang.Util/hasheq c))
                   (bit-xor (clojure.lang.Util/hasheq phm))))]
    (c/bench (f))
    (c/bench (hash juhm))))

Mean execution time as reported by Criterium:

version xor (ns) j.u.HM (µs)
unpatched 1.6.0 148.128748 1.701640
0005 patch 272.039667 21.201178
original patch 268.670316 21.169436
-alternative patch 271.747043 20.755397

The substring patch is broken (see below), so I skipped it. The patch I'm describing as the "original" one is attached as 0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch.

Decisions common to all the patches:

1. One extra if statement in hasheq just above the default return with a three-way instanceof check.

2. The types tested for are j.u.Iterable, j.u.Map.Entry and j.u.Map.

3. Murmur3.hashOrdered takes Iterable, so that's why it's on the list. Map does not extend Iterable, so it's listed separately. Map.Entry is on the list, because ultimately the way to hash maps is to iterate over and hash their entries.

4. The actual hashing of the "alien" / host types is done by a separate static method – clojure.lang.Util.doalienhasheq – on the theory that this will permit hasheq to be inlined more aggressively and limit the worst of the perf hit to alien collections.

5. doalienhasheq checks for Map, Map.Entry, Set and List; entries are converted to lists for hashing, maps are hashed through entry sets and lists and sets are passed directly to Murmur3.

6. There is also a default case for other Iterable types – we must return hashCode or the result of composing some other function with hashCode for these, since we use equals to test them for equivalence.

The 0005 patch has hasheq call a separate private static method to perform the three-way type check, whereas the others put the check directly in the actual if test. The -alternative patch and the 0005 patch return hashCode in the default case, whereas the original patch composes Murmur3.hashInt with hashCode.

The substring patch only works for java.util.** classes and so doesn't solve the problem (it wouldn't correctly hash Guava collections, for example).

All of the patches change c.l.Util.hasheq and add one or two new static methods to clojure.lang.Util that act as helpers for hasheq. None of them changes anything else. Murmuring hashCode was a performance experiment that appeared to have a slight positive impact on some of the "fast cases" (in fact it's still the best performer among the current three patches in the microbenchmark presented above, although the margin of victory is of course extremely tiny). Thus I think all the current patches are in fact limited in scope to changes directly relevant to the ticket; the -alternative patch and the 0005 patch certainly are.

Comment by Michał Marczyk [ 14/May/14 3:29 AM ]

For completeness, branching on Map, Set etc. directly in hasheq, as with Jozef's original patch, results in the following timings in the microbenchmark introduced in my previous comment:

xor 315.866626 ns
juhm 18.520133 µs
Comment by Michał Marczyk [ 14/May/14 4:01 AM ]

New patch (0006) that leaves out the Map.Entry check; instead, two methods are introduced in the Murmur3 class to handle j.u.maps.

Java map entries aren't really integrated into Clojure – you can't use them like vectors, can't call seq on them etc. – so I don't think they need to match Clojure map entries in hasheq as long as j.u.maps do.

Timings:

xor 233.341689 ns
juhm 9.104637 µs
Comment by Michał Marczyk [ 14/May/14 4:17 AM ]

Checking for Map/Iterable in-line doesn't seem to affect xor benchmark results very much, but makes juhm hashing quicker. This is rather surprising to me. In any case, here's a new patch (0007) and the timings:

xor 233.062337 ns
juhm 8.629149 µs
Comment by Alex Miller [ 14/May/14 7:17 AM ]

What are equivalent timings without the patch?

Comment by Michał Marczyk [ 14/May/14 7:43 AM ]

They're listed in the table in the comment introducing the benchmark – 148.128748 ns for xor, 1.701640 µs for juhm.

Comment by Alex Miller [ 14/May/14 8:02 AM ]

What if we override hasheq for different types instead of using instanceof?

Comment by Michał Marczyk [ 14/May/14 12:50 PM ]

Overloaded methods are resolved statically, so there's no avoiding testing for type in the Object overload.

A more specific overload could be used to speed up hashing for its parameter type given a type hint or for literals, since the compiler would emit calls to that overload given appropriate compile-time information. There wouldn't be any speed-up in "implicit" hashing during hash map / set ops, however.





[CLJ-1364] Primitive VecSeq does not implement equals or hashing methods Created: 19/Feb/14  Updated: 19/Feb/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections


 Description   

VecSeq (as produced by (seq (vector-of :int 1 2 3))) does not implements equals, hashCode, or hasheq and does not play with any other Clojure collections or sequences appropriately in this regard.

user=> (def rs (range 3))
user=> (def vs (seq (vector-of :int 0 1 2)))
user=> rs
(0 1 2)
user=> vs
(0 1 2)
user=> (.equals rs vs)
true
user=> (.equals vs rs)    ;; expect: true
false
user=> (.equiv rs vs)
true
user=> (.equiv vs rs)
true
user=> (.hashCode rs)
29824
user=> (.hashCode vs)     ;; expect to match (.hashCode rs)
2081327893
user=> (System/identityHashCode vs)  ;; show that we're just getting Object hashCode
2081327893
user=> (.hasheq rs)
29824
user=> (.hasheq vs)       ;; expect same as (.hasheq rs) but not implemented at all
IllegalArgumentException No matching field found: hasheq for class clojure.core.VecSeq  clojure.lang.Reflector.getInstanceField (Reflector.java:271)





[CLJ-1362] Reduce broken on some primitive vectors Created: 18/Feb/14  Updated: 25/Apr/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3, Release 1.4, Release 1.5, Release 1.6
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Nathan Davis Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Attachments: Text File clj-1362-v1.patch    
Patch: Code and Test
Approval: Screened

 Description   

In some cases, reduce over a sequence from a primitive vector created with vector-of will return incorrect answers:

user=> (into [] (drop 32 (into [] (range 33))))
[32]
user=> (into [] (drop 32 (into (vector-of :int) (range 33))))
[0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32]

Second call should return [32] just like the first one.

Cause: VecSeq (seq on primitive Vec obtained with vector-of) maintains two flags: i is the total number of elements prior to the current node in this seq. offset is the offset in the current anode. When using internal-reduce on a VecSeq, the starting index for the reduce was using offset and ignoring i.

Solution: Use (+ i offset) as the starting index.

Patch: clj-1362-v1.patch

Screened by:



 Comments   
Comment by Alex Miller [ 18/Feb/14 10:18 PM ]

We did some debugging on this at the St. Louis Clojure Meetup tonight and suspect the problem is happening when drop walks through the chunked seq over the vector. Specifically, in the VecSeq's implementation of IChunkedSeq.chunkedNext() at https://github.com/clojure/clojure/blob/master/src/clj/clojure/gvec.clj#L116 particularly the offset 0 at the end.

Comment by Alex Miller [ 19/Feb/14 2:41 PM ]

Upon further review, the VecSeq seems to be created properly during chunking. The real issue is in internal-reduce where the starting index is improperly computed.

Comment by Stuart Sierra [ 25/Apr/14 1:05 PM ]

Screened.





[CLJ-1360] clojure.string/split strips trailing delimiters Created: 18/Feb/14  Updated: 18/Feb/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Tim McCormack Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   

clojure.string/split and clojure.string/split-lines inherit the bizarre default behavior of java.lang.String#split(String,int) in stripping trailing consecutive delimiters:

(clojure.string/split "banana" #"an")
⇒ ["b" "" "a"]
(clojure.string/split "banana" #"na")
⇒ ["ba"]
(clojure.string/split "nanabanana" #"na")
⇒ ["" "" "ba"]

In the case of split-lines, processing a file line by line and rejoining results in truncation of trailing newlines in the file. In both cases, the behavior is surprising and cannot be inferred from the docstrings.

This behavior should either be fixed or documented.



 Comments   
Comment by Andy Fingerhut [ 18/Feb/14 10:51 AM ]

Probably documenting would be safer than changing the behavior at this point, given that some people may actually rely on the current behavior after testing, deploying, etc.

I don't currently have a suggestion for a modified doc string, but note that there are examples of this behavior and how one can use an extra "-1" limit argument at the end to get all split strings: http://clojuredocs.org/clojure_core/clojure.string/split





[CLJ-1325] Report warnings if *unchecked-math* and boxing happens Created: 16/Jan/14  Updated: 16/May/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 1
Labels: errormsgs, math

Attachments: File boxed.diff     Text File boxedmath.txt     Text File clj-1325.patch     Text File clj-1325-v2.patch     Text File clj-1325-v3.patch    
Patch: Code and Test
Approval: Screened

 Description   

Currently, it is difficult to tell that the compiler is using boxed math unless you look at the generated bytecode. The proposed enhancement here is to emit new warnings if *unchecked-math* is on and boxed math is occurring.

Approach: In the compiler, when compiling a StaticMethodExpr, if *unchecked-math* is true and the class is clojure.lang.Numbers and one of the parameters of static method is of type java.lang.Object or java.lang.Number, then emit a warning at compile-time.

In addition, there is a new WarnBoxedMath Java annotation - a small number of methods on Numbers with Object parameters use this annotation to indicate that warning should not take place. The same annotation can be (but is not currently) used to mark methods on Numbers without Object/Number params that should warn. See boxedmath.txt for a list of methods and categories.

Patch: clj-1325-v3.patch

Screened by:



 Comments   
Comment by Alex Miller [ 14/Apr/14 10:56 PM ]

Moving to 1.7.

Comment by Alex Miller [ 15/Apr/14 10:17 AM ]

List of methods in Numbers and whether they should be considered "boxed math" or not, with some questions.

Comment by Alex Miller [ 14/May/14 2:34 PM ]

Ready for screening.

Comment by Alex Miller [ 16/May/14 11:19 AM ]

clj-1325-v2.patch is identical to last except for a cleaned up the commit message.

Comment by Alex Miller [ 16/May/14 11:51 AM ]

Added v3 patch that just reworks block/indentation style to match surrounding code better.

Comment by Stuart Sierra [ 16/May/14 1:15 PM ]

Screened. Comments:

1) There is no way to get both overflow checks and boxed-math warnings at the same time. Maybe this doesn't matter.

2) The error messages aren't ideal, because they refer to clojure.lang.Numbers, but we can assume that anyone savvy enough to be using *unboxed-math* will also be savvy enough to know what clojure.lang.Numbers is.

3) This doesn't protect me from autoboxing in arbitrary Java method calls, but normal reflection warnings should catch most real-world cases, since few Java APIs overload on primitive and Object.





[CLJ-1322] doseq with several bindings causes "ClassFormatError: Invalid Method Code length" Created: 10/Jan/14  Updated: 28/Jun/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Miikka Koskinen Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Clojure 1.5.1, java 1.7.0_25, OpenJDK Runtime Environment (IcedTea 2.3.10) (7u25-2.3.10-1ubuntu0.12.04.2)


Attachments: Text File doseq-bench.txt     Text File doseq.patch     File script.clj    
Patch: Code
Approval: Screened

 Description   

Important Perf Note the new impl is faster for collections that are custom-reducible but not chunked, and is also faster for large numbers of bindings. The original implementation is hand tuned for chunked collections, and wins for larger chunked coll/smaller binding count scenarios, presumably due to the fn call/return tracking overhead of reduce. Details are in the comments.
Screened By Stu
Patch doseq.patch

user=> (def a1 (range 10))
#'user/a1
user=> (doseq [x1 a1 x2 a1 x3 a1 x4 a1 x5 a1 x6 a1 x7 a1 x8 a1] (do))
CompilerException java.lang.ClassFormatError: Invalid method Code length 69883 in class file user$eval1032, compiling:(NO_SOURCE_PATH:2:1)

While this example is silly, it's a problem we've hit a couple of times. It's pretty surprising when you have just a couple of lines of code and suddenly you get the code length error.



 Comments   
Comment by Kevin Downey [ 18/Apr/14 12:20 AM ]

reproduces with jdk 1.8.0 and clojure 1.6

Comment by Nicola Mometto [ 22/Apr/14 5:35 PM ]

A potential fix for this is to make doseq generate intermediate fns like `for` does instead of expanding all the code directly.

Comment by Ghadi Shayban [ 25/Jun/14 8:39 PM ]

Existing doseq handles chunked-traversal internally, deciding the
mechanics of traversal for a seq. In addition to possibly conflating
concerns, this is causing a code explosion blowup when more bindings are
added, approx 240 bytes of bytecode per binding (without modifiers).

This approach redefs doseq later in core.clj, after protocol-based
reduce (and other modern conveniences like destructuring.)

It supports the existing :let, :while, and :when modifiers.

New is a stronger assertion that modifiers cannot come before binding
expressions. (Same semantics as let, i.e. left to right)

valid: [x coll :when (foo x)]
invalid: [:when (foo x) x coll]

This implementation does not suffer from the code explosion problem.
About 25 bytes of bytecode + 1 fn per binding.

Implementing this without destructuring was not a party, luckily reduce
is defined later in core.

Comment by Andy Fingerhut [ 26/Jun/14 12:25 AM ]

For anyone reviewing this patch, note that there are already many tests for correct functionality of doseq in file test/clojure/test_clojure/for.clj. It may not be immediately obvious, but every test for 'for' defined with deftest-both is a test for 'for' and also for 'doseq'.

Regarding the current implementation of doseq: it in't simply that it is too many bytes per binding, it is that the code size doubles with each additional binding. See these results, which measures size of the macroexpanded form rather than byte code size, but those two things should be fairly linearly related to each other here:

(defn formsize [form]
  (count (with-out-str (print (macroexpand form)))))

user=> (formsize '(doseq [x (range 10)] (print x)))
652
user=> (formsize '(doseq [x (range 10) y (range 10)] (print x y)))
1960
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10)] (print x y z)))
4584
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10) w (range 10)] (print x y z w)))
9947
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10) w (range 10) p (range 10)] (print x y z w p)))
20997

Here are results for the same expressions after Ghadi's patch doseq.patch dated June 25 2014:

user=> (formsize '(doseq [x (range 10)] (print x)))
93
user=> (formsize '(doseq [x (range 10) y (range 10)] (print x y)))
170
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10)] (print x y z)))
247
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10) w (range 10)] (print x y z w)))
324
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10) w (range 10) p (range 10)] (print x y z w p)))
401

It would be good to see some performance results with and without this patch, too.

Comment by Stuart Halloway [ 28/Jun/14 2:21 PM ]

In the tests below, the new impl is called "doseq2", vs. the original impl "doseq"

(def hund (into [] (range 100)))
(def ten (into [] (range 10)))
(def arr (int-array 100))
(def s "superduper")

;; big seq, few bindings: doseq2 LOSES
(dotimes [_ 5]
  (time (doseq [a (range 100000000)])))
;; 1.2 sec

(dotimes [_ 5]
  (time (doseq2 [a (range 100000000)])))
;; 1.8 sec

;; small unchunked reducible, few bindings: doseq2 wins
(dotimes [_ 5]
  (time (doseq [a s b s c s])))
;; 0.5 sec

(dotimes [_ 5]
  (time (doseq2 [a s b s c s])))
;; 0.2 sec

(dotimes [_ 5]
  (time (doseq [a arr b arr c arr])))
;; 40 msec

(dotimes [_ 5]
  (time (doseq2 [a arr b arr c arr])))
;; 8 msec

;; small chunked reducible, few bindings: doseq2 LOSES
(dotimes [_ 5]
  (time (doseq [a hund b hund c hund])))
;; 2 msec

(dotimes [_ 5]
  (time (doseq2 [a hund b hund c hund])))
;; 8 msec

;; more bindings: doseq2 wins bigger and bigger
(dotimes [_ 5]
  (time (doseq [a ten b ten c ten d ten ])))
;; 2 msec

(dotimes [_ 5]
  (time (doseq2 [a ten b ten c ten d ten ])))
;; 0.4 msec

(dotimes [_ 5]
  (time (doseq [a ten b ten c ten d ten e ten])))
;; 18 msec

(dotimes [_ 5]
  (time (doseq2 [a ten b ten c ten d ten e ten])))
;; 1 msec
Comment by Ghadi Shayban [ 28/Jun/14 6:23 PM ]

Hmm, I cannot reproduce your results.

I'm not sure whether you are testing with lein, on what platform, what jvm opts.

Can we test using this little harness instead directly against clojure.jar? I've attached a the harness and two runs of results (one w/ default heap, the other 3GB w/ G1GC)

I added a medium and small (range) too.

Anecdotally, I see doseq2 outperform in all cases except the small range. Using criterium shows a wider performance gap favoring doseq2.

I pasted the results side by side for easier viewing.

core/doseq                          doseq2
"Elapsed time: 1610.865146 msecs"   "Elapsed time: 2315.427573 msecs"
"Elapsed time: 2561.079069 msecs"   "Elapsed time: 2232.479584 msecs"
"Elapsed time: 2446.674237 msecs"   "Elapsed time: 2234.556301 msecs"
"Elapsed time: 2443.129809 msecs"   "Elapsed time: 2224.302855 msecs"
"Elapsed time: 2456.406103 msecs"   "Elapsed time: 2210.383112 msecs"

;; med range, few bindings:
core/doseq                          doseq2
"Elapsed time: 28.383197 msecs"     "Elapsed time: 31.676448 msecs"
"Elapsed time: 13.908323 msecs"     "Elapsed time: 11.136818 msecs"
"Elapsed time: 18.956345 msecs"     "Elapsed time: 11.137122 msecs"
"Elapsed time: 12.367901 msecs"     "Elapsed time: 11.049121 msecs"
"Elapsed time: 13.449006 msecs"     "Elapsed time: 11.141385 msecs"

;; small range, few bindings:
core/doseq                          doseq2
"Elapsed time: 0.386334 msecs"      "Elapsed time: 0.372388 msecs"
"Elapsed time: 0.10521 msecs"       "Elapsed time: 0.203328 msecs"
"Elapsed time: 0.083378 msecs"      "Elapsed time: 0.179116 msecs"
"Elapsed time: 0.097281 msecs"      "Elapsed time: 0.150563 msecs"
"Elapsed time: 0.095649 msecs"      "Elapsed time: 0.167609 msecs"

;; small unchunked reducible, few bindings:
core/doseq                          doseq2
"Elapsed time: 2.351466 msecs"      "Elapsed time: 2.749858 msecs"
"Elapsed time: 0.755616 msecs"      "Elapsed time: 0.80578 msecs"
"Elapsed time: 0.664072 msecs"      "Elapsed time: 0.661074 msecs"
"Elapsed time: 0.549186 msecs"      "Elapsed time: 0.712239 msecs"
"Elapsed time: 0.551442 msecs"      "Elapsed time: 0.518207 msecs"

core/doseq                          doseq2
"Elapsed time: 95.237101 msecs"     "Elapsed time: 55.3067 msecs"
"Elapsed time: 41.030972 msecs"     "Elapsed time: 30.817747 msecs"
"Elapsed time: 42.107288 msecs"     "Elapsed time: 19.535747 msecs"
"Elapsed time: 41.088291 msecs"     "Elapsed time: 4.099174 msecs"
"Elapsed time: 41.03616 msecs"      "Elapsed time: 4.084832 msecs"

;; small chunked reducible, few bindings:
core/doseq                          doseq2
"Elapsed time: 31.793603 msecs"     "Elapsed time: 40.082492 msecs"
"Elapsed time: 17.302798 msecs"     "Elapsed time: 28.286991 msecs"
"Elapsed time: 17.212189 msecs"     "Elapsed time: 14.897374 msecs"
"Elapsed time: 17.266534 msecs"     "Elapsed time: 10.248547 msecs"
"Elapsed time: 17.227381 msecs"     "Elapsed time: 10.022326 msecs"

;; more bindings:
core/doseq                          doseq2
"Elapsed time: 4.418727 msecs"      "Elapsed time: 2.685198 msecs"
"Elapsed time: 2.421063 msecs"      "Elapsed time: 2.384134 msecs"
"Elapsed time: 2.210393 msecs"      "Elapsed time: 2.341696 msecs"
"Elapsed time: 2.450744 msecs"      "Elapsed time: 2.339638 msecs"
"Elapsed time: 2.223919 msecs"      "Elapsed time: 2.372942 msecs"

core/doseq                          doseq2
"Elapsed time: 28.869393 msecs"     "Elapsed time: 2.997713 msecs"
"Elapsed time: 22.414038 msecs"     "Elapsed time: 1.807955 msecs"
"Elapsed time: 21.913959 msecs"     "Elapsed time: 1.870567 msecs"
"Elapsed time: 22.357315 msecs"     "Elapsed time: 1.904163 msecs"
"Elapsed time: 21.138915 msecs"     "Elapsed time: 1.694175 msecs"
Comment by Ghadi Shayban [ 28/Jun/14 6:47 PM ]

It's good that the benchmarks contain empty doseq bodies in order to isolate the overhead of traversal. However, that represents 0% of actual real-world code.

At least for the first benchmark (large chunked seq), adding in some tiny amount of work did not change results signifantly. Neither for (map str [a])

(range 10000000) =>  (map str [a])
core/doseq
"Elapsed time: 586.822389 msecs"
"Elapsed time: 563.640203 msecs"
"Elapsed time: 369.922975 msecs"
"Elapsed time: 366.164601 msecs"
"Elapsed time: 373.27327 msecs"
doseq2
"Elapsed time: 419.704021 msecs"
"Elapsed time: 371.065783 msecs"
"Elapsed time: 358.779231 msecs"
"Elapsed time: 363.874448 msecs"
"Elapsed time: 368.059586 msecs"

nor for intrisics like (inc a)

(range 10000000)
core/doseq
"Elapsed time: 317.091849 msecs"
"Elapsed time: 272.360988 msecs"
"Elapsed time: 215.501737 msecs"
"Elapsed time: 206.639181 msecs"
"Elapsed time: 206.883343 msecs"
doseq2
"Elapsed time: 241.475974 msecs"
"Elapsed time: 193.154832 msecs"
"Elapsed time: 198.757873 msecs"
"Elapsed time: 197.803042 msecs"
"Elapsed time: 200.603786 msecs"

I still see reduce-based doseq ahead of the original, except for small seqs





[CLJ-1315] Don't initialize classes when importing them Created: 28/Dec/13  Updated: 13/May/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.1, Release 1.2, Release 1.3, Release 1.4, Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Aaron Cohen Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: aot, compiler, interop

Attachments: Text File 0001-Don-t-initialize-classes-during-import.patch    
Patch: Code
Approval: Triaged

 Description   

Problem: When classes are imported in Clojure, the class is loaded using Class.forName(), which causes its static initialisers to be executed. This differs from Java where compilation does not cause classes to be loaded.

Motivation: In many cases when those classes are normally loaded by Java code during execution of a framework of some kind (IntelliJ in my case, and RoboVM is another culprit mentioned in that thread) the initialiser expects some infrastructure to be in place and will fail when it's not. This means that it's impossible to AOT compile namespaces importing these classes, which is a fairly serious limitation.

Approach: Modify ImportExpr to call RT.classForNameNonLoading() instead of Class.forName(), which will load the class but not initialise it. This change causes the Clojure test suite to fail, since clojure.test-clojure.genclass imports a gen-class'ed class which no longer loads its namespace on initialisation. I'm not sure if this is considered an incorrect use of such a class (IIRC with records it's required to import the class and require its namespace), but given that it's in the Clojure test case it's reasonable to assume that this fix would be a breaking change for more code out there. This test failure is also corrected in the attached patch.

Patch: 0001-Don-t-initialize-classes-during-import.patch

Alternative: This patch enables the change unconditionally, but depending on the extent of breakage it causes, it might need to be enabled with a configuration flag. I propose we make it unconditional in an early 1.7 beta and monitor the fall-out.

Background: This issue has been discussed in the following threads
https://groups.google.com/d/topic/clojure/tWSEsOk_pM4/discussion
https://groups.google.com/forum/#!topic/clojure-dev/qSSI9Z-Thc0



 Comments   
Comment by Alex Miller [ 29/Dec/13 12:23 PM ]

From original post:

This issue was originally reported by Zach Oakes and Colin Fleming and this patch was also tested by Colin.

I'm duplicating here my suggested release notes for this issue, which includes my current thoughts on potential breakage (it's also in the commit message of the patch):

    "import" no longer causes the imported class to be initialized. This
    change better matches Java's import behavior and allows the importing of
    classes that do significant work at initialization time which may fail.
    This semantics change is not expected to effect most code, but certain
    code may have depended on behavior that is no longer true.

    1) importing a Class defined via gen-class no longer causes its defining
    namespace to be loaded, loading is now deferred until first reference. If
    immediate loading of the namespace is needed, "require" it directly.
    2) Some code may have depended on import to initialize the class before it
    was used. It may now be necessary to manually call (Class/forName
    "org.example.Class") when initialization is needed. In most cases, this
    should not be necessary because the Class will be initialized
    automatically before first use.
Comment by Greg Chapman [ 13/May/14 6:25 PM ]

I'm not sure if this should also be fixed, but it would be nice if you could emit the code for a proxy of one of these non-initialized classes without forcing initialization. For example, the following raises an exception (I'm using Java 8):

Clojure 1.6.0
user=> (def cname "javafx.scene.control.ListCell")
#'user/cname
user=> (let [cls (Class/forName cname false (clojure.lang.RT/baseLoader))] (.importClass *ns* cls))
javafx.scene.control.ListCell
user=> (defn fails [] (proxy [ListCell] [] (updateItem [item empty] (proxy-super item empty))))
CompilerException java.lang.ExceptionInInitializerError, compiling:(NO_SOURCE_PATH:3:16)

The exception was ultimately caused by "IllegalStateException Toolkit not initialized", which javafx throws if you attempt to initialize a Control class outside of Application.launch.





[CLJ-1297] try to catch using - instead of _ in filenames so the compiler can give a better error message for people who don't know that you need to use _ in file names Created: 19/Nov/13  Updated: 27/Jun/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 11
Labels: compiler, errormsgs

Attachments: File better-error-messages-for-require.diff    
Patch: Code and Test
Approval: Screened

 Description   

Screener's Note: This works as advertised, but I have reservations about the approach. We could accept the patch as-is, or a much simpler patch that handles the only important (IMO) case: a-b-c to a_b_c – without generating and testing for unlikely errors like a-b_c. Please advise.

Problem: Clojure requires the files that back a namespace that has dashes in it to have the dashes replaced with underscores on the filesystem (ie a.b_c.clj for namespace a.b-c). If you require a file that has been mistakenly saved as b-c.clj instead, you will get an error message:

Exception in thread "main" java.io.FileNotFoundException: Could not locate a/b_c__init.class or a/b_c.clj on classpath:
	at clojure.lang.RT.load(RT.java:443)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5018.invoke(core.clj:5530)
	at clojure.core$load.doInvoke(core.clj:5529)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5336)
	at clojure.core$load_lib$fn__4967.invoke(core.clj:5375)
	at clojure.core$load_lib.doInvoke(core.clj:5374)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:619)
	at clojure.core$load_libs.doInvoke(core.clj:5413)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:619)
	at clojure.core$require.doInvoke(core.clj:5496)

Proposed:

  • When loading the resource-root of lib throws a FileNotFoundException, the lib is analyzed...
  • ... if the lib was a name that would be munged, it examines the combinatorial explosion of munge candidates and .clj or .class files in the classpath ...
  • ... if any of these candidates exist, it informs the user of the file's existence, and that a change to that filename would lead to that resource being loaded.
  • ... if none of these candidates exist, it throws the original exception.

It also modifies clojure.lang.RT to expose the behavior around finding clj or class files from a resource root.

Patch: better-error-messages-for-require.diff



 Comments   
Comment by Joshua Ballanco [ 20/Nov/13 12:15 AM ]

A perhaps even better solution would be to simply allow the use of dashes in *.clj[s] filenames. I can't imagine the extra disk access per-namespace would be a huge performance burden, and (since dashes aren't allowed currently) I don't think there would be any issues with backwards compatibility.

Comment by Gary Fredericks [ 20/Nov/13 8:40 AM ]

It's worth mentioning the combinatorial explosion for namespaces with multiple dashes – if I (require 'foo-bar.baz-bang), should clojure search for all four possible filenames? Does the jvm have a way to search for files by regex or similar to avoid nasty degenerate cases (like (require 'foo-------------))?

Comment by Joshua Ballanco [ 20/Nov/13 11:08 AM ]

According to the docs, the FileSystem class's "getPathMatcher" method accepts path globs, so you'd merely have to replace each instance of "-" or "_" with "{-,_}". Actual runtime characteristics would likely depend on the underlying filesystem's implementation.

Comment by Alex Miller [ 20/Nov/13 12:02 PM ]

I don't think the FileSystem stuff applies when looking up classes on the classpath. Note that Java class names cannot contain "-".

Comment by Phil Hagelberg [ 21/Nov/13 12:05 PM ]

According to the spec, Java class names can't contain dashes (though IIRC OpenJDK and Oracle's JDK accept them anyway) but the requirement that Clojure source files have names which align with their AOT'd class file eqivalents is something we've imposed upon ourselves. Introducing the disconnect between .clj files and .class files makes way more sense than disconnecting namespaces and .clj files, but arguably it's too late to fix that mistake.

In any case a check for dashed files (resulting only in a more informative compiler error, not a more permissive compiler) which only triggers when a .clj file cannot be found imposes zero overhead in the case where things are already working.

Comment by scott tudd [ 09/Dec/13 2:19 PM ]

As Clojure seems to be idiomatic to have sometimes-dashed-namespace-and-function-names as opposed to the ubiquitous camelCaseFunctionNames in java ... I agree to have the compiler automagically handle 'knowing' to look in dir_struct AND dir-struct for requisite files.

or at the least print out a nice message explaining the quirk when files "can't" be found ... WHEN there are dashes and underscores involved... anything to aid in helping things "just work" as one would think they're supposed to.

Comment by Obadz [ 12/Dec/13 5:28 AM ]

I would have saved a few hours as well.

Comment by Alexander Redington [ 14/Feb/14 2:29 PM ]

This patch changes clojure.core/load such that:

  • When loading the resource-root of lib throws a FileNotFoundException, the lib is analyzed...
  • ... if the lib was a name that would be munged, it examines the combinatorial explosion of munge candidates and .clj or .class files in the classpath ...
  • ... if any of these candidates exist, it informs the user of the file's existance, and that a change to that filename would lead to that resource being loaded.
  • ... if none of these candidates exist, it throws the original exception.

It also modifies clojure.lang.RT to expose the behavior around finding clj or class files from a resource root.

Comment by Andy Fingerhut [ 20/Mar/14 1:16 PM ]

I do not know whether it handles all of the cases proposed in this discussion, but I encourage folks to check out the filename/namespace consistency checking in the latest Eastwood release (version 0.1.1) to see if it catches the cases they would hope to catch. It does a static check based on the files in a Leiningen project, nothing at run time. https://github.com/jonase/eastwood

Of course changes to Clojure itself to give warnings about such things can still be very useful, since not everyone will be using a 3rd party tool to check for such things.

Comment by Alex Miller [ 27/Jun/14 2:24 PM ]

Re the screener's note at the top, my preference would be for the simpler approach.





[CLJ-1289] aset-* and aget perform poorly on multi-dimensional arrays even with type hints. Created: 01/Nov/13  Updated: 14/Feb/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Michael O. Church Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: arrays, performance
Environment:

Clojure 1.5.1.

Dependencies: criterium


Attachments: Text File CLJ-1289-p1.patch    
Patch: Code
Approval: Triaged

 Description   

Here's a transcript of the behavior. I don't know for sure that reflection is being done, but the performance penalty (about 1300x) suggests it.

user=> (use 'criterium.core)
nil
user=> (def b (make-array Double/TYPE 1000 1000))
#'user/b
user=> (quick-bench (aget ^"[[D" b 304 175))
WARNING: Final GC required 3.5198021166354323 % of runtime
WARNING: Final GC required 29.172288684474303 % of runtime
Evaluation count : 63558 in 6 samples of 10593 calls.
             Execution time mean : 9.457308 µs
    Execution time std-deviation : 126.220954 ns
   Execution time lower quantile : 9.344450 µs ( 2.5%)
   Execution time upper quantile : 9.629202 µs (97.5%)
                   Overhead used : 2.477107 ns

One workaround is to use multiple agets.

user=> (quick-bench (aget ^"[D" (aget ^"[[D" b 304) 175))
WARNING: Final GC required 40.59820310542545 % of runtime
Evaluation count : 62135436 in 6 samples of 10355906 calls.
             Execution time mean : 6.999273 ns
    Execution time std-deviation : 0.112703 ns
   Execution time lower quantile : 6.817782 ns ( 2.5%)
   Execution time upper quantile : 7.113845 ns (97.5%)
                   Overhead used : 2.477107 ns

Cause: The inlined version only applies to arity 2, and otherwise it reflects.



 Comments   
Comment by Gary Fredericks [ 08/Dec/13 9:28 PM ]

A glance at the source makes it obvious that the hypothesis is correct – the inlined version only applies to arity 2, and otherwise it reflects.

I thought this would be as simple as converting the inline function to be variadic (using reduce), but after trying it I realized this is tricky as you have to generate the correct type hints for each step. E.g., given ^"[[D" the inline function needs to type-hint the intermediate result with ^"[D". This isn't difficult if we're just dealing with strings that begin with square brackets, but I don't know for sure that those are the only possibilities.

Comment by Yaron Peleg [ 13/Feb/14 4:44 AM ]

Bump. I just got bitten bad by this.

There are two seperate issues here:
1) (aget 2d-array-doubles 0 0 ) doesn't emit a reflection warning.
2) It seems like the compiler has enough information to avoid the reflective call here.

Note this gets exp. worse as number of dimensions grows, i.e (get doubles3d 0 0 0)
will be 1M slower, etc' Not true, unless you iterate over all elements. it's
simply n_dims*1000x per lookup.

Nasty surprise, especially considering you often go to primitive arrays for speed,
and a common use case is an inner loop(s) that iterate over arrays.

Comment by Gary Fredericks [ 13/Feb/14 7:08 AM ]

I can probably take a stab at this.

Comment by Gary Fredericks [ 13/Feb/14 8:34 PM ]

I think the reflection warning problem is pretty much impossible to solve without changing code elsewhere in the compiler, because the reflection done in aget is a different kind than normal clojure reflection – it's explicitly in the function body rather than emitted by the compiler. Since the compiler isn't emitting it, it doesn't reasonably know it's even there. So even if aget is fixed for other arities, you still won't get the warning when it's not inlined.

I can imagine some sort of metadata that you could put on a function telling the compiler that it will reflect if not inlined. Or maybe a more generic not-inlined warning?

The global scope of adding another compiler flag seems about balanced by the seriousness of array functions not being able to warn on reflection.

Comment by Gary Fredericks [ 13/Feb/14 8:52 PM ]

Attached CLJ-1289-p1.patch which simply inlines variadic calls to aget. It assumes that if it sees a :tag on the array arg that is a string beginning with [, it can assume that the return value from one call to aget can be tagged with the same string with the leading [ stripped off.

I'm not a jvm expert, but having read through the spec a little bit I think this is a reasonable assumption.

Comment by Alex Miller [ 14/Feb/14 3:34 PM ]

I think this probably is actually true, but a more official way to ask that question would be to get the array class and ask for Class.getComponentType() (and less janky than string munging).

Comment by Gary Fredericks [ 14/Feb/14 3:40 PM ]

How would you get the array class based on the :tag type hint?

Comment by Gary Fredericks [ 14/Feb/14 7:05 PM ]

I see (-> s (Class/forName) (.getComponentType) (.getName)) does the same thing – is that route preferred, or is there another one?





[CLJ-1280] Create reusable exception that can carry file/line/col info Created: 18/Oct/13  Updated: 18/Apr/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   

This concept already exists in multiple places in Clojure - Compiler$CompilerException and the Exception classes buried in EdnReader and LispReader. It would also be useful in other places where IllegalArgument or other other exceptions are thrown.

For example, this protocol exception throws an IllegalArgumentException and could transmit the file, line, and column info at the location of the error but it seems weird to use any of the existing exceptions for this purpose.

(defprotocol Bar (m [this]) (m [this arg]))


 Comments   
Comment by Kevin Downey [ 18/Apr/14 12:09 AM ]

seems like ExceptionInfo can do this





[CLJ-1279] Fix confusing macroexpand1 ArityException handling Created: 16/Oct/13  Updated: 05/Feb/14

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

Type: Defect Priority: Major
Reporter: Alex Coventry Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: Compiler, errormsgs, macro

Attachments: Text File 0001-Edit-macro-ArityException-in-AFn.patch     Text File 0001-Fix-macroexpand1-s-handling-of-ArityException.patch    
Patch: Code and Test

 Description   

macros can give very confusing error messages when they execute a form which generates an ArityException. clojure.lang.Compiler.macroexpand1 assumes that any ArityException comes from the call to the macro itself, which need not be the case. For instance:

user> (do (defmacro f [] (assoc)) (f))
ArityException Wrong number of args (-2) passed to: core$assoc clojure.lang.Compiler.macroexpand1 (Compiler.java:6488)
user> (use 'clojure.repl) (pst)
nil
ArityException Wrong number of args (-2) passed to: core$assoc
clojure.lang.Compiler.macroexpand1 (Compiler.java:6488)
clojure.lang.Compiler.macroexpand (Compiler.java:6544)
clojure.lang.Compiler.eval (Compiler.java:6618)
clojure.lang.Compiler.eval (Compiler.java:6624)
clojure.lang.Compiler.eval (Compiler.java:6597)
clojure.core/eval (core.clj:2864)
clojure.main/repl/read-eval-print-6596/fn-6599 (main.clj:260)
clojure.main/repl/read-eval-print--6596 (main.clj:260)
clojure.main/repl/fn--6605 (main.clj:278)
clojure.main/repl (main.clj:278)
clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn--1251 (interruptible_eval.clj:56)
clojure.core/apply (core.clj:617)
nil

Easy enough to see the source of the problem in this case, but because both the number of arguments actually passed is off by two, and the stacktrace element for the call to assoc has been dropped, this shortcut by macroexpand1 can get super confusing.

The attached patch corrects this behavior. E.g.

user=> (do (defmacro f [] (assoc)) (f))
ArityException Wrong number of args (0) passed to: core$assoc clojure.lang.AFn.throwArity (AFn.java:437)
user=> (use 'clojure.repl) (pst)
nil
ArityException Wrong number of args (0) passed to: core$assoc
user/f (NO_SOURCE_FILE:1)
clojure.lang.Var.invoke (Var.java:419)
clojure.lang.Var.applyTo (Var.java:532)
clojure.lang.Compiler.macroexpand1 (Compiler.java:6507)
clojure.lang.Compiler.macroexpand (Compiler.java:6580)
clojure.lang.Compiler.eval (Compiler.java:6654)
clojure.lang.Compiler.eval (Compiler.java:6660)
clojure.lang.Compiler.eval (Compiler.java:6633)
clojure.core/eval (core.clj:2864)
clojure.main/repl/read-eval-print-6594/fn-6597 (main.clj:260)
clojure.main/repl/read-eval-print--6594 (main.clj:260)
clojure.main/repl/fn--6603 (main.clj:278)
nil



 Comments   
Comment by Alex Coventry [ 17/Oct/13 11:01 AM ]

Patch with test

Comment by Alex Coventry [ 23/Oct/13 11:42 PM ]

Amended patch to deal more gracefully with unexpected stack trace structure.

Comment by Alex Miller [ 24/Oct/13 12:09 AM ]

Also see CLJ-397 and CLJ-383.

Comment by Alex Coventry [ 24/Oct/13 2:46 PM ]

Thanks, Alex. It would be easy enough to move most of the logic into ArityException, which would be a compromise between Stu's[1] options 1 and 2. Is that worth doing?

Amending clojure.lang.AFn.throwArity to check whether "this" is a macro and adjust the arg count there accordingly might be the simplest way. I can see why Rich prefers all the logic to go into ArityException, but since ArityExceptions are used for things other than macros, I don't see a way to make an honest error message there without groveling the stack trace.

[1] http://dev.clojure.org/jira/browse/CLJ-397?focusedCommentId=24090&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-24090

Comment by Alex Miller [ 24/Oct/13 2:57 PM ]

I would have to take more time than I have to make an informed opinion but I can say that from a general point of view inspecting StackTraceElements does not seem like the right solution to (almost) any problem.

Comment by Alex Coventry [ 24/Oct/13 10:26 PM ]

This patch causes Var.setMacro to set instance attribute AFn.macrop to true, so that AFn.throwArity can reduce the number of arguments reported.

I'm not used to negotiating java class hierarchies, so it's possible there's a cleaner way. Since Var.fn() returns an IFn, I added macrop handling methods IFn.setMacro and IFn.isMacro. These then needed to be implemented in Ref and Keyword, as well as AFn (where I wanted them) because they implement the IFn interface but don't inherit from AFn.

The real drawback I see with this approach is the duplicated state, though: ^{:macro true} vs AFn.macrop==true.

Comment by Andy Fingerhut [ 25/Oct/13 6:33 PM ]

I have not investigated the reason yet, but neither patch applies cleanly after the latest commits to Clojure master on Oct 25 2013. Given that what kinds of solution methods would be acceptable for this issue, it sounds like more thinking and code changes are probably needed anyway before worrying too much about that.





[CLJ-1274] Unable to set compiler options via system properties except for AOT compilation Created: 02/Oct/13  Updated: 27/Jun/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: compiler

Attachments: Text File CLJ-1274.patch    
Patch: Code
Approval: Screened

 Description   

The code that converts JVM system properties into keys under the *compiler-options* var is present only inside the clojure.lang.Compile class. This is a problem when using a debugger inside an IDE and not AOT compiling; specifying -Dclojure.compiler.disable-locals-clearing=true has no effect here when it would be most useful!

Patch: CLJ-1274.patch
Screened by: Stu



 Comments   
Comment by Howard Lewis Ship [ 02/Oct/13 4:52 PM ]

Obviously, that's supposed to be *compiler-options*.

Comment by Howard Lewis Ship [ 02/Dec/13 4:03 PM ]

Changes initialization of *compiler-options* to occur statically inside Compiler; now available to all forms of Clojure, not just AOT compilation; however, the initial *compiler-options* value is now defined as a root binding, rather than a per-thread binding, which has slightly different semantics.

Comment by Stuart Halloway [ 27/Jun/14 1:45 PM ]

Patch is straightforward, marking screened.

I am left wondering if other options that are set only in Compile.java ought also to be moved.





[CLJ-1272] Agent thread executors do not use the global uncaught exception handler Created: 01/Oct/13  Updated: 05/Feb/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: David Greenberg Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: agents


 Description   

If you use Thread.setDefaultUncaughtExceptionHandler to catch all application exceptions, and then throw an exception in a future, that exception will get swallowed up in deployment environments that don't watch stdout. It seems that the agent's executors ought to delegate to the global handler.

This issue bit us, in that we deploy and monitor our system only through its logs and metrics, and never actually saw that exceptions were being thrown in futures.






[CLJ-1263] Allow static compilation of function invocations Created: 14/Sep/13  Updated: 07/Nov/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler


 Description   

This proposal is to allow metadata on functions to prevent a fully dynamic var deref to be used whenever the function is called.

When the function is invoked, JVM "invokevirtual" instruction will be used, which is faster than the current implementation (var deref + IFn cast + invokinterface) and has less restrictions (no need to predefine interfaces to match the function parameters). The JVM is generally able to compile such invokevirtual instructions into extremely efficient code - effectively as fast as pure Java.

This is intended to pave the way to better support for statically compiled, high performance code. In particular, it allow:

  • Supporting arbitrary JVM primitives (float, int, byte, char etc.) as well as just double/long.
  • Supporting typed return values e.g. "String". This could eliminate many casts and type checks.
  • Supporting typed reference arguments (e.g. String).

Suggested usage:

(defn ^:static foo ^int [^String a ^String b]
(+ (count a) (Integer/parseInt b)))

Existing code / semantics should not be affected



 Comments   
Comment by Alex Fowler [ 18/Sep/13 5:08 AM ]

Very nice! That is what would really improve experience with certain tasks. I think it will also make possible to work with primitive arrays without the conversions?

Comment by Mike Anderson [ 19/Sep/13 5:44 PM ]

Hi Alex - which aspect of "work with primitive arrays" are you referring to? This feature would certainly help with passing primitive arguments to/from functions that use primitive arrays. It would also potentially help avoid some casts of primitive array arguments themselves. I don't think it helps in any other way - perhaps a separate issue would be appropriate if there is another thing you are trying to do?

Comment by Kevin Downey [ 29/Oct/13 11:50 AM ]

this issue is confusing, because there was/is a :static feature in clojure(which seems to be disabled in the compiler at the moment) and this proposal doesn't mention the existing work at all.

I also think this proposal is begging the question, there is no discussion of other possible solutions to the performance problem (whatever that is) that this is trying to solve.

the (var.deref()(IFn)).invoke(...) is pretty fundamental to the feel of clojure, in fact the existing :static keyword seems to be disabled in the compiler exactly because it complicates those semantics. so we should have a very clear proposal (not a wishlist) if we want to change that with some very clear wins.

maybe an optimizing clojure compiler would be a better approach.

Comment by Mike Anderson [ 30/Oct/13 11:01 PM ]

Hi Kevin,

This is partly in response to this discussion on Clojure-Dev, where we discussed there are quite a lot of performance issues around the way that Clojure passes arguments currently:
https://groups.google.com/d/topic/clojure/H5P25eYKBj4/discussion

Also I believe it reinstates the original intention of "^:static": I can't find where this is/was officially documented, but Arthur's answer in this SO question suggests that this was the case:
http://stackoverflow.com/questions/7552632/what-does-static-do-in-clojure

I think the proposal is relatively clear: it's probably the minimal change required to get static/direct (i.e. not via an indirect var reference / IFn) function invocations without affecting any of the semantics of current code.

This is sufficiently important for me that it's preventing me from shifting some performance-critical code to Clojure (even with primitive type hints). e.g. here's a simple case with a small primitive function:

(defn ^long foo [^long x]
(inc x))

(c/quick-bench (dotimes [i 100] (foo i))) ;; c = criterium
=> Execution time mean : 194.334293 ns

(c/quick-bench (dotimes [i 100] (inc i)))
=> Execution time mean : 71.539048 ns

i.e. the indirect function invocation is costing us nearly 170% overhead. In Java the equivalent functions perform identically: the overhead is zero because with static function invocation the JVM JIT is able to eliminate all the function call overhead.

In the long term, I agree that a proper optimising compiler would be the best way forward (perhaps Clojure 2.0/CinC can give us this?) but in the meantime I think this is a pragmatic way to improve performance with minimal impact on existing code. Even with an optimising compiler, I think we' would need some way to specifiy the "optimised" semantics rather than the indirect var deref behaviour, and "^:static" seems like a reasonable way to do so (unless anyone has a better idea?)

Comment by Kevin Downey [ 04/Nov/13 3:58 PM ]

have you looked at the definition of int and how it uses :inline/definline to avoid the call overhead?

Comment by Mike Anderson [ 05/Nov/13 4:27 AM ]

Good point Kevin - :inline and definline seem like a good approach in many cases (although it's marked as "experimental" - does that mean we can't rely on it to work in future releases?).

This proposal is still somewhat different: the inline solutions and its variants are effectively doing macro expansion to generate code without a function call on the Clojure side. The approach in this proposal would still emit a function call in bytecode, but do so in a way that the JVM can subsequently inline and optimise much more efficiently. Both have their uses, I think?

Commented edited Nov 7 2013 by Andy Fingerhut: Regarding definline marked as experimental, it has been so marked since Clojure 1.0's release, and the plan is to keep it marked that way in the pending Clojure 1.6 release. See discussion thread on CLJ-1281. No plans to remove it that I am aware of.

Comment by Kevin Downey [ 06/Nov/13 2:06 PM ]

my point is your benchmark above is not a comparison of clojure's current deref + cast + invoke vs. invokevirtual, inc is being inlined in to a static method call there

Comment by Kevin Downey [ 06/Nov/13 2:32 PM ]

I've been noodling around this, and it is entirely possible to generate and invoke code in clojure right now without paying the extra deref() cost:

 (defn ^long fib [^long n]
   (case n
     0 0
     1 1
     (+ (fib (dec n))
        (fib  (dec (dec n))))))

can be written as

(declare TheR1798)

(definterface I1797
  (^long fib_Invoke_1 [^long n]))

(deftype R1798 []
  I1797
  (^long fib_Invoke_1
    [this1799 ^long n]
    (case n
      0 0
      1 1
      (+ (.fib_Invoke_1 this1799 (dec n))
         (.fib_Invoke_1 this1799 (dec (dec n)))))))

(def TheR1798 (new R1798))

(defn ^long fib [^long n]
  (.fib_Invoke_1 TheR1798  n)))

now the recursive calls are invokeinterfaces, and the resulting function seems to have mean execution time about 5 times smaller using criterium to bench mark

it is entirely possible to write a macro that translates one in to other, and the weird names in the above are because I have a little proof of concept that does that.

the body of the bytecode for the regular fib function first shown looks something like:

  public final java.lang.Object invokePrim(long);
    Code:
       0: lload_1       
       1: lstore_3      
       2: lload_3       
       3: l2i           
       4: tableswitch   { // 0 to 1
                     0: 28
                     1: 40
               default: 52
          }
      28: lconst_0      
      29: lload_3       
      30: lcmp          
      31: ifne          52
      34: getstatic     #33                 // Field const__1:Ljava/lang/Object;
      37: goto          94
      40: lconst_1      
      41: lload_3       
      42: lcmp          
      43: ifne          52
      46: getstatic     #37                 // Field const__3:Ljava/lang/Object;
      49: goto          94
      52: getstatic     #57                 // Field const__5:Lclojure/lang/Var;
      55: invokevirtual #70                 // Method clojure/lang/Var.getRawRoot:()Ljava/lang/Object;
      58: checkcast     #6                  // class clojure/lang/IFn$LO
      61: lload_1       
      62: invokestatic  #75                 // Method clojure/lang/Numbers.dec:(J)J
      65: invokeinterface #77,  3           // InterfaceMethod clojure/lang/IFn$LO.invokePrim:(J)Ljava/lang/Object;
      70: getstatic     #57                 // Field const__5:Lclojure/lang/Var;
      73: invokevirtual #70                 // Method clojure/lang/Var.getRawRoot:()Ljava/lang/Object;
      76: checkcast     #6                  // class clojure/lang/IFn$LO
      79: lload_1       
      80: invokestatic  #75                 // Method clojure/lang/Numbers.dec:(J)J
      83: invokestatic  #75                 // Method clojure/lang/Numbers.dec:(J)J
      86: invokeinterface #77,  3           // InterfaceMethod clojure/lang/IFn$LO.invokePrim:(J)Ljava/lang/Object;
      91: invokestatic  #81                 // Method clojure/lang/Numbers.add:(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Number;
      94: areturn       
    LineNumberTable:
      line 243: 0
      line 244: 2
      line 247: 52
      line 247: 52
      line 247: 61
      line 248: 70
      line 248: 79
      line 248: 79
    LocalVariableTable:
      Start  Length  Slot  Name   Signature
             2      92     3 G__301   J
             0      94     0  this   Ljava/lang/Object;
             0      94     1     n   J

  public java.lang.Object invoke(java.lang.Object);
    Code:
       0: aload_0       
       1: aload_1       
       2: checkcast     #89                 // class java/lang/Number
       5: invokestatic  #93                 // Method clojure/lang/RT.longCast:(Ljava/lang/Object;)J
       8: invokeinterface #77,  3           // InterfaceMethod clojure/lang/IFn$LO.invokePrim:(J)Ljava/lang/Object;
      13: areturn       

the body of the "optimized" version looks like:

  public long fib_Invoke_1(long);
    Code:
       0: lload_1       
       1: lstore_3      
       2: lload_3       
       3: l2i           
       4: tableswitch   { // 0 to 1
                     0: 28
                     1: 38
               default: 48
          }
      28: lconst_0      
      29: lload_3       
      30: lcmp          
      31: ifne          48
      34: lconst_0      
      35: goto          80
      38: lconst_1      
      39: lload_3       
      40: lcmp          
      41: ifne          48
      44: lconst_1      
      45: goto          80
      48: aload_0       
      49: checkcast     #6                  // class com/thelastcitadel/kernel/I2364
      52: lload_1       
      53: invokestatic  #53                 // Method clojure/lang/Numbers.dec:(J)J
      56: invokeinterface #55,  3           // InterfaceMethod com/thelastcitadel/kernel/I2364.fib_Invoke_1:(J)J
      61: aload_0       
      62: checkcast     #6                  // class com/thelastcitadel/kernel/I2364
      65: lload_1       
      66: invokestatic  #53                 // Method clojure/lang/Numbers.dec:(J)J
      69: invokestatic  #53                 // Method clojure/lang/Numbers.dec:(J)J
      72: invokeinterface #55,  3           // InterfaceMethod com/thelastcitadel/kernel/I2364.fib_Invoke_1:(J)J
      77: invokestatic  #59                 // Method clojure/lang/Numbers.add:(JJ)J
      80: lreturn       
    LineNumberTable:
      line 251: 0
      line 251: 2
      line 251: 48
      line 251: 48
      line 251: 52
      line 251: 61
      line 251: 65
      line 251: 65
    LocalVariableTable:
      Start  Length  Slot  Name   Signature
             2      78     3 G__2363   J
             0      80     0  this   Lcom/thelastcitadel/kernel/R2365;
             0      80     1     n   J

so the calls are not invokevirtual (due to the way clojure compiles stuff, you cannot type anything inside a record as being that record's type), but the interface is unique and only has one instance, so I think the jvm's class hierarchy analysis makes short work of that.

if I have time I may try and complete my macro and release it as a library, but given tools.analyzer.jvm someone should be able to do better than my little proof of concept very quickly.

Comment by Andy Fingerhut [ 07/Nov/13 12:48 PM ]

I don't know if my editing of Mike Anderson's Nov 5 2013 comment is notified to people watching this ticket, so adding a new comment so those interested in definline's experimental status can know to go back and re-read it.





[CLJ-1256] Support type-hinted overrides of function parameters Created: 06/Sep/13  Updated: 09/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler, interop, typehints


 Description   

Problem: in many cases, the Clojure compiler has enough information about the type of a function argument to statically emit maximally efficient code on the JVM (i.e. without instance? checks, type casts or other forms of dynamic polymorphic dispatch). We are currently unable to do so in Clojure, which pushes developers with strong performance requirements to use some unidiomatic or convoluted workarounds.

Proposal is simply to allow functions to take type-hinted overloads of function arguments, e.g.

(defn foo
([^double x] (Math/floor x))
([^float x] (Math/floor (double x)))
([^String s] (count s)))

An "Object" version of the code with the correct arity will always be emitted, which will maintain compatibility with the IFn interface and ensure that the function can still be used in dynamic / interactive contexts. If the "Object" version is not explicitly provided, then it will be generated to use instance? checks that subsequently delegate to the appropriate typed version of the function (or throw an InvalidArgumentException if no match is found).

Matching rules would be the same as Java.

This will be backwards compatible with all existing uses of defn. In particular, it should extend / enhance / supercede the existing handling of primitive functions.

In the future, this technique might be used alongside core.typed to ensure that the most efficient function version is chosen based on type inference.






[CLJ-1255] Support Abstract Base Classes with Java-only variant of "reify" Created: 06/Sep/13  Updated: 01/Jul/14

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

Type: Enhancement Priority: Major
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: interop

Approval: Triaged

 Description   

Problem:

  • Various Java APIs depend on extension of abstract base classes rather than interfaces
  • "proxy" has limitations (no access to protected fields or super)
  • "proxy" has performance overhead because of an extra layer of functions / parameter boxing etc.
  • "gen-class" is complex and is complected with compilation / bytecode generation

In summary: Clojure does not currently have a good / convenient way to extend a Java abstract base class dynamically.

The proposal is to create a variant of "reify" that allows the extension of a single abstract base class (optionally also with interfaces/protocols). Code generation would occur as if the abstract base class had been directly extended in Java (i.e. with full access to protected members and with fully type-hinted fields).

Since this is a JVM-only construct, it should not affect the portable extension methods in Clojure (deftype etc.). We propose that it is placed in an separate namespace that could become the home for other JVM-specific interop functionality, e.g. "clojure.java.interop"



 Comments   
Comment by Alex Miller [ 30/Jun/14 8:18 AM ]

From Rich: we do not want to support abstract classes in a portable construct (reify, deftype). However, this would be considered as a new Java-only construct (extend-class or reify-class). If you could modify the ticket appropriately, will move back to Triaged.





[CLJ-1241] NPE when AOTing overrided clojure.core functions Created: 30/Jul/13  Updated: 13/May/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Phil Hagelberg Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: aot, compiler

Attachments: Text File 0001-fix-CLJ-1241.patch    
Patch: Code
Approval: Screened

 Description   

When performing AOT compilation on a namespace that overrides a clojure.core function without excluding the original clojure.core function from the ns, you get a NullPointerException.

To reproduce aot compile a namespace like "(ns x) (defn get [])"

For example:

$ lein new aot-get
$ cd aot-get
$ sed -i s/foo/get/
$ lein compile :all
WARNING: get already refers to: #'clojure.core/get in namespace: aot-get.core, being replaced by: #'aot-get.core/get
Exception in thread "main" java.lang.NullPointerException
	at clojure.lang.Compiler$ObjExpr.emitVar(Compiler.java:4858)
	at clojure.lang.Compiler$DefExpr.emit(Compiler.java:428)
	at clojure.lang.Compiler.compile1(Compiler.java:7152)
	at clojure.lang.Compiler.compile(Compiler.java:7219)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)

Cause: DefExpr.parse does not call registerVar for vars overridding clojure.core ones, thus when AOT compiling the var is not registered in the constant table.

Proposed: The attached patch makes DefExpr.parse call registerVar for vars overridding clojure.core ones.

Patch: 0001-fix-CLJ-1241.patch

Screened by: Alex Miller



 Comments   
Comment by Nicola Mometto [ 30/Jul/13 7:29 PM ]

DefExpr.parse was not calling registerVar for vars overridding clojure.core ones.

Comment by Alex Miller [ 31/Jul/13 12:25 AM ]

Verified on Clojure 1.5.1.

Comment by Javier Neira Sanchez [ 27/Aug/13 8:34 AM ]

Reproduced with `key` function without `(:refer-clojure :exclude [key])`

Comment by Rich Hickey [ 05/Sep/13 8:32 AM ]

This doesn't meet triage guidelines - i.e. there is this problem, therefore we will fix it by _____ so it then does _____

Comment by Aaron Cohen [ 26/Mar/14 12:52 PM ]

This is still present in the 1.6 release. I think it's mis-classified as low priority.

Comment by Aaron Cohen [ 26/Mar/14 12:52 PM ]

See for instance the cascalog mailing list: https://groups.google.com/forum/#!topic/cascalog-user/Pe5QIpmU0vA

Comment by Andy Fingerhut [ 26/Mar/14 1:07 PM ]

It may help if someone could clarify Rich's comment.

Does it mean that the ticket should include a plan of the form "therefore we will fix it by _____ so it then does _____", but this ticket doesn't have that?

Or perhaps it means that the ticket should not include a plan of that form, but this ticket does? If so, I don't see it, except perhaps the very last sentence of the description. If that is a problem for vetting a ticket, perhaps we could just delete that sentence and proceed from there?

Something else?

Comment by Nicola Mometto [ 26/Mar/14 1:13 PM ]

Andy, I added the two last lines in the description after reading Rich's comment to explain why this bug happens and how the patch I attached works around this.

I don't know if this is what he was asking for though.

Comment by Alex Miller [ 27/Mar/14 11:00 AM ]

I think Rich meant that a ticket should have a plan of that form but does not. My own take on "triaged" is that it should state actual and expected results demonstrating a problem - I don't think it needs to actually describe the solution (as that can happen later in development). It is entirely possible that Rich and I differ in our interpretation of that. I will see if I can rework the description a bit to match what I've been doing elsewhere.

Comment by Andy Fingerhut [ 31/Mar/14 9:34 AM ]

Alex, I have looked through the existing wiki pages on the ticket tracking process, and do not recall seeing anything about this desired aspect of a triaged ticket. Is it already documented somewhere and I missed it? Not that it has to be documented, necessarily, but Rich saying "triage guidelines" makes it sound like a filter he applies that ticket creators and screeners maybe should know about.

Comment by Alex Miller [ 31/Mar/14 11:57 AM ]

To me, Triage (and Vetting) is all about having good problem statements. For a defect, it is most important to demonstrate the problem (what happens now) and what you expect to happen instead. I do not usually expect there to necessarily be "by ____" in the ticket - to me that is part of working through the solution (although it is typical to have this in an enhancement). This ticket, as it stands now, seems to have both a good problem statement and a good cause/solution statement so seems to exceed Triaging standards afaik.

Two places where I have tried to write about these things in the past are http://dev.clojure.org/display/community/Creating+Tickets and in the Triage process on the workflow page http://dev.clojure.org/display/community/JIRA+workflow.





[CLJ-1237] reduce gives a SO for pathological seqs Created: 27/Jul/13  Updated: 25/Aug/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None
Environment:

1.5.1


Attachments: Text File CLJ-1237c.patch    
Patch: Code and Test

 Description   

reduce gives a StackOverflowError on long sequences that contain many transitions between chunked and unchunked:

(->> (repeat 50000 (cons :x [:y]))
     (apply concat)
     (reduce (constantly nil)))
;; throws StackOverflowError

Such a sequence is well behaved under most other sequence operations, and its underlying structure can even be masked such that reduce succeeds:

(->> (repeat 50000 (cons :x [:y]))
     (apply concat)
     (take 10000000)
     (reduce (constantly nil)))
;; => nil

I don't think Clojure developers normally worry about mixing chunked and unchunked seqs, so the existence of such a sequence is not at all unreasonable (and indeed this happened to me at work and was very difficult to debug).

It seems obvious what causes this given the implementation of reduce – it bounces back and forth between the chunked impl and the unchunked impl, consuming more and more stack as it goes. Without proper tail call optimization, it's not obvious to me what a good fix would be.

Presumed bad solutions

Degrade to naive impl after first chunk

In the IChunkedSeq implementation, instead of calling internal-reduce when the
sequence stops being chunked, it could have an (inlined?) unoptimized implementation,
ensuring that no further stack space is taken up. This retains the behavior that a
generic seq with a chunked tail will still run in an optimized fashion, but a seq with
two chunked portions would only be optimized the first time.

Use clojure.core/trampoline

This would presumably work, but requires wrapping the normal return values from all
implementations of internal-reduce.

Proposed Solution

(attached as CLJ-1237c.patch)

Similar to using trampoline, but create a special type (Unreduced) that signals
an implementation change. The two implementation-change points in internal-reduce
(in the IChunkedSeq impl and the Object impl) are converted to return an instance
of Unreduced instead of a direct call to internal-reduce.

Then seq-reduce is converted to check for instances of Unreduced before returning,
and recurs if it finds one.

Pros

  • Only requires one additional check in most cases
  • Reduces stack usage for existing heterogeneous reductions that weren't extreme enough to crash
  • Should be compatible with 3rd-party implementations of internal-reduce, which can still use the old style (direct recursive calls to internal-reduce) or the optimized style if desired.

Cons

  • internal-reduce is slightly more complicated
  • There's an extra check at the end of seq-reduce – is that a performance worry?


 Comments   
Comment by Gary Fredericks [ 25/Aug/13 4:13 PM ]

Added patch.





[CLJ-1232] Functions with non-qualified return type hints force import of hinted classes when called from other namespace Created: 18/Jul/13  Updated: 22/Jul/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: typehints

Approval: Triaged

 Description   

You can add a type hint to function arglists to indicate the return type of a function like so.

user> (import '(java.util List))
java.util.List
user> (defn linkedlist ^List [] (java.util.LinkedList.))
#'user/linkedlist
user> (.size (linkedlist))
0

The problem is that now when I call `linkedlist` exactly as above from another namespace, I'll get an exception because java.util.List is not imported in there.

user> (in-ns 'user2)
#<Namespace user2>
user2> (refer 'user)
nil
user2> (.size (linkedlist))
CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: List, compiling:(NO_SOURCE_PATH:1:1)
user2> (import '(java.util List)) ;; Too bad, need to import List here, too.
java.util.List
user2> (.size (linkedlist))
0

There are two workarounds: You can import the hinted type also in the calling namespace, or you always use fully qualified class names for return type hints. Clearly, the latter is preferable.

But clearly, that's a bug that should be fixed. It's not in analogy to type hints on function parameters which may be simple-named without having any consequences for callers from other namespaces.



 Comments   
Comment by Andy Fingerhut [ 16/Apr/14 3:47 PM ]

To make sure I understand, Nicola, in this ticket you are asking that the Clojure compiler change behavior so that the sample code works correctly with no exceptions, the same way as it would work correctly without exceptions if one of the workarounds were used?

Comment by Tassilo Horn [ 17/Apr/14 12:18 AM ]

Hi Andy. Tassilo here, not Nicola. But yes, the example should work as-is. When I'm allowed to use type hints with simple imported class names for arguments, then doing so for return values should work, too.

Comment by Rich Hickey [ 10/Jun/14 10:41 AM ]

Type hints on function params are only consumed by the function definition, i.e. in the same module as the import/alias. Type hints on returns are just metadata, they don't get 'compiled' and if the metadata is not useful to consumers in other namespaces, it's not a useful hint. So, if it's not a type in the auto-imported set (java.lang), it should be fully qualified.

Comment by Alex Miller [ 10/Jun/14 11:55 AM ]

Based on Rich's comment, this ticket should probably morph into an enhancement request on documentation, probably on http://clojure.org/java_interop#Java Interop-Type Hints.

Comment by Andy Fingerhut [ 10/Jun/14 3:13 PM ]

I would suggest something like the following for a documentation change, after this part of the text on the page Alex links in the previous comment:

For function return values, the type hint can be placed before the arguments vector:

(defn hinted
(^String [])
(^Integer [a])
(^java.util.List [a & args]))

-> #user/hinted

If the return value type hint is for a class that is outside of java.lang, which is the part auto-imported by Clojure, then it must be a fully qualified class name, e.g. java.util.List, not List.

Comment by Nicola Mometto [ 10/Jun/14 4:02 PM ]

I don't understand why we should enforce this complexity to the user.
Why can't we just make the Compiler (or even defn itself) update all the arglists tags with properly resolved ones? (that's what I'm doing in tools.analyzer.jvm)

Comment by Alexander Kiel [ 19/Jul/14 10:02 AM ]

I'm with Nicola here. I also think that defn should resolve the type hint according the imports of the namespace defn is used in.

Comment by Max Penet [ 22/Jul/14 7:06 AM ]

Same here, I was bit by this in the past. The current behavior is clearly counterintuitive.





[CLJ-1224] Records do not cache hash like normal maps Created: 24/Jun/13  Updated: 27/Jun/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: defrecord, performance

Attachments: Text File 0001-cache-hasheq-and-hashCode-for-records.patch     Text File 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records.patch    
Patch: Code
Approval: Vetted

 Description   

Records do not cache their hash codes like normal Clojure maps, which affects their performance. This problem has been fixed in CLJS, but still affects JVM CLJ.

Approach: Cache hash values in record definitions, similar to maps.

Patch: 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records.patch

Also see: http://dev.clojure.org/jira/browse/CLJS-281



 Comments   
Comment by Nicola Mometto [ 14/Feb/14 5:46 PM ]

I want to point out that my patch breaks ABI compatibility.
A possible approach to avoid this would be to have 3 constructors instead of 2, I can write the patch to support this if desired.

Comment by Stuart Halloway [ 27/Jun/14 11:09 AM ]

The patch 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records.patch is broken in at least two ways:

  • The fields __hash and __hasheq are adopted by new records created by .assoc and .without, which will cause those records to have incorrect (and likely colliding) hash values
  • The addition of the new fields breaks the promise of defrecord, which includes an N+2 constructor taking meta and extmap. With the patch, defrecords get an N+4 constructor letting callers pick hash codes.

I found these problems via the following reasoning:

  • Code has been touched near __extmap
  • Grep for all uses of __extmap and see what breaks
Comment by Nicola Mometto [ 27/Jun/14 2:53 PM ]

Patch 0001-cache-hasheq-and-hashCode-for-records.patch fixes both those issues, reintroducing the N+2 arity constructor

Comment by Alex Miller [ 27/Jun/14 4:08 PM ]

Questions addressed, back to Vetted.





[CLJ-1221] Should repackage jsr166 and include known version with Clojure Created: 20/Jun/13  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: reducers


 Description   

Clojure 1.5 reducers work with either the JDK version of forkjoin (JDK 1.7+) or with an external jsr166 jar. This causes complexity for users and complexity in the build to deal with the two options.

jsr166 code is public domain and it is common for other projects to repackage the handful of files and ship it with the project (similar to what we do with asm). This would allow us just use a known existing version of jsr166 across all jdks and we could get rid of the custom build wrangling we introduced in Clojure 1.5.

jsr166y is compatible with JDK 1.6+ and is the version that (for example) Scala currently repackages. That's the best choice for JDK 1.6 and 1.7. In JDK 1.8, the best choice will (temporarily) be the built-in version in java.util.concurrent which tracks jsr166e but then as soon as there are updates will become jsr166e. Many fork/join fixes are ported to both y and e right now.

Some choices here for JDK 1.8:

  • go for maximal compatibility just use repackaged jsr166y regardless of JDK (simplest)
  • check for jdk version # and use java.util.concurrent instead
  • check for jdk version # and repackage jsr166e and use it instead

Not sure yet which of these is best choice right now.






[CLJ-1212] Silent truncation/downcasting of primitive type on reflection call to overloaded method (Math/abs) Created: 28/May/13  Updated: 05/Feb/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Matthew Willson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: primitives, typehints
Environment:

Clojure 1.5.1
OpenJDK Runtime Environment (IcedTea6 1.12.5) (6b27-1.12.5-0ubuntu0.12.04.1)



 Description   

I realise relying on reflection when calling these kinds of methods isn't a great idea performance-wise, but it shouldn't lead to incorrect or dangerous behaviour.

Here it seems to trigger a silent downcast of the input longs, giving a truncated integer output:

user> (defn f [a b] (Math/abs (- a b)))
Reflection warning, NO_SOURCE_PATH:1:15 - call to abs can't be resolved.
#'user/f
user> (f 1000000000000 2000000000000)
727379968
user> (class (f 1000000000000 2000000000000))
java.lang.Integer
user> (defn f [^long a ^long b] (Math/abs (- a b)))
#'user/f
user> (f 1000000000000 2000000000000)
1000000000000
user> (class (f 1000000000000 2000000000000))
java.lang.Long



 Comments   
Comment by Matthew Willson [ 28/May/13 12:50 PM ]

For an even simpler way to replicate the issue:

user> (#(Math/abs %) 1000000000000)
Reflection warning, NO_SOURCE_PATH:1:3 - call to abs can't be resolved.
727379968

Comment by Andy Fingerhut [ 28/May/13 1:36 PM ]

I was able to reproduce the behavior you see with these Java 6 JVMs on Ubuntu 12.04.2:

java version "1.6.0_27"
OpenJDK Runtime Environment (IcedTea6 1.12.5) (6b27-1.12.5-0ubuntu0.12.04.1)
OpenJDK 64-Bit Server VM (build 20.0-b12, mixed mode)

java version "1.6.0_45"
Java(TM) SE Runtime Environment (build 1.6.0_45-b06)
Java HotSpot(TM) 64-Bit Server VM (build 20.45-b01, mixed mode)

However, I tried two Java 7 JVMs, and it gave the following behavior which looks closer to what you would hope for. I do not know what is the precise difference between Java 6 and Java 7 that leads to this behavior difference, but this is some evidence that this has something to do with Java 6 vs. Java 7.

user=> (set! warn-on-reflection true)
true
user=> (defn f [a b] (Math/abs (- a b)))
Reflection warning, NO_SOURCE_PATH:1:15 - call to abs can't be resolved.
#'user/f
user=> (f 1000000000000 2000000000000)
1000000000000
user=> (class (f 1000000000000 2000000000000))
java.lang.Long

Above behavior observed with Clojure 1.5.1 on these JVMs:

Ubuntu 12.04.2 plus this JVM:
java version "1.7.0_21"
Java(TM) SE Runtime Environment (build 1.7.0_21-b11)
Java HotSpot(TM) 64-Bit Server VM (build 23.21-b01, mixed mode)

Mac OS X 10.8.3 plus this JVM:
java version "1.7.0_15"
Java(TM) SE Runtime Environment (build 1.7.0_15-b03)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)

Comment by Matthew Willson [ 29/May/13 5:17 AM ]

Ah, interesting.
Maybe it's a difference in the way the reflection API works in java 7?

Here's the bytecode generated incase anyone's curious:

public java.lang.Object invoke(java.lang.Object);
Code:
0: ldc #14; //String java.lang.Math
2: invokestatic #20; //Method java/lang/Class.forName:(Ljava/lang/String;)Ljava/lang/Class;
5: ldc #22; //String abs
7: iconst_1
8: anewarray #24; //class java/lang/Object
11: dup
12: iconst_0
13: aload_1
14: aconst_null
15: astore_1
16: aastore
17: invokestatic #30; //Method clojure/lang/Reflector.invokeStaticMethod:(Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/Object;
20: areturn

Comment by Matthew Willson [ 29/May/13 5:20 AM ]

Just an idea (and maybe this is what's happening under java 7?) but given it's a static method and all available overloaded variants are presumably known at compile time, perhaps it could generate code along the lines of:

(cond
(instance? Long x) (Math/abs (long x))
(instance? Integer x) (Math/abs (int x))
;; ...
)

Comment by Andy Fingerhut [ 29/May/13 3:19 PM ]

In Reflector.java method invokeStaticMethod(Class c, String methodName, Object[] args) there is a call to getMethods() followed by a call to invokeMatchingMethod(). getMethods() returns the 4 java.lang.Math/abs methods in different orders on Java 6 and 7, causing invokeMatchingMethod() to pick a different one on the two JVMs:

java version "1.6.0_39"
Java(TM) SE Runtime Environment (build 1.6.0_39-b04)
Java HotSpot(TM) 64-Bit Server VM (build 20.14-b01, mixed mode)

user=> (pprint (seq (clojure.lang.Reflector/getMethods java.lang.Math 1 "abs" true)))
(#<Method public static int java.lang.Math.abs(int)>
#<Method public static long java.lang.Math.abs(long)>
#<Method public static float java.lang.Math.abs(float)>
#<Method public static double java.lang.Math.abs(double)>)
nil

java version "1.7.0_21"
Java(TM) SE Runtime Environment (build 1.7.0_21-b11)
Java HotSpot(TM) 64-Bit Server VM (build 23.21-b01, mixed mode)

user=> (pprint (seq (clojure.lang.Reflector/getMethods java.lang.Math 1 "abs" true)))
(#<Method public static double java.lang.Math.abs(double)>
#<Method public static float java.lang.Math.abs(float)>
#<Method public static long java.lang.Math.abs(long)>
#<Method public static int java.lang.Math.abs(int)>)
nil

That might be a sign of undesirable behavior in invokeMatchingMethod() that is too dependent upon the order of methods given to it.

As you mention, type hinting is good for avoiding the significant performance hit of reflection.





[CLJ-1208] Namespace is not loaded on defrecord class init Created: 03/May/13  Updated: 10/Jun/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Tim McCormack Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: defrecord

Approval: Vetted

 Description   

As a user of Clojure interop from Java, I want defrecords (and deftypes?) to load their namespaces upon class initialization so that I can simply construct and use AOT'd record classes without manually requiring their namespaces first.

Calling the defrecord's constructor may or may not result in "Attempting to call unbound fn" exceptions, depending on what code has already been run.

This issue has been raised several times over the years, but I could not find an existing ticket for it:






[CLJ-1192] vec function is substantially slower than into function Created: 06/Apr/13  Updated: 25/Jul/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance

Approval: Vetted

 Description   

(vec coll) and (into [] coll) do exactly the same thing. However, due to into using transients, it is substantially faster. On my machine:

(time (dotimes [_ 100] (vec (range 100000))))
"Elapsed time: 732.56 msecs"

(time (dotimes [_ 100] (into [] (range 100000))))
"Elapsed time: 491.411 msecs"

This is consistently repeatable.

Since vec's sole purpose is to transform collections into vectors, it should do so at the maximum speed available.



 Comments   
Comment by Andy Fingerhut [ 07/Apr/13 5:50 PM ]

I am pretty sure that Clojure 1.5.1 also uses transient vectors for (vec (range n)) (probably also some earlier versions of Clojure, too).

Look at vec in core.clj. It checks whether its arg is a java.util.Collection, which lazy seqs are, so calls (clojure.lang.LazilyPersistentVector/create coll).

LazilyPersistentVector's create method checks whether its argument is an ISeq, which lazy seqs are, so it calls PersistentVector.create(RT.seq(coll)).

All 3 of PersistentVector's create() methods use transient vectors to build up the result.

I suspect the difference in run times are not because of transients or not, but because of the way into uses reduce, and perhaps may also have something to do with the perhaps-unnecessary call to RT.seq in LazilyPersistentVector's create method (in this case, at least – it is likely needed for other types of arguments).

Comment by Alan Malloy [ 14/Jun/13 2:17 PM ]

I'm pretty sure the difference is that into uses reduce: since reducers were added in 1.5, chunked sequences know how to reduce themselves without creating unnecessary cons cells. PersistentVector/create doesn't use reduce, so it has to allocate a cons cell for each item in the sequence.

Comment by Gary Fredericks [ 08/Sep/13 1:55 PM ]

Is there any downside to (defn vec [coll] (into [] coll)) (or the inlined equivalent)?

Comment by Ghadi Shayban [ 11/Apr/14 5:13 PM ]

While I agree that there are improvements and possibly low-hanging fruit, FWIW https://github.com/clojure/tools.analyzer/commit/cf7dda81a22f4c9c1fe64c699ca17e7deed61db4#commitcomment-5989545

showed a 5% slowdown from a few callsites in tools.analyzer.

This ticket's benchmark is incomplete in that it covers a single type of argument (chunked range), and flawed as it timing the expense of realizing the range. (That could be a legit benchmark case, but it shouldn't be the only one).

Sorry to rain on a parade. I promise like speed too!

Comment by Greg Chapman [ 25/Apr/14 5:23 PM ]

One thing to note is that vec has a subtle difference from into when the collection is an Object array of length <= 32. In that case, vec aliases the supplied array, rather than copying it (this is noted in the warning here: http://clojuredocs.org/clojure_core/clojure.core/vec). I believe I read some place that this behavior is intentional, but I can't find the citation.

Comment by Andy Fingerhut [ 25/Apr/14 10:18 PM ]

Greg, CLJ-893 might be what you remember. That is the ticket that was closed by a patch updating the documentation of vec.

Comment by Mike Anderson [ 18/May/14 7:41 AM ]

I think there are quite a few performance improvements that can be made to vec in general. For example, if given a List it should use PersistentVector.create(List) rather than producing an unnecessary seq, which appears to be the case at the moment. Also it should probably return the same object if passed an existing IPersistentVector.

Basically there are a number of cases that we could be handling more efficiently....

I'm taking a look at this now.... will propose a quick patch if it seems there is a good solution.

Comment by Mike Anderson [ 24/Jul/14 4:01 AM ]

I've looked at this issue and it is quite complex. There are multiple types that need to potentially be converted into vectors, and doing so efficiently will often require making use of reduce-style operations on the source collections.

Doing this efficiently will probably in turn require making use of the IReduce interface, which doesn't yet seem to be fully utilised across the Clojure code based. If we do this, lots of operations (not just vec!) can be made faster but it will be quite a major change.

I have a branch that implements some of this but would appreciate feedback if this is the right direction before I take it any further:
https://github.com/mikera/clojure/tree/clj-1192-vec-performance

Comment by Alex Miller [ 24/Jul/14 9:45 AM ]

Thanks Mike! It may take a few days before I can get back to you about this.

Comment by Mike Anderson [ 25/Jul/14 3:44 AM ]

Basically the approach I am proposing is:

  • Make various collections implement IReduce efficiently (if they don't already). Especially applied to chunked seqs etc.
  • Have RT.reduce(...) methods that implement reduce on the Java side
  • Make the Clojure side use IReduce where relevant (should be as simple as extending the existing protocols)
  • Implement vec (and other similar operations) in terms of IReduce - which will solve this specific issue

If we really care about pushing vector performance even further, we can also consider:

  • Create specialised small vector types where appropriate - e.g. a specialised SmallPersistentVector class for <32 elements. This should outperform the more generic PersistentVector which is better suited for large vectors.
  • Some dedicated construction functions that know how to efficiently exploit knowledge about the data source (e.g. creating a vec from a segment of a big Object array can be done with a bunch of arraycopys into 32-element chunks and then constructing a PersistentVector around these)

This should give us a decent speedup overall (of course it would need benchmarking... but I'd hope to see some sort of measurable improvement on a macro benchmark like building and testing Clojure).





[CLJ-1191] Improve apropos to show some indication of namespace of symbols found Created: 04/Apr/13  Updated: 10/Jun/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: repl

Attachments: Text File clj-1191-patch-v1.txt    
Patch: Code
Approval: Vetted

 Description   

apropos does find all symbols in all namespaces that match the argument, but the return value gives no clue as to which namespace the found symbols are in. It can even return multiple occurrences of the same symbol, which only gives a clue that the symbol exists in more than one namespace, but not which ones. For example:

user=> (apropos "replace")
(postwalk-replace prewalk-replace replace re-quote-replacement replace replace-first)

It would be nice if the returned symbols could indicate the namespace, either always, or if the found symbol is not in the current namespace.



 Comments   
Comment by Andy Fingerhut [ 04/Apr/13 8:25 PM ]

Path clj-1191-patch-v1.txt enhances apropos to put a namespace/ qualifier before every symbol found that is not in the current namespace ns. It also finds the shortest namespace alias if there is more than one. Examples of output with patch:

user=> (apropos "replace")
(replace clojure.string/re-quote-replacement clojure.string/replace clojure.string/replace-first clojure.walk/postwalk-replace clojure.walk/prewalk-replace)

user=> (require '[clojure.string :as str])
nil
user=> (apropos "replace")
(replace clojure.walk/postwalk-replace clojure.walk/prewalk-replace str/re-quote-replacement str/replace str/replace-first)

user=> (in-ns 'clojure.string)
#<Namespace clojure.string>
clojure.string=> (clojure.repl/apropos "replace")
(re-quote-replacement replace replace-by replace-first replace-first-by replace-first-char replace-first-str clojure.core/replace clojure.walk/postwalk-replace clojure.walk/prewalk-replace)

Comment by Colin Jones [ 05/Apr/13 1:34 PM ]

+1

apropos as it already stands is quite helpful for already-referred vars, but not for vars that are only in other nses.

This update includes the information someone would need to further investigate the output.





[CLJ-1180] defprotocol doesn't resolve tag classnames Created: 10/Mar/13  Updated: 05/Feb/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: protocols

Attachments: Text File 001-CLJ-1180.patch    
Patch: Code and Test

 Description   

defprotocol doesn't resolve tag classnames, this results in exceptions being thrown when the declared protocol uses as a tag an imported class that is not imported in the namespace that uses it.

user=> (import 'clojure.lang.ISeq)
clojure.lang.ISeq
user=> (defprotocol p (^ISeq f [_]))
p
user=> (ns x)
nil
x=> (defn x [y] (let [z (user/f y)] (inc z)))
CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: ISeq, compiling:(NO_SOURCE_PATH:4:33)



 Comments   
Comment by Alex Miller [ 03/Sep/13 9:38 AM ]

Similer to CLJ-1232.





[CLJ-1169] Report line,column, and source in defmacro errors Created: 22/Feb/13  Updated: 28/Jun/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Andrei Kleschinski Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs
Environment:

Windows


Attachments: Text File 0001-CLJ-1169-proposed-patch.patch     Text File 0002-CLJ-1169-fix-unit-tests.patch     Text File CLJ-1169-code-and-test-1.patch     File defn_error_message.clj    
Patch: Code
Approval: Screened

 Description   

Summary This patch grew out of a desire to have defn report filename and line numbers for parameter declaration errors, but the approach chosen does something more broad, and likely very useful: Anytime defmacro is throwing a non-CompilerException, wrap it in a CompilerException that captures LINE, COLUMN, and SOURCE. Presumably this would improve reporting for many other macros as well. The patch also tweaks errors messages to add quotes, e.g. "problem" instead of problem, which seems useful.

Screened By Stu
Patch CLJ-1169-code-and-test-1.patch, which aggregates the work in other patches to a single patch that works on current master.

When mistyping parameter list in defn declaration, e.g.

(defn test
 (some-error))

error message shows name of parameter (without quotes), but not function name, filename or line number:

Exception in thread "main" java.lang.IllegalArgumentException: Parameter declaration some-error should be a vector
        at clojure.core$assert_valid_fdecl.invoke(core.clj:6567)
        at clojure.core$sigs.invoke(core.clj:220)
        at clojure.core$defn.doInvoke(core.clj:294)
        at clojure.lang.RestFn.invoke(RestFn.java:467)
        at clojure.lang.Var.invoke(Var.java:427)
        at clojure.lang.AFn.applyToHelper(AFn.java:172)
        at clojure.lang.Var.applyTo(Var.java:532)
        at clojure.lang.Compiler.macroexpand1(Compiler.java:6366)
        at clojure.lang.Compiler.macroexpand(Compiler.java:6427)
        at clojure.lang.Compiler.eval(Compiler.java:6495)
        at clojure.lang.Compiler.load(Compiler.java:6952)
        at clojure.lang.Compiler.loadFile(Compiler.java:6912)
        at clojure.main$load_script.invoke(main.clj:283)
        at clojure.main$init_opt.invoke(main.clj:288)
        at clojure.main$initialize.invoke(main.clj:316)
        at clojure.main$null_opt.invoke(main.clj:349)
        at clojure.main$main.doInvoke(main.clj:427)
        at clojure.lang.RestFn.invoke(RestFn.java:421)
        at clojure.lang.Var.invoke(Var.java:419)
        at clojure.lang.AFn.applyToHelper(AFn.java:163)
        at clojure.lang.Var.applyTo(Var.java:532)
        at clojure.main.main(main.java:37)


 Comments   
Comment by Andrei Kleschinski [ 22/Feb/13 5:39 AM ]

Proposed patch for issue
Process exceptions in macroexpand1 and wraps them in CompilerException with source,line,column information.

Also patch adds quotes around invalid symbol name in error message to make them more distinguishable from rest of message.

Comment by Andy Fingerhut [ 01/Mar/13 9:32 AM ]

Patch 0001-CLJ-1169-proposed-patch.patch dated Feb 22 2013 causes several tests to fail. Run "./antsetup.sh" then "ant" to see which ones (or "mvn package").

Comment by Andrei Kleschinski [ 01/Mar/13 10:25 AM ]

Fix for failed unit-tests

Comment by Stuart Halloway [ 27/Jun/14 2:40 PM ]

Andrei, can you please sign the CA (e-form at http://clojure.org/contributing) so that we can consider this patch?

Thanks!

Comment by Andrei Kleschinski [ 27/Jun/14 3:05 PM ]

Ok, I have signed the CA.

Comment by Alex Miller [ 27/Jun/14 4:06 PM ]

I can confirm that Andrei has signed the CA. Back in Vetted.





[CLJ-1149] Unhelpful error message from :use (and use function) when arguments are malformed Created: 17/Jan/13  Updated: 28/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Sean Corfield Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs

Approval: Triaged

 Description   

the following exception happens when you have something like this(bad):

(ns runtime.util-test
(:use [midje.sweet :reload-all]))

as opposed to any of these(correct):

(ns runtime.util-test
(:use midje.sweet :reload-all))

(ns runtime.util-test
(:use [midje.sweet] :reload-all))

and the exception is:
Exception in thread "main" java.lang.IllegalArgumentException: No value supplied for key: true
at clojure.lang.PersistentHashMap.create(PersistentHashMap.java:77)
at clojure.core$hash_map.doInvoke(core.clj:365)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:617)
at clojure.core$load_lib.doInvoke(core.clj:5352)
at clojure.lang.RestFn.applyTo(RestFn.java:142)
at clojure.core$apply.invoke(core.clj:619)
at clojure.core$load_libs.doInvoke(core.clj:5403)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:621)
at clojure.core$use.doInvoke(core.clj:5497)

Note that this is similar to the equally unhelpful message shown in http://dev.clojure.org/jira/browse/CLJ-1140 although that is a different root cause.

Probably best to enhance the `use` function to validate its arguments before trying to apply hash-map?



 Comments   
Comment by Gary Fredericks [ 26/May/13 3:17 PM ]

I believe this applies to require as well.





[CLJ-1136] Type hinting for array classes does not work in binding forms Created: 20/Dec/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop, typehints
Environment:

replicated on OpenJDK 7u9 on Ubuntu 12.04, and Hotspot 1.6.0_37 on OSX Lion



 Description   

Type hints don't work as expected in binding forms.

The following form results in a reflection warning:

(let [^{:tag (Class/forName "[Ljava.lang.Object;")} a (make-array Object 2)]
(aget a 0))

However, hinting does appear to work correctly on vars:

(def ^{:tag (Class/forName "[Ljava.lang.Object;")} a (make-array Object 2))
(aget a 0) ;; no reflection warning



 Comments   
Comment by Ghadi Shayban [ 20/Dec/12 10:51 PM ]

It's a little more insidious than type hinting: the compiler doesn't evaluate metadata in the binding vec.

This doesn't throw the necessary exception...

(let [^{:foo (Class/forName "not real")} bar 42]
bar)

neither this...

(let [^{gyorgy ligeti} a 42]
a)

Gyorgy Ligeti never resolves.

These two equivalent examples don't reflect:
(let [^objects a (make-array Object 2)]
(aget a 0))

(let [a ^objects (make-array Object 2)]
(aget a 0))

Comment by Ghadi Shayban [ 21/Dec/12 11:09 AM ]

On only the left-hand side of a local binding, metadata on a symbol is not analyzed or evaluated.





[CLJ-1130] when unable to match a method, report arity caller was looking for Created: 17/Dec/12  Updated: 10/Apr/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Howard Lewis Ship Assignee: Stuart Halloway
Resolution: Unresolved Votes: 0
Labels: errormsgs

Attachments: Text File clj-1130-v1.txt     File clj-1130-v2.diff     File clj-1130-v2-ignore-ws.diff     Text File clj-1130-v2.txt     File clj-1130-v3.diff     File clj-1130-v4.diff     File clj-1130-v5.diff    
Patch: Code and Test
Approval: Vetted

 Description   

Original motivation: Incorrectly invoking a static method with 0 parameters yields a NoSuchFieldException:

user=> (Long/parseLong)
CompilerException java.lang.NoSuchFieldException: parseLong, compiling:(NO_SOURCE_PATH:1:1) 
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: No matching method: parseLong, compiling:(NO_SOURCE_PATH:2:1)

Incorrectly invoking an instance method with 0 parameters yields a message about fields as well:

user=> (.setTime (java.util.Date.))
IllegalArgumentException No matching field found: setTime for class java.util.Date  clojure.lang.Reflector.getInstanceField (Reflector.java:271)

Patch: clj-1130-v5.diff

Approach: Primum non nocere. Error reporting enhanced at the site the errors happen, compiler logic unchanged.



 Comments   
Comment by Michael Drogalis [ 06/Jan/13 6:44 PM ]

It looks like it's first trying to resolve a field by name, since field access with / is legal. For example:

user=> (Integer/parseInt)
CompilerException java.lang.NoSuchFieldException: parseInt, compiling:(NO_SOURCE_PATH:1)

user=> (Integer/MAX_VALUE)
2147483647

Would trying to resolve a method before a field fix this?

Comment by Alex Miller [ 03/Sep/13 10:10 AM ]

Similarities to CLJ-1248 (there a warning, here an error).

Comment by Andy Fingerhut [ 09/Sep/13 12:36 AM ]

Patch clj-1130-v1.txt changes the error message in a situation when one attempts to invoke a static method with no args, and there is no such 0-arg static method. The message now says that there is no such method with that name and 0 args, rather than that there is no such static field with that name.

Comment by Alex Miller [ 04/Oct/13 3:56 PM ]

I updated the patch to simplify it a bit but more importantly to remove the check by exception and instead use the Reflector method that can answer this question.

Comment by Andy Fingerhut [ 12/Oct/13 3:11 PM ]

Alex, thank you for the improvements to the code. It looks better to me.

Comment by Rich Hickey [ 25/Oct/13 7:30 AM ]

due to indentation changes, this patch appears to touch much more than it probably does, making it difficult to approve.

Comment by Andy Fingerhut [ 25/Oct/13 10:59 AM ]

Any suggestions on what can be done to make progress here? Would it help to attach a patch made with "-w" option to ignore lines that differ only in whitespace? Provide git diff command line options that do this, after the patch is applied to your local workspace? Make a patch that leaves the indentation 'incorrect' after the change (involuntary shudder)?

Comment by Alex Miller [ 25/Oct/13 11:17 AM ]

The indentation has intentionally changed because the if/else structure has changed. I don't think making the patch incorrect to reduce changes is a good idea.

Comment by Andy Fingerhut [ 25/Oct/13 11:32 AM ]

Well, the 'incorrect' was in quotes because I was asking about a proposed patch that had the correct logic, but misleading indentation. Agreed it isn't a good idea, hence the shudder. I'm just brainstorming ideas to make the patch less difficult to approve.

Comment by Howard Lewis Ship [ 25/Oct/13 11:43 AM ]

At some point, you may need to bite the bullet and reformat some of the Clojure code .... Compiler.java had a crazy mix of tabs, spaces, and just completely wrong stuff.

Comment by Alex Miller [ 03/Nov/13 10:47 PM ]

Re-marking screened. Not sure what else to do.

Comment by Andy Fingerhut [ 04/Nov/13 8:35 AM ]

clj-1130-v2-ignore-ws.diff is identical to clj-1130-v2.diff, except it was produced with a command that ignores differences in a line due only to whitespace, i.e.: 'git format-patch master --stdout -w > clj-1130-v2-ignore-ws.diff'

It is not intended as the patch to be applied. It is only intended to make it easier to see that many of the lines in clj-1130-v2.diff are truly only differences in indentation.

Comment by Alex Miller [ 04/Nov/13 8:55 AM ]

Thanks Andy...

Comment by Rich Hickey [ 22/Nov/13 7:59 AM ]

This patch ignores the fact that method is checked for first above:

if(c != null)
  maybeField = Reflector.getMethods(c, 0, munge(sym.name), true).size() == 0;

Which is why the field code is unconditional. I'm fine with making errors better, but changing logic as well deserves more scrutiny.

Comment by Alex Miller [ 06/Dec/13 9:01 PM ]

This patch is intentionally trying to avoid calling StaticFieldExpr in the field code as that is where the (Long/parseLong) case (erroneously calling an n-arity static method with 0 args) will throw a field-oriented exception instead of a method-oriented exception. By adding the extra check here, this case falls through into the method case and throws later on calling StaticMethodExpr instead.

The early check is a check for methods of the specified arity. The later check is for the existence of a field of matching name. Combined, they lead to a better error message.

However, another alternative is to set maybeField in the first check based on field existence, not on invocation arity. That just improves the maybeField informaiton and the existing code then naturally throws the correct exception (and the patch is much simpler).

The similar case for calling n-arity instance methods with 0-arity has the same problem for the same reason:

user=> (.setTime (java.util.Date.))
IllegalArgumentException No matching field found: setTime for class java.util.Date  clojure.lang.Reflector.getInstanceField (Reflector.java:271)

Thus we can also adjust the other call that sets maybeField (which now is much less maybe).

I will attach a patch that covers these cases and update the ticket for someone to screen.

Comment by Stuart Sierra [ 08/Dec/13 12:24 PM ]

Screened. The patch clj-1130-v3.diff works as advertised.

This patch only improves error messages for cases when the type of the
target object is known to the compiler. In reflective calls, the error
messages are still the same.

Example, after this patch, given these definitions:

(def v 42)
(defn untagged-f [] 42)
(defn ^Long tagged-f [] 42)

The following expressions produce new error messages:

(.foo v 1)
;; IllegalArgumentException No matching method found: foo taking 1 args
;; for class java.lang.Long clojure.lang.Reflector.invokeMatchingMethod
;; (Reflector.java:53)

(.foo (tagged-f))
;; IllegalArgumentException No matching method found: foo taking 0 args
;; for class java.lang.Long clojure.lang.Reflector.invokeMatchingMethod
;; (Reflector.java:53)

These expressions still use the old error messages:

(.foo v)
;; IllegalArgumentException No matching field found: foo for class
;; java.lang.Long clojure.lang.Reflector.getInstanceField
;; (Reflector.java:271)

(.foo (untagged-f))
;; IllegalArgumentException No matching field found: foo for class
;; java.lang.Long clojure.lang.Reflector.getInstanceField
;; (Reflector.java:271)
Comment by Rich Hickey [ 03/Jan/14 8:41 AM ]

Changing the logic to get a different error message is something that needs to be done with great care. This now seems to prefer fields over methods, changing the semantics.

Comment by Stuart Halloway [ 31/Jan/14 3:12 PM ]

v4 patch simply enhances error messaages

Comment by Andy Fingerhut [ 31/Jan/14 3:18 PM ]

clj-1130-v4.diff has the same patch repeated twice in the file. clj-1130-v5.diff is identical, except deleting the redundant copy.





[CLJ-1108] Allow to specify an Executor instance to be used with future-call Created: 18/Nov/12  Updated: 27/Dec/12

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Max Penet Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File bac37b91230d8e4ab3a1e6042a6e8c4b7e9cbf53.patch     Text File clj-1108-enhance-future-call-patch-v2.txt    
Patch: Code

 Description   

This adds an arity to future-call that expects a java.util.concurrent/ExecutorService instance to be used instead of clojure.lang.Agent/soloExecutor.



 Comments   
Comment by Andy Fingerhut [ 26/Dec/12 4:50 PM ]

Rich Hickey committed a change on Dec 21, 2012 to the future-call function that made the patch bac37b91230d8e4ab3a1e6042a6e8c4b7e9cbf53.patch dated Nov 18 2012 no longer apply cleanly.

clj-1108-enhance-future-call-patch-v2.txt dated Dec 26 2012 is identical to that earlier patch, except it has been updated to apply cleanly to the latest master.

It would be best if Max Penet, author of the earlier patch, could verify I've merged his patch to the latest Clojure master correctly.

Comment by Max Penet [ 27/Dec/12 2:25 AM ]

It's verified, it applies correctly to the latest master 00978c76edfe4796bd6ebff3a82182e235211ed0 .

Thanks Andy.





[CLJ-1107] 'get' should throw exception on non-Associative argument Created: 13/Nov/12  Updated: 17/Jun/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 9
Labels: None

Attachments: Text File 0001-CLJ-1107-Throw-exception-for-get-called-on-unsupport.patch     Text File 0003-CLJ-1107-Throw-exception-for-get-on-unsupported-type.patch     Text File clj-1107-throw-on-unsupported-get-v4.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The implementation of clojure.core/get returns nil if its argument is not an associative collection.

This behavior can obscure common programmer errors such as:

(def a (atom {:a 1 :b 2})

(:foo a)   ; forgot to deref a
;;=> nil

Calling get on something which is neither nil nor an Associative collection is almost certainly a bug, and should be indicated by an exception.

CLJ-932 was accepted as a similar enhancement to clojure.core/contains?

Patch: 0003-CLJ-1107-Throw-exception-for-get-on-unsupported-type.patch

Approach: Throw IllegalArgumentException as final fall-through case in RT.getFrom instead of returning nil.



 Comments   
Comment by Andy Fingerhut [ 24/May/13 12:31 PM ]

Patch clj-1107-throw-on-get-for-unsupported-types-patch-v2.txt dated May 24 2013 is identical to 0001-CLJ-1107-Throw-exception-for-get-called-on-unsupport.patch dated Nov 13 2012, except it applies cleanly to latest master. A recent commit for CLJ-1099 changed many IllegalArgumentException occurrences to Throwable in the tests, which is the only thing changed in this updated patch.

Comment by Andy Fingerhut [ 30/Jan/14 5:01 PM ]

Patch clj-1107-throw-on-get-for-unsupported-types-patch-v2.txt applied cleanly to latest Clojure master as of Jan 23 2014, but no longer does with commits made to Clojure between then and Jan 30 2014. I have not checked to see how difficult or easy it may be to update this patch.

Comment by Stuart Sierra [ 11/Feb/14 7:23 AM ]

New patch 0003-CLJ-1107-Throw-exception-for-get-on-unsupported-type.patch created from master at 5cc167a.

Comment by Andy Fingerhut [ 26/Mar/14 11:55 AM ]

Patch clj-1107-throw-on-unsupported-get-v4.patch dated Mar 26 2014 is identical to Stuart Sierra's patch 0003-CLJ-1107-Throw-exception-for-get-on-unsupported-type.patch, and retains his authorship. The only difference is in one line of diff context required in order to make it apply cleanly to latest master.

Comment by Rich Hickey [ 10/Jun/14 10:54 AM ]

This would be a breaking change

Comment by Stuart Sierra [ 17/Jun/14 6:59 PM ]

Arguably so was CLJ-932 (contains?), which did "break" some things that were already broken.

This is a more invasive change than CLJ-932, but I believe it is more likely to expose hidden bugs than to break intentional behavior.





[CLJ-1096] Make destructuring emit direct keyword lookups Created: 29/Oct/12  Updated: 06/Jun/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5, Release 1.6
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Christophe Grand Assignee: Christophe Grand
Resolution: Unresolved Votes: 2
Labels: performance

Attachments: File desctructure-keyword-lookup.diff     File inline-get-keyword.diff    
Patch: Code
Approval: Triaged

 Description   

Currently associative destructuring emits calls to get. The attached patch modify desctruture to emit direct keyword lookups when possible.

Approved here https://groups.google.com/d/msg/clojure-dev/MaYcHQck8VA/nauMus4mzPgJ



 Comments   
Comment by Christophe Grand [ 04/Sep/13 3:40 AM ]

Rethinking about this patch now, it may be too specific: get's inline expansion should be modified when the key is a literal keyword.

Comment by Christophe Grand [ 04/Sep/13 3:41 AM ]

More generic patch (inline-get-keyword.diff): all get calls with literal keywords as keys are inlined to direct keyword lookup.

Comment by John Hume [ 19/May/14 1:14 PM ]

Is this only stalled out of lack of interest?

Comment by Andy Fingerhut [ 19/May/14 6:13 PM ]

There are currently about 50 tickets "triaged", i.e. marked for Rich to look at and decide whether they are things he is interested in seeing a patch for, and another 25 or so that were triaged and he has "vetted" them, and they are in various stages of having patches written for them, screened, etc. That doesn't mean anything for this ticket in particular – just wanted to make it clear that there are a bunch of other tickets that are getting some attention, and a bunch of others that are not.

What gets triaged depends somewhat upon how severe the issue appears. You can vote on the ticket, and try to persuade others to do so as well, if they think this would enhance the performance of some commonly-written types of Clojure code. You could also consider doing some benchmarking with & without these patches to see how much performance they can gain.





[CLJ-1079] Don't squash explicit :line and :column metadata in the MetaReader Created: 29/Sep/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: reader

Attachments: File CLJ-1079.diff    
Patch: Code and Test

 Description   

I have been experimenting with using cljx to produce Clojure and ClojureScript source from a single file. This has gone well so far, with the exception that, due to the way the source transformation works, all of the linebreaks and other formatting is gone from the output. There is an option to include the original :line metadata in the output though, like so:

;;This file autogenerated from 
;;
;;  src/cljx/com/foo/hosty.cljx
;;
^{:line 1} (ns com.foo.hosty)
^{:line 3} (defn ^{:clj true} system-hash [x] ^{:line 5} (System/identityHashCode x))

(Hopefully, such hackery won't be necessary in the future with sjacket or something like it...)

Unfortunately, when read in using a LineNumberingPushbackReader, code like this has its :line metadata squashed by the line numbers coming from that. A REPL-friendly example would be:

=> (meta (read (clojure.lang.LineNumberingPushbackReader.
                 (java.io.StringReader. "^{:line 66} ()"))))
{:line 1}
=> (meta (read (java.io.PushbackReader.
                 (java.io.StringReader. "^{:line 66} ()"))))
{:line 66}

The latter seems more correct to me (and is equivalent to read-string).



 Comments   
Comment by Chas Emerick [ 29/Sep/12 7:07 PM ]

{{CLJ-1097.diff}} contains a fix for this issue, as well as a separate commit that eliminates a series of casts in order to improve readability in the area.

Comment by Andy Fingerhut [ 05/Oct/12 8:23 AM ]

Chas, your patch doesn't apply cleanly to latest Clojure master as of Oct 5 2012. I'm not sure, but I think some recent commits to Clojure on Oct 4 2012 caused that. I also take it as evidence of your awesomeness that you can write patches for tickets that haven't been filed yet

Comment by Chas Emerick [ 05/Oct/12 9:24 AM ]

"patches for tickets that haven't been filed yet?"

Anyway, tweaking up this patch is a small price to pay for having column meta. New {{CLJ-1097.diff}} patch attached, applies clean on master as of now. Otherwise same contents as in the original patch, except:

  • the same dynamic is also applied to :column metadata, now that it's available
  • the changes have been rebased into a single commit (including the elimination of the casts in MetaReader, which were becoming so numerous that the code was less readable than most
Comment by Nicola Mometto [ 05/Oct/12 9:39 AM ]

"patches for tickets that haven't been filed yet?"

He was referring to the fact that you uploaded "CLJ-1097.diff" while the ticket is #1079

Comment by Chas Emerick [ 05/Oct/12 9:42 AM ]

Oh, hah! Twice now, even! One more data point recommending my having slight dyslexia or somesuch. :-P

I've replaced the attached patch with one that is named properly to avoid any later confusion.

Comment by Chas Emerick [ 07/Oct/12 3:57 PM ]

Refreshed patch to apply cleanly to master after the recent off by one patch for :column metadata.

Comment by Stuart Halloway [ 19/Oct/12 3:12 PM ]

This feels backwards to me. If a special purpose tool wants to convey information via metadata, why does it use names that collide with those used by LispReader?

Comment by Chas Emerick [ 19/Oct/12 7:36 PM ]

The information being conveyed is the same :line and :column metadata conveyed by LispReader — in fact, that's where it comes from in the first place.

Kibit (and cljx) is essentially an out-of-band source transformation tool. Given an input like this:

(ns com.foo.hosty)

(defn ^:clj system-hash
  [x]
 (System/identityHashCode x))

(defn ^:cljs system-hash
  [x]
  (goog/getUid x))

…it produces two files, a .clj for Clojure, and a .cljs for ClojureScript. (The first code listing in the ticket description is the former.)

However, because there's no way to transform Clojure code/data without losing formatting, anything that depends on line/column numbers (stack traces, stepping debuggers) is significantly degraded. If LispReader were to defer to :line and :column metadata already available on the loaded forms (there when the two generated files are spit out with *print-meta* on), this would not be the case.

Comment by Andy Fingerhut [ 07/Feb/13 8:47 AM ]

clj-1079-patch-v2.txt dated Feb 7 2013 is identical to Chas's CLJ-1079.diff dated Oct 7 2012, except it applies cleanly to latest master. I believe the only difference is that some white space in the context lines is updated.

Comment by Andy Fingerhut [ 07/Feb/13 12:35 PM ]

Sorry for the noise. I've removed clj-1079-patch-v2.txt mentioned in the previous comment, because I learned that CLJ-1079.diff dated Oct 7 2012 applies cleanly to latest master and passes all tests if you use this command to apply it.

% git am --keep-cr -s --ignore-whitespace < CLJ-1079.diff

I will update the JIRA workflow page instructions for applying patches to mention this, too, because there are multiple patches that fail without --ignore-whitespace, but apply cleanly with that option. That will eliminate the need to update patches merely for whitespace changes.





[CLJ-1077] thread-bound? returns true (implying set! should succeed) even for non-binding thread Created: 26/Sep/12  Updated: 20/Aug/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Paul Stadig Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: File thread-bound.diff    
Patch: Code

 Description   

thread-bound? returns true for a non-binding thread, this result (according to the docstring) implies that set! should succeed. However, thread-bound? does not check that any binding that might exist was created by the current thread, and calling set! fails with an exception when it is called from a non-binding thread, even though thread-bound? returns true.

thread-bound? should return false if there is a binding, and that binding was not established by the current thread.

Here is an example REPL session where a thread establishes a binding, those bindings are conveyed to a second thread, the second thread checks thread-bound? to see if it can set the binding, thread-bound? returns true indicating that the binding can be set, the second thread tries to set the binding, and the second thread gets an IllegalStateException:

    Clojure 1.5.1
    user=> (def ^:dynamic *set-me* nil)
    #'user/*set-me*
    user=> (defn try-to-set [] (binding [*set-me* 1] (doall (pcalls #(if (thread-bound? #'*set-me*) (set! *set-me* (inc *set-me*)))))))
    #'user/try-to-set
    user=> (try-to-set)
    IllegalStateException Can't set!: *set-me* from non-binding thread  clojure.lang.Var.set (Var.java:230)
    user=> 


 Comments   
Comment by Paul Stadig [ 01/Oct/12 10:07 AM ]

I have attached a patch that changes clojure.lang.Var and clojure.core/thread-bound? to only return true if a Var is set!-able.

Comment by Alex Miller [ 19/Aug/13 12:16 PM ]

REPL example?

Comment by Joe Gallo [ 20/Aug/13 7:55 AM ]

Sure thing, Alex – here's a repl example I just ran this morning.

; nREPL 0.1.7
user> (def ^:dynamic *set-me* nil)
#'user/*set-me*
user> (defn try-to-set [] (binding [*set-me* 1] (doall (pcalls #(if (thread-bound? #'*set-me*) (set! *set-me* (inc *set-me*)))))))
#'user/try-to-set
user> (try-to-set)
IllegalStateException Can't set!: *set-me* from non-binding thread  clojure.lang.Var.set (Var.java:230)
user>




[CLJ-1059] PersistentQueue doesn't implement java.util.List, causing nontransitive equality Created: 03/Sep/12  Updated: 31/Mar/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Philip Potter Assignee: Philip Potter
Resolution: Unresolved Votes: 1
Labels: queue

Attachments: File 001-clj-1059-make-persistentqueue-implement-list.diff     File 002-clj-1059-asequential-rebased-to-cached-hasheq.diff    
Patch: Code and Test

 Description   

PersistentQueue implements Sequential but doesn't implement java.util.List. Lists form an equality partition, as do Sequentials. This means that you can end up with nontransitive equality:

(def q (conj clojure.lang.PersistentQueue/EMPTY 1 2 3))
;=> #user/q
(def al (doto (java.util.ArrayList.) (.add 1) (.add 2) (.add 3)))
;=> #user/al
(def v [1 2 3])
;=> #user/v
(= al v)
;=> true
(= v q)
;=> true
(not= al q)
;=> true

This happens because PersistentQueue is a Sequential but not a List, ArrayList is a List but not a Sequential, and PersistentVector is both.



 Comments   
Comment by Philip Potter [ 15/Sep/12 3:41 AM ]

Whoops, according to http://dev.clojure.org/display/design/JIRA+workflow I should have emailed clojure-dev before filing this ticket. Here is the discussion:

https://groups.google.com/d/topic/clojure-dev/ME3-Ke-RbNk/discussion

Comment by Philip Potter [ 15/Sep/12 2:37 PM ]

Attached 001-make-PersistentQueue-implement-List.diff, 15/Sep/12

Note that this patch has a minor conflict with the one I added to CLJ-1070, because both add an extra interface to PersistentQueue - List in this case, IHashEq in CLJ-1070.

Comment by Chouser [ 18/Sep/12 1:04 AM ]

Philip, patch looks pretty good – thanks for doing this. A couple notes:

This is only my opinion, but I prefer imports be listed without wildcards, even if it means an extra couple lines at the top of a .java file.

I noticed the "List stuff" code is a copy of what's in ASeq and EmptyList. I suppose this is copied because EmptyList and PersistentQueue extend Obj and therefore can't extend ASeq. Is this the only reason? It seems a shame to duplicate these method definitions, but I don't know of a better solution, do you?

It would also be nice if the test check a couple of the List methods you've implemented.

Comment by Chouser [ 18/Sep/12 1:08 AM ]

oh, also "git am" refused to apply the patch, but I'm not sure why. "patch -p 1" worked perfectly.

Comment by Philip Potter [ 18/Sep/12 1:19 AM ]

did you use the --keep-cr option to git am?

I struggled to know whether I should be adding CRs or not to line endings, because the files I was editing weren't consistent in their usage. If you open them in emacs, half the lines have ^M at the end.

Comment by Philip Potter [ 18/Sep/12 1:21 AM ]

Will submit another patch, with the import changed. I'll have a think about the list implementation and see what ideas I can come up with.

Comment by Philip Potter [ 18/Sep/12 3:17 PM ]

Attached 002-make-PersistentQueue-implement-Asequential.diff

This patch is an alternative to 001-make-PersistentQueue-implement-List.diff

So I took on board what you said about ASeq, but it didn't feel right making PersistentQueue directly implement ISeq, somehow.

So I split ASeq into two parts – ASequential, which implements j.u.{Collection,List} and manages List-equality and hashcodes; and ASeq, which... doesn't seem to be doing much anymore, to be honest.

As a bonus, this patch fixes CLJ-1070 too, so I went and added the tests from that ticket in to demonstrate this fact. It also tidies up PersistentQueue by removing all equals/hashcCode stuff and all Collection stuff.

(It turns out that because ASeq was already implementing Obj, the fact that PersistentQueue was implementing Obj was no barrier to using it.)

Would appreciate comments on this approach, and how it differs from the previous patch here and the patch on CLJ-1070.

Comment by Philip Potter [ 18/Sep/12 3:44 PM ]

Looking at EmptyList's implementation of List, it is a duplicate of the others, but it shouldn't be. I think its implementation of indexOf is the biggest culprit - it should just be 'return -1;' but it has a great big for loop! But this is beyond the scope of this ticket, so I won't patch that here.

Comment by Andy Fingerhut [ 20/Oct/12 12:29 PM ]

Philip, now that the patch for CLJ-1070 has been applied, these patches no longer apply cleanly. Would you be willing to update them? If so, please remove the obsolete patches.

Comment by Philip Potter [ 22/Oct/12 5:10 AM ]

Andy, thanks so much for your efforts to make people aware of these things. I will indeed submit new patches, hopefully later this week.

Comment by Philip Potter [ 03/Nov/12 12:23 PM ]

Replaced existing patches with new ones which apply cleanly to master.

There are two patches:

001-clj-1059-make-persistentqueue-implement-list.diff

This fixes equality by making PersistentQueue implement List directly. I also took the opportunity to remove the wildcard import and to add tests for the List methods, as compared with the previous version of the patch.

002-clj-1059-asequential.diff

This fixes equality by creating a new abstract class ASequential, and making PersistentQueue extend this.

My preferred solution is still the ASequential patch, but I'm leaving both here for comparison.

Comment by Timothy Baldridge [ 30/Nov/12 3:37 PM ]

Vetting.

Comment by Andy Fingerhut [ 11/Dec/12 12:50 PM ]

Philip, this time I think it was patches that were committed for CLJ-1000 that make your patch 002-clj-1059-asequential.diff not apply cleanly. I often fix up stale patches where the change is straightforward and mechanical, but in this case you are moving some methods that CLJ-1000's patch changed the implementation of, so it would be best if someone figured out a way to update this patch in a way that doesn't clobber the CLJ-1000 changes.

Comment by Philip Potter [ 11/Dec/12 1:57 PM ]

Thanks Andy. Submitted a new patch, 002-clj-1059-asequential-rebased-to-cached-hasheq.diff, which supersedes 002-clj-1059-asequential.diff.

The patch 001-clj-1059-make-persistentqueue-implement-list.diff still applies cleanly, and is still an alternative to 002-clj-1059-asequential-rebased-to-cached-hasheq.diff.

Comment by Andy Fingerhut [ 30/Jan/14 4:50 PM ]

With the commits to Clojure master made in the week leading up to Jan 30 2014, particularly changes to hasheq, patch 002-clj-1059-asequential-rebased-to-cached-hasheq.diff no longer applies cleanly.

Patch 001-clj-1059-make-persistentqueue-implement-list.diff still does.

Comment by Andy Fingerhut [ 31/Mar/14 5:33 PM ]

This issue was run into again and a duplicate ticket CLJ-1374 created – later closed as a duplicate of this one. Just wanted to record that this issue is being hit by others besides those originally reporting it.





[CLJ-1046] Drop-while as a reducer Created: 18/Aug/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reducers

Attachments: Text File drop-while-reducer.patch    
Patch: Code and Test

 Description   

Implement drop-while as a reducer. Follows the same atom-based strategy as drop and take.

Does not depend on any of my other reducer patches, but there will probably be some minor merge conflicts unless it is merged after CLJ-1045, and before CLJ-992 and CLJ-993.






[CLJ-1045] Generalize/refactor implementation of PersistentVector/coll-fold Created: 18/Aug/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reducers

Attachments: Text File clj-1045-fold-by-halves-patch-v2.txt     Text File fold-by-halves.patch    
Patch: Code

 Description   

Vector currently contains a specialized implementation of the folding algorithm "split the collection in half until the pieces are small enough". The attached commit lifts out the general strategy so that it can be reused by other collection types amenable to splitting.

CLJ-993 depends on this patch, as it uses the new fold-by-halves function.



 Comments   
Comment by Andy Fingerhut [ 25/Jan/13 2:29 PM ]

clj-1045-fold-by-halves-patch-v2.txt dated Jan 25 2013 is identical to fold-by-halves.patch dated Aug 18 2012, except it updates one line of context changed by a recent commit to Clojure master.





[CLJ-1037] Allow doc strings for both interfaces and concrete implementations Created: 04/Aug/12  Updated: 17/Apr/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Warren Lynn Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

In this post
http://groups.google.com/group/clojure/browse_thread/thread/84de74740928da76#

I mentioned the rationale (I think) why this is important and needed. Thank you for consideration.



 Comments   
Comment by Kevin Downey [ 17/Apr/14 10:08 PM ]

clojure's documentation system has two parts:

1. docstrings are attached to the metadata of objects

2. the doc macro (and some other tools) read the docstrings from objects and display them

the two parts work together, without the doc macro, docstrings are just comments that also take up memory at runtime, and the doc macro has no purpose without the docstrings.

the two main places docstrings are hung are var metadata and namespace metadata.

for multimethods and protocol functions the docstrings are hung on vars.

for the implementations of multimethods and protocols there are no distinct vars to hang documentation information on, and it is not clear how you would look up those doc strings.

so to support docs on defmethods and protocol implementations would require enhancements to doc and some design work, so a wiki page to come up with a design would be a good idea.





[CLJ-1022] gen-class destroys method annotations Created: 03/Jul/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Maris Orbidans Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop


 Description   

When extending a class gen-class doesn't preserve method annotations.

If class com.bar.Foo has annotated methods then in MyClass all annotations are gone.

(gen-class
:name com.my.MyClass
:extends com.bar.Foo
:implements [com.google.common.base.Supplier]
:prefix demo-
:post-init post-init)

(defn demo-post-init [this]
(info "initialized")
(swank.swank/start-server :port 68478))

(defn demo-get [_]
(get-msg))

Class<?> aClass = Class.forName("com.my.MyClass");
Method[] methods = aClass.getMethods();

for (Method m : methods) {
Annotation[] annotations = m.getAnnotations();
System.out.println(m.getName()+" "+annotations.length);
for (Annotation a : annotations) { System.out.println(a.annotationType().getClass().getName()); }
}






[CLJ-1016] Global scope overrides lexical scope for classes (Clojure assumes no classes in default package / Clojure cannot handle yFiles JARs in classpath) Created: 21/Jun/12  Updated: 24/May/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Edward Z. Yang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File collision-workaround.patch    

 Description   

The most visible symptom of this bug is having a class named 'w' (default package) in your classpath (such classes are produced by Java obfuscation tools such as yFiles) and then attempting to load Clojure's core class. For example:

java -cp hotspotapi.jar:clojure-1.4.0-slim.jar clojure.main

(where hotspotapi.jar is a stereotypical example of an obfuscated JAR) results in:

Exception in thread "main" java.lang.ExceptionInInitializerError
at clojure.main.<clinit>(main.java:20)
Caused by: java.lang.NoSuchFieldException: close, compiling:(clojure/core.clj:6139)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6462)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyze(Compiler.java:6223)
at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5618)
at clojure.lang.Compiler$TryExpr$Parser.parse(Compiler.java:2178)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6455)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyze(Compiler.java:6223)
at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5618)
at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5919)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6455)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6443)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6443)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyze(Compiler.java:6223)
at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5618)
at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5054)
at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3674)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6453)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6443)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.access$100(Compiler.java:37)
at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:518)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6455)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyze(Compiler.java:6223)
at clojure.lang.Compiler.eval(Compiler.java:6515)
at clojure.lang.Compiler.load(Compiler.java:6952)
at clojure.lang.RT.loadResourceScript(RT.java:359)
at clojure.lang.RT.loadResourceScript(RT.java:350)
at clojure.lang.RT.load(RT.java:429)
at clojure.lang.RT.load(RT.java:400)
at clojure.lang.RT.doInit(RT.java:436)
at clojure.lang.RT.<clinit>(RT.java:318)
... 1 more
Caused by: java.lang.NoSuchFieldException: close
at java.lang.Class.getField(Class.java:1537)
at clojure.lang.Compiler$StaticFieldExpr.<init>(Compiler.java:1180)
at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:923)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6455)
... 37 more
Could not find the main class: clojure.main. Program will exit.

To understand what is going on, consider this simple test:

import java.io.StringReader;

import clojure.lang.Compiler;
import clojure.lang.RT;

public class Test {
public static void main(String[] args) { RT.var("clojure.core", "require"); String s = "(let [mumble (new java.io.StringReader \"\")] (. mumble close))"; Compiler.load(new StringReader(s)); }
}

It should be clear that 'mumble' in the dot operator is referencing the locally defined mumble. However, if we define a class named 'mumble' in the default package, Clojure picks that one up instead.

To forestall any objections: yes, we know that placing classes in the default package is extremely poor form. Point of the matter is, the Java ecosystem is extremely diverse and there are a lot of JARs people may not have control over. While one might argue, "Don't put classes in the default namespace", point of the matter is, Clojure is wrong here, and these situations arise in practice, through no fault of the implementer.



 Comments   
Comment by Edward Z. Yang [ 21/Jun/12 11:01 AM ]

Here is a workaround patch which makes this error less likely to occur.

Comment by Andy Fingerhut [ 27/Aug/12 7:37 PM ]

Edward, it is Rich Hickey's policy only to consider for inclusion in Clojure patches written by people who have signed a Contributor Agreement: http://clojure.org/contributing

Were you interested in becoming a contributor?

Comment by Edward Z. Yang [ 27/Aug/12 9:24 PM ]

Sure, although the patch attached is emphatically not the one you want to actually applying, since it only band-aids the problem.

Comment by Andy Fingerhut [ 24/May/13 1:21 PM ]

I am not sure, but this ticket may be related to CLJ-1171. At least, there the issue was a global name not being shadowed by a local name bound with let. That seems similar to this issue.





[CLJ-1013] Clojure's classloader cannot handle out-of-order loading Created: 13/Jun/12  Updated: 18/Apr/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Edward Z. Yang Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   

Here is a minimal test-case:

import java.io.IOException;

import clojure.lang.PersistentTreeMap;
import clojure.lang.RT;

public class TestClass {

static Class y = RT.class;
//static PersistentTreeMap x = PersistentTreeMap.EMPTY;

/**

  • @param args
  • @throws ClassNotFoundException
  • @throws IOException
    */
    public static void main(String[] args) throws IOException, ClassNotFoundException { PersistentTreeMap x = PersistentTreeMap.EMPTY; }

}

This results in the exception:

Exception in thread "main" java.lang.ExceptionInInitializerError
at java.lang.Class.forName0(Native Method)
at java.lang.Class.forName(Class.java:247)
at clojure.lang.RT.loadClassForName(RT.java:2056)
at clojure.lang.RT.load(RT.java:419)
at clojure.lang.RT.load(RT.java:400)
at clojure.lang.RT.doInit(RT.java:436)
at clojure.lang.RT.<clinit>(RT.java:318)
at clojure.lang.PersistentTreeMap.<init>(PersistentTreeMap.java:45)
at clojure.lang.PersistentTreeMap.<clinit>(PersistentTreeMap.java:32)
at TestClass.main(TestClass.java:19)
Caused by: java.lang.NullPointerException
at clojure.lang.APersistentSet.contains(APersistentSet.java:33)
at clojure.lang.RT.contains(RT.java:700)
at clojure.core$contains_QMARK_.invoke(core.clj:1386)
at clojure.core$load_lib.doInvoke(core.clj:5255)
at clojure.lang.RestFn.applyTo(RestFn.java:142)
at clojure.core$apply.invoke(core.clj:603)
at clojure.core$load_libs.doInvoke(core.clj:5298)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:603)
at clojure.core$require.doInvoke(core.clj:5381)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.core__init.load(Unknown Source)
at clojure.core__init.<clinit>(Unknown Source)
... 10 more

The crux of the issue appears Clojure's classloader doesn't understand how to handle out-of-order classloading.



 Comments   
Comment by Kevin Downey [ 18/Apr/14 12:31 AM ]

exception still happens with clojure 1.6





[CLJ-1005] Use transient map in zipmap Created: 30/May/12  Updated: 10/Jun/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Michał Marczyk Assignee: Aaron Bedra
Resolution: Unresolved Votes: 5
Labels: performance

Attachments: Text File 0001-Use-transient-map-in-zipmap.2.patch     Text File 0001-Use-transient-map-in-zipmap.patch    
Patch: Code
Approval: Vetted

 Description   

The attached patch changes zipmap to use a transient map internally. The definition is also moved so that it resides below that of #'transient. The original definition is commented out (like that of #'into).



 Comments   
Comment by Aaron Bedra [ 14/Aug/12 9:24 PM ]

Why is the old implementation left and commented out? If we are going to move to a new implementation, the old one should be removed.

Comment by Michał Marczyk [ 15/Aug/12 4:17 AM ]

As mentioned in the ticket description, the previously attached patch follows the pattern of into whose non-transient-enabled definition is left in core.clj with a #_ in front – I wasn't sure if that's something desirable in all cases.

Here's a new patch with the old impl removed.

Comment by Andy Fingerhut [ 15/Aug/12 10:37 AM ]

Thanks for the updated patch, Michal. Sorry to raise such a minor issue, but would you mind using a different name for the updated patch? I know JIRA can handle multiple attached files with the same name, but my prescreening code isn't quite that talented yet, and it can lead to confusion when discussing patches.

Comment by Michał Marczyk [ 15/Aug/12 10:42 AM ]

Thanks for the heads-up, Andy! I've reattached the new patch under a new name.

Comment by Andy Fingerhut [ 16/Aug/12 8:24 PM ]

Presumptuously changing Approval from Incomplete back to None after the Michal's updated patch was added, addressing the reason the ticket was marked incomplete.

Comment by Aaron Bedra [ 11/Apr/13 5:32 PM ]

The patch looks good and applies cleanly. Are there additional tests that we should run to verify that this is providing the improvement we think it is. Also, is there a discussion somewhere that started this ticket? There isn't a lot of context here.

Comment by Michał Marczyk [ 11/Apr/13 6:19 PM ]

Hi Aaron,

Thanks for looking into this!

From what I've been able to observe, this change hugely improves zipmap times for large maps. For small maps, there is a small improvement. Here are two basic Criterium benchmarks (transient-zipmap defined at the REPL as in the patch):

;;; large map
user=> (def xs (range 16384))
#'user/xs
user=> (last xs)
16383
user=> (c/bench (zipmap xs xs))
Evaluation count : 13920 in 60 samples of 232 calls.
             Execution time mean : 4.329635 ms
    Execution time std-deviation : 77.791989 us
   Execution time lower quantile : 4.215050 ms ( 2.5%)
   Execution time upper quantile : 4.494120 ms (97.5%)
nil
user=> (c/bench (transient-zipmap xs xs))
Evaluation count : 21180 in 60 samples of 353 calls.
             Execution time mean : 2.818339 ms
    Execution time std-deviation : 110.751493 us
   Execution time lower quantile : 2.618971 ms ( 2.5%)
   Execution time upper quantile : 3.025812 ms (97.5%)

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 25.4675 % Variance is moderately inflated by outliers
nil

;;; small map
user=> (def ys (range 16))
#'user/ys
user=> (last ys)
15
user=> (c/bench (zipmap ys ys))
Evaluation count : 16639020 in 60 samples of 277317 calls.
             Execution time mean : 3.803683 us
    Execution time std-deviation : 88.431220 ns
   Execution time lower quantile : 3.638146 us ( 2.5%)
   Execution time upper quantile : 3.935160 us (97.5%)
nil
user=> (c/bench (transient-zipmap ys ys))
Evaluation count : 18536880 in 60 samples of 308948 calls.
             Execution time mean : 3.412992 us
    Execution time std-deviation : 81.338284 ns
   Execution time lower quantile : 3.303888 us ( 2.5%)
   Execution time upper quantile : 3.545549 us (97.5%)
nil

Clearly the semantics are preserved provided transients satisfy their contract.

I think I might not have started a ggroup thread for this, sorry.





[CLJ-1001] Proxy cannot call proper super-class method Created: 23/May/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2, Release 1.3
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Guanpeng Xu Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop
Environment:

Linux herberteuler 3.2.0-2-amd64 #1 SMP Sat May 12 23:08:28 UTC 2012 x86_64 GNU/Linux


Attachments: File proxy-bug.clj    

 Description   

Attached is a program that reproduces this issue. We have a proxy, `p', which sub-classes java.io.InputStream. There are three methods named `read' in java.io.InputStream: abstract int read(); int read(byte[] b); and int read(byte[] b, int off, int len); see http://docs.oracle.com/javase/6/docs/api/java/io/InputStream.html. In the definition of proxy `p', we implement the abstract variant of method `read', making `p' a concrete instance of java.io.InputStream.

The first invocation, (. p read), returns -1, which is expected.

The second invocation, (. p (read b 0 n)), should call int read(byte[] b, int off, int len); in java.io.InputStream. But these are actual behavior:

$ clojure1.2 ~/tmp/proxy-bug.clj
Exception in thread "main" java.lang.IllegalArgumentException: Wrong number of args (4) passed to: user$eval1$fn (proxy-bug.clj:0)
at clojure.lang.Compiler.eval(Compiler.java:5441)
at clojure.lang.Compiler.load(Compiler.java:5858)
at clojure.lang.Compiler.loadFile(Compiler.java:5821)
at clojure.main$load_script.invoke(main.clj:221)
at clojure.main$script_opt.invoke(main.clj:273)
at clojure.main$main.doInvoke(main.clj:354)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.lang.Var.invoke(Var.java:365)
at clojure.lang.AFn.applyToHelper(AFn.java:161)
at clojure.lang.Var.applyTo(Var.java:482)
at clojure.main.main(main.java:37)
Caused by: java.lang.IllegalArgumentException: Wrong number of args (4) passed to: user$eval1$fn
at clojure.lang.AFn.throwArity(AFn.java:437)
at clojure.lang.AFn.invoke(AFn.java:51)
at user.proxy$java.io.InputStream$0.read(Unknown Source)
at user$eval1.invoke(proxy-bug.clj:9)
at clojure.lang.Compiler.eval(Compiler.java:5425)
... 10 more

$ clojure1.2 ~/tmp/proxy-bug.clj
Exception in thread "main" java.lang.IllegalArgumentException: Wrong number of args (4) passed to: user$eval1$fn (proxy-bug.clj:0)
at clojure.lang.Compiler.eval(Compiler.java:5441)
at clojure.lang.Compiler.load(Compiler.java:5858)
at clojure.lang.Compiler.loadFile(Compiler.java:5821)
at clojure.main$load_script.invoke(main.clj:221)
at clojure.main$script_opt.invoke(main.clj:273)
at clojure.main$main.doInvoke(main.clj:354)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.lang.Var.invoke(Var.java:365)
at clojure.lang.AFn.applyToHelper(AFn.java:161)
at clojure.lang.Var.applyTo(Var.java:482)
at clojure.main.main(main.java:37)
Caused by: java.lang.IllegalArgumentException: Wrong number of args (4) passed to: user$eval1$fn
at clojure.lang.AFn.throwArity(AFn.java:437)
at clojure.lang.AFn.invoke(AFn.java:51)
at user.proxy$java.io.InputStream$0.read(Unknown Source)
at user$eval1.invoke(proxy-bug.clj:9)
at clojure.lang.Compiler.eval(Compiler.java:5425)
... 10 more



 Comments   
Comment by Guanpeng Xu [ 23/May/12 10:24 PM ]

The second behavior should be in Clojure 1.3:

$ clojure1.3 ~/tmp/proxy-bug.clj
Exception in thread "main" clojure.lang.ArityException: Wrong number of args (4) passed to: user$eval1$fn
at clojure.lang.AFn.throwArity(AFn.java:437)
at clojure.lang.AFn.invoke(AFn.java:51)
at user.proxy$java.io.InputStream$0.read(Unknown Source)
at user$eval1.invoke(proxy-bug.clj:9)
at clojure.lang.Compiler.eval(Compiler.java:6468)
at clojure.lang.Compiler.load(Compiler.java:6905)
at clojure.lang.Compiler.loadFile(Compiler.java:6866)
at clojure.main$load_script.invoke(main.clj:282)
at clojure.main$script_opt.invoke(main.clj:342)
at clojure.main$main.doInvoke(main.clj:426)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.lang.Var.invoke(Var.java:401)
at clojure.lang.AFn.applyToHelper(AFn.java:161)
at clojure.lang.Var.applyTo(Var.java:518)
at clojure.main.main(main.java:37)

Sorry for the inconvenience.

Comment by Russell Mull [ 01/Sep/13 3:12 AM ]

Verified with Clojure 1.5.1:

Stack Trace
clojure.lang.ArityException: Wrong number of args (4) passed to: user$eval147$fn
                                      AFn.java:437 clojure.lang.AFn.throwArity
                                       AFn.java:51 clojure.lang.AFn.invoke
                                  (Unknown Source) user.proxy/java.io.InputStream[fn]
                                  NO_SOURCE_FILE:9 user/eval147
                                Compiler.java:6619 clojure.lang.Compiler.eval
                                Compiler.java:6582 clojure.lang.Compiler.eval
                                     core.clj:2852 clojure.core/eval
                                      main.clj:259 clojure.main/repl[fn]
                                      main.clj:259 clojure.main/repl[fn]
                                      main.clj:277 clojure.main/repl[fn]
                                      main.clj:277 clojure.main/repl
                                  RestFn.java:1096 clojure.lang.RestFn.invoke
                         interruptible_eval.clj:56 clojure.tools.nrepl.middleware.interruptible-eval/evaluate[fn]
                                      AFn.java:159 clojure.lang.AFn.applyToHelper
                                      AFn.java:151 clojure.lang.AFn.applyTo
                                      core.clj:617 clojure.core/apply
                                     core.clj:1788 clojure.core/with-bindings*
                                   RestFn.java:425 clojure.lang.RestFn.invoke
                         interruptible_eval.clj:41 clojure.tools.nrepl.middleware.interruptible-eval/evaluate
                        interruptible_eval.clj:171 clojure.tools.nrepl.middleware.interruptible-eval/interruptible-eval[fn]
                                     core.clj:2330 clojure.core/comp[fn]
                        interruptible_eval.clj:138 clojure.tools.nrepl.middleware.interruptible-eval/run-next[fn]
                                       AFn.java:24 clojure.lang.AFn.run
                      ThreadPoolExecutor.java:1110 java.util.concurrent.ThreadPoolExecutor.runWorker
                       ThreadPoolExecutor.java:603 java.util.concurrent.ThreadPoolExecutor$Worker.run
                                   Thread.java:722 java.lang.Thread.run




[CLJ-995] sorted-set doesn't support IEditableCollection Created: 13/May/12  Updated: 05/Feb/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Moritz Ulrich Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: collections


 Description   

I think sorted-set (PersistentTreeSet) should implement the transient interface. It's a special-purpose set and should be usable just like every normal set.



 Comments   
Comment by Michel Alexandre Salim [ 04/Jun/12 2:32 AM ]

Note that this would require PersistentTreeMap to implement IEditableCollection as well.





[CLJ-994] repeat reducer Created: 11/May/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Jason Jackson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: reducers

Attachments: Text File 0001-repeat-for-clojure.core.reducers.patch    
Patch: Code and Test

 Description   

i'm working on clojure.core/repeat reducer.



 Comments   
Comment by Andy Fingerhut [ 17/May/12 6:18 PM ]

Jason, have you tried to build this using JDK 1.6.0? I've tried on Mac OS X 10.6.8 + Oracle/Apple JDK 1.6.0 and Ubuntu 11.10 + IBM JDK 1.6.0, and on both it compiles, but during the tests fails with a ClassNotFoundException for class jsr166y.ForkJoinTask.

It builds and tests cleanly on Ubuntu 11.10 + Oracle JDK 1.7.0 for me.

Comment by Jason Jackson [ 17/May/12 6:41 PM ]

That's an issue that applies to all of core.reducers. Alan Malloy experienced it as well. I tried fixing it, but eventually just upgraded to JDK 1.7. I don't understand why it's happening.

Comment by Jason Jackson [ 19/May/12 2:55 PM ]

This issue is isolated to mvn test afaik.

When I include clojure inside a leiningen project, and add jsr166y.jar to lib directory, core.reducers works fine with java 1.6.

Comment by Andy Fingerhut [ 20/May/12 3:00 AM ]

Jason, you say it applies to all of core.reducers in your May 17, 2012 comment. I don't understand. Without your patch applied, I can run "./antsetup.sh ; ant" in a freshly-pulled Clojure git repo on either of the JDK 1.6.0 versions mentioned in my earlier comment, and do not get any errors during the tests. Are you saying perhaps that core.reducers currently has no tests that exercise the problem now, but your patch adds such tests that fail, even with no other changes to the code?

Comment by Jason Jackson [ 20/May/12 11:55 AM ]

Yah that's right. Now that you mention it, my patch is the first unit test to call r/fold (the existing tests do non-parallel reductions).

Comment by Andy Fingerhut [ 08/Jun/12 7:11 PM ]

With Stuart Halloway's commit to Clojure master on June 8, 2012 titled "let reducers tests work under ant", patch 0001-repeat-for-clojure.core.reducers.patch dated May 11, 2012 now runs correctly even the new unit tests requiring class jsr166y.ForkJoinTask with Oracle/Apple JDK 1.6 and Linux IBM JDK 1.6.

Comment by Jason Jackson [ 14/Aug/12 1:17 AM ]

I'm on the contributors list. Is this patch still needed?
sorry for long long delay.

Comment by Jason Jackson [ 14/Sep/12 2:37 PM ]

This patch should wait until http://dev.clojure.org/jira/browse/CLJ-993 is committed. I think there's a some shared code.





[CLJ-993] `range` reducer Created: 10/May/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: reducers

Attachments: Text File 0001-CLJ-993-implement-range-and-iterate-as-reducers.patch     Text File 0001-CLJ-993-implement-range-and-iterate-as-reducers.patch     Text File 0001-CLJ-993-implement-range-as-a-reducer.patch     Text File 0002-Make-iterate-and-range-Seqable.patch     Text File 0003-Implement-fold-for-Range-objects.patch     Text File just-iseq.patch     Text File range-reducer.patch    
Patch: Code and Test

 Description   

Rich mentioned in IRC today he'd welcome a reducer implementation of clojure.core/range. Now that I've figured out how to do iterate, I figure I'll knock out range as well by the end of the night. Just opening the issue early to announce my intentions to anyone else interested in doing it.



 Comments   
Comment by Alan Malloy [ 10/May/12 10:45 PM ]

Implemented range. A separate commit is attached, making iterate and range also Seqable, since I'm not sure if that's desired. Apply it or not, as you prefer.

Comment by Jason Jackson [ 11/May/12 11:20 AM ]

Range should be foldable

Comment by Alan Malloy [ 11/May/12 12:53 PM ]

Yep, so it should. Time for me to dig into the folding implementations!

Comment by Alan Malloy [ 11/May/12 2:42 PM ]

Should I fold (har har) all of these commits into one? I don't know what is preferred on JIRA, and I also don't know whether range/iterate should be seqable or if I should just drop the second commit.

Comment by Rich Hickey [ 11/May/12 3:21 PM ]

Yes, please merge these together, it's hard to see otherwise (I can barely read diffs as is . range and iterate shouldn't be novel in reducers, but just enhanced return values of core fns. The enhancement (e.g. protocol extensions) can come by requiring reducers since it can't be leveraged without it. Also, I'm not sure how I feel about an allocating protocol for 'splittable' - I've avoided it thus far.

Comment by Alan Malloy [ 11/May/12 3:30 PM ]

So you want clojure.core/range to return some object (a Range), which implements Counted and Seqable (but isn't just a lazy-seq), and then inside of clojure.core.reducers I extend CollReduce and CollFold to that type? Okay, I can do that.

I don't quite follow what you mean by an allocating protocol. I see your point that my fold-by-halves which takes a function in is analogous to a protocol with a single function, but it doesn't allocate anything more than foldvec already does - I just pulled that logic out so that the fork/join fiddly work doesn't need to be repeated in everything foldable. Do you have an alternative recommendation, or is it just something that makes you uneasy and you're still thinking about?

Comment by Rich Hickey [ 11/May/12 3:52 PM ]

While vector-fold allocs subvecs, the halving-fn must return a new vector, for all implementations. It's ok, I don't think it's likely to dominate (since fj needs new closures anyway). Please proceed, but keep range and iterate in core. They are sources, not transformers, and only transformers (which must be different from their seq-based counterparts) must reside in reducers. Thanks!

Comment by Stuart Sierra [ 11/May/12 5:01 PM ]

One big patch file is preferred, although that file may contain multiple commits if that makes the intent clearer.

When adding a patch, update the description of the ticket to indicate which file is the most recent. Leave old patch files around for historical reference.

Comment by Alan Malloy [ 11/May/12 9:00 PM ]

It's looking harder than I expected to move iterate and range into core.clj. My plan was to just have them implement Seqable, which is easy enough, but currently they are actually instances of ISeq, because they inherit from LazySeq. A bunch of code all over the place (eg, to print them in the repl) depends on them being ISeq, so I can't just ignore it. To implement all of these methods (around thirty) would take a large amount of code, which can't easily be shared between Iteration, Range, and any future reducible sources that are added to core.clj.

I could write a macro like (defseq Range [start end step] Counted (count [this] ...) ...) which takes normal deftype args and also adds in implementations for ISeq, Collection, and so forth in terms of (.seq this), which will be a LazySeq. However, this seems like a somewhat awkward approach that I would be a little embarrassed to clutter up core.clj with. If anyone has a better alternative I will be pleased to hear it. In the mean time, I will go ahead with this macro implementation, in case it turns out to be the best choice.

Comment by Alan Malloy [ 11/May/12 11:52 PM ]

– This patch subsumes all previous patches to this issue and to CLJ-992

In order to create an object which is both a lazy sequence and a
reducible source, I needed to add a macro named defseq to core_deftype.
It is basically a reimplementation of clojure.lang.LazySeq as a clojure
macro, so that I can "mix in" lazy-sequence functions into a new class
with whatever methods are needed for reducing and folding.

If we wanted, we could use this macro to implement lazy-seq in clojure instead of in java, but that's unrelated so I didn't do that in this patch.

As noted in a previous comment, defseq may not be the right approach, but this works until something better is suggested.

Comment by Alan Malloy [ 11/May/12 11:58 PM ]

I accidentally included an implementation of drop-while in this patch, which I was playing around with to make sure I understood how this all works. I guess I'll leave it in for the moment, since it works and is useful, but I can remove it, or move it to a new JIRA ticket, if it's not wanted at this time.

Comment by Rich Hickey [ 12/May/12 10:52 AM ]

Ok, I think this patch is officially off the rails. There must be a better way. Let's start with: touching core/deftype and reimplementing lazy-seq as a macro are off the table. The return value of range doesn't have to be a LazySeq, it has to be a lazy seq, .e.g. implement ISeq (7 methods, not 30) which it can do by farming out to its existing impl. It can also implement some new interface for use by the reducer logic. There is also still clojure.lang.Range still there, which is another approach. Please take an extremely conservative approach in these things.

Comment by Alan Malloy [ 12/May/12 5:53 PM ]

Okay, thanks for the feedback - I'm glad I went into that last patch knowing it was probably wrong . I thought I would need to implement the java collection interfaces that LazySeq does, eg java.util.List, in order to avoid breaking interop functions like (defn range-list [n] (ArrayList. (range n))). If it's sufficient to implement ISeq (and thus IPersistentCollection), then that's pretty manageable.

It's still an unpleasant chunk of boilerplate for each new source, though; would you welcome a macro like defseq if I didn't put it in core_deftype? If so, it seems like it might as well implement the interop interfaces; if not, I can skip them and implement the 7 (isn't it more like 9?) methods in ISeq, IPersistentCollection, and Seqable for each new source type.

Thanks for pointing out clojure.lang.Range to me - I didn't realize we had it there. Of course with implementation inheritance it would be easy to make Range, Iteration, etc inherit from LazySeq and just extend protocols from them. But that means moving functionality out of clojure and into java, which I didn't think we'd want to do.

I'll put together a patch that just implements ISeq by hand for both of these new types, and attach it probably later today.

Comment by Alan Malloy [ 12/May/12 7:49 PM ]

So I've written a patch that implements ISeq, but not the java Collections interfaces, and it mostly works but there are definitely assumptions in some parts of clojure.core and clojure.lang that assume seqs are Collections. The most obvious to me (ie, it shows up when running mvn test) is RT/toArray - it tests for Collection, but never for ISeq, implying that it's not willing to handle an ISeq that is not also a collection. Functions which rely on toArray (eg to-array and vec) now fail.

This patch subsumes all previous patches on this issue, but is not suitable for application because it leaves some failing tests behind - it is intended only for intermediate feedback.

Comment by Rich Hickey [ 13/May/12 8:50 AM ]

It would be a great help if, time permitting, you could please write up the issues, challenges and options you've discovered somewhere on the dev wiki (even a simple table would be fantastic). I realize this has been a challenging task, and at this point perhaps we should opt for the more modest reducers/range and reducers/iterate and leave the two worlds separate. I'd like at some point to unify range, as there are many extant ranges it would be nice to be able to fold, as we can extant vectors.

Comment by Jason Jackson [ 13/May/12 9:24 AM ]

Should r/range return something Seqable and Counted?

If so, I'll do the same for r/repeat.

Comment by Alan Malloy [ 13/May/12 1:59 PM ]

I've sketched out a description of the issues and options. I'm not very familiar with the dev wiki and couldn't figure out where was the right place to put this. "release.next" seems to still be about 1.4 issues, and I don't know if it's "appropriate" to create a whole new category for this. It's available as a gist until a better home can be found for it.

Comment by Alan Malloy [ 23/May/12 7:54 PM ]

Here's a single patch summing up the state Rich suggested "rolling back" to: separate r/range and r/iterate functions. I haven't heard any feedback since doing the writeup Rich asked for, so am not making any further progress at the moment; if something other than this patch is desired just let me know.

Comment by Rich Hickey [ 14/Aug/12 2:07 PM ]

I prefer not to see the use of extend like this for new types. Perhaps this code is too DRY? Also, it does a lot in one patch which makes it hard to parse and accept. This adds Range, switches impl of vector folds etc. Can it be broken up into separate tickets that do each step that builds on the previous, e.g. one ticket could be: capture vector fold impl for reuse by similar things.

Comment by Alan Malloy [ 18/Aug/12 6:19 PM ]

Okay, I should be able to split it up over the weekend. I'll also see about converting fold-by-halves into a function that is used by Range/Vector, rather than a function that gets extended onto them.

Comment by Alan Malloy [ 18/Aug/12 7:18 PM ]

As requested, I have split up the large patch on this issue into four smaller tickets. The other three are: CLJ-1045, CLJ-1046, and CLJ-992.

CLJ-1045 contains the implementation of fold-by-halves, and as such this patch cannot be applied until CLJ-1045 is accepted. This ticket does not depend on the other two, but there will be minor merge conflicts if this is merged before them.





[CLJ-992] `iterate` reducer Created: 10/May/12  Updated: 09/Apr/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reducers

Attachments: Text File 0001-Add-reducers-iterate.patch     Text File iterate-reducer.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Added a reducer implementation mirroring clojure.core/iterate.

Patch: 0001-Add-reducers-iterate.patch

Screened by:



 Comments   
Comment by Alan Malloy [ 10/May/12 9:50 PM ]

Should I have made this implement Seqable as well? It wasn't clear to me, because as far as I could see this was the only function in clojure.core.reducers that's generating a brand-new sequence rather than transforming an existing one.

Comment by Alan Malloy [ 10/May/12 10:24 PM ]

Previous version neglected to include the seed value of the iteration in the reduce.

Comment by Jason Jackson [ 11/May/12 11:23 AM ]

Currying iterate seems useless, albeit not harmful.

While implementing repeat, I couldn't use currying. Because 1-arity is already reserved for infinite repeat ([n x] and [x], not [n x] and [n] if currying)

How about we just support currying for functions where last param is reducible?

Comment by Alan Malloy [ 18/Aug/12 7:16 PM ]

This new patch replaces the previous patch. As requested, I am splitting up the large issue CLJ-993 into smaller tickets.

Does not depend on any of my other reducer patches, but there will probably be some minor merge conflicts unless it is merged after CLJ-1045 and CLJ-1046, and before CLJ-993.





[CLJ-986] Adds an exit function to exit clojure process Created: 06/May/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

There is no standard function to exit the clojure process.
In java implementation,we use (System/exit 0),but in other implementations(CLR), i have to use another function.

Why not add a standard function in clojure.core?
For example:

(defn exit
([] (exit 0)
([status] (System/exit status)))

I think it's useful for us.






[CLJ-979] map->R returns different class when invoked from AOT code Created: 03/May/12  Updated: 29/Mar/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3, Release 1.4, Release 1.5, Release 1.6
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Edmund Jackson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: aot
Environment:

Mac OS X 10.5, lein 1.7 and lein 2.0


Attachments: Text File clj-979-symptoms.patch    

 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


 Comments   
Comment by Scott Lowe [ 12/May/12 9:05 PM ]

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.

Comment by Edmund Jackson [ 13/May/12 2:20 AM ]

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.

Comment by Edmund Jackson [ 13/May/12 4:23 AM ]

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 ?

Comment by Scott Lowe [ 13/May/12 8:41 AM ]

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.

Comment by Scott Lowe [ 13/May/12 10:35 PM ]

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.

Comment by Steve Miner [ 03/Sep/13 9:54 AM ]

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

Comment by Ambrose Bonnaire-Sergeant [ 29/Mar/14 1:11 PM ]

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

Comment by Ambrose Bonnaire-Sergeant [ 29/Mar/14 2:27 PM ]

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.





[CLJ-978] bean unable to handle non-public classes Created: 30/Apr/12  Updated: 05/Feb/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Charles Duffy Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: File clojure--bean-support-for-private-implementation-classes-v3.diff    
Patch: Code and Test

 Description   

Take the following Java as an example:

public interface IFoo {
  String getBar();
}

class FooImpl {
  String getBar() { return "bar"; }
}

As presently implemented, (bean my-foo) tries to invoke the following:

(. #<Method public java.lang.String FooImpl.getBar> (invoke my-foo nil))

However, as FooImpl is not public, this fails:

java.lang.IllegalAccessException: Class clojure.core$bean$fn__1827$fn__1828 can not access a member of class FooImpl with modifiers "public"
 at sun.reflect.Reflection.ensureMemberAccess (Reflection.java:65)
    java.lang.reflect.Method.invoke (Method.java:588)
    clojure.core$bean$fn__1827$fn__1828.invoke (core_proxy.clj:382)
    clojure.core$bean$v__1832.invoke (core_proxy.clj:388)
    clojure.core$bean$fn__1838$thisfn__1839$fn__1840.invoke (core_proxy.clj:406)
    clojure.lang.LazySeq.sval (LazySeq.java:42)
    clojure.lang.LazySeq.seq (LazySeq.java:60)
    clojure.lang.RT.seq (RT.java:473)

However, the same thing succeeds if we call #<Method public java.lang.String Foo.getBar> rather than #<Method public java.lang.String FooImpl.getBar>.



 Comments   
Comment by Charles Duffy [ 30/Apr/12 10:40 PM ]

Fix inaccurate documentation string

Comment by Charles Duffy [ 01/May/12 9:41 AM ]

Apache Commons Beanutils has their own implementation of this, at http://www.docjar.com/html/api/org/apache/commons/beanutils/MethodUtils.java.html#771 – notably, it tries to reflect a method with the given signature and catches the exception on failure, rather than iterating through the whole list. This may be a better approach – I'm unfamiliar with how the cost of exception handling compares with that of reflecting on the full method list of a class.

Comment by Charles Duffy [ 01/May/12 10:11 AM ]

Prior version of patch were missing new test suite files. Corrected.

Comment by Andy Fingerhut [ 04/May/12 2:48 AM ]

Thanks for the patches, Charles. Could you please create a patch in the desired format and attach that, and then remove the obsolete patches? Instructions for creating a patch are under the heading "Development" at this page: http://dev.clojure.org/display/design/JIRA+workflow

Instructions for removing patches are under the heading "Removing patches" on that same page.

Comment by Charles Duffy [ 06/May/12 2:59 PM ]

Added a patch created per documented process.

Comment by Gary Trakhman [ 04/Oct/12 6:44 PM ]

I found in my code that it's possible to get a NPE if there is no read-method, for instance on the http://docs.cascading.org/cascading/2.0/javadoc/cascading/flow/hadoop/HadoopFlow.html object which has a setCascade method but no getter. I fixed this in our code by inlining the is-zero-args check into the public-method definition and and-ing the whole thing with 'method' like the original 'bean' code, like so:

public-method (and method (zero? (alength (. method (getParameterTypes))))
(or (and (java.lang.reflect.Modifier/isPublic (. c (getModifiers)))
method)
(public-version-of-method method)))

Comment by Rich Hickey [ 29/Nov/12 10:01 AM ]

Charles, I think we should follow Apache BeanUtils on this. Exceptions not thrown are cheap. Ordinarily, exception for control flow are bad, but this is forced by bad design of reflection API.





[CLJ-969] Symbol/keyword implements IFn for lookup but a non-collection argument produces non-intuitive results Created: 09/Apr/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Sean Corfield Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs


 Description   

('+ 1 2) ;; return 2 because it is treated as (get 1 '+ 2)

Whilst this is "consistent" once you know the lookup behavior, it's confusing for Clojure newbies and it seems to be a non-useful behavior.

Proposal: modify Keyword.invoke() and Symbol.invoke() to restrict first Object argument to instanceof ILookup, Map or IPersistentSet (or null) so that the "not found" behavior doesn't produce non-intuitive behavior.






[CLJ-968] ns emitting gen-class before imports results in imported annotations being discarded. Created: 09/Apr/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3, Release 1.4
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Charles Duffy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop


 Description   

The following discards the imported annotations:

(ns com.example.BaseXModuleTest
  (:import (org.basex.query QueryModule QueryModule$Deterministic))
  (:gen-class
     :extends org.basex.query.QueryModule
     :methods [
       [^{QueryModule$Deterministic {}}
        addOne [int] int]]))

However, when moving the gen-class call out of the ns declaration, the annotation is correctly applied:

(ns com.example.BaseXModuleTest
  (:import (org.basex.query QueryModule QueryModule$Deterministic)))

(gen-class
  :extends org.basex.query.QueryModule
  :name com.example.BaseXModuleTest
  :methods [
    [^{QueryModule$Deterministic {}}
     addOne [int] int]])

It appears that imported names are not yet in-scope when gen-class is run from a ns declaration.






[CLJ-919] cannot create anonymous primitive functions Created: 27/Jan/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Ben Mabey Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: typehints


 Description   

Primitive functions only work (e.g. return primitive types) when defined with `defn`. An equivalent function created with `fn` does not behave the same way as when created with `defn`. For example:

(definterface IPrimitiveTester
(getType [^int x])
(getType [^long x])
(getType [^float x])
(getType [^double x])
(getType [^Object x]))

(deftype PrimitiveTester []
IPrimitiveTester
(getType [this ^int x] :int)
(getType [this ^long x] :long)
(getType [this ^float x] :float)
(getType [this ^double x] :double)
(getType [this ^Object x] :object))

(defmacro pt [x]
`(.getType (PrimitiveTester.) ~x))

(defn with-defn ^double [^double x]
(+ x 0.5))

(pt (with-defn 1.0)) ; => :double

(let [a (fn ^double [^double x] (+ x 0.5))]
(pt (a 0.1))) ; => :object

Please see the discussion on the mailing list for more details and thoughts on what is happening:
http://groups.google.com/group/clojure/browse_thread/thread/d83c8643a7c7d595?hl=en






[CLJ-911] 'proxy' prevents overriding Object.finalize (and doesn't document it) Created: 16/Jan/12  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Norman Gray Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop
Environment:

OS X, Java 1.6.0?



 Description   

It appears to be impossible to override Object.finalize() using proxy. If the method is defined using proxy, then it cannot be called straightforwardly (see below), and it is not called as a finalizer during normal program execution (not demonstrated below).

See extensive discussion at: https://groups.google.com/group/clojure/browse_thread/thread/a1e2fca45af6c1af

user=> (def m (proxy [java.util.HashMap] []
(finalize []
;(proxy-super finalize)
(prn "finalizing..."))
(hashCode []
99)))
#'user/m
user=> (.hashCode m)
99
user=> (.finalize m)
IllegalArgumentException No matching field found: finalize for class user.proxy$java.util.HashMap$0 clojure.lang.Reflector.getInstanceField (Reflector.java:289)

There is at least one of two bugs here (thanks to Cedric Greevey for summarising this way):

  • If the inability to override finalize() is unintentional, that's a bug.
  • If it's intentional for some reason, then (a) that's not documented, and (b) the failure is silent, in the sense that an explicit call produces an apparently completely unrelated error (above), and the failure to call the method during object finalization is completely silent.





[CLJ-903] extend-protocol does not allow classnames as a String Created: 30/Dec/11  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2, Release 1.3, Release 1.4
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Meikel Brandmeyer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop


 Description   

In various places Clojure accepts classnames as String, eg. in gen-class or type hints. However it does not in extend-protocol. This does not allow simple specification of array types.

See also here: http://groups.google.com/group/clojure/browse_thread/thread/722a0c09d02bb0ac






[CLJ-899] Accept and ignore colon between key and value in map literals Created: 18/Dec/11  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: reader


 Description   

Original title was 'treat colons as whitespace' which isn't a problem description but a (flawed) implementation approach

For JSON compatibility
known problems when no spaces - x:true and y:false



 Comments   
Comment by Tassilo Horn [ 23/Dec/11 3:22 AM ]

Discussed here: https://groups.google.com/d/msg/clojure/XvJUzaY1jec/l8xEwlFl8EUJ

Comment by Kevin Downey [ 11/Jan/12 2:23 PM ]

please no

Comment by Tavis Rudd [ 16/Jan/12 12:17 PM ]

Alan Malloy raises a good point in the google group discussion (https://groups.google.com/d/msg/clojure/XvJUzaY1jec/aVpWBicwGhsJ) about accidental confusion between trailing (or floating) and leading colons:
"It isn't even as simple as "letting them
be whitespace", because presumably you want (read-string "{a: b}") to
result in (hash-map 'a 'b), but (read-string "{a :b}") to result in
(hash-map 'a :b)."

This issue could be avoided by only treating a colon as whitespace when followed by a comma. As easy cut-paste of json seems the be the key motivation here, the commas are going to be there anyway: valid {"v":, 1234} vs syntax error {a-key: should-be-a-keyword}.

Comment by Alex Baranosky [ 16/Jan/12 5:23 PM ]

This would be visually confusing imo.

Comment by Laurent Petit [ 17/Jan/12 5:01 PM ]

Please, oh please, no.

Comment by Tavis Rudd [ 18/Jan/12 2:40 PM ]

Er, brain fart. I was typing faster than I was thinking and put the comma in the wrong place. In my head I meant the form following the colon would have to have a comma after it. Thus, {"a-json-key": 1234, ...} would be valid while {"a-json-key": was-supposed-to-be-a-keyword "another-json-key" foo} would complain about the colon being an Invalid Token. I don't see the need for it, however.

Comment by Joseph Smith [ 27/Feb/12 10:55 AM ]

Clojure already has reader syntax for a map. If we support JSON, do we also support ruby map literals? Seems like this addition would only add confusion, imo, given colons are used in keywords and keywords are frequently used in maps - e.g., when de-serializing from XML, or even JSON.

Comment by David Nolen [ 27/Feb/12 11:19 AM ]

Clojure is no longer a language hosted only on the JVM. Clojure is also hosted on the CLR, and JavaScript. In particular ClojureScript can't currently easily deal with JSON literals - an extremely common (though problematic) data format. By allowing colon whitespace in map literals - Clojure data structures can effectively become an extensible JSON superset - giving the succinctness of JSON and the expressiveness of XML.

+1 from me.

Comment by Tim McCormack [ 13/Nov/12 7:27 PM ]

Clojure is only hosted on the JVM; ClojureScript is hosted on JS VMs. If this is useful for CLJS, it should just be a CLJS feature.

Comment by Mike Anderson [ 10/Dec/12 11:51 PM ]

-1 for this whole idea: that way madness lies....

If we keep adding syntactical oddities like this then the language will become unmaintainably complex. It's the exact opposite of simple to have lots of special cases and ambiguities that you have to remember.

If people want to use JSON that is fine, but then the best approach use a specific JSON parser/writer, not just paste it into Clojure source and expect it to work.

Comment by Laszlo Török [ 11/Dec/12 4:54 AM ]

-1 for reasons mentioned by Allan Malloy and Mike Anderson





[CLJ-888] defprotocol should throw error when signatures include variable number of parameters Created: 29/Nov/11  Updated: 05/Feb/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Greg Chapman Assignee: Stuart Halloway
Resolution: Unresolved Votes: 2
Labels: errormsgs, protocols

Attachments: Text File 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch    
Patch: Code and Test
Approval: Triaged

 Description   

I tried to use & in the signature for a method in defprotocol. Apparently (see below), this is compiled so that & becomes a simple parameter name, and there is no special handling for variable number of parameters. I think the use of & in a protocol signature ought to be detected and immediately cause an exception (I also think this restriction on the signatures ought to be documented; I couldn't find it specified in the current documentation, though of course it is implied (as I later realized) by the fact that defprotocol creates a Java interface).

user=> (defprotocol Applier (app [this f & args]))
Applier
user=> (deftype A [] Applier (app [_ f & args] (prn f & args) (apply f args)))
user.A
user=> (app (A.) + 1 2)
#<core$PLUS clojure.core$PLUS@5d9d0d20> 1 2
IllegalArgumentException Don't know how to create ISeq from: java.lang.Long
clojure.lang.RT.seqFrom (RT.java:487)



 Comments   
Comment by Alex Coventry [ 21/Oct/13 4:21 PM ]

Patch with test code attached. I have it throwing a CompilerException so that it shows source code location. Not sure whether this is kosher in clojure code, but I wish more macros provided this in their error handling.

Comment by Tassilo Horn [ 22/Oct/13 6:26 AM ]

This issue has already been discussed in CLJ-1024. There I provided a patch that forbids varargs and destructuring forms at various places including defprotocol/definterface. My patch had been applied shortly before clojure 1.5 was released, but it had a bug (forbid too many uses), so it got reverted and the bug closed and declined.

I was told to bring up the issue again after 1.5 has been released.

So here is my patch again. This time it's much more relaxed and only forbids varargs in defprotocol/definterface method declarations, and in deftype/defrecord and reify method implementations.

Comment by Alex Coventry [ 22/Oct/13 7:30 AM ]

Thanks, Tassilo. If there's anywhere in the JIRA system where I could check for prior work like that for other similar issues, I'd be grateful for a pointer.

Best regards,
Alex

Comment by Tassilo Horn [ 22/Oct/13 7:39 AM ]

New version of my patch.

Now I use a CompilerException with proper file/line/column information like Alex did. I also added his test case (which passes).

Concerning your question, Alex: a search for "varargs" would have listed CLJ-1024, but probably you wouldn't have looked into it anyway, because it's a closed issue...

Comment by Tassilo Horn [ 22/Oct/13 7:44 AM ]

Alex, if you don't object could we remove your patch in favor of mine which covers a bit more cases?

Comment by Alex Coventry [ 22/Oct/13 10:57 AM ]

Yep. Just read through 1024 and the associated mailing list discussion. You should totally get the credit: Your patch is more comprehensive and you have been on this a long time. Thanks for folding in the good parts of my patch.

Best regards,
Alex

Comment by Tassilo Horn [ 22/Oct/13 12:15 PM ]

Ok, great.

It seems I don't have the permissions to delete other peoples' attachments, so could you please delete your patch yourself?

Comment by Alex Coventry [ 23/Oct/13 2:44 PM ]

Sure, Tassilo. It's done.

I think this also needs a regression test for the case hugod originally pointed out. I initially made the same mistake as you there, but amalloy pointed it out[1] before I submitted the patch, so it is a natural mistake to make and should probably be documented in the source code.

Best regards,
Alex

[1] http://logs.lazybot.org/irc.freenode.net/%23clojure/2013-10-21.txt search for 14:48:34.

Comment by Tassilo Horn [ 24/Oct/13 2:00 AM ]

Alex, I've added the regression test you suggested. Thanks for pointing that out.

Also, I added tests checking definterface method declarations, and tests checking inline method implementations made with defrecord, deftype, and reify.

However, there's a problem with the tests for deftype and reify I don't know how to fix. When I eval the macroexpand forms used in the tests in a REPL, I can see that the CompilerException is successfully thrown and printed. But it also seems to be caught somewhere in the middle, so that the macroexpand returns a form and the exception doesn't make it to the (is (thrown? ...)). Therefore, I've commented the these tests and added a big FIXME.

Comment by Tassilo Horn [ 24/Oct/13 2:28 AM ]

New version of the patch with now all tests uncommented and passing. Andy Fingerhut made me aware that for the 4 deftype and reify tests, I need eval instead of just macroexpand.

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

I have not investigated the reason yet, but patch 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch no longer applies cleanly after the latest commits to Clojure master on Oct 25 2013.

Comment by Tassilo Horn [ 28/Oct/13 2:21 AM ]

I've rebased the patch onto the current master so that it applies cleanly again.

Comment by Tassilo Horn [ 28/Oct/13 2:25 AM ]

Stu, I've assigned this issue to you because you've been assigned to CLJ-1165 which I have closed as duplicate of this issue.

One minor difference between my patch to this issue and CLJ-1165 is that here I use a CompilerException with file/line/column info whereas in CLJ-1165 I've used `ex-info`. I think the CE is more appropriate/informative, as the error is already triggered during macro expansion.





[CLJ-887] Error when calling primitive functions with destructuring in the arg vector Created: 29/Nov/11  Updated: 14/May/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6, Release 1.7
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Alexander Taggart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

Attachments: Text File 0001-don-t-remove-meta-from-arg-vector-in-maybe-destructu.patch    
Patch: Code
Approval: Screened

 Description   

If one defines a primitive-taking function with destructuring, calling that function will result in a ClassCastException, IFF the primitive return-type hint is present.

Clojure 1.4.0-master-SNAPSHOT
user=> (defn foo [[a b] ^long x ^long y] 0)
#'user/foo
user=> (foo [1 2] 3 4)
0
user=> (defn foo ^long [[a b] ^long x ^long y] 0)
#'user/foo
user=> (foo [1 2] 3 4)
ClassCastException user$foo cannot be cast to clojure.lang.IFn$OLLL  user/eval9 (NO_SOURCE_FILE:4)
user=> (pst)
ClassCastException user$foo cannot be cast to clojure.lang.IFn$OLLL
	user/eval9 (NO_SOURCE_FILE:4)
	clojure.lang.Compiler.eval (Compiler.java:6493)
	clojure.lang.Compiler.eval (Compiler.java:6459)
	clojure.core/eval (core.clj:2796)
	clojure.main/repl/read-eval-print--5967 (main.clj:244)
	clojure.main/repl/fn--5972 (main.clj:265)
	clojure.main/repl (main.clj:265)
	clojure.main/repl-opt (main.clj:331)
	clojure.main/main (main.clj:427)
	clojure.lang.Var.invoke (Var.java:397)
	clojure.lang.Var.applyTo (Var.java:518)
	clojure.main.main (main.java:37)
nil

Cause: This was happening because maybe-destructured returned the arg vector without the type hint, so the function was getting compiled to a IFn$OLLO rather than a IFn$OLLL but the :arglists vector in the var meta was still tagged, so the compiler thought that foo was a IFn$OLLL.

Approach: This patch addresses this by preserving the original meta on the fn arglist.

Patch: 0001-don-t-remove-meta-from-arg-vector-in-maybe-destructu.patch

Screened by: Alex Miller



 Comments   
Comment by Nicola Mometto [ 03/Apr/14 1:35 PM ]

This was happening because maybe-destructured returned the arg vector without the type hint, so the function was getting compiled to a IFn$OLLO rather than a IFn$OLLL but the :arglists vector in the var meta was still tagged, so the compiler thought that foo was a IFn$OLLL.

This patch addresses this by preserving the original meta on the fn arglist

Comment by Alex Miller [ 03/Apr/14 1:40 PM ]

Weirdly, I saw this happen today in my own code.





[CLJ-864] defrecord positional arity factory fn should have an inline version that calls the record constructor Created: 26/Oct/11  Updated: 04/Dec/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord


 Description   

defrecord positional arity factory fn should have an inline version that calls the record constructor



 Comments   
Comment by Gary Fredericks [ 26/May/13 3:39 PM ]

I had the idea recently that the factory fn was useful partly for being a redefinable var, e.g. that you could wrap with contracts or anything else. This idea would preclude that.

It makes sense though if the only purpose of ->Foo is to avoid having to :import something.

Comment by Kevin Downey [ 13/Aug/13 4:04 PM ]

interesting, that is a good point

Comment by Gary Fredericks [ 04/Dec/13 7:21 AM ]

Another thought – using the factory fns rather than constructors directly gives you a little bit of protection against code-reloading issues, does it not? I don't think I understand the code reloading issues in great detail, so I'm not confident about this. My assumption is that the compiled code refers to vars rather than classes.





[CLJ-859] Built in dynamic vars don't have :dynamic metadata Created: 19/Oct/11  Updated: 24/Feb/12

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Anthony Simpson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

I'm sure 'built in' is probably not the right term here, but I'm not sure what these are called.

I ran into this issue earlier today while fixing a bug in clojail. Built in vars, particularly ones listed here without a source link: http://clojure.github.com/clojure/clojure.core-api.html, do not have :dynamic metadata despite being dynamic. This includes *in*, *out*, and *err* among others. Here are some examples:

user=> (meta #'*err*)
{:ns #<Namespace clojure.core>, :name *err*, :added "1.0", :doc "A java.io.Writer object representing standard error for print operations.\n\n  Defaults to System/err, wrapped in a PrintWriter"}
user=> (meta #'*in*)
{:ns #<Namespace clojure.core>, :name *in*, :added "1.0", :doc "A java.io.Reader object representing standard input for read operations.\n\n  Defaults to System/in, wrapped in a LineNumberingPushbackReader"}
user=> (meta #'*out*)
{:ns #<Namespace clojure.core>, :name *out*, :added "1.0", :doc "A java.io.Writer object representing standard output for print operations.\n\n  Defaults to System/out, wrapped in an OutputStreamWriter", :tag java.io.Writer}
user=> (meta #'*ns*)
{:ns #<Namespace clojure.core>, :name *ns*, :added "1.0", :doc "A clojure.lang.Namespace object representing the current namespace.", :tag clojure.lang.Namespace}


 Comments   
Comment by Ben Smith-Mannschott [ 19/Oct/11 12:03 PM ]

This recent discussion on the users list seems relevant: Should intern obey :dynamic?.

It seems to boil down to this the information that a Var is dynamic (or not) is duplicated. Once as metadata with the key :dynamic, and once as a boolean field on the Var class which implements Clojure's variables. This boolean can be obtained by calling the method isDynamic() on the Var.

The confusion arises because apparently :dynamic and .isDynamic can get out of sync with each other. .isDynamic is the source of truth in this case.

Comment by Ben Smith-Mannschott [ 19/Oct/11 12:18 PM ]

Compiler$Parser.parse(...) finds the :dynamic entry left in the metadata of the symbol by LispReader and passes this on when creating a new DefExpr, which in turn, generates the code that will call setDynamic(...) on the var when it is created at runtime.

As far as I can tell, the :dynamic entry is irrelevant once that has occurred. It seems to be implemented only as a way to communicate (by way of the reader) with the compiler. Once the compiler's gotten the message, it isn't needed anymore. Keeping it around seems to just cause confusion.

Dynamic vars created by the Java layer of Clojure core don't use the :dynamic mechanism, they just setDynamic() directly. That's why they don't have :dynamic in their meta-data map.

  • Perhaps the compiler should elide :dynamic from the metadata map available at runtime, since it has served its purpose.
  • Perhaps Clojure should supply the function dynamic?.
    (defn dynamic? [^clojure.lang.Var v] (.isDynamic v))

Or, perhaps one might consider, for 1.4, replacing :dynamic altogether and just enforcing the established naming convention: *earmuffs* are dynamic, everything-else isn't. (The compile warns about violations of this convention in 1.3.)

Comment by Andy Fingerhut [ 24/Feb/12 11:39 AM ]

I recently noticed several lines like this one in core.clj. Depending upon how many symbols are like this, perhaps this method could be used to add :dynamic metadata to symbols in core, along with a unit test to verify that all symbols in core have :dynamic if and only if .isDynamic returns true?

Comment by Andy Fingerhut [ 24/Feb/12 12:41 PM ]

Ugh. In my previous comment, by "several lines like this one" I meant to paste the following as an example:

(alter-meta! #'agent assoc :added "1.0")





[CLJ-840] Add a way to access the current test var in :each fixtures for clojure.test Created: 21/Sep/11  Updated: 23/Nov/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Hugo Duncan Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File add-test-var.diff     File clj840-2.diff    
Patch: Code

 Description   

When looking at (log) output from tests written with clojure.test, I would like to be able to identify the output associated with each test. A mechanism to expose the current test var within an :each fixture would enable this.

One mechanism might be to bind a test-var var with the current test var before calling the each-fixture-fn in clojure.test/test-all-vars.



 Comments   
Comment by Stuart Sierra [ 07/Oct/11 4:33 PM ]

Or just pass the Var directly into the fixture. Vars are invokable.

Comment by Hugo Duncan [ 07/Oct/11 4:45 PM ]

I don't think that works, since the the function passed to the fixture is not the test var, but a function calling test-var on the test var.

Comment by Hugo Duncan [ 21/Oct/11 10:34 PM ]

Patch to add test-var

Comment by Stuart Sierra [ 25/Oct/11 6:04 PM ]

*testing-vars* already has this information, but it's not visible to the fixture functions because it gets bound inside test-var.

Perhaps the :each fixture functions should be called in test-var rather than in test-all-vars. (The namespace of a Var is available in its metadata.) But then we have to call join-fixtures inside test-var every time.

Comment by Stuart Sierra [ 25/Oct/11 6:26 PM ]

Try this patch: clj840-2.diff.

This makes *testing-vars* visible to :each fixture functions, which seems intuitively more correct.

BUT it slightly changes the behavior of test-var, which I'm less happy about.

Comment by Hugo Duncan [ 25/Oct/11 8:07 PM ]

Might it make sense to provide a function on top of testing-vars to return the current test-var?

Comment by Stuart Sierra [ 28/Oct/11 9:14 AM ]

No, that function is first

Comment by Hugo Duncan [ 28/Oct/11 11:31 AM ]

I agree with having the dynamic vars as part of the extension interface, but would have thought that having a function for use when writing tests would have been cleaner. Just my 2c.

Comment by Andy Fingerhut [ 23/Nov/13 12:42 AM ]

With a commit made on Nov 22, 2013, patch clj840-2.diff no longer applies cleanly to latest master. Updating it appears like it might be straightforward, but best for someone who knows this part of the code well to do so.





[CLJ-825] Protocol implementation inconsistencies when overloading arity Created: 08/Aug/11  Updated: 10/Jun/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2, Release 1.3, Release 1.4, Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Carl Lerche Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: protocols
Environment:

All


Attachments: Text File clj-825-1.patch     File scribbles.clj    
Patch: Code and Test
Approval: Triaged

 Description   

The forms required for implementing arity-overloaded protocol methods are inconsistent between the "extend-*" macros and "defrecord".

The "extend" family of macros requires overloaded method definitions to follow the form used by defn:

(method ([arg1] ...) ([arg1 arg2] ...))

However, "defrecord" requires implementations to be defined separately:

(method [arg1] ...)
(method [arg1 arg2] ...)

Furthermore, the error modes if you get it wrong are unhelpful.

If you use the "defrecord" form with "extend-*", it evals successfully, but later definitions silently overwrite lexically previous definitions.

If you use the "extend-*" form with "defrecord", it gives a cryptic error about "unsupported binding form" on the body of the method.

This is not the same issue as CLJ-1056: That pertains to the syntax for declaring a protocol, this problem is with the syntax for implementing a protocol.

(defprotocol MyProtocol
  (mymethod
    [this arg]
    [this arg optional-arg]))

(extend-protocol MyProtocol
  Object
  (mymethod
    ([this arg] :one-arg)
    ([this arg optional-arg] :two-args)))

;; BAD! Blows up with "Unsupported binding form: :one-arg"
(defrecord MyRecord []
  MyProtocol
  (mymethod
    ([this arg] :one-arg)
    ([this arg optional-arg] :two-args)))

;; Works...
(defrecord MyRecord []
  MyProtocol
  (mymethod [this arg] :one-arg)
  (mymethod [this arg optional-arg] :two-args))

;; Evals...
(extend-protocol MyProtocol
  Object
  (mymethod [this arg] :one-arg)
  (mymethod [this arg optional-arg] :two-args))

;; But then... Error! "Wrong number of args"
(mymethod :obj :arg)

;; 2-arg version is invokable...
(mymethod :obj :arg1 :arg2)


 Comments   
Comment by Paavo Parkkinen [ 17/Nov/13 6:02 AM ]

Attached a patch for this.

For defrecord, I check which style is used for defining methods, and transform into the original style if the new style is used. For the check I do what I believe defn does, which is (vector? (first fdecl)).

For extend-*, I skip the checking, and just transform everything into the same format.

Tests included for both.

All tests pass.

Comment by Rich Hickey [ 10/Jun/14 11:00 AM ]

What the proposal?





[CLJ-792] Refactor method resolution code out of Compiler and into Reflector Created: 11/May/11  Updated: 05/Feb/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alexander Taggart Assignee: Alexander Taggart
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File clj-792-reorg-reflector-patch2.txt     Text File reorg-reflector.patch    

 Description   

Issues:

  1. Code for obtaining method/constructor instances is duplicated across the Compiler
  2. Code for resolving a preferred overloaded method lives in the Compiler

By consolidating the duplicated code, moving the reflection-related parts into Reflector, and providing a straightforward API, it should be easier to read and understand the method resolution process. Further, improvements to (e.g., CLJ-445) the mechanism for reflecting on class members can largely be isolated from the Compiler. And the few points of coordination (e.g., Compiler emitting same arg and return types as Reflector does when invoking) can be clearly identified and documented.



 Comments   
Comment by Stuart Sierra [ 17/Feb/12 2:28 PM ]

Patch does not apply as of commit f5bcf64.

Comment by Alexander Taggart [ 17/Feb/12 3:14 PM ]

Yeah, year-old patches tend to do that.

Comment by Andy Fingerhut [ 20/Feb/12 1:11 PM ]

I don't know if this is helpful or not, but this updated version of Alexander's patch applies cleanly to latest Clojure head as of Feb 20, 2012. It compiles, but does not pass ant test.





[CLJ-790] Primitive type hints on function names should print error message Created: 10/May/11  Updated: 03/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alan Dipert Assignee: Alan Dipert
Resolution: Unresolved Votes: 0
Labels: errormsgs


 Description   

Functions returning primitives are hinted with metadata on the argument list, not on the function name. Using a primitive type hint on a function name should print an error message.

Currently, misplaced primitive hints are read without error.






[CLJ-787] transient blows up when passed a vector created by subvec Created: 03/May/11  Updated: 23/May/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Alexander Redington Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File CLJ-787-p1.patch    
Patch: Code and Test
Approval: Incomplete

 Description   

Subvectors created with subvec from a PersistentVector cannot be made transient:

user=> (transient (subvec [1 2 3 4] 2))
ClassCastException clojure.lang.APersistentVector$SubVector cannot be cast to clojure.lang.IEditableCollection  clojure.core/transient (core.clj:2864)

Cause: APersistentVector$SubVector does not implement IEditableCollection

Patch: CLJ-787-p1.patch

Approach: Create a TransientSubVector based on an underlying TransientVector.

Two assumptions:

  • It's okay for TransientSubVector to delegate the ensureEditable functionality to the underlying TransientVector (sometimes explicitly, sometimes implicitly) - calling ensureEditable explicitly also requires that the field for the underlying vector be the concrete TransientVector type rather than the ITransientVector interface.
  • When an operation that would throw an exception on a PersistentVector happens from the wrong thread (or after persistent!), we throw that exception rather than the IllegalAccessError that transients throw when accessed inappropriately.


 Comments   
Comment by Stuart Sierra [ 31/May/11 9:28 AM ]

Confirmed. APersistentVector$SubVector does not implement IEditableCollection.

The current implementation of TransientVector depends on implementation details of PersistentVector, so it is not a trivial fix. The simplest fix might be to implement IEditableCollection.asTransient in SubVector by creating a new PersistentVector, but I do not know the performance implications.

Comment by Gary Fredericks [ 25/May/13 8:11 PM ]

We could get the same performance characteristics as SubVector by creating a TransientSubVector based on an underlying TransientVector, right?

Preparing a patch to that effect.

Comment by Gary Fredericks [ 25/May/13 10:58 PM ]

Text from the commit msg:

Made two assumptions:

  • It's okay for TransientSubVector to delegate the ensureEditable
    functionality to the underlying TransientVector (sometimes
    explicitely, sometimes implicitely) – calling ensureEditable
    explicitely also requires that the field for the underlying vector
    be the concrete TransientVector type rather than the
    ITransientVector interface.
  • When an operation that would throw an exception on a
    PersistentVector happens from the wrong thread (or after
    persistent!), we throw that exception rather than the
    IllegalAccessError that transients throw when accessed
    inappropriately.
Comment by Alex Miller [ 11/Oct/13 4:17 PM ]

I think there are some assumptions being made in this patch about the class structure here that do not hold. The structure is, admittedly, quite twisty.

A counter-example that highlights one of a few subtypes of APersistentVector that are not PersistentVector (like MapEntry):

user=> (transient (subvec (first {:a 1}) 0 1))
ClassCastException clojure.lang.MapEntry cannot be cast to clojure.lang.IEditableCollection  clojure.lang.APersistentVector$TransientSubVector.<init> (APersistentVector.java:592)

PersistentVector.SubVector expects to work on anything that implements IPersistentVector. Note that this includes concrete types such as MapEntry and LazilyPersistentVector, but could also be any user-implemented type IPersistentVector type too. TransientSubVector is making the assumption that the IPersistentVector in a SubVector question is also an IEditableCollection (that can be converted to be transient). Note that while PersistentVector implements TransientVector (and IEditableCollection), APersistentVector does not. To really implement this in tandem with SubVector, I think you would need to guarantee that IPersistentVector extended IEditableCollection and I don't think that's something we want to do.

I don't see an easy solution. Any time I see all these modifiers (Transient, Sub, etc) being created in different combinations, it is a clear sign that independent kinds of functionality are being remixed into single inheritance OO trees. You can see the same thing in most collection libraries (even Java's - need a ConcurrentIdentitySortedMap? too bad!).

Needs more thought.

Comment by Andy Fingerhut [ 08/Nov/13 10:17 AM ]

Patch CLJ-787-p1.patch no longer applies cleanly to latest master, but it is only because of some new tests added to the transients.clj file since the patch was created, so it is trivial to update in that sense. Not updating it now due to other more significant issues with the patch described above.

Comment by Alex Miller [ 17/Jan/14 10:19 AM ]

No good solution to consider right now, removing from 1.6.





[CLJ-740] Unnecessary boxing of primitives in case form Created: 17/Feb/11  Updated: 01/Mar/11

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Mikhail Kryshen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Found this while profiling some performance-critical code.

Consider the following Clojure function:

(defn test-case ^double [^long i ^double d1 ^double d2]
  (case (int i)
    0 d1
    d2))

Current Clojure 1.3 snapshot compiles it to:

public final double invokePrim(long, double, double)   throws java.lang.Exception;
  Code:
   0:	lload_1
   1:	invokestatic	#67; //Method clojure/lang/RT.intCast:(J)I
   4:	istore	7
   6:	iload	7
   8:	i2l
   9:	invokestatic	#73; //Method clojure/lang/Numbers.num:(J)Ljava/lang/Number;
   12:	invokestatic	#79; //Method clojure/lang/Util.hash:(Ljava/lang/Object;)I
   15:	iconst_0
   16:	ishr
   17:	iconst_1
   18:	iand
   19:	tableswitch{ //0 to 0
		0: 36;
		default: 58 }
   36:	iload	7
   38:	i2l
   39:	invokestatic	#73; //Method clojure/lang/Numbers.num:(J)Ljava/lang/Number;
   42:	getstatic	#45; //Field const__3:Ljava/lang/Object;
   45:	invokestatic	#83; //Method clojure/lang/Util.equals:(Ljava/lang/Object;Ljava/lang/Object;)Z
   48:	ifeq	58
   51:	dload_3
   52:	invokestatic	#88; //Method java/lang/Double.valueOf:(D)Ljava/lang/Double;
   55:	goto	63
   58:	dload	5
   60:	invokestatic	#88; //Method java/lang/Double.valueOf:(D)Ljava/lang/Double;
   63:	checkcast	#92; //class java/lang/Number
   66:	invokevirtual	#96; //Method java/lang/Number.doubleValue:()D
   69:	dreturn
}

This bytecode contains boxing of primitives (calls to clojure/lang/Numbers.num and java/lang/Double.valueOf) and calls to clojure/lang/Util.hash and clojure/lang/Util.equals that does not seem necessary.

At 60-66 primitive double is boxed into Double only to be converted back into primitive.

The equivalent Java code compiles to much simpler and faster bytecode:

public double testCase(long, double, double);
  Code:
   0:	lload_1
   1:	l2i
   2:	lookupswitch{ //1
		0: 20;
		default: 22 }
   20:	dload_3
   21:	dreturn
   22:	dload	5
   24:	dreturn
}


 Comments   
Comment by Alexander Taggart [ 28/Feb/11 2:16 PM ]

Improved via patch on CLJ-426.

(defn test-case ^double [^long i ^double d1 ^double d2]
  (case (int i)
    0 d1
    d2))

now emits as

 0  lload_1 [i]
 1  invokestatic clojure.lang.RT.intCast(long) : int [67]
 4  istore 7 [G__7903] // let-bound expression
 6  iload 7 [G__7903]
 8  tableswitch default: 32
      case 0: 28
28  dload_3 [d2]
29  goto 34
32  dload 5 [arg2]
34  dreturn

or if the int cast of the expression is omitted:

 0  lload_1 [i]
 1  lstore 7 [G__7903] // let-bound expression
 3  lload 7 [G__7903]
 5  l2i
 6  tableswitch default: 35
      case 0: 24
24  lconst_0           // match, verify long expr wasn't truncated
25  lload 7 [G__7903]
27  lcmp
28  ifne 35
31  dload_3 [d2]
32  goto 37
35  dload 5 [arg2]
37  dreturn




[CLJ-735] Improve error message when a protocol method is not found Created: 04/Feb/11  Updated: 28/Sep/13

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: errormsgs

Attachments: File protocolerr.diff    
Patch: Code
Approval: Triaged

 Description   

If you call a protocol function but pass the wrong arity (forget an argument for example), you currently a message that says "No single method ... of interface ... found for function ... of protocol ...". The code in question is getting matching methods from the Reflector and creates this message if the number of matches != 1.

There are really two cases there:

  • matches == 0 - this happens frequently due to typos
  • matches > 1 - this presumably happens infrequently

I propose that the == 0 case instead should have slightly different text at the beginning and a hint as to the intended arity within it:

"No method: ... of interface ... with arity ... found for function ... of protocol ...".

The >1 case should have similar changes: "Multiple methods: ... of interface ... with arity ... found for function ... of protocol ...".

Patch is attached. I used case which presumably should have better performance than a nested if/else. I was not sure whether the reported arity should match the actual Java method arity or Clojure protocol function arity (including the target). I did the former.

I did not add a test as I wasn't sure whether checking error messages in tests was appropriate or not. Happy to add that if requested.



 Comments   
Comment by Chas Emerick [ 14/Jul/11 6:39 AM ]

I was not sure whether the reported arity should match the actual Java method arity or Clojure protocol function arity (including the target). I did the former.

I think it should be the latter. The message is emitted when the protocol methods are being invoked through the corresponding function, so it should be consistent with the errors emitted by regular functions.

+1 for some tests, too. There certainly are tests for reflection warnings and such.

FWIW, I'm happy to take this on if Alex is otherwise occupied.





[CLJ-705] Make sorted maps and sets implement j.u.SortedMap and SortedSet interfaces Created: 05/Jan/11  Updated: 02/Jun/12

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Rich Hickey Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Comments   
Comment by Andy Fingerhut [ 02/Jun/12 2:29 PM ]

This might be a duplicate of CLJ-248. See that one before working on this one, at least.





[CLJ-701] Compiler loses 'loop's return type in some cases Created: 03/Jan/11  Updated: 26/Jul/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Backlog
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Chouser Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Clojure commit 9052ca1854b7b6202dba21fe2a45183a4534c501, version 1.3.0-master-SNAPSHOT


Attachments: File hoistedmethod-pass-1.diff     File hoistedmethod-pass-2.diff     File hoistedmethod-pass-3.diff     File hoistedmethod-pass-4.diff     File hoistedmethod-pass-5.diff    
Patch: Code
Approval: Vetted

 Description   
(set! *warn-on-reflection* true)
(fn [] (loop [b 0] (recur (loop [a 1] a))))

Generates the following warnings:

recur arg for primitive local: b is not matching primitive, had: Object, needed: long
Auto-boxing loop arg: b

This is interesting for several reasons. For one, if the arg to recur is a let form, there is no warning:

(fn [] (loop [b 0] (recur (let [a 1] a))))

Also, the compiler appears to understand the return type of loop forms just fine:

(use '[clojure.contrib.repl-utils :only [expression-info]])
(expression-info '(loop [a 1] a))
;=> {:class long, :primitive? true}

The problem can of course be worked around using an explicit cast on the loop form:

(fn [] (loop [b 0] (recur (long (loop [a 1] a)))))

Reported by leafw in IRC: http://clojure-log.n01se.net/date/2011-01-03.html#10:31



 Comments   
Comment by a_strange_guy [ 03/Jan/11 4:36 PM ]

The problem is that a 'loop form gets converted into an anonymous fn that gets called immediately, when the loop is in a expression context (eg. its return value is needed, but not as the return value of a method/fn).

so

(fn [] (loop [b 0] (recur (loop [a 1] a))))

gets converted into

(fn [] (loop [b 0] (recur ((fn [] (loop [a 1] a))))))

see the code in the compiler:
http://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L5572

this conversion already bites you if you have mutable fields in a deftype and want to 'set! them in a loop

http://dev.clojure.org/jira/browse/CLJ-274

Comment by Christophe Grand [ 23/Nov/12 2:28 AM ]

loops in expression context are lifted into fns because else Hotspot doesn't optimize them.
This causes several problems:

  • type inference doesn't propagate outside of the loop[1]
  • the return value is never a primitive
  • mutable fields are inaccessible
  • surprise allocation of one closure objects each time the loop is entered.

Adressing all those problems isn't easy.
One can compute the type of the loop and emit a type hint but it works only with reference types. To make it works with primitive, primitie fns aren't enough since they return only long/double: you have to add explicit casts.
So solving the first two points can be done in a rather lccal way.
The two other points require more impacting changes, the goal would be to emit a method rather than a fn. So it means at the very least changing ObjExpr and adding a new subclassof ObjMethod.

[1] beware of CLJ-1111 when testing.

Comment by Alex Miller [ 21/Oct/13 10:28 PM ]

I don't think this is going to make it into 1.6, so removing the 1.6 tag.

Comment by Kevin Downey [ 21/Jul/14 7:14 PM ]

an immediate solution to this might be to hoist loops out in to distinct non-ifn types generated by the compiler with an invoke method that is typed to return the getJavaClass() type of the expression, that would give us the simplifying benefits of hoisting the code out and free use from the Object semantics of ifn

Comment by Kevin Downey [ 22/Jul/14 8:39 PM ]

I have attached a 3 part patch as hoistedmethod-pass-1.diff

3ed6fed8 adds a new ObjMethod type to represent expressions hoisted out in to their own methods on the enclosing class

9c39cac1 uses HoistedMethod to compile loops not in the return context

901e4505 hoists out try expressions and makes it possible for try to return a primitive expression (this might belong on http://dev.clojure.org/jira/browse/CLJ-1422)

Comment by Kevin Downey [ 22/Jul/14 8:54 PM ]

with hoistedmethod-pass-1.diff the example code generates bytecode like this

user=> (println (no.disassemble/disassemble (fn [] (loop [b 0] (recur (loop [a 1] a))))))
// Compiled from form-init1272682692522767658.clj (version 1.5 : 49.0, super bit)
public final class user$eval1675$fn__1676 extends clojure.lang.AFunction {
  
  // Field descriptor #7 Ljava/lang/Object;
  public static final java.lang.Object const__0;
  
  // Field descriptor #7 Ljava/lang/Object;
  public static final java.lang.Object const__1;
  
  // Method descriptor #10 ()V
  // Stack: 2, Locals: 0
  public static {};
     0  lconst_0
     1  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [16]
     4  putstatic user$eval1675$fn__1676.const__0 : java.lang.Object [18]
     7  lconst_1
     8  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [16]
    11  putstatic user$eval1675$fn__1676.const__1 : java.lang.Object [20]
    14  return
      Line numbers:
        [pc: 0, line: 1]

 // Method descriptor #10 ()V
  // Stack: 1, Locals: 1
  public user$eval1675$fn__1676();
    0  aload_0 [this]
    1  invokespecial clojure.lang.AFunction() [23]
    4  return
      Line numbers:
        [pc: 0, line: 1]
  
  // Method descriptor #25 ()Ljava/lang/Object;
  // Stack: 3, Locals: 3
  public java.lang.Object invoke();
     0  lconst_0
     1  lstore_1 [b]
     2  aload_0 [this]
     3  lload_1 [b]
     4  invokevirtual user$eval1675$fn__1676.__hoisted1677(long) : long [29]
     7  lstore_1 [b]
     8  goto 2
    11  areturn
      Line numbers:
        [pc: 0, line: 1]
      Local variable table:
        [pc: 2, pc: 11] local: b index: 1 type: long
        [pc: 0, pc: 11] local: this index: 0 type: java.lang.Object

 // Method descriptor #27 (J)J
  // Stack: 2, Locals: 5
  public long __hoisted1677(long b);
    0  lconst_1
    1  lstore_3 [a]
    2  lload_3
    3  lreturn
      Line numbers:
        [pc: 0, line: 1]
      Local variable table:
        [pc: 2, pc: 3] local: a index: 3 type: long
        [pc: 0, pc: 3] local: this index: 0 type: java.lang.Object
        [pc: 0, pc: 3] local: b index: 1 type: java.lang.Object

}
nil
user=> 
  

the body of the method __hoisted1677 is the inner loop

for reference the part of the bytecode from the same function compiled with 1.6.0 is pasted here https://gist.github.com/hiredman/f178a690718bde773ba0 the inner loop body is missing because it is implemented as its own IFn class that is instantiated and immediately executed. it closes over a boxed version of the numbers and returns an boxed version

Comment by Kevin Downey [ 23/Jul/14 12:43 AM ]

hoistedmethod-pass-2.diff replaces 901e4505 with f0a405e3 which fixes the implementation of MaybePrimitiveExpr for TryExpr

with hoistedmethod-pass-2.diff the largest clojure project I have quick access to (53kloc) compiles and all the tests pass

Comment by Alex Miller [ 23/Jul/14 12:03 PM ]

Thanks for the work on this!

Comment by Kevin Downey [ 23/Jul/14 2:05 PM ]

I have been working through running the tests for all the contribs projects with hoistedmethod-pass-2.diff, there are some bytecode verification errors compiling data.json and other errors elsewhere, so there is still work to do

Comment by Kevin Downey [ 25/Jul/14 7:08 PM ]

hoistedmethod-pass-3.diff

49782161 * add HoistedMethod to the compiler for hoisting expresssions out well typed methods
e60e6907 * hoist out loops if required
547ba069 * make TryExpr MaybePrimitive and hoist tries out as required

all contribs whose tests pass with master pass with this patch.

the change from hoistedmethod-pass-2.diff in this patch is the addition of some bookkeeping for arguments that take up more than one slot

Comment by Nicola Mometto [ 26/Jul/14 1:37 AM ]

Kevin there's still a bug regarding long/doubles handling:
On commit 49782161, line 101 of the diff, you're emitting gen.pop() if the expression is in STATEMENT position, you need to emit gen.pop2() instead if e.getReturnType is long.class or double.class

Test case:

user=> (fn [] (try 1 (finally)) 2)
VerifyError (class: user$eval1$fn__2, method: invoke signature: ()Ljava/lang/Object;) Attempt to split long or double on the stack  user/eval1 (NO_SOURCE_FILE:1)
Comment by Kevin Downey [ 26/Jul/14 1:46 AM ]

bah, all that work to figure out the thing I couldn't get right and of course I overlooked the thing I knew at the beginning. I want to get rid of some of the code duplication between emit and emitUnboxed for TryExpr, so when I get that done I'll fix the pop too

Comment by Kevin Downey [ 26/Jul/14 12:52 PM ]

hoistedmethod-pass-4.diff logically has the same three commits, but fixes the pop vs pop2 issue and rewrites emit and emitUnboxed for TryExpr to share most of their code

Comment by Kevin Downey [ 26/Jul/14 12:58 PM ]

hoistedmethod-pass-5.diff fixes a stupid mistake in the tests in hoistedmethod-pass-4.diff





[CLJ-700] contains? broken for transient collections Created: 01/Jan/11  Updated: 27/Jun/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: None

Attachments: Java Source File 0001-Refactor-of-some-of-the-clojure-.java-code-to-fix-CL.patch     File clj-700-7.diff     File clj-700.diff     Text File clj-700-patch4.txt     Text File clj-700-patch6.txt    
Patch: Code and Test
Approval: Vetted

 Description   

Behavior with Clojure 1.6.0:

user=> (contains? (transient {:x "fine"}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentArrayMap$TransientArrayMap  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient (hash-map :x "fine")) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashMap$TransientHashMap  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient [1 2 3]) 0)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentVector$TransientVector  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient #{:x}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashSet$TransientHashSet  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (:x (transient #{:x}))
nil
;; expected: :x

user=> (get (transient #{:x}) :x)
nil
;; expected: :x

Behavior with latest Clojure master as of Jun 27 2014 (same as Clojure 1.6.0) plus patch clj-700-7.diff. In all cases it matches the expected results shown in comments above:

user=> (contains? (transient {:x "fine"}) :x)
true
user=> (contains? (transient (hash-map :x "fine")) :x)
true
user=> (contains? (transient [1 2 3]) 0)
true
user=> (contains? (transient #{:x}) :x)
true
user=> (:x (transient #{:x}))
:x
user=> (get (transient #{:x}) :x) 
:x

Analysis by Alexander Redington: This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Questions on this approach from Stuart Halloway to Rich Hickey:

1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?

2. what are good names for the interfaces introduced here?

Alex Miller: Should also keep an eye on CLJ-787 as it may have some collisions with this one.

Patch: clj-700-7.diff

One 'trailing whitespace' warning is perfectly normal when applying this patch to latest Clojure master as of Jun 27 2014, as shown below. This is simply because of carriage returns at the end of lines in file Associative.java. I know of no way to avoid such a warning without removing CRs from all Clojure source files (e.g. CLJ-1026):

% git am -s --keep-cr --ignore-whitespace < ~/clj/patches/clj-700-7.diff 
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/admin/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq


 Comments   
Comment by Herwig Hochleitner [ 01/Jan/11 8:01 PM ]

the same is also true for TransientVectors

{{(contains? (transient [1 2 3]) 0)}}

false

Comment by Herwig Hochleitner [ 01/Jan/11 8:25 PM ]

As expected, TransientSets have the same issue; plus an additional, probably related one.

(:x (transient #{:x}))

nil

(get (transient #{:x}) :x)

nil

Comment by Alexander Redington [ 07/Jan/11 2:07 PM ]

This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Comment by Stuart Halloway [ 28/Jan/11 10:35 AM ]

Rich: Patch doesn't currently apply, but I would like to get your take on approach here. In particular:

  1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?
  2. what are good names for the interfaces introduced here?
Comment by Alexander Redington [ 25/Mar/11 7:44 AM ]

Rebased the patch off the latest pull of master as of 3/25/2011, it should apply cleanly now.

Comment by Stuart Sierra [ 17/Feb/12 2:59 PM ]

Latest patch does not apply as of f5bcf647

Comment by Andy Fingerhut [ 17/Feb/12 5:59 PM ]

clj-700-patch2.txt does patch cleanly to latest Clojure head as of a few mins ago. No changes to patch except in context around changed lines.

Comment by Andy Fingerhut [ 07/Mar/12 3:23 AM ]

Sigh. Git patches applied via 'git am' are fragile beasts indeed. Look at them the wrong way and they fail to apply.

clj-700-patch3.txt applies cleanly to latest master as of Mar 7, 2012, but not if you use this command:

git am -s < clj-700-patch3.txt

I am pretty sure this is because of DOS CR/LF line endings in the file src/jvm/clojure/lang/Associative.java. The patch does apply cleanly if you use this command:

git am --keep-cr -s < clj-700-patch3.txt

Comment by Andy Fingerhut [ 23/Mar/12 6:34 PM ]

This ticket was changed to Incomplete and waiting on Rich when Stuart Halloway asked for feedback on the approach on 28/Jan/2011. Stuart Sierra changed it to not waiting on Rich on 17/Feb/2012 when he noted the patch didn't apply cleanly. Latest patch clj-700-patch3.txt does apply cleanly, but doesn't change the approach used since the time Stuart Halloway's concern was raised. Should it be marked as waiting on Rich again? Something else?

Comment by Stuart Halloway [ 08/Jun/12 12:44 PM ]

Patch 4 incorporates patch 3, and brings it up to date on hashing (i.e. uses hasheq).

Comment by Andy Fingerhut [ 08/Jun/12 12:52 PM ]

Removed clj-700-patch3.txt in favor of Stuart Halloway's improved clj-700-patch4.txt dated June 8, 2012.

Comment by Andy Fingerhut [ 18/Jun/12 3:06 PM ]

clj-700-patch5.txt dated June 18, 2012 is the same as Stuart Halloway's clj-700-patch4.txt, except for context lines that have changed in Clojure master since Stuart's patch was created. clj-700-patch4.txt no longer applies cleanly.

Comment by Andy Fingerhut [ 19/Aug/12 4:47 AM ]

Adding clj-700-patch6.txt, which is identical to Stuart Halloway's clj-700-patch4.txt, except that it applies cleanly to latest master as of Aug 19, 2012. Note that as described above, you must use the --keep-cr option to 'git am' when applying this patch for it to succeed. Removing clj-700-patch5.txt, since it no longer applies cleanly.

Comment by Stuart Sierra [ 24/Aug/12 1:08 PM ]

Patch fails as of commit 1c8eb16a14ce5daefef1df68d2f6b1f143003140

Comment by Andy Fingerhut [ 24/Aug/12 1:53 PM ]

Which patch did you try, and what command did you use? I tried applying clj-700-patch6.txt to the same commit, using the following command, and it applied, albeit with the warning messages shown:

% git am --keep-cr -s < clj-700-patch6.txt
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/jafinger/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq

Note the --keep-cr option, which is necessary for this patch to succeed. It is recommended in the "Screening Tickets" section of the JIRA workflow wiki page here: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Andy Fingerhut [ 28/Aug/12 5:48 PM ]

Presumptuously changing Approval from Incomplete back to None, since the latest patch does apply cleanly if the --keep-cr option is used. It was in Screened state recently, but I'm not so presumptuous as to change it to Screened

Comment by Alex Miller [ 19/Aug/13 12:26 PM ]

I think through a series of different hands on this ticket it got knocked way back in the list. Re-marking vetted as it's previously been all