<< Back to previous view

[CLJ-1472] The locking macro fails bytecode verification on ART runtime Created: 23/Jul/14  Updated: 23/Jul/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: 0
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.






[CLJ-1471] Option to print type info Created: 21/Jul/14  Updated: 21/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Pascal Germroth Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: pprint


 Description   

I've had an issue with defrecord-types being converted into ordinary maps somewhere, which was relatively hard to track down inside a deep structure since they are pprinted as the same thing by default.
The following code patches into the pprint dispatch and prints the type around values; it turned out to be quite useful, but feels hackish.
Maybe something like that would be useful to integrate into clojure.pprint directly (there are a number of cosmetic options already), i.e. into clojure.pprint/write-out.

Only printing (type) may not be enough in some cases; so an option to print all metadata would be nice.
Maybe something like :metadata nil as default, :metadata :type to print types (but also for non-IMetas, using (type) and :metadata true to print metadata for IMetas using (meta).

(defn pprint-with-type
  ([object] (pprint object *out*))
  ([object writer]
   ; keep original dispatch.
   ; calling it directly will print only that object,
   ; but return to our dispatch for subobjects.
   (let [dispatch clojure.pprint/*print-pprint-dispatch*]
     (binding [clojure.pprint/*print-pprint-dispatch*
               (fn [obj]
                 (if (instance? clojure.lang.IMeta obj)
                   (do (print "^{:type ")
                       (dispatch (type obj))
                       (print "} ")
                       (clojure.pprint/pprint-newline :fill)
                       (dispatch obj))
                   (do (print "(^:type ")
                       (dispatch (type obj))
                       (print " ")
                       (clojure.pprint/pprint-newline :fill)
                       (dispatch obj)
                       (print ")"))))]
       (clojure.pprint/pprint object writer)))))





[CLJ-1470] Make Atom and ARef easy to subclass Created: 20/Jul/14  Updated: 23/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Aaron Craelius Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJ-1470-v1.patch    
Patch: Code

 Description   

Atom is currently defined as final and ARef.validate() is package-private. This makes it impossible to define a subclass of an Atom and difficult subclass ARef (if validate() needs to be called).

I propose removing the final modifier from Atom, making ARef.validate() protected and also making Atom.state protected (it is currently package-private).

I'm not sure if there is a specific reason why Atom is final - if this is for performance reasons or to prevent someone from doing strange things with Atom's, but I can see a use case for sub-classing it.

One use-case is to create reactive Atom that allows derefs to be tracked (as in reagent). I have some Clojure (not Clojurescript) code where I'm trying to play with this idea and I've had to copy the entire Atom class (because it's sealed) and place it in the clojure.lang package (because ARef.validate() is package-private): https://github.com/aaronc/freactive/blob/master/src/java/clojure/lang/ReactiveAtom.java. In addition, I need to copy the defns for swap! and reset! into my own namespace. This seems a bit inconvenient.



 Comments   
Comment by Kevin Downey [ 23/Jul/14 12:55 AM ]

related to http://dev.clojure.org/jira/browse/CLJ-803





[CLJ-1469] Emit KeywordInvoke callsites only when keyword is not namespaced Created: 18/Jul/14  Updated: 18/Jul/14

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

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

Attachments: Text File kwinvoke.patch    
Patch: Code

 Description   

KeywordInvoke callsites exist to fastpath lookups into records, but involve a lot of callsite machinery. Records don't have any fastpath for namespaced keyword accesses. This patch just falls back to an InvokeExpr when the keyword access is namespaced.

(:ns/kw foo)






[CLJ-1468] Add deep-merge and deep-merge-with Created: 18/Jul/14  Updated: 18/Jul/14  Resolved: 18/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Stuart Sierra Assignee: Unassigned
Resolution: Declined Votes: 1
Labels: None

Attachments: Text File CLJ-1468-deep-merge-01.patch    
Patch: Code and Test
Approval: Triaged

 Description   

When dealing with nested map structures, one often wants to merge two maps recursively.

The deep-merge-with function was originally written by Chris Houser for clojure.contrib.map-utils but was not maintained after clojure-contrib was split into separate modules.

deep-merge and deep-merge-with are widely copied, usually with the same implementation, in utility libraries. For example:



 Comments   
Comment by Rich Hickey [ 18/Jul/14 11:42 AM ]

Vague semantics and docs strings





[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-1466] clojure.core/bean should implement Iterable Created: 16/Jul/14  Updated: 16/Jul/14

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

Type: Defect Priority: Minor
Reporter: Ambrose Bonnaire-Sergeant Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: File iterable-bean-v2.diff    
Approval: Triaged

 Description   

The changes in Clojure 1.6 hashing revealed that `bean` does not return a map that implements Iterable:

user=> (hash (bean (java.util.Date.)))

AbstractMethodError clojure.lang.APersistentMap.iterator()Ljava/util/Iterator;  clojure.core.proxy$clojure.lang.APersistentMap$ff19274a.iterator (:-1)

Patch adds `iterator` method to clojure.core/bean.



 Comments   
Comment by Alex Miller [ 16/Jul/14 10:22 AM ]

One workaround:

(hash (apply hash-map (bean (java.util.Date.))))

Interestingly, into does not help b/c into uses reduce, which internally uses the iterator too.

Comment by Alex Miller [ 16/Jul/14 11:01 AM ]

APersistentMap implements Iterable and expects subclasses to fulfill that contract. The bean proxy does not. Instead of changing APersistentMap, why not add:

(iterator [] (.iterator pmap)

to the bean proxy definition?

Comment by Ambrose Bonnaire-Sergeant [ 16/Jul/14 11:19 AM ]

It seemed like an oversight that APersistentMap lacked a default iterator method.

That said, I haven't used OO inheritance for 4 years. Should I change the patch?

Comment by Ambrose Bonnaire-Sergeant [ 16/Jul/14 11:47 AM ]

Added new patch that just adds iterator to bean.





[CLJ-1465] SubVector leaks memory by design (?) Created: 14/Jul/14  Updated: 14/Jul/14  Resolved: 14/Jul/14

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

Type: Defect Priority: Minor
Reporter: Szymon Witamborski Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: memory


 Description   

While reading through the code of SubVector class, I've noticed that the only thing that it does is creating a "subview" of an existing vector without doing anything to elements that are not accessible any more. Please correct me if I'm wrong but I think this leads to memory leaks in this scenario:

(let [a [1 2 3 4 5]
      b (subvec a 2 3)]
  b)
[3]

In this example we no longer have any reference to a once the (let...) expression returned. Elements of a that are no longer accessible will not be garbage collected until b is garbage collected because b still holds a reference to a (the v field in SubVector class). Ideally, these elements should be garbage collectible as soon as a is gone.



 Comments   
Comment by Alex Miller [ 14/Jul/14 2:40 PM ]

The subvec docstring says:

"Returns a persistent vector of the items in vector from start (inclusive) to end (exclusive). If end is not supplied, defaults to (count vector). This operation is O(1) and very fast, as the resulting vector shares structure with the original and no trimming is done."

The implementation is intentional to make this a constant-time operation. If you are willing to make the tradeoff re shared structure and object retention, this constant operation has better performance. In other words: working as intended.





[CLJ-1464] Incorrectly named parameter to fold function in reducers.clj Created: 12/Jul/14  Updated: 14/Jul/14

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

Type: Defect Priority: Trivial
Reporter: Jason Jackson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Rename-a-function-parameter-to-reflect-the-fold-func.patch    
Patch: Code
Approval: Triaged

 Description   

https://github.com/clojure/clojure/blob/master/src/clj/clojure/core/reducers.clj#L95
The 2-arity fold accepts reducef as parameter and then uses it as a combinef.
Instead it should accept combinef as parameter and then use it as a reducef, as every combine fn (monoid) is a reduce fn, but not every reduce fn is a combine fn (it's not associative).



 Comments   
Comment by Jason Jackson [ 12/Jul/14 2:58 PM ]

this is my first patch for clojure please double check everything. CA is done.

Comment by Andy Fingerhut [ 12/Jul/14 7:29 PM ]

Everything gets double checked whether it is your first patch or your 50th

At least as far as the format of the patch being correct, that it applies cleanly to the latest version of Clojure on the master branch, compiles and passes all tests cleanly, all of that is good.

Whether there is interest in taking your proposed change is to be decided by others. It may be some time before it is examined further.

Comment by Jozef Wagner [ 13/Jul/14 4:11 AM ]

This is not a defect. Quoting Rich, "If no combining fn is supplied, the reducing fn is used." (source)

There are three user supplied operations in fold: getting identity element (combinef :: -> T), reducing function (reducef :: T * E -> T) and combining function (combinef :: T * T -> T). For reduce, combining function is not needed but the rest two operations are needed. Thus reducing function (reducef) supplies identity element for reducers and only in folders the identity element is produced by combining function. In case where reducing fn is used for both reducing and combining, it must of course be associative and must handle objects of types T and E as a second argument.

Comment by Jason Jackson [ 14/Jul/14 12:14 AM ]

@Jozef I appreciate the feedback I still think my patch is correct, although I admit everyone's time is better spent not debating this small refactoring so feel free to close it.

One perspective to view my patch from is if we had protocols p/Monoid and p/Reducer. It's possible to reify any object that implements p/Monoid into p/Reducer, but not the other way around since not every p/Reducer is associative.

Comment by Jason Jackson [ 14/Jul/14 12:30 AM ]

From that perspective you could also say that in the 2-arity case the parameter "reducef" requires objects that implement both p/Monoid and p/Reducer, but in the 3-arity case the parameter "reducef" only requires p/Reducer

Comment by Jozef Wagner [ 14/Jul/14 1:38 AM ]

Note that reducef and combinef take different type of second argument, so not every combining function can be used as a reducing one. Your proposal is thus no better than the status quo. Consider following example:

(fold clojure.set/union conj [1 1 2 2 3 3 4 4])




[CLJ-1463] Providing own ClassLoader for eval is broken Created: 10/Jul/14  Updated: 10/Jul/14

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

Type: Defect Priority: Minor
Reporter: Volkert Oakley Jurgens Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler
Environment:

Clojure 1.6.0



 Description   

clojure.lang.Compiler has a method with the signature

public static Object eval(Object form, boolean freshLoader)

but the freshLoader argument is ignored since https://github.com/clojure/clojure/commit/2c2ed386ed0f6f875342721bdaace908e298c7f3

Is there a good reason this still needs to be "hotfixed" like this?

We would like to provide our own ClassLoader for eval to manage the lifecycle of the generated classes.






[CLJ-1462] cl-format throws ClassCastException: Writer cannot be cast to Future/IDeref Created: 07/Jul/14  Updated: 09/Jul/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: Defect Priority: Minor
Reporter: Pascal Germroth Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print


 Description   

Using ~I and ~_ etc fails in many situations, the most trivial one being:

Clojure 1.6.0 and 1.5.1:

user=> (clojure.pprint/cl-format true "~I")
ClassCastException java.io.PrintWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)
user=> (clojure.pprint/cl-format nil "~I")
ClassCastException java.io.StringWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)
user=> (clojure.pprint/cl-format nil "~_")
ClassCastException java.io.StringWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)

Clojure 1.4.0

user=> (clojure.pprint/cl-format true "~I")
ClassCastException java.io.OutputStreamWriter cannot be cast to clojure.lang.IDeref  clojure.core/deref (core.clj:2080)
user=> (clojure.pprint/cl-format nil "~I")
ClassCastException java.io.StringWriter cannot be cast to clojure.lang.IDeref  clojure.core/deref (core.clj:2080)
user=> (clojure.pprint/cl-format nil "~_")
ClassCastException java.io.StringWriter cannot be cast to clojure.lang.IDeref  clojure.core/deref (core.clj:2080)

These work in other implementations, i.e. clisp, creating empty output in these trivial cases:

> (format t "~I")
NIL
> (format nil "~I")
""
> (format nil "~_")
""


 Comments   
Comment by Andy Fingerhut [ 07/Jul/14 11:01 AM ]

The tilde-underscore sequence is for "conditional newline", according to the CLHS here: http://www.lispworks.com/documentation/lw51/CLHS/Body/22_cea.htm

Tilde-capital-letter-I is for indent: http://www.lispworks.com/documentation/lw51/CLHS/Body/22_cec.htm

Comment by Pascal Germroth [ 07/Jul/14 12:09 PM ]

Ah, didn't think to try that. It fails without cl-format as well:

user=> (clojure.pprint/pprint-newline :linear)
ClassCastException java.io.PrintWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)
user=> (clojure.pprint/pprint-indent :block 0)
ClassCastException java.io.PrintWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)

Manually creating a pretty writer does work though:

user=> (binding [*out* (clojure.pprint/get-pretty-writer *out*)] (clojure.pprint/pprint-newline :linear))
nil

In the get-pretty-writer doc it says:

Generally, it is unnecessary to call this function, since pprint,
write, and cl-format all call it if they need to.

Which appears to not be true for cl-format, and it would be nice if it would be applied automatically for all functions that need a pretty writer.

Comment by Pascal Germroth [ 09/Jul/14 6:37 PM ]

More bad news!
Manually creating a pretty-writer doesn't do the trick either, because it is not being properly flushed:

user=> (binding [*out* (get-pretty-writer *out*)] (cl-format true "hello ~_world~%"))
hello world
nil
user=> (binding [*out* (get-pretty-writer *out*)] (cl-format true "hello ~_world"))
hellonil
user=> (binding [*out* (get-pretty-writer *out*)] (cl-format true "hello ~_world") (.ppflush *out*))
hello worldnil

The ~% inserts an unconditional newline like \n, which also works as expected.

Insert ~_ before and it only prints up to that one. But I've also managed to get it to abort at other ~_ s, maybe because other commands flushed it.

Manually flushing it, like the inexplicably private with-pretty-writer macro does works though.
I don't understand why get-pretty-writer is exposed but not the macro that is needed to use it properly. Also all functions using pretty-writer facilities should use with-pretty-writer, that's what it appears to be specifically designed for. Then there's no need to expose it (or get-pretty-writer).





[CLJ-1461] print-dup is broken for some clojure collections Created: 06/Jul/14  Updated: 06/Jul/14

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Approval: Triaged

 Description   
user=> (print-dup (sorted-set 1) *out*)
#=(clojure.lang.PersistentTreeSet/create [1])nil
user=> (read-string (with-out-str (print-dup (sorted-set 1) *out*)))
ClassCastException Cannot cast clojure.lang.PersistentVector to clojure.lang.ISeq  java.lang.Class.cast (Class.java:3258)
user=> (print-dup (subvec [1] 0) *out*)
#=(clojure.lang.APersistentVector$SubVector/create [1])nil
user=> (read-string (with-out-str (print-dup (subvec [1] 0) *out*)))
IllegalArgumentException No matching method found: create  clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:53)

print-dup assumes all IPersistentCollections not defined via defrecord have a static /create method that take an IPersistentCollection, but this is not true for many clojure collections






[CLJ-1460] Clojure transforms literals of custom IPersistentCollections not created via deftype/defrecord to their generic clojure counterpart Created: 06/Jul/14  Updated: 06/Jul/14

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Approval: Triaged

 Description   
user=> (class (eval (sorted-map 1 1)))
clojure.lang.PersistentArrayMap ;; expected: clojure.lang.PersistentTreeMap


 Comments   
Comment by Alex Miller [ 06/Jul/14 5:35 PM ]

Seems related to CLJ-1093.

Comment by Nicola Mometto [ 06/Jul/14 5:51 PM ]

The symptoms are indeed similar but there are differences: CLJ-1093 affects all empty IPersistentCollections, this one affects all {ISeq,IPersistentList,IPersistentMap,IPersistentVector,IPersistentSet} collections that are not IRecord/IType.





[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-1458] Use transients in merge and merge-with Created: 04/Jul/14  Updated: 04/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: newbie, performance

Approval: Triaged

 Description   

It would be nice if merge used transients.






[CLJ-1457] once the compiler pops the dynamic classloader from the stack, attempts to read record reader literals will fail Created: 30/Jun/14  Updated: 01/Jul/14

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

Type: Defect Priority: Minor
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

reproduction case

java -jar target/clojure-1.7.0-master-SNAPSHOT.jar -e "(do (ns foo.bar) (defrecord Foo []) (defn -main [] (prn (->Foo)) (read-string \"#foo.bar.Foo[]\")))" -m foo.bar

result

#'foo.bar/-main
#foo.bar.Foo{}
Exception in thread "main" java.lang.ClassNotFoundException: foo.bar.Foo
	at java.net.URLClassLoader$1.run(URLClassLoader.java:372)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:361)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.net.URLClassLoader.findClass(URLClassLoader.java:360)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:308)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:340)
	at clojure.lang.RT.classForNameNonLoading(RT.java:2076)
	at clojure.lang.LispReader$CtorReader.readRecord(LispReader.java:1195)
	at clojure.lang.LispReader$CtorReader.invoke(LispReader.java:1164)
	at clojure.lang.LispReader$DispatchReader.invoke(LispReader.java:609)
	at clojure.lang.LispReader.read(LispReader.java:183)
	at clojure.lang.RT.readString(RT.java:1737)
	at clojure.core$read_string.invoke(core.clj:3497)
	at foo.bar$_main.invoke(NO_SOURCE_FILE:1)
	at clojure.lang.Var.invoke(Var.java:375)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.core$apply.invoke(core.clj:624)
	at clojure.main$main_opt.invoke(main.clj:315)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at clojure.lang.Var.invoke(Var.java:394)
	at clojure.lang.AFn.applyToHelper(AFn.java:165)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)

what happens is the evaluator pushes a dynamicclassloader, evaluates some code, then -m foo.bar causes foo.bar/-main to be called, which tries to read in a literal for the just defined record, but it fails because when foo.bar/-main is called clojure.lang.Compiler/LOADER is unbound so RT uses the sun.misc classloader to try and find the class, which it knows nothing about



 Comments   
Comment by Kevin Downey [ 01/Jul/14 11:42 AM ]

this means that you cannot depend on ever being able to deserialize a record with read unless you are at the repl (the only place clojure.lang.Compiler/LOADER is guaranteed to be bound).

1. print/read support for records is broken
2. behavior is inconsistent between the repl and other environments
which will drive people crazy when the try to figure out why their code isn't working

Comment by Alex Miller [ 01/Jul/14 4:43 PM ]

I would appreciate more understanding about how this affects code run in a more normal scenario (than calling clojure.main with -e and -m).





[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-1455] Postcondition in defrecord: Compiler unable to resolve symbol % Created: 28/Jun/14  Updated: 29/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: defrecord


 Description   

Clojure's postconditions[1] are a splendiferous, notationally
idiot-proof way to scrutinize a function's return value without
inadvertently causing it to return something else.

Functions (implementing protocols) for a record type may be defined in
its defrecord or with extend-type. In functions defined in
extend-type, postconditions work as expected. Therefore, it is a
surprise that functions defined in defrecord cannot use
postconditions.

Actually it appears defrecord sees a pre/postcondition map as ordinary
code, so the postcondition runs at the beginning of the function (not
the end) and the symbol % (for return value) is not bound.

The code below shows a protocol and two record types that implement
it. Type "One" has an in-the-defrecord function definition where the
postcondition does not compile. Type "Two" uses extend-type and the
postcondition works as expected.

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(defprotocol ITimesThree
  (x3 [a]))

;; defrecord with functions inside cannot use postconditions.
(defrecord One
    []
  ITimesThree
  (x3 [a]
    {:pre [(do (println "One x3 pre") 1)] ;; (works fine)
     :post [(do (println "One x3 post, %=" %) 1)]
     ;; Unable to resolve symbol: % in this context.
     ;; With % removed, it compiles but runs at start, not end.
     }
    (* 1 3)))

;; extend-type can add functions with postconditions to a record.
(defrecord Two
    [])
(extend-type Two
  ITimesThree
  (x3 [a]
    {:pre [(do (println "Two x3 pre") 1)] ;; (works fine)
     :post [(do (println "Two x3 post, %=" %) 1)] ;; (works fine)
     }
    (* 2 3)))

(defn -main
  "Main"
  []
  (println (x3 (->One)))
  (println (x3 (->Two))))

[1] http://clojure.org/special_forms, in the fn section.






[CLJ-1454] Companion to swap! which returns the old value Created: 28/Jun/14  Updated: 30/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Philip Potter Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: atom

Approval: Triaged

 Description   

Sometimes, when mutating an atom, it's desirable to know what the value before the swap happened. The existing swap! function returns the new value, so is unsuitable for this use case. Currently, the only option is to roll your own using a loop and compare-and-set!

An example of this would be where the atom contains a PersistentQueue and you want to atomically remove the head of the queue and process it: if you run (swap! a pop), you have lost the reference to the old head of the list so you can't process it.

It would be good to have a new function swap-returning-old! which returned the old value instead of the new.



 Comments   
Comment by Philip Potter [ 28/Jun/14 4:00 PM ]

Overtone already defines functions like this in overtone.helpers.ref, which get used by overtone.libs.event. These return both the old and the new value, although in all existing use cases only the old value gets used.

flatland/useful defines a trade! fn which returns the old value, although the implementation is less clean than a compare-and-set! based solution would be.

Comment by Philip Potter [ 29/Jun/14 6:23 AM ]

Chris Ford suggested "swap-out!" as a name for this function. I definitely think "swap-returning-old!" isn't the ideal name.

Comment by Jozef Wagner [ 30/Jun/14 1:33 AM ]

I propose a switch! name. The verb switch is defined as "substitute (two items) for each other; exchange.", and as you get the old value back, it evokes slightly the exchange of items.

Comment by Philip Potter [ 30/Jun/14 3:03 AM ]

Medley also has a deref-swap! which does the same thing.

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

I think deref-swap! seems like a morally equivalent name to Java's AtomicReference.getAndSet() which is the same idea.

Comment by Philip Potter [ 30/Jun/14 1:19 PM ]

Funny you say that Alex, because prismatic/plumbing defines a get-and-set! (also defined by other projects), equivalent to deref-reset! in medley. Plumbing also defines swap-pair! which returns both old and new values, like the overtone fn, although once again the only usage I can find only uses the old value.

Comment by Alex Miller [ 30/Jun/14 3:37 PM ]

I think it's important to retain the notion that you are not switching/exchanging values but applying the update model of applying a function to the old value to produce the new value. I don't even particularly like "swap!" as I think that aspect is lost in the name (alter and alter-var-root are better). I like that "deref-swap!" combines two words with existing connotations and orders them appropriately.

Comment by Timothy Baldridge [ 30/Jun/14 3:43 PM ]

except that that naming doesn't fit well compared to functions like nfirst which are defined as (comp next first). This function is not (comp deref swap!).





[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-1452] clojure.core/*rand* for seedable randomness Created: 20/Jun/14  Updated: 01/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: math

Attachments: Text File CLJ-1452.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Clojure's random functions currently use Math.random and related features, which makes them impossible to seed. This seems like an appropriate use of a dynamic var (compared to extra arguments), since library code that wants to behave randomly could transparently support seeding without any extra effort.

I propose (def ^:dynamic *rand* (java.util.Random.)) in clojure.core, and that rand, rand-int, rand-nth, and shuffle be updated to use *rand*.

I think semantically this will not be a breaking change.

Criterium Benchmarks

I did some benchmarking to try to get an idea of the performance implications of using a dynamic var, as well as to measure the changes to concurrent access.

The code used is at https://github.com/gfredericks/clj-1452-tests; the output and interpretation from the README are reproduced below for posterity.

rand is slightly slower, while shuffle is insignificantly faster. Using shuffle from 8 threads is insignificantly slower, but switching to a ThreadLocalRandom manually in the patched version results in a 2.5x speedup.

Running on my 8 core Linode VM:

$ echo "Clojure 1.6.0"; lein with-profile +clj-1.6 run; echo "Clojure 1.6.0 with *rand*"; lein with-profile +clj-1452 run
Clojure 1.6.0

;;;;;;;;;;;;;;;;;;
;; Testing rand ;;
;;;;;;;;;;;;;;;;;;
WARNING: Final GC required 1.261632096547911 % of runtime
Evaluation count : 644646900 in 60 samples of 10744115 calls.
             Execution time mean : 61.297605 ns
    Execution time std-deviation : 7.057249 ns
   Execution time lower quantile : 56.872437 ns ( 2.5%)
   Execution time upper quantile : 84.483045 ns (97.5%)
                   Overhead used : 16.319772 ns

Found 6 outliers in 60 samples (10.0000 %)
    low-severe   1 (1.6667 %)
    low-mild     5 (8.3333 %)
 Variance from outliers : 75.5119 % Variance is severely inflated by outliers

;;;;;;;;;;;;;;;;;;;;;
;; Testing shuffle ;;
;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 4780800 in 60 samples of 79680 calls.
             Execution time mean : 12.873832 µs
    Execution time std-deviation : 251.388257 ns
   Execution time lower quantile : 12.526871 µs ( 2.5%)
   Execution time upper quantile : 13.417559 µs (97.5%)
                   Overhead used : 16.319772 ns

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

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Testing threaded-shuffling ;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 420 in 60 samples of 7 calls.
             Execution time mean : 150.863290 ms
    Execution time std-deviation : 2.313755 ms
   Execution time lower quantile : 146.621548 ms ( 2.5%)
   Execution time upper quantile : 155.218897 ms (97.5%)
                   Overhead used : 16.319772 ns
Clojure 1.6.0 with *rand*

;;;;;;;;;;;;;;;;;;
;; Testing rand ;;
;;;;;;;;;;;;;;;;;;
Evaluation count : 781707720 in 60 samples of 13028462 calls.
             Execution time mean : 63.679152 ns
    Execution time std-deviation : 1.798245 ns
   Execution time lower quantile : 61.414851 ns ( 2.5%)
   Execution time upper quantile : 67.412204 ns (97.5%)
                   Overhead used : 13.008428 ns

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

;;;;;;;;;;;;;;;;;;;;;
;; Testing shuffle ;;
;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 4757940 in 60 samples of 79299 calls.
             Execution time mean : 12.780391 µs
    Execution time std-deviation : 240.542151 ns
   Execution time lower quantile : 12.450218 µs ( 2.5%)
   Execution time upper quantile : 13.286910 µs (97.5%)
                   Overhead used : 13.008428 ns

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

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Testing threaded-shuffling ;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 420 in 60 samples of 7 calls.
             Execution time mean : 152.471310 ms
    Execution time std-deviation : 8.769236 ms
   Execution time lower quantile : 147.954789 ms ( 2.5%)
   Execution time upper quantile : 161.277200 ms (97.5%)
                   Overhead used : 13.008428 ns

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

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Testing threaded-local-shuffling ;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 960 in 60 samples of 16 calls.
             Execution time mean : 64.462853 ms
    Execution time std-deviation : 1.407808 ms
   Execution time lower quantile : 62.353265 ms ( 2.5%)
   Execution time upper quantile : 67.197368 ms (97.5%)
                   Overhead used : 13.008428 ns

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


 Comments   
Comment by Gary Fredericks [ 21/Jun/14 7:50 PM ]

Attached CLJ-1452.patch, with the same code used in the benchmarks.

Comment by Gary Fredericks [ 23/Jun/14 8:34 AM ]

Should we be trying to make Clojure's random functions thread-local by default while we're mucking with this stuff? We could have a custom subclass of Random that has ThreadLocal logic in it (avoiding ThreadLocalRandom because Java 6), and make that the default value of *rand*.





[CLJ-1451] Add take-until Created: 20/Jun/14  Updated: 22/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Alexander Taggart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJ-1451-drop-until.patch     Text File CLJ-1451-take-until.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Discussion: https://groups.google.com/d/topic/clojure-dev/NaAuBz6SpkY/discussion

It comes up when I would otherwise use (take-while pred coll), but I need to include the first item for which (pred item) is false.

(take-while pos? [1 2 0 3]) => (1 2)
(take-until zero? [1 2 0 3]) => (1 2 0)

Impl:

(defn take-until
  "Returns a lazy sequence of successive items from coll until
  (pred item) returns true, including that item. pred must be
  free of side-effects."
  [pred coll]
  (lazy-seq
    (when-let [s (seq coll)]
      (if (pred (first s))
        (cons (first s) nil)
        (cons (first s) (take-until pred (rest s)))))))

List of other suggested names: take-upto, take-to, take-through. It is not easy to find something in English that is short and unambiguously means "up to and including". That is one of the dictionary definitions for "through".



 Comments   
Comment by Alex Miller [ 20/Jun/14 10:21 AM ]

Patch welcome (w/tests).

Comment by Alexander Taggart [ 20/Jun/14 2:00 PM ]

Impl and tests for take-until and drop-until, one patch for each.

Comment by Jozef Wagner [ 20/Jun/14 3:01 PM ]

Please change :added metadata to "1.7".

Comment by Alexander Taggart [ 20/Jun/14 3:12 PM ]

Updated to :added "1.7"

Comment by John Mastro [ 21/Jun/14 6:26 PM ]

I'd like to propose take-through and drop-through as alternative names. I think "through" communicates more clearly how these differ from take-while and drop-while.





[CLJ-1450] Add a variant of range that's inclusive with respect to its upper boundary Created: 19/Jun/14  Updated: 19/Jun/14  Resolved: 19/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Such a variant of range might behave like this:

```
(irange 1 5)
;;=> (1 2 3 4 5)

(irange 1 2 5)
;;=> (1 3 5)
```

I'm suggesting this because often in practice we have to deal with inclusive ranges and having to add 1 to the upper boundary in such scenarios seems to reduce the clarity of the code. I guess something like this applies to some extend to lower boundary as well:

```
(erange 1 5)
;;=> (2 3 4)
```



 Comments   
Comment by Alex Miller [ 19/Jun/14 2:55 PM ]

We're not interested in adding these to core, thanks.





[CLJ-1449] Add starts-with? and ends-with? to clojure.string Created: 19/Jun/14  Updated: 20/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: string

Approval: Triaged

 Description   

Add clojure.string/starts-with? and ends-with? similar to java.lang.String's startsWith/endsWith. In addition to making these easier to find and use, this provides a place to add a portable ClojureScript variant.



 Comments   
Comment by Alex Miller [ 19/Jun/14 12:53 PM ]

Re substring, there is a clojure.core/subs for this (predates the string ns I believe).

clojure.core/subs
([s start] [s start end])
Returns the substring of s beginning at start inclusive, and ending
at end (defaults to length of string), exclusive.

Comment by Jozef Wagner [ 20/Jun/14 3:21 AM ]

As strings are collection of characters, you can use Clojure's sequence facilities to achieve such functionality:

user=> (= (first "asdf") \a)
true
user=> (= (last "asdf") \a)
false
Comment by Alex Miller [ 20/Jun/14 8:33 AM ]

Jozef, String.startsWith() checks for a prefix string, not just a prefix char.

Comment by Bozhidar Batsov [ 20/Jun/14 9:42 AM ]

Re substring, I know about subs, but it seems very odd that it's not in the string ns. After all most people will likely look for string-related functionality in clojure.string. I think it'd be best if `subs` was added to clojure.string and clojure.core/subs was deprecated.





[CLJ-1448] Suggest alength in error message on attempt to access array length via .length Created: 19/Jun/14  Updated: 19/Jun/14  Resolved: 19/Jun/14

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

Type: Enhancement Priority: Trivial
Reporter: Colin Taylor Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: errormsgs

Attachments: Text File dot-length-recommend-alength.patch    
Patch: Code

 Description   

Problem:

Newcomers are easily confused by the inability to access <array>.length via (.length <array>).

Approach:

Append to invalid field access message, a suggestion to use alength for this specific case (class.isArray and field = "length")

user=> (.length (int-array 2))
IllegalArgumentException No matching field found: length for class [I, use alength function for array length


 Comments   
Comment by Alex Miller [ 19/Jun/14 2:56 PM ]

We don't have any plans to add anything this specific to the error check, thanks.





[CLJ-1447] Make proxy work with protocols directly (like reify does) Created: 18/Jun/14  Updated: 18/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Currently Proxy only supports interfaces and abstract classes. While protocols are supported via the protocol's interface, this means that the method names must be java mangled. E.g. the method name for set-value! becomes set_value_BANG_. However, the only possible way to subclass abstract classes in Clojure is currently via gen-class (doesn't work from the REPL) or proxy.






[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-1445] pprint prints some metadata when *print-meta* bound to true, but not all 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: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: File clj-1445-workaround-v2.clj    

 Description   

Short example illustrating the behavior:

user=> *clojure-version*
{:major 1, :minor 6, :incremental 0, :qualifier nil}

user=> (def f1 '(defn foo [^Integer x] ^{:bar 8} (inc x)))
#'user/f1

;; pr shows all metadata, as expected

user=> (binding [*print-meta* true] (pr f1))
^{:line 2, :column 10} (defn foo [^Integer x] ^{:bar 8, :line 2, :column 33} (inc x))nil

;; pprint shows some metadata, but not all

user=> (binding [*print-meta* true] (clojure.pprint/pprint f1))
(defn foo [^Integer x] (inc x))
nil

I have not dug into the details yet, but it appears that this may be because pprint uses pr to show symbols, but not to show collections. Thus pprint shows metadata on symbols, but not collections.

It would be nice if pprint could instead show all metadata, as pr does, when print-meta is bound to true.



 Comments   
Comment by Andy Fingerhut [ 13/Jun/14 11:30 AM ]

Attached file clj-1445-workaround-v1.clj is a function that pprints with more metadata than clojure.pprint does. As noted in the comments, it may not show metadata on other metadata. Please update with an enhanced version if you create one.

Comment by Andy Fingerhut [ 13/Jun/14 12:26 PM ]

Attached file clj-1445-workaround-v2.clj supersedes the earlier one, which I will delete.

The included function pprint-meta appears to be a correct way to pprint values with all metadata, even if the metadata maps themselves have metadata on them.





[CLJ-1444] Fix unquote splicing for empty seqs Created: 11/Jun/14  Updated: 11/Jun/14

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reader, syntax-quote

Attachments: Text File 0001-Fix-unquote-splicing-for-empty-seqs.patch    
Patch: Code and Test

 Description   

Current behaviour:

user=> `(~@())
nil
user=> `[~@()]
[]

Expected behaviour:

user=> `(~@())
()
user=> `[~@()]
[]





[CLJ-1443] reduce docstring partly incorrect with reducers. Created: 10/Jun/14  Updated: 10/Jun/14

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

Type: Defect Priority: Minor
Reporter: Greg Chapman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, reducers


 Description   

The docstring for reduce includes this: "If val is not supplied, returns the result of applying f to the first 2 items in coll". This is true if coll is a sequence, but not if it is a reducer. For example:

user=> (->> (range 0 10 2) (reduce (fn[x y] (+ x y))))
20
user=> (->> (range 0 10 2) (r/map #(/ % 2)) (reduce (fn[x y] (+ x y))))
ArityException Wrong number of args (0)

The docstring should be updated to make it clear that reducers (used without an initial seed value) require the reducing function to support a 0 arity overload returning the identity value for the reduction operation.






[CLJ-1442] Tag gensym sourced symbols with metadata Created: 09/Jun/14  Updated: 07/Jul/14

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

Type: Enhancement Priority: Trivial
Reporter: Reid McKenzie Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File 0001-Annotate-generated-symbols-with-metadata.patch     Text File 0001-Annotate-generated-symbols-with-metadata.patch    

 Description   

For static analysis tools derived from TANAL it is frequently useful to determine whether a symbol is user defined or the result of code generation. As tools analyzer depends on the Clojure core for evaluation and symbol generation a user wishing to annotate generated symbols must currently provide a binding replacing clojure.core/gensym with a snippet equivalent to the following patch. Such overloading is not appropriate for TANAL, TE* or user code as it is a redefinition of clojure.core behavior which should be standard rather than subjected to users with crowbars.



 Comments   
Comment by Gary Trakhman [ 09/Jun/14 2:11 PM ]

This could eventually help with filtering out def'd symbols like 't131045 coming from reify in CLJS. I've been seeing this behavior with core.async namespaces in an autodoc-cljs proof-of-concept, which could eventually target tools.analyzer.

Comment by Alex Miller [ 09/Jun/14 2:57 PM ]

Re the patch, why not call the Symbol constructor that takes meta instead of with-meta? For performance, it might also be useful to use the same constant map as well.

Comment by Reid McKenzie [ 09/Jun/14 3:10 PM ]

Because the compiler will emit the meta map as a static field the patch as-is will share the same map instance between all annotated symbols. Calling the metadata constructor is reasonable, I'll update the patch.

Comment by Reid McKenzie [ 09/Jun/14 3:28 PM ]

So the metadata constructor of Symbol is private, see https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Symbol.java#L100. Without changing this directly constructing symbols with metadata is not possible from the core. If you're worried about escaping the var indirection cost of adding metadata via with-meta inlining with-meta is an option, however then we're building two symbols for no good reason. Exposing the currently private metadata constructor is probably the right fix, abet its own ticket.

Comment by Andy Fingerhut [ 01/Jul/14 6:41 PM ]

From the comments above it appears that this is not planned to be a final version of this patch, but FYI some automated scripts I have found that patch 0001-Annotate-generated-symbols-with-metadata.patch dated Jun 9 2014 applies cleanly to the latest Clojure master as of Jul 1 2014, but Clojure fails to build.

Comment by Reid McKenzie [ 02/Jul/14 1:37 AM ]

Thanks Andy, I'll rework and test it in the morning

Comment by Reid McKenzie [ 07/Jul/14 9:49 AM ]

Because of the work that clojure.lang.Symbol/intern does, exposing and using the metadata constructor directly makes no sense. The updated patch directly invokes clojure.lang.Symbol/withMeta rather than indirecting through clojure.core/with-meta and taking the performance hit of calling through a Var. Builds cleanly on my system.





[CLJ-1441] Provide docs on how to reference imports that conflict with default ns class imports Created: 07/Jun/14  Updated: 07/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: documentation


 Description   

This is related to CLJ-1440; a name clash on class "Compiler" between clojure.lang and another package.

The documentation does not address how to handle this cleanly; specifically, refer would appear to allow a way to exclude clojure.lang.Compiler, but does not.



 Comments   
Comment by Alex Miller [ 07/Jun/14 7:56 PM ]

refer is all about symbols that refer to Var. refer's docstring seems pretty clear on that to me.

Your conflict is on symbols that refer to a Class, which is the domain of import and has no exclusion facilities. The set of default imports is defined in RT.DEFAULT_IMPORTS and includes clojure.lang.Compiler along with everything in java.lang.*.

You can always fully-qualify any class you want to use in your ns, so that is one workaround available. Another is what Nicola suggested in CLJ-1440 - post-modify the ns after load.

Either ns or import could theoretically document more explicitly the list of auto-imports and recommend a solution to this problem. I'm not sure whether this is worth doing or would be accepted given the infrequency of the use case and availability of workarounds.

I tweaked http://clojure.org/namespaces to mention this.





[CLJ-1440] Unable to exclude clojure.lang.Compiler using :refer-clojure Created: 06/Jun/14  Updated: 07/Jun/14  Resolved: 07/Jun/14

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

Type: Defect Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: compiler, interop


 Description   
(ns io.aviso.twixt.js-minification
  "Provides support for JavaScript minification using the Google Closure compiler."
  (:refer-clojure :exclude [Compiler])
  (:import (com.google.javascript.jscomp CompilerOptions ClosureCodingConvention DiagnosticGroups CheckLevel
                                         SourceFile Result Compiler))
  (:require [clojure.java.io :as io]
            [io.aviso.twixt.utils :as utils]
            [io.aviso.tracker :as t]
            [clojure.string :as str]))

Results in:

clojure.lang.Compiler$CompilerException: java.lang.IllegalStateException: Compiler already refers to: class clojure.lang.Compiler in namespace: io.aviso.twixt.js-minification, compiling:(/Users/hlship/workspaces/annadale/twixt/src/io/aviso/twixt/js_minification.clj:1:1)
        java.lang.IllegalStateException: Compiler already refers to: class clojure.lang.Compiler in namespace: io.aviso.twixt.js-minification
                                     clojure.lang.Namespace.referenceClass                    Namespace.java:  140
                                        clojure.lang.Namespace.importClass                    Namespace.java:  158
                                        clojure.lang.Namespace.importClass                    Namespace.java:  164
                   io.aviso.twixt.js-minification/eval4104/loading--auto--               js_minification.clj:    1
                                   io.aviso.twixt.js-minification/eval4104               js_minification.clj:    1
                                                clojure.lang.Compiler.eval                     Compiler.java: 6703
                                                clojure.lang.Compiler.eval                     Compiler.java: 6692
                                                clojure.lang.Compiler.load                     Compiler.java: 7130
                                   io.aviso.twixt.js-minification/eval4100  form-init4106199735960171933.clj:    1
                                                clojure.lang.Compiler.eval                     Compiler.java: 6703
                                                clojure.lang.Compiler.eval                     Compiler.java: 6666
                                                         clojure.core/eval                          core.clj: 2927
                                      clojure.main/repl/read-eval-print/fn                          main.clj:  239
                                         clojure.main/repl/read-eval-print                          main.clj:  239
                                                      clojure.main/repl/fn                          main.clj:  257
                                                         clojure.main/repl                          main.clj:  257
                                                clojure.lang.RestFn.invoke                       RestFn.java: 1096
             clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn            interruptible_eval.clj:   56
                                            clojure.lang.AFn.applyToHelper                          AFn.java:  152
                                                  clojure.lang.AFn.applyTo                          AFn.java:  144
                                                        clojure.core/apply                          core.clj:  624
                                               clojure.core/with-bindings*                          core.clj: 1862
                                                clojure.lang.RestFn.invoke                       RestFn.java:  425
                clojure.tools.nrepl.middleware.interruptible-eval/evaluate            interruptible_eval.clj:   41
clojure.tools.nrepl.middleware.interruptible-eval/interruptible-eval/fn/fn            interruptible_eval.clj:  171
                                                      clojure.core/comp/fn                          core.clj: 2402
             clojure.tools.nrepl.middleware.interruptible-eval/run-next/fn            interruptible_eval.clj:  138
                                                      clojure.lang.AFn.run                          AFn.java:   22
                         java.util.concurrent.ThreadPoolExecutor.runWorker           ThreadPoolExecutor.java: 1145
                        java.util.concurrent.ThreadPoolExecutor$Worker.run           ThreadPoolExecutor.java:  615
                                                      java.lang.Thread.run                       Thread.java:  724


 Comments   
Comment by Nicola Mometto [ 06/Jun/14 4:52 PM ]

refer and thus refer-clojure only works for Vars.
a workaround is:

(ns ..)
(ns-unmap *ns* 'Compiler)
(import 'com.google.javascript.jscomp.Compiler)
Comment by Alex Miller [ 07/Jun/14 11:34 AM ]

Ditto what Nicola said. Or just fully-qualify.





[CLJ-1439] Reduce keyword string interning cost Created: 05/Jun/14  Updated: 05/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Kyle Kingsbury Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: keywords, performance

Attachments: Text File 0001-Improve-Keyword.intern-performance.patch    
Patch: Code
Approval: Vetted

 Description   

Breaking dev.clojure.org/jira/browse/CLJ-1415 into a separate ticket because reasons






[CLJ-1438] bit-* functions don't check for overflow Created: 05/Jun/14  Updated: 05/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Pascal Germroth Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: math


 Description   

The bit* functions, in contrast to the other numerical functions, don't appear to check for overflow, i.e. (bit-test 13 200000) returns true.

It would be nice if the behaviour would fit the other numerical operators, i.e. throw on overflow and provide a variant that doesn't, and one that works with arbitrary precision, also not currently supported:
(bit-test (bigint 13) 20000), (bit-test (biginteger 13) 20000) throw IllegalArgumentException.






[CLJ-1437] Keyword cache lookup could be faster Created: 05/Jun/14  Updated: 05/Jun/14  Resolved: 05/Jun/14

Status: Closed
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: Duplicate Votes: 0
Labels: keywords, performance


 Description   

Breaking this piece out from CLJ-1415. Keyword cache lookup includes symbol creation and interning which could be avoided, making cache lookup faster.






[CLJ-1436] Deref throws an unhelpful error message when used on something not dereferencable Created: 03/Jun/14  Updated: 03/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Phillip Lord Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs

Approval: Triaged

 Description   

Consider the following code:

(def x 1)
(def y (ref 2))

(+ @x y)

Clojure throws a ClassCastException on cast to Future. This is a very unhelpful error message; why a Future, why not Ref, Atom etc. It would be nice if this failed more gracefully.






[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-1434] The doc string for `trampoline` suggests that it applies to mutual recursion in general. It doesn't: it applies to mutual *tail* recursion only. Created: 29/May/14  Updated: 29/May/14

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

Type: Task Priority: Trivial
Reporter: Colin Hastie Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, patch,
Environment:

Any.



 Description   

The doc-string for `trampoline` starts "trampoline can be used to convert algorithms requiring mutual
recursion without stack consumption. ". This is inaccurate: `trampoline` applies only to mutual tail recursion.

Replace this with "trampoline can be used to convert algorithms employing mutual tail recursion into a form that does not consume stack".






[CLJ-1433] proxy-super calls generally use reflection Created: 28/May/14  Updated: 28/May/14

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

Type: Defect Priority: Minor
Reporter: Greg Chapman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: typehints


 Description   

For example:

user=> (proxy [java.util.BitSet] []
  (flip [bitIndex]
    (proxy-super flip bitIndex)))
Reflection warning, NO_SOURCE_PATH:73:5 - call to method flip can't be resolved (target class is unknown).

I believe this issue might be fixed by simply adding type-hint metadata to the 'this symbol emitted by the proxy macro. I have not tried this change, but this macro seems to indicate it should work:

(defmacro proxy-super-cls [cls meth & args]
  (let [thissym (with-meta (gensym) {:tag cls})]
    `(let [~thissym ~'this]
      (proxy-call-with-super (fn [] (. ~thissym ~meth ~@args)) ~thissym ~(name meth))
    )))
;;;;;;;;;;;;;;;;;;;;;;
user=> (proxy [java.util.BitSet] []
  (flip [bitIndex]
    (proxy-super-cls java.util.BitSet flip bitIndex)))
#<BitSet$ff19274a {}>





[CLJ-1432] NullPointerException on function with primitive result declaration Created: 26/May/14  Updated: 30/May/14

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

Type: Enhancement Priority: Minor
Reporter: Gunnar Völkel Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: primitives, typehints


 Description   

The following minimal example shows the error:

(defn f ^double [])
(f)
=> NullPointerException

When decompiling the function `f` I found the following return expression:

return null.doubleValue();

This happened in a Java interop scenario where the called Java method had no return value but was in the return position of the primitive Clojure function.
The compiler should check for `null` on compilation.

Another example - calling a method with void return as the last expression fails in a similar way:

(defn f ^double [^SomeClassToAvoidRuntimeReflection obj, x, y]
  (.someMethod obj, x, y))
(f obj, x, y)
=> NullPointerException


 Comments   
Comment by Alex Miller [ 26/May/14 11:19 PM ]

What do you expect to happen in this case? You declared a function as returning a double but didn't return one.

Comment by Gunnar Völkel [ 27/May/14 8:48 AM ]

Since this is only the minimal example the error is relatively easy to spot.
Consider the following small example with Java interop:

(defn f ^double [^SomeClassToAvoidRuntimeReflection obj, x, y]
  (.someMethod obj, x, y))
(f obj, x, y)
=> NullPointerException

In this example it is much harder to find the reason for the NPE because you'd first suspect `obj` to be `null`.

I expect a check in the compiler at the point where "return null.doubleValue();" is emitted, followed by an error message, e.g. "Primitive return value of type 'double' expected, but no value is returned.".

Comment by Jozef Wagner [ 28/May/14 2:15 AM ]

Your second example seems perfectly OK to me, compiler should not report any error and NPE check must be at runtime.

Comment by Gunnar Völkel [ 28/May/14 2:46 AM ]

@Jozef: No, you are wrong. The compiler infers via reflection at compile time that the called method does not return a value and emits "return null.doubleValue()". So this can and should be reported as explicit error at compile time. I added a typehint to make it clear that there is no runtime reflection involved.
You would be right, if the compiler emitted something like "return somevar.doubleValue();" because then at compile time there is no knowledge about a possible "null" value.

Comment by Andy Fingerhut [ 28/May/14 10:00 AM ]

Gunnar, in your example, is the method 'someMethod' declared to return void, or something else? Adding that info to your example might help clarify it.

Comment by Jozef Wagner [ 29/May/14 2:26 AM ]

Gunnar, the second example was ambiguous and strayed away the discussion. Anyway, whether returning wrong type is through the native method or not, it is a user error in the first place. Right now it is reported at runtime. For me this ticket should be a minor enhancement instead of defect.

Comment by Gunnar Völkel [ 30/May/14 4:40 AM ]

Yes, the reason is a user error. But one that is harder to debug than necessary.
Also, it is clearly a defect since emitting 'null.doubleValue()' can not be considered as a valid compilation.

Andy, yes 'someMethod' is declared to return void. I'd edit the original ticket text to add the example and the java method return value information, but it seems jira does not let me.

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

I added the second example (with clarifying void comment) to the description.





[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-1430] Improve performance of partial Created: 23/May/14  Updated: 02/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance

Attachments: File partial-perf.diff    
Patch: Code
Approval: Screened

 Description   

This patch improves performance of partial by only using apply when needed. The code structure follows that of juxt.

Performance benchmark:

(ns partial-test.core
  (:require [criterium.core :refer [bench]])
  (:gen-class))

(defn -main []
  (let [f (partial + 1 1)]
    (println "Starting")
    (bench (f 1 1))
    (println "Done")))

Results for 1.6.0:

Evaluation count : 228751140 in 60 samples of 3812519 calls.
             Execution time mean : 266.700063 ns
    Execution time std-deviation : 2.966851 ns
   Execution time lower quantile : 262.641023 ns ( 2.5%)
   Execution time upper quantile : 274.207916 ns (97.5%)
                   Overhead used : 1.610513 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

Results for 1.7.0 with this patch:

 Evaluation count : 348208140 in 60 samples of 5803469 calls.
              Execution time mean : 171.210533 ns
     Execution time std-deviation : 2.011660 ns
    Execution time lower quantile : 168.819526 ns ( 2.5%)
    Execution time upper quantile : 176.015584 ns (97.5%)
                    Overhead used : 2.644128 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

Benchmarks performed via lein uberjar + running via the commandline.

Patch: partial-perf.diff

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 23/May/14 10:46 AM ]

Screened, looks as expected.

Comment by Andy Fingerhut [ 02/Jun/14 10:50 AM ]

Timothy, just a nit that I would not have noticed except for my program that checks for name and email address of patch authors, to see if they are on my contributor's list, but do you really have both of the email addresses tbaldridge@gmail.com and tbaldidge@gmail.com (note the spelling difference)? The latter is the one on this patch.

Comment by Timothy Baldridge [ 02/Jun/14 11:04 AM ]

fixed email

Comment by Timothy Baldridge [ 02/Jun/14 11:05 AM ]

nice catch! it was a typeo in my .gitconfig defaults. I've fixed the patch.

Comment by Alex Miller [ 02/Jun/14 11:19 AM ]

Tim (and anyone really) - please let someone know if you need to change a screened patch! Looks fine here, but screener should be notified so they can re-screen.





[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-1428] restart-agent is ignored inside an fn passed to set-error-handler. Created: 19/May/14  Updated: 19/May/14

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

Type: Defect Priority: Minor
Reporter: Rafik NACCACHE Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: agents
Environment:

Linux, jdk 1.7, emacs / cider



 Description   

If I pass a function containing start-agent to set-error-handler of an agent, if an exception occurs the restart-agent is ignored.
for example:

(def a (agent 0))

(set-error-handler! a (fn [the-agent the-exception] (restart-agent the-agent)) )

If I now issue : (send! a #(/ 1 0)), I still have a failed agent. It did not restart.

I know I can set the error-mode to the agent to :continue to have my agent up after a crash, but I wished I could fix the conditions that caused the exception in the first-place then restart the agent programmatically in the set-error-handler.

Maybe it is a known beahviour, but then it is not documented ?






[CLJ-1427] restart-agent is ignored inside an fn passed to set-agent-handler. It shall maybe documented ? Created: 19/May/14  Updated: 20/May/14  Resolved: 20/May/14

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

Type: Defect Priority: Minor
Reporter: Rafik NACCACHE Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None
Environment:

Linux, jdk 1.7, emacs / cider



 Description   

If I pass a function containing start-agent to set-error-handler of an agent, if an exception occurs the restart-agent is ignored.
for example:

(def a (agent 0))

(set-error-handler! a (fn [the-agent the-exception] (restart-agent the-agent)) )

If I now issue : (send! a #(/ 1 0)), I still have a failed agent. It did not restart.

I know I can set the error-mode to the agent to :continue to someone have my agent up after a crash, but I wished I could fix the conditions that caused the exception and restart the agent programmatically in the set-error-handler.

Maybe it is a known feature, but it is not documented.



 Comments   
Comment by Rafik NACCACHE [ 20/May/14 11:15 AM ]

Sorry, I probably hit return while typing, this one is duplicate of clj-1428





[CLJ-1426] "/" in keyword leads to unexpected behavior when calling `name` Created: 18/May/14  Updated: 21/May/14  Resolved: 21/May/14

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

Type: Defect Priority: Minor
Reporter: Darrell Hamilton Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Because `clojure.lang.Keyword` delegates it's `getName` functionality to `clojure.lang.Symbol`, there is some unexpected behavior when calling `name` on a keyword that contains a "/" in it. For example:

(name (keyword "foo/bar"))
=> "bar"

This is due to `Symbol` stripping all namespace qualifiers and considering only the content trailing the "/" as part of the name.



 Comments   
Comment by Darrell Hamilton [ 20/May/14 9:29 PM ]

Totally misunderstood the behavior of namespaced keywords. this can be closed.

/derp





[CLJ-1425] Defer literal map construction of syntax-quoted maps to allow for semantically valid unquote splicing Created: 16/May/14  Updated: 17/May/14

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

Type: Enhancement Priority: Minor
Reporter: Jon Distad Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reader
Environment:

Any


Attachments: Text File 0001-Fix-map-unquote-splicing.patch    
Patch: Code and Test

 Description   

At present one cannot unquote-splice into a map literal unless the map contains an even number of literal forms, even if one of them is a null unquote (~@[]).

E.g.: `{~@[1 2]} ;=> RuntimeException Map literal must contain an even number of forms clojure.lang.Util.runtimeException (Util.java:219)

However, within the context of a syntax-quote, it is not essential that the map literal be represented internally as a map since the syntax-quote emits code to build the map and not the map itself. The syntaxQuote method on SyntaxQuoteReader does not even operate the map, but rather a flattened sequence of interleaved keys and values.

With the aid of metadata and a LispReader-global Var, we can track that a collection of elements within a syntax quote will become a map, and emit the proper code forms from the SyntaxQuoteReader. There is a small edge case in metadata literals, but an with additional piece of metadata containing the proto-map we can still generate the appropriate (with-meta ...) form at syntax-quote emission time.

Importantly, none of the hand-waving involved ever escapes the reader, and the eval/compile environment is none the wiser.

This allows the following:

`{~@[1 2]} ;=> after eval: {1 2}
`^{~@[:foo :bar]} sym ;=> metadata of 'sym after eval: {:foo :bar}

But not:
`~{1} ;=> RuntimeException ...

Or:{1} ;=> RuntimeException ...

And `{~@[1]} has the same semantics as the currently required `{~@[1] ~@[]}
;=> IllegalArgumentException No value supplied for key: 1 clojure.lang.PersistentHashMap.create (PersistentHashMap.java:77)

The changes in my patch pass all existing tests and include an additional test for the newly-supported map unquote-splicing form.



 Comments   
Comment by Jon Distad [ 16/May/14 5:57 PM ]

Modified from this morning- more tests, plus bugfix for the new cases caught.

Comment by Jon Distad [ 17/May/14 10:47 AM ]

Updated patch.

Now uses two distinct paths for adding metadata. Old version potentially stacked with-meta calls, which could result in lost keys.





[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-1423] Applying a var to an infinite arglist consumes all available memory Created: 15/May/14  Updated: 15/May/14

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

Type: Defect Priority: Minor
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File apply-var.patch    
Patch: Code and Test

 Description   

It is possible to apply a function to an infinite argument list: for example, (apply distinct? (repeat 1)) immediately returns false, after realizing just a few elements of the infinite sequence (repeat 1). However, (apply #'distinct? (repeat 1)) attempts to realize all of (repeat 1) into memory at once.

This happens because Var.applyTo delegates to AFn.applyToHelper to decide which arity of Var.invoke to dispatch to; but AFn doesn't expect infinite arglists (mostly those use RestFn). So it uses RT.seqToArray, which doesn't work well in this case.

Instead, Var.applyTo(args) can just dispatch to deref().applyTo(args), and let the function being stored figure out what to do with the arglist.

I've changed Var.applyTo to do this, and added a test (which fails before my patch is applied, and passes afterwards).






[CLJ-1422] Recur around try boxes primitives Created: 14/May/14  Updated: 22/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Kyle Kingsbury Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, performance, typehints


 Description   

Primitive function and recur variables can't pass through a (try) cleanly; they're boxed to Object instead. This causes reflection warnings for fns or loops that use primitive types.

user=> (set! *warn-on-reflection* true)
true
 
user=> (fn [] (loop [t 0] (recur t)))
#<user$eval676$fn__677 user$eval676$fn__677@3d80023a>
 
user=> (fn [] (loop [t 0] (recur (try t))))
NO_SOURCE_FILE:1 recur arg for primitive local: t is not matching primitive, had: Object, needed: long
Auto-boxing loop arg: t
#<user$eval680$fn__681 user$eval680$fn__681@5419323a>

user=> (fn [^long x] (recur (try x)))
NO_SOURCE_FILE:1 recur arg for primitive local: x is not matching primitive, had: Object, needed: long

CompilerException java.lang.IllegalArgumentException:  recur arg for primitive local: x is not matching primitive, had: Object, needed: long, compiling:(NO_SOURCE_PATH:1:1)


 Comments   
Comment by David James [ 15/Jun/14 10:27 PM ]

Without commenting on the most desirable behavior, the following code does not cause reflection warnings:

user=> (set! *warn-on-reflection* true)
true
user=> (fn [] (loop [t 0] (recur (long (try t)))))
#<user$eval673$fn__674 user$eval673$fn__674@4e56c411>
Comment by Nicola Mometto [ 16/Jun/14 6:33 AM ]

Similar ticket http://dev.clojure.org/jira/browse/CLJ-701

Comment by Kevin Downey [ 21/Jul/14 6:59 PM ]

try/catch in the compiler only implements Expr, not MaybePrimitiveExpr, looking at extending TryExpr with MaybePrimitiveExpr it seems simple enough, but it turns out recur analyzes it's arguments in the statement context, which causes (try ...) to essentially wrap itself in a function like ((fn [] (try ...))), at which point it is an invokeexpr which is much harder to add maybeprimitiveexpr too and it reduces to the same case as CLJ-701

Comment by Kevin Downey [ 22/Jul/14 9:27 PM ]

http://dev.clojure.org/jira/browse/CLJ-701 has a patch that I think solves this





[CLJ-1421] It's not a digit but regexp classify this one as a digit. Created: 14/May/14  Updated: 14/May/14  Resolved: 14/May/14

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

Type: Defect Priority: Major
Reporter: Ale Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: None

Attachments: PNG File scr.png    

 Comments   
Comment by Ale [ 14/May/14 12:53 AM ]

[org.clojure/clojure "1.5.1"]

Comment by Ale [ 14/May/14 4:20 AM ]

I have no possibility to close this issue. It was wrong "\" before "1".





[CLJ-1420] ThreadLocalRandom instead of Math/random Created: 11/May/14  Updated: 20/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Linus Ericsson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: math, performance
Environment:

Requires Java >=1.7!


Attachments: Text File 0001-rand-using-ThreadLocalRandom-and-tests-for-random.patch    
Patch: Code
Approval: Vetted

 Description   

The standard Math.random() is thread-safe through being declared as a synchronized static method.

The patch uses java.util.concurrent.ThreadLocalRandom which actually seems to be two times faster than the ordinary Math.random() in a simple single threaded criterium.core/bench:

The reason I investigated the function at all was to be sure random-number generation was not a bottleneck when performance testing multithreaded load generation.

If necessary, one could of course make a conditional declaration (like in fj-reducers) based on the existence of the class java.util.concurrent.ThreadLocalRandom, if Clojure 1.7 is to be compatible with Java versions < 1.7



 Comments   
Comment by Linus Ericsson [ 11/May/14 11:05 AM ]

Benchmark on current rand (clojure 1.6.0), $ java -version
java version "1.7.0_51"
OpenJDK Runtime Environment (IcedTea 2.4.4) (7u51-2.4.4-0ubuntu0.13.10.1)
OpenJDK 64-Bit Server VM (build 24.45-b08, mixed mode)

:jvm-opts ^:replace [] (ie no arguments to the JVM)

(bench (rand 10))
Evaluation count : 1281673680 in 60 samples of 21361228 calls.
Execution time mean : 43.630075 ns
Execution time std-deviation : 0.420801 ns
Execution time lower quantile : 42.823363 ns ( 2.5%)
Execution time upper quantile : 44.456267 ns (97.5%)
Overhead used : 3.194591 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

Clojure 1.7.0-master-SNAPSHOT.

(bench (rand 10))
Evaluation count : 2622694860 in 60 samples of 43711581 calls.
Execution time mean : 20.474605 ns
Execution time std-deviation : 0.248034 ns
Execution time lower quantile : 20.129894 ns ( 2.5%)
Execution time upper quantile : 21.009303 ns (97.5%)
Overhead used : 2.827337 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

I had similar results on Clojure 1.6.0, and ran several different tests with similar results. java.util.Random.nextInt is suprisingly bad. The ThreadLocalRandom version of .nextInt is better, but rand-int can take negative integers, which would lead to some argument conversion for (.nextInt (ThreadLocalRandom/current) n) since it need upper and lower bounds instead of a simple multiplication of a random number [0,1).

CHANGE:

The (.nextDouble (ThreadLocalRandom/current) argument) is very quick, but cannot handle negative arguments. The speed given a plain multiplication is about 30 ns.

Comment by Linus Ericsson [ 11/May/14 12:44 PM ]

Added some simplistic tests to be sure that rand and rand-int accepts ratios, doubles and negative numbers of various kinds. A real test would likely include repeated generative testing, these tests are mostly for knowing that various arguments works etc.

Comment by Linus Ericsson [ 11/May/14 1:38 PM ]

0001-rand-using-ThreadLocalRandom-and-tests-for-random.patch contains the changed (rand) AND the test cases.

Comment by Alex Miller [ 11/May/14 5:45 PM ]

Clojure requires Java 1.6.0 so this will need to be reconsidered at a later date. We do not currently have any plans to bump the minimum required JDK in Clojure 1.7 although that could change of course.

Comment by Gary Fredericks [ 11/May/14 5:54 PM ]

I've always thought that the randomness features in general are of limited utility due to the inability to seed the PRNG, and that a clojure.core/rand dynamic var would be a reasonable way to do that.

Maybe both of these problems could be partially solved with a standard library? I started one at https://github.com/fredericksgary/four, but presumably a contrib library would be easier for everybody to standardize on.

Comment by Linus Ericsson [ 12/May/14 2:17 AM ]

Gary, I'm all for creating some well-thought out random-library, which could be a candidate for some library clojure.core.random if that would be useful.

Please have a look at http://code.google.com/p/javarng/ since that seems to do what you library four does (and more). Probably we could salvage either APIs, algorithms or both from this library.

I'll contact you via mail!

Comment by Gary Fredericks [ 20/Jun/14 10:21 AM ]

Come to think of it, a rand var in clojure.core shouldn't be a breaking change, so I'll just make a ticket for that to see how it goes. That should at the very least allow solving the concurrency issue with binding. The only objection I can think of is perf issues with dynamic vars?

Comment by Gary Fredericks [ 20/Jun/14 10:42 AM ]

New issue is at CLJ-1452.





[CLJ-1419] Report errors on missing param list or return type of methods in gen-class and gen-interface Created: 10/May/14  Updated: 12/May/14

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

Type: Enhancement Priority: Trivial
Reporter: Nathan Zadoks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, gen-class

Attachments: Text File 0001-CLJ-1419-default-to-void-return-type-in-gen-interfac.patch     Text File 0001-CLJ-1419-map-nil-to-void-in-prim-class.patch     File clj1419.clj     Text File fail.log    
Patch: Code

 Description   

The following are invalid and should produce errors when invoked on gen-class or gen-interface:

(gen-interface :name clj1419.IFail :methods [[myMethod java.lang.String]])  ;; no params, throws error
(gen-interface :name clj1419.IFail :methods [[myMethod []]]) ;; no return type
(gen-interface :name clj1419.IFail :methods [[myMethod]])  ;; no params or return type

The first example throws an error. The second and third do not but will generate an invalid class, verify with:

(.getMethods clj1419.IFail)
ClassNotFoundException java.lang.  java.net.URLClassLoader$1.run (URLClassLoader.java:366)

Add checks to prevent these errors.



 Comments   
Comment by Nathan Zadoks [ 10/May/14 1:34 PM ]

I've implemented both fixes, and attached them as patches.

Comment by Nathan Zadoks [ 10/May/14 1:40 PM ]

I'd argue that the behaviour of asm-type is at fault here (it can output an invalid type name when passed a nil argument), so I prefer that fix over the purely symptomatic generate-interface fix.

Comment by Andy Fingerhut [ 10/May/14 2:33 PM ]

Nathan, were you planning on submitting a signed Clojure Contributor's Agreement, or already have? Details here if you have not: http://clojure.org/contributing

Patches from non-contributors cannot be committed to Clojure.

Note: I cannot promise you that one of your patches will be accepted into Clojure if you sign a CA – only that it will not if you do not sign one.

Comment by Alex Miller [ 10/May/14 4:19 PM ]

Please add an example of how this happens and the current error.

Comment by Nathan Zadoks [ 11/May/14 3:45 AM ]

Andy — Yep, I've read up on that. My CA will be underway to Rich soon. (filled in, signed, in an envelope, just need to await the arrival of those bloody international stamps…)

Alex Miller — Tahdah!

A demonstration of the issue, both attached and as a gist: https://gist.github.com/nathan7/3a7e3a09e458f1354cbb

Comment by Nathan Zadoks [ 11/May/14 3:48 AM ]

and here's log of the compiler crash that results (also added to the gist now)

Comment by Nathan Zadoks [ 11/May/14 4:27 AM ]

Whoops, both of my patches were rather broken due to a misunderstanding on my side.
I forgot entirely that asm-type takes a symbol, not a string.
Modifying asm-type was definitely a bad idea, that check just looks whether it should defer to prim->class.
Adding nil to prim->class would work (and I've attached my patch for that too), but it's starting to look rather inelegant compared to just patching gen-interface.
(on a side note: I'm having a lot of fun exploring the Clojure codebase! thanks for that, humans!)

Comment by Alex Miller [ 11/May/14 7:26 AM ]

My reading of the docstring of gen-interface is that method declarations must specify a parameter list and a valid return type. I would expect all of these to be invalid:

(gen-interface :name clj1419.IFail :methods [[fail nil]])
(gen-interface :name clj1419.IFail :methods [[fail [] nil]])
(gen-interface :name clj1419.IFail :methods [[fail []]])

"nil" is not a valid type - you can use "void" for this purpose and this works fine:

(gen-interface :name clj1419.IFail :methods [[fail [] void]])

If this ticket is (as the title states) a request to allow omitting the return type or using "nil" as a return type, then I think the answer is no. If the ticket is a request to improve the error reporting of the failure cases above, then I think we can consider that but it will be very low priority.

Comment by Nathan Zadoks [ 12/May/14 8:19 AM ]

The code seems to suggest otherwise though, seeing the explicit extra branch for pclasses being nil.
As much as I like PL trivia, I haven't run into `void` in Clojure anywhere else yet, and I'm surprised to see it here.
Maintaining the principle of least surprise seems preferable to pedantry about whether nil is a type: (= "nil" (str (type (.methodReturningVoid obj)))

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

The two places to look for words to rely on are docstrings and the http://clojure.org/documentation pages. Implementation details are just that.

"nil" is not a type. "void" is a documented type identifier indicating the absence of a return value - http://clojure.org/java_interop#Java%20Interop-Aliases

Comment by Nathan Zadoks [ 12/May/14 8:27 AM ]

Okay. Better error-checking in asm-type then?

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

I have updated the title and description based on my understanding of what this ticket should be, which is enhanced error-checking on the method specs for gen-class and gen-interface. I'm not sure if that's in asm-type or somewhere earlier.





[CLJ-1418] make as-> macro compatible with destructuring Created: 09/May/14  Updated: 09/May/14

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

Type: Enhancement Priority: Minor
Reporter: Nahuel Greco Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

all environments


Patch: Code

 Description   

The as-> macro doesn't work with destructuring. This is invalid code:

(-> [1 2] 
    (as-> [a & b] 
          [a (inc b)] 
          [(inc a) b]))

because it is expanded to:

(let [[a & b] [1 2]
        [a & b] [a (inc b)]
        [a & b] [(inc a) b]]
       [a & b])  ;; this last expression will not compile

but with a little redefinition is possible to make as-> work with
destructuring:

(defmacro as->
  "Binds name to expr, evaluates the first form in the lexical context
  of that binding, then binds name to that result, repeating for each
  successive form, returning the result of the last form."
  {:added "1.5"}
  [expr name & forms]
  `(let [~name ~expr
         ~@(interleave (repeat name) (butlast forms))]
     ~(last forms)))

now the previous example will expand to:

(let [[a & b] [1 2]
      [a & b] [a (inc b)]]
     [(inc a) b])

The following example shows why an as-> destructuring compatible
macro can be useful. This code parses a defmulti like parameter list
by reusing a destructuring form:

(defmacro defmulti2 [mm-name & opts]
 (-> [{} opts]
      (as-> [m [e & r :as o]] 
            (if (string? e) 
              [(assoc m :docstring e) r] 
              [m                      o])
            (if (map? e)
              [(assoc m :attr-map e :dispatch-fn (first r)) (next r)]
              [(assoc m             :dispatch-fn e)         r])
            ...

Compare with the original defmulti:

(defmacro defmulti [mm-name & options]
  (let [docstring   (if (string? (first options))
                      (first options)
                      nil)
        options     (if (string? (first options))
                      (next options)
                      options)
        m           (if (map? (first options))
                      (first options)
                      {})
        options     (if (map? (first options))
                      (next options)
                      options)
        dispatch-fn (first options)
        options     (next options)
        m           (if docstring
                      (assoc m :doc docstring)
                      m)
        ...


 Comments   
Comment by Nahuel Greco [ 09/May/14 2:12 AM ]

note, this issue is badly formated, for a more legible form:

https://gist.github.com/nahuel/a34a9fe967c035a3d069





[CLJ-1417] clojure.java.io/input-stream has incorrect docstring Created: 07/May/14  Updated: 07/May/14

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

Type: Defect Priority: Trivial
Reporter: Dario Bertini Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, io, newbie

Approval: Triaged

 Description   

clojure/java/io.clj line 125

"Default implementations are defined for OutputStream, File, URI, URL,"

Should read

"Default implementations are defined for InputStream, File, URI, URL,"






[CLJ-1416] Support transients in gvec Created: 06/May/14  Updated: 05/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Unresolved Votes: 0
Labels: collections, transient

Attachments: Text File 0001-CLJ-1416-transients-hash-caching-for-gvec-Object-met.patch     Text File 0002-CLJ-1416-transients-hash-caching-interop-improvement.patch    
Patch: Code
Approval: Triaged

 Description   

Vectors of primitives produced by vector-of do not support transients.

core.rrb-vector implements transient support for vectors of primitives. Such transient-enabled vectors of primitives can be obtained in a number of ways: (1) using a gvec instance as an argument to fv/catvec (if RRB concatenation happens, which is not guaranteed) or fv/subvec; (2) passing a gvec instance to fv/vec, which as of core.rrb-vector 0.0.11 will simply rewrap the gvec tree in an RRB wrapper; (3) using fv/vector-of instead of clojure.core/vector-of. Native support in gvec would still be useful as part of an effort to make supported functionality consistent across vector flavours (see CLJ-787 in this connection); gvec is also simpler and still has (and is likely to maintain) a performance edge.

A port of core.rrb-vector's transient support to gvec is available here:

https://github.com/michalmarczyk/clojure/tree/transient-gvec

I'll bring it up to date with current master shortly.

See the clojure-dev thread for some benchmarks:

https://groups.google.com/d/msg/clojure-dev/9ozYI1e5SCM/BAIazVOkUmcJ



 Comments   
Comment by Michał Marczyk [ 13/May/14 5:32 AM ]

Here's the current version of the patch (0001-CLJ-1416-transients-hash-caching-for-gvec-Object-met.patch). It includes a few additional changes – here's the commit message:

CLJ-1416: transients, hash caching for gvec, Object methods for gvec seqs

  • Adds transient support to gvec
  • Adds hash{eq,Code} caching to gvec and gvec seqs
  • Implements hashCode, equals, toString for gvec seqs

https://github.com/michalmarczyk/clojure/tree/transient-gvec-1.6

Comment by Michał Marczyk [ 05/Jul/14 2:59 AM ]

Here's an updated patch with some additional interop-related improvements.

The new commit message:

CLJ-1416: transients, hash caching, interop improvements for gvec

  • Adds transient support to gvec
  • Adds hash{eq,Code} caching to gvec and gvec seqs
  • Implements hashCode, equals, toString for gvec seqs
  • Correctly implements iterator-related methods for gvec and gvec seqs
  • Introduces throw-unsupported and caching-hash (both marked private)




[CLJ-1415] Keyword cache cleanup incurs linear scan of cache Created: 06/May/14  Updated: 23/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Kyle Kingsbury Assignee: Alex Miller
Resolution: Unresolved Votes: 6
Labels: keywords, performance

Attachments: File faster-keywords.diff     File keyword-cache.diff     Text File kw-clean-future.patch     File unified-kw-patch.diff    
Patch: Code
Approval: Vetted

 Description   

If the GC reclaims a keyword, any subsequent attempt to create a keyword requires an O(n) scan over the entire keyword table via Util.clearCache. This is a significant performance cost in keyword-heavy operations; e.g. JSON parsing.

Patch: keyword-cache.diff - patch to defer cleaning till portion of the table is dead and avoid multiple threads cleaning simultaneously.

Patch: kw-clean-future.patch - patch to spin cache cleaning into a future. Found that 1) this introduces some ordering constraints and circularity between Agent and Keyword (fixable) and 2) using the future pool for this means shutdown-agents would always need to be called (in the patch I avoided this by changing agent's soloExecutor to use daemon threads.

Patch: unified-kw-patch.diff - Alternative to keyword-cache and clean-future.patch. Combines all keyword-perf changes, including the EDN kw parser improvement, improved table lookup performance, and has threads cooperate to empty the table refqueue with a minimum of contention.



 Comments   
Comment by Alex Miller [ 06/May/14 5:53 PM ]

Any perf-related ticket will need some clear before/after timings (with good methodology and how to repro) and also a consideration of cases where the change may introduce any perf degradation in normal usage.

Comment by Kyle Kingsbury [ 07/May/14 9:54 PM ]

I've experimented with a patch reducing the cache clearing cost and removing the need for String.intern. Preliminary results are good, but I want to try a few alternative approaches for cache keys. For instance, could we use pure strings like "foo" and "clojure.core/foo" as the cache keys, removing a level of memory indirection? If we're being really sneaky, we could share those same strings with the Symbol _str field to halve our memory use, assuming it's OK to reach in and mutate it.

https://gist.github.com/aphyr/f72e72992dade4578232
http://imgur.com/a/YSgUa#2

Comment by Alex Miller [ 08/May/14 12:29 PM ]

Great start on this - having the perf data is hugely important. One thing I don't see you've covered yet is what the corresponding memory increase you're incurring with CacheKey to get the benefit - we need to quantify both sides of the tradeoff here (latency/throughput vs memory) to fully judge.

Questions/comments on your patch...

1) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L101 - do we need the (o instanceof CacheKey) check? If the usage of this is constrained then we might be able to skip it (and it will blow up on the next line if something is wrong).

2) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L110 - shouldn't we precompute and save the hash code!? The only thing we're making this for is fast hash comparisons. That hash computation is string length dependent - it ain't cheap.

3) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L126 - have you tested with other values here? Should have some justification for this.

4) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L126 - have you tested with other values here? Should have some justification that this is a reasonable number.

5) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L169 - there is a race here (actually more than one if you include getting the tableSize):

Th1: orphansCount = orphans.get()
Th2: orphansCount = orphans.get()
Th2: orphansNew = orphans.getAndSet(0)
Th2: orphansNew > orphansCount -> start cleaning
<huge gc, 10 zillion orphans are created>
Th1: orphansNew = orphans.getAndSet(0)
Th1: orphansNew > orphansCount -> start cleaning

but I guess this is "safe"; we just have multiple threads cleaning in that case.

6) In general it seems pretty sloppy (I don't mean that pejoratively) how the orphans, rq, and cleaning thread are working together and may be out of sync. I get it and I even believe it will work (usually) but I worry about timing issues that I am too dumb to think of.

Is there a simpler strategy? What if every Nth call to intern() cleaned M entries (or up to M% of table)?

7) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L177 - if you made the iterator explicit in this loop, it would be safe to call iterator.remove() instead of table.remove() and I believe that would be faster on CHM.

8) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L198 - could have two versions of this with/without the symbol. Not sure if that would be faster but they might both inline better into their callers then?

9) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L242 - what's the use case for finding an external CacheKey? Fast re-lookup for specialized use? Do we want to commit to this in the API?

Comment by Kyle Kingsbury [ 08/May/14 2:41 PM ]

1) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L101 - do we need the (o instanceof CacheKey) check? If the usage of this is constrained then we might be able to skip it (and it will blow up on the next line if something is wrong).

I'm usually wary of violating equality/hashCode contracts, and this doesn't even appear as a blip on the radar in profiling. I think we could drop it

2) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L110 - shouldn't we precompute and save the hash code!? The only thing we're making this for is fast hash comparisons. That hash computation is string length dependent - it ain't cheap.

It's less memoizable than you might think; each CacheKey is only indexed a few times, and only at query time; it also doesn't help us for equality checks, since those only occur after hashing. I can add a memoizing field for it at the cost of another 32 bits/kw; we'll see how it impacts performance.

3) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L126 - have you tested with other values here? Should have some justification for this.

I experimented with several values on the Clojure test suite, benchmarks, and some real-world hadoop code. Diminishing returns, as you'd expect. 0.1 and 0.5 are essentially identical in runtime tradeoff. We could drop to 0.01 if desired; it'll only make a difference in large (10-100K) extant keyword benchmarks, as far as I can tell.

4) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L126 - have you tested with other values here? Should have some justification that this is a reasonable number.

Same question as #3?

5) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L169 - there is a race here (actually more than one if you include getting the tableSize):

Th1: orphansCount = orphans.get()
Th2: orphansCount = orphans.get()
Th2: orphansNew = orphans.getAndSet(0)
Th2: orphansNew > orphansCount -> start cleaning
<huge gc, 10 zillion orphans are created>
Th1: orphansNew = orphans.getAndSet(0)
Th1: orphansNew > orphansCount -> start cleaning

but I guess this is "safe"; we just have multiple threads cleaning in that case.

Yep. This check is only there as an optimization--and note that if a huge GC occurs, it's likely we want to schedule a followup traversal of the table anyway, because the thread that's already cleaned some of the table has probably missed some subsequently GC'ed elements. The number of concurrently cleaning threads is bounded by the rate of GC churn, and in the most pathological case (sadly, I haven't been able to produce this race experimentally), this degenerates to the old Clojure behavior of every thread doing a full scan.

6) In general it seems pretty sloppy (I don't mean that pejoratively) how the orphans, rq, and cleaning thread are working together and may be out of sync. I get it and I even believe it will work (usually) but I worry about timing issues that I am too dumb to think of.

Is there a simpler strategy? What if every Nth call to intern() cleaned M entries (or up to M% of table)?

Every nth call is just fine, but it degrades more poorly for large tables. In general, I try to lean towards scale-invariant solutions, which is why I aimed to reclaim roughly a tenth of the entries in the map every time. Maybe more, maybe less, depending on CAS retries, delayed threads resetting the counter to zero, etc.

Doing M entries or M% is more tricky; gotta figure out which threads collect what fraction when, how you efficiently access that subsection of the hash, make sure elements don't fall through the cracks, etc.

7) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L177 - if you made the iterator explicit in this loop, it would be safe to call iterator.remove() instead of table.remove() and I believe that would be faster on CHM.

I agree. I figured Rich had a good reason for doing it this way, but if you concur I'll change it.

8) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L198 - could have two versions of this with/without the symbol. Not sure if that would be faster but they might both inline better into their callers then?

I agree. We can do that dispatch statically and cut down on branch misprediction, too.

9) https://gist.github.com/aphyr/f72e72992dade4578232#file-gistfile1-diff-L242 - what's the use case for finding an external CacheKey? Fast re-lookup for specialized use? Do we want to commit to this in the API?

Keep forgetting Java's obsession with encapsulation. I'll privatize.

Comment by Alex Miller [ 08/May/14 10:14 PM ]

On several of these - 2, 7, 8 - I think those are worth a test. If faster, we should consider.

On 9, I thought maybe you were opening it up so it would be possible to save off a CacheKey and reuse it or something else. If it's not needed externally, then might be good to private-ize CacheKey itself so we can change it later.

Comment by Kyle Kingsbury [ 09/May/14 6:04 PM ]

http://imgur.com/a/1bv3P#0
https://gist.github.com/aphyr/f72e72992dade4578232

These charts show the performance impact of several changes. In order, they are:

1.7                 baseline
kw                  initial patch
kw-static-paths     Separate codepaths for interning symbols vs strings. Iterator
                    .remove for cache cleaning. Fix a bug for null comparisons
                    in CacheKey namespaces. Internal functions now protected, not
                    public. Not much performance impact.
kw-memo-hash        Memoize hashcodes for CacheKeys. Performance is a wash.
kw-string-cachekeys Observing that String.indexOf('/') consumed a significant 
                    fraction of interning time, use a combined "ns/name" string for
                    Cachekeys instead of separate strings. Significant performance 
                    boost in all tests; 40% reduction in median latencies in 1000-
                    kw allocation test, for instance.
kw-string-keys      Use raw strings for CacheKeys. Improves performance by removing
                    a level of memory indirection, even without cached hashcodes.
kw-interned-keys    Intern those strings to reduce memory consumption, sharing
                    them with the underlying symbol's strings. Slightly slower.

Performance is even better now. Creating 1000 keywords median latency changed from 900 to 200 micros; .999s lower, throughput from 4000 to 20,000/second. JSON parsing median latency fell from 170 micros to 100 micros; throughput doubled from 17500 docs/sec to 36,000 docs/sec.

We're still suffering from poor dispersal in ConcurrentHashmap's use of the string hashCode on JDK7/8, but maybe that's a subject for a different patch.

Memory impact is now minimal. We intern every key string in the table, and those strings are interned by the symbols anyway, so they're essentially the same object. For namespaced symbols, we pay a slightly higher cost--forcing the interning of the "ns/name" string instead of deferring it to Symbol.toString() time. For non-namespaced symbols, these strings are interned as a part of the symbol creation process so there's no memory overhead.

At the repl, I tested by allocating and retaining a million keywords:

(def x (mapv keyword (map (partial str "test-kw-") (range 1e6))))

Retained size (bytes)             1.7   string-kw
----------------------------------------------------
Total retained heap        221.    MB  221.    MB
clojure.lang.Symbols       104.820 MB   32.900 MB
clojure.lang.Keywords       24.021 MB   56.049 MB
java.lang.Strings           89.537 MB   81.786 MB
clojure.lang.Keyword class  72.447 MB   72.451 MB

Total memory use is unchanged, but note that clojure.lang.Symbol retains less, since its strings are now shared by the Keyword table. Keywords, by contrast, retains more. Strings and the keyword table are essentially unchanged.

Comment by Kyle Kingsbury [ 09/May/14 6:08 PM ]

I can't figure out how to edit the ticket description, but I updated the same gist with the cumulative changes. Comments welcome!

Comment by Alex Miller [ 09/May/14 9:51 PM ]

Excellent, thanks for the data. I added a group to your auth so I think you should be able to edit descriptions now. If not, let me know. I'll re-review the patch next week. It would be good either at this point or after that to turn this into an actual patch file instead of a gist.

Comment by Kyle Kingsbury [ 12/May/14 4:24 PM ]

I've attached a cumulative patch. It's comprised of 8 commits; one for each stage we've discussed. I can rebase into a single commit if you'd like.

Comment by Alex Miller [ 13/May/14 7:31 AM ]

I would like a single cumulative rebased patch. I hope to have some time to look at it today.

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

On another look, I think it would be useful to separate this ticket into two parts - the first is about optimizing keyword creation and lookup to avoid unnecessary work (avoiding symbol creation and interning, using Strings as keys in the cache). The second part is really about optimizing cache clearing. Do you think these can be separated into two tickets?

Regarding the cache clearing part, have you tested a simpler strategy of just counting calls to clearCache() and running the clean scan every N calls? If that was almost as good, I'd be in favor of that over what is in the patch.

The kw-static paths version did not seem to be an improvement - perhaps you should try pulling them back together to simplify the code? Only worth it if there is a real improvement from it.

On the various find methods - if you could retain their ordering and minimize noise in the diffs that would really help make it easier to screen.

Finally, we need to do some tests to verify that we have not altered the performance of using keywords and symbols as keys in a map for lookup.

Comment by Kyle Kingsbury [ 05/Jun/14 2:36 PM ]

> On another look, I think it would be useful to separate this ticket into two parts - the first is about optimizing keyword creation and lookup to avoid unnecessary work (avoiding symbol creation and interning, using Strings as keys in the cache). The second part is really about optimizing cache clearing. Do you think these can be separated into two tickets?

Created dev.clojure.org/jira/browse/CLJ-1439 for reduced intern cost

> Regarding the cache clearing part, have you tested a simpler strategy of just counting calls to clearCache() and running the clean scan every N calls? If that was almost as good, I'd be in favor of that over what is in the patch.

I'm not confident that this work will be merged, so I'm kinda reticent to go off and spend another N hours doing benchmarks.

> The kw-static paths version did not seem to be an improvement - perhaps you should try pulling them back together to simplify the code? Only worth it if there is a real improvement from it.

It was obsoleted by a later commit; only included it in the benchmark because you asked about the perf impact.

> On the various find methods - if you could retain their ordering and minimize noise in the diffs that would really help make it easier to screen.

Done.

> Finally, we need to do some tests to verify that we have not altered the performance of using keywords and symbols as keys in a map for lookup.

This doesn't touch the lookup path; costs are equivalent.

Comment by Alex Miller [ 09/Jun/14 1:53 PM ]

reduced patch with only the keyword cache clearing changes

Comment by Alex Miller [ 09/Jun/14 8:53 PM ]

Patch that spins cache cleaning into a future

Comment by Kyle Kingsbury [ 21/Jul/14 2:20 PM ]

Just as a followup: got bit by this issue again in a data analysis project today: JSON parsing thrashes the reference queue really hard. Merging this patch would definitely be appreciated. Yourkit screenshot here: http://aphyr.com/media/clojure-keyword-refqueue.png

Comment by Kyle Kingsbury [ 21/Jul/14 4:58 PM ]

Oh yeah, once these two are merged, here's a patch that speeds up my EDN parsing-heavy hadoop jobs by 2-3x. Avoids constructing the symbol every time.

--- a/src/jvm/clojure/lang/EdnReader.java
+++ b/src/jvm/clojure/lang/EdnReader.java
@@ -299,10 +299,9 @@ private static Object matchSymbol(String s){
                        return null;
                        }
                boolean isKeyword = s.charAt(0) == ':';
-               Symbol sym = Symbol.intern(s.substring(isKeyword ? 1 : 0));
                if(isKeyword)
-                       return Keyword.intern(sym);
-               return sym;
+                       return Keyword.intern(s.substring(1));
+               return Symbol.intern(s);
                }
        return null;
 }
Comment by Kyle Kingsbury [ 21/Jul/14 6:33 PM ]
public static void clearCache() {
  if(rq.poll() != null) {
    Agent.soloExecutor.submit(new Runnable() {
      public void run() {
        Util.clearCache(rq,table);
      }
    });
  }
}

This spawns literally hundreds of new threads – 30-40 at a time in my workload – which fight over the referencequeue. Also it causes a fair bit of contention on the executor itself during keyword-heavy allocation-all threads have to synchronize on the executor's queue-but that seems secondary to the cost of the cache-clearing threads themselves.

How about adding a single-thread executor to Agent for these sorts of housekeeping jobs?

Comment by Alex Miller [ 21/Jul/14 8:14 PM ]

I actually built another patch that did exactly that but never got around to attaching it; a single-threaded executor reserved solely for cache clearing. I tried actually making it completely independent but I found it was pretty easy for it to fall behind - it needs to be connected into the construction process to avoid blowing the queue up too big.

I have not been able to build an isolated test that actually showed any significant performance difference with just your cache-clearing change (what's currently attached as keyword-cache.diff) and not the other changes. I had many problems getting your test code to run but when I did get something to run I was not able to see any significant difference with just the keyword-cache.diff.

Comment by Kyle Kingsbury [ 22/Jul/14 6:43 PM ]

Managed to eliminate the refqueue contention by having only one thread involved in GCing at a time. Also doesn't require messing with background threads, and is less susceptible to the queue-overflow problem. Since the various extant patches don't apply cleanly on top of each other, I've re-written them in unified-kw-patch.diff, attached. Roughly doubles throughput compared to your patch, at least on a 24-core xeon running openjdk7.

http://aphyr.com/media/clojure-reduced-kw-refqueue-contention.png

Can you please reconsider merging? I've put over a hundred hours into writing, testing, and refining this patchset; it's been stable in production for months and has made a dramatic difference in several of our data-heavy Clojure programs.

Comment by Alex Miller [ 23/Jul/14 10:58 AM ]

Hey Kyle, I appreciate the time you've put into this. However, having a big giant patch tuned on a single use case is not an effective way to evolve the language. We need to separate and describe problems, then explore the solution space for each one, as independently as possible, while considering the impacts on all other use cases.

This particular ticket is concerned solely with the linear cleanup of the reference queue. Can you split out just a patch that deals with this issue? It would be helpful to have a test that demonstrates the performance problem and how this patch address the problem. My testing so far with the prior patch did not demonstrate any improvement.

It would also be helpful to have a squashed version of the complement of the changes related to interning on CLJ-1439 for consideration of that as a separate problem. (And maybe there is further splitting that could be done; I have not looked closely at the interning changes.)

Comment by Alex Miller [ 23/Jul/14 11:00 AM ]

The EdnReader changes, for example, should be a separate ticket.

Comment by Kyle Kingsbury [ 23/Jul/14 12:30 PM ]

Could you at least merge dev.clojure.org/jira/browse/CLJ-1439 first? I split it into a separate ticket over a month ago and these changes depend on it.

Comment by Alex Miller [ 23/Jul/14 1:27 PM ]

I would be happy to consider CLJ-1439 first. Can you update the patch there to be current and focused on the intern/cache?

Comment by Kyle Kingsbury [ 23/Jul/14 2:35 PM ]

The patch is current, and it is focused on the intern/cache.





[CLJ-1414] sort's docstring should say whether it is stable Created: 02/May/14  Updated: 05/May/14

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

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: collections, docstring

Approval: Triaged

 Description   

sort's docstring does not address whether the sort will be stable.

Stability is a useful property. It appears to be customary among programming tools to document whether their sort is stable. Java's Collections javadoc pledges a stable sort. The man-page of GNU coreutils sort in Ubuntu mentions its stability. The perldoc of Perl's sort function indicates it is a stable sort now but was not always.

Pillars of the Clojure community have commented on sort's stability:

(1) A recent book assembled by Cognitect consultants, "Clojure Cookbook", says Clojure's sort function "uses Java's built-in sort" and that "[t]he sort is also stable".

(2) In a 2011 discussion thread, "Clojure sort: is it specified to be stable for all targets?" https://groups.google.com/forum/#!topic/clojure/j3aNAmEJW9A , Stuart Sierra replied that "if it's not specified in the doc string, then it's not a promise. That said, [...] I would generally expect a language built-in `sort` routine to be stable, so take that for what it's worth."

Let's promote this open secret / blue-ribbon rumor to a statement in the official documentation.



 Comments   
Comment by Alex Miller [ 05/May/14 10:23 AM ]

Sounds reasonable. Needs patch from contributor.





[CLJ-1413] A problem involving user.clj and defrecord Created: 29/Apr/14  Updated: 11/May/14

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

Type: Defect Priority: Minor
Reporter: Lars Bohl Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord
Environment:

Java 1.7.0_51 OpenJDK 64-Bit Server VM


Attachments: File CLJ-1413.tar.bz2     File new-src-folder.tar.bz2    

 Description   

On-the-fly generation of defrecord classes fails under certain circumstances.

This github documents the issue: https://github.com/methylene/class-not-found

This was created after a leiningen ticket was closed: https://github.com/technomancy/leiningen/issues/1517



 Comments   
Comment by Alex Miller [ 29/Apr/14 11:33 PM ]

This ticket needs some work to have a better more concise description of the problem and how to reproduce on the ticket.

Comment by Lars Bohl [ 30/Apr/14 1:49 PM ]

Prerequisites:

  • a leiningen installation for the first step (install.sh, creates a library jar).
  • The scripts compile.sh and run.sh expect clojure-1.6.0.jar to be in $HOME/Downloads

Reproduce:

by running ./install.sh && ./compile.sh && ./run.sh

This should print something like "#record_holder.def.ParallelAggregator{}". See the main method in src/class_not_found/core.clj.

Instead it fails with a stacktrace, in the last step (./run.sh)

Notice how ./run.sh doesn't fail, after enabling aot in lib/record-holder/project.clj, and then running ./install.sh again.

Also, notice how ./run.sh doesn't fail, after commenting out this line from test/user.clj: (require '[record-holder.def]), and then running ./compile.sh again.

Comment by Lars Bohl [ 11/May/14 4:48 PM ]

Update:

Attaching new src folder..
The shell scripts mentioned above are not needed.
Also, leiningen is not needed.
Reproduce the error by running error.sh, with these contents:

[CLJ-1413(master)]$ cat error.sh
#!/bin/sh
CLOJURE_JAR=$HOME/Downloads/clojure-1.6.0.jar
rm -rf target && mkdir target
echo -e "(set! *compile-path* \"target\")\n(compile 'class-not-found.core)\n" | java -cp src:$CLOJURE_JAR clojure.main -
java -cp target:$CLOJURE_JAR class_not_found.core

There must be a src folder containing user.clj, core.clj and def.clj as follows:

[CLJ-1413]$ find src/ -type f
src/user.clj
src/class_not_found/core.clj
src/record_holder/def.clj

[CLJ-1413(master)]$ find src/ -type f -exec cat "{}" ";"
;; user.clj
(ns user)
(require '[record-holder.def]) ;; remove this line to get rid of error
;; core.clj
(ns class-not-found.core
  (:gen-class)
  (:require record-holder.def)
  (:import record_holder.def.ParallelAggregator))
(defn -main [& _] (println (ParallelAggregator.)))
;; def.clj
(ns record-holder.def)
(defrecord ParallelAggregator [])




[CLJ-1412] Add 2-arity version of `cycle` that takes the numer of times to "repeat" the coll Created: 28/Apr/14  Updated: 28/Apr/14

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Add-2-arity-version-of-cycle-that-takes-the-number-o.patch    
Patch: Code

 Description   

There are already similar arities for repeat/repeatedly and similar functions, this patch adds a 2-arity version of cycle that behaves like this:

user> (cycle 0 '(1 2))
()
user> (cycle -1 '(1 2))
()
user> (cycle 3 '(1 2))
(1 2 1 2 1 2)
user> (cycle 1 '(1 2))
(1 2)





[CLJ-1411] Special symbols can be shadowed inconsistently Created: 28/Apr/14  Updated: 29/Apr/14

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

Type: Defect Priority: Minor
Reporter: Volkert Oakley Jurgens Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler


 Description   

The compiler does not complain about let binding (or def-ing) special symbols, but the binding only works if not used at the beginning of a list:

These work:

(let [try :a]
  try)
=> :a
(let [try (constantly :a)]
  (apply try :b))
=> :a

This doesn't work:

(let [try (constantly :a)]
  (try :b))
=> :b

This is true for all special symbols, not just publicly exposed ones like try and new, but also internal ones like fn*.

I would expect consistent behaviour: either the compiler does not permit shadowing special symbols at all, or shadowing them works in all cases.



 Comments   
Comment by Nicola Mometto [ 28/Apr/14 10:01 AM ]

I don't think that shadowing special symbols is a good idea, but probably having all the special symbols namespace qualified (clojure.core/import* is the only one ns-qualified atm) along with checking for the symbol in the locals env first and fallbacking to the special symbols map after that, would probably help in those scenarios

Comment by Volkert Oakley Jurgens [ 29/Apr/14 12:48 AM ]

I think that shadowing special symbols is a bad idea. If that was possible, we'd have to change most macros in clojure.core to make them safe (i.e. explicitly add a namespace to each special symbol usage). And how would we handle special symbols that are not just implementation specific, like try and new? Every 3rd party macro that uses those might become unsafe.

My personal preference would be to prohibit the shadowing of special symbols.

Comment by Nicola Mometto [ 29/Apr/14 5:37 AM ]

That won't be the case since what I'm proposing includes making syntax-quote aware of the namespaced special symbols.
`def would expand to 'clojure.core/def for example.

Comment by Volkert Oakley Jurgens [ 29/Apr/14 5:58 AM ]

That's true, but macros don't have to use the syntax quote. See for example the definition of when.





[CLJ-1410] Optimization: allow `set`/`vec` to pass through colls that satisfy `set?`/`vector?` Created: 26/Apr/14  Updated: 05/May/14

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

Type: Enhancement Priority: Minor
Reporter: Peter Taoussanis Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: performance

Attachments: File benchmarks.clj     Text File clj1410-bench.txt     Text File CLJ-1410.patch    
Patch: Code
Approval: Triaged

 Description   

set and vec currently reconstruct their inputs even when they are already of the requested type. Since it's a pretty common pattern to call set/vec on an input to ensure its type, this seems like an easy performance win in a common case.

Proposed: Check for set? in set and vec? in vec and return the coll as is if already of the requested type.

Performance:

See attached clj1410-bench.txt for test details :

Input/size Function Original Patched Comment
set/10 set 1.452 µs 0.002 µs 726x faster
set/1000 set 248.842 µs 0.006 µs 41473x faster
vector/10 set 1.288 µs 1.323 µs slightly slower
vector/1000 set 222.992 µs 221.116 µs ~same
set/10 vec 0.614 µs 0.592 µs ~same
set/1000 vec 56.876 µs 55.920 µs ~same
vector/10 vec 0.252 µs 0.007 µs 36x faster
vector/1000 vec 24.428 µs 0.007 µs 3500x faster

As expected, if an instance of the correct type is passed, then the difference is large (with bigger savings for sets which do more work for dupe checking). In cases where the type is different, there is an extra instance? check (which looks to be jit'ed away or negligible). We only see a slower time in the case of passing a small vector to the set function - 3% slower (35 ns). The benefit seems greater than the cost for this change.

Screened by:

More info:

Group discussion: https://groups.google.com/forum/#!topic/clojure-dev/fg4wtqzu0eY



 Comments   
Comment by Alex Miller [ 26/Apr/14 10:18 AM ]

Re:

*Open question*
Would it be better to pass-through arguments that satisfy the general (`set?`,`vec?`) or concrete (`PersistentHashSet`,`PersistentVector`) type?

I don't think there is any question that relying on abstractions via set?/vec? is better than referring to concrete types.

Comment by Alex Miller [ 26/Apr/14 10:20 AM ]

Please add perf difference info in the description. Please also combine the patches into a single patch.

Comment by Peter Taoussanis [ 26/Apr/14 10:52 AM ]

Combined earlier patches, removed docstring changes.

Comment by Peter Taoussanis [ 26/Apr/14 11:39 AM ]

Attached some simple benchmarks. These were run with HotSpot enabled, after a 100k lap warmup.

Google Doc times: http://goo.gl/W7EACR

The `set` benefit can be substantial, and the overhead in non-benefitial cases is negligible.

The effect on `vec` is subtler: the benefit is relatively smaller and the overhead relatively larger.

Comment by Reid McKenzie [ 04/May/14 12:01 PM ]

Patch looks good to me, and I've reproduced the claimed low "worst case" overhead and significant potential savings numbers to within 1ms. +1.

Comment by Alex Miller [ 05/May/14 10:21 AM ]

I added a more extensive set of tests performed using Criterium which should give better insight into single operation performance.





[CLJ-1409] Add support for marking gen-class methods as native Created: 21/Apr/14  Updated: 21/Apr/14

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

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: gen-class, interop


 Description   

As far as I know, there is no support for creating a Java instance in Clojure with native methods. Everything else needed exists, but there is no way to get the right annotation on the method right now (similar to static).

Here's an example (http://benchmarksgame.alioth.debian.org/u64q/program.php?test=pidigits&lang=clojure&id=4) from Alioth perf tests where ASM is being used directly to generate a class with native methods where gen-class would have been perfectly adequate with this enhancement. (Equivalent Java: http://benchmarksgame.alioth.debian.org/u64q/program.php?test=pidigits&lang=java&id=2).

Suggested implementation is to mark ^{:native true} on a method and omit the body.






[CLJ-1408] Add transient keyword to cached toString() value in _str Created: 19/Apr/14  Updated: 20/Jun/14

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

Type: Defect Priority: Minor
Reporter: Tomasz Nurkiewicz Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop

Attachments: Text File CLJ-1408-2.patch     Text File CLJ-1408.patch    
Patch: Code
Approval: Incomplete

 Description   

_str field in Keyword and Symbol classes lazily caches result of toString(). Because this field is not transient, serializing (using Java serialization) any keyword before and after calling toString() for the first time yields different results:

(import (java.io ByteArrayOutputStream ObjectOutputStream
                 ByteArrayInputStream  ObjectInputStream))

(defn- serialize [obj]
  (with-open [bos (ByteArrayOutputStream.)
              stream (ObjectOutputStream. bos)]
    (.writeObject stream obj)
    (-> bos .toByteArray seq)))

(def s1 (serialize :k))
;(println :k)
(def s2 (serialize :k))
(println (= s1 s2))

Uncomment (println :k) and suddenly s1 and s2 are no longer equal.

This issue came up when I was trying to use keywords as key in [Hazelcast](https://github.com/hazelcast/hazelcast) map. Hazelcast uses serialized keys in various scenarios, thus if I first put something to map under key :k and then print :k, I can no longer find such key.

Patch: CLJ-1408-2.patch

Screened by:



 Comments   
Comment by Alex Miller [ 19/Apr/14 7:28 AM ]

Hi Tomasz, it would be good to fix this, can you sign the CLA?

Comment by Tomasz Nurkiewicz [ 20/Apr/14 7:26 AM ]

Thanks, I'll sign and send CLA ASAP.

Comment by Tomasz Nurkiewicz [ 08/May/14 4:10 PM ]

My contributor greement arrived, please merge this patch whenever you find suitable.

Comment by Alex Miller [ 08/May/14 10:16 PM ]

Hi Tomasz, I noticed you added the private keyword - please remove that and update the patch.

Comment by Tomasz Nurkiewicz [ 09/May/14 3:55 PM ]

Removed `private` keyword

Comment by Alex Miller [ 20/Jun/14 9:22 AM ]

On second thought, it looks like we have most of the infrastructure for serialization testing anyways, so would appreciate an updated patch with the example turned into a serialization test. Please see test/clojure/test_clojure/serialization.clj for a place to put this (using existing roundtrip function).





[CLJ-1407] Recur mismatch might cause multiple evaluation Created: 17/Apr/14  Updated: 17/Apr/14

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, macro


 Description   

Since mismatching recurs cause the loop body to be re-analyzed, macroexpansion in the loop body might happen more than once, causing any side effects that happen during macroexpansion to be evaluated potentially multiple times

Clojure 1.7.0-master-SNAPSHOT
user=> (defmacro x [] (println "foo"))
#'user/x
user=> (fn [] (loop [y 1] (x) (recur (Integer. 1))))
foo
foo
#<user$eval6$fn__7 user$eval6$fn__7@71687585>


 Comments   
Comment by Andy Fingerhut [ 17/Apr/14 6:59 PM ]

This is not a question about whether the behavior in the description is a bug or not, but rather a curiosity about how often people write macros that have side effects at macroexpansion time. I think the following in Clojure itself do, but there may be others:

  • gen-class, and also ns because it uses gen-class
  • gen-interface, and also definterface because it uses gen-interface
  • clojure.core/compile-if (private) calls eval on its expr arg, but as used now doesn't cause macroexpansion-time side effects
  • doc seems to have one case that prints at macroexpansion time
  • I am not sure whether defprotocol or deftype have macroexpansion time side effects, or whether they are limited to run time
Comment by Nicola Mometto [ 17/Apr/14 9:20 PM ]

Andy, I don't think there are that many macros that side-effect at macroexpansion time and I haven't discovered this bug in real code but while thinking about how loop locals invalidation was implemented in Compiler.java.

Because there are a really a small number of side-effecting macros, this is unlikely to cause problems in real code, so I changed the priority to minor.





[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-1405] clojure runtime does not work with onejar-maven-plugin Created: 16/Apr/14  Updated: 06/Jul/14  Resolved: 06/Jul/14

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

Type: Defect Priority: Minor
Reporter: Sean Shubin Assignee: Unassigned
Resolution: Duplicate Votes: 1
Labels: None
Environment:

I was able to repeat this problem on windows, linux, and osx


Attachments: Text File onejar-maven-plugin-fix.patch    
Patch: Code

 Description   

I have created a sample project, steps to repeat the problem, and a proposed patch here:
https://github.com/SeanShubin/clojure-one-jar

I have also attached my proposed patch to this ticket, and pasted the relevant portion of README.md below:

clojure-one-jar
===============

Sample project to demonstrate problem with combining clojure runtime with onejar-maven-plugin

Steps to repeat the problem
===========================

  • mvn package
  • java -jar target/greeter.jar

Behavior before patch is applied
================================
Exception in thread "main" java.lang.reflect.InvocationTargetException
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:483)
at com.simontuffs.onejar.Boot.run(Boot.java:340)
at com.simontuffs.onejar.Boot.main(Boot.java:166)
Caused by: java.lang.ExceptionInInitializerError
at com.seanshubin.clojure_one_jar.Greeter.main(Greeter.java:12)
... 6 more
Caused by: java.lang.NullPointerException
at clojure.lang.RT.lastModified(RT.java:387)
at clojure.lang.RT.load(RT.java:421)
at clojure.lang.RT.load(RT.java:411)
at clojure.lang.RT.doInit(RT.java:447)
at clojure.lang.RT.<clinit>(RT.java:329)
... 7 more

Behavior after patch is applied
===============================
No need to call RT.init() anymore
Hello, world!

Patch
=====
I applied this patch to my local copy of clojure 1.7.0-master-SNAPSHOT
https://github.com/SeanShubin/clojure-one-jar/blob/master/onejar-maven-plugin-fix.patch



 Comments   
Comment by Alex Miller [ 16/Apr/14 9:45 PM ]

I think this is a dupe of CLJ-971 - can you verify?

Comment by Sean Shubin [ 16/Apr/14 9:49 PM ]

Yes, I looked over CLJ-971 and can confirm this matches my observations regarding why this is happening.

Comment by Sean Shubin [ 16/Apr/14 9:54 PM ]

My solution is a bit different in that it still tries to get the date of the file rather than defaulting to the date of the entire jar. I don't have a solid enough understanding of the calling code to know if this is important.

Comment by Alex Miller [ 16/Apr/14 10:17 PM ]

FYI, to consider your patch, I would need you to sign a Contributor Agreement - see http://clojure.org/contributing for details. I do think that is an area that something is needed, will need to assess the options a bit more.

Comment by Sean Shubin [ 16/Apr/14 10:56 PM ]

Sure thing, I just signed and sent a Contributor Agreement, per the instructions at http://clojure.org/contributing

Comment by Gus Heck [ 05/Jul/14 9:42 PM ]

This effects the one-jar-gradle plugin too. Any progress on reviewing Sean's patch?

Comment by Sean Shubin [ 06/Jul/14 12:13 AM ]

Gus, I suggest moving this conversation over to CLJ-971.

The main problem is that I had not time to get it under test coverage, see Stuart Halloway's comment regarding that on CLJ-971. I have been pretty busy with my other projects, and being that I am not familiar with the clojure code base, I am going to have to block out some time to figure out how to get the appropriate tests in place, perhaps I will have some time for that next weekend.

Comment by Alex Miller [ 06/Jul/14 6:11 PM ]

Same issue as CLJ-971, which is farther along in the process.





[CLJ-1404] clojure.core/vals returns nil on an empty map instead of an empty sequence Created: 14/Apr/14  Updated: 14/Apr/14  Resolved: 14/Apr/14

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

Type: Defect Priority: Trivial
Reporter: Satshabad Khalsa Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Is this a bug? maybe I just don't understand. The documentation says: Returns a sequence of the map's values. Is nil a sequence?

This caused an unexpected nil to propagate through a bunch of list processing stuff.



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

An empty sequence is represented by nil, so this is consistent. For example: (seq (range 0)) => nil





[CLJ-1403] ns-resolve might throw ClassNotFoundException but should return nil Created: 14/Apr/14  Updated: 14/Apr/14

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   

The doc of ns-resolve states that in case the symbol cannot be resolved, it should return nil.

user=> (doc ns-resolve)
-------------------------
clojure.core/ns-resolve
([ns sym] [ns env sym])
  Returns the var or Class to which a symbol will be resolved in the
  namespace (unless found in the environment), else nil.  Note that
  if the symbol is fully qualified, the var/Class to which it resolves
  need not be present in the namespace.
nil

However if the symbol contains dots and is not a resolvable Class, a ClassNotFoundException is thrown

user=> (ns-resolve *ns* 'foo.bar)
ClassNotFoundException foo.bar  java.net.URLClassLoader$1.run (URLClassLoader.java:372)
user=> (pst *e)
ClassNotFoundException foo.bar
	java.net.URLClassLoader$1.run (URLClassLoader.java:372)
	java.net.URLClassLoader$1.run (URLClassLoader.java:361)
	java.security.AccessController.doPrivileged (AccessController.java:-2)
	java.net.URLClassLoader.findClass (URLClassLoader.java:360)
	clojure.lang.DynamicClassLoader.findClass (DynamicClassLoader.java:61)
	java.lang.ClassLoader.loadClass (ClassLoader.java:424)
	java.lang.ClassLoader.loadClass (ClassLoader.java:357)
	java.lang.Class.forName0 (Class.java:-2)
	java.lang.Class.forName (Class.java:340)
	clojure.lang.RT.classForName (RT.java:2065)
	clojure.lang.Compiler.maybeResolveIn (Compiler.java:6963)
	clojure.core/ns-resolve (core.clj:4026)
nil


 Comments   
Comment by Alex Miller [ 14/Apr/14 2:07 PM ]

Can you include the (pst *e) ?

Comment by Nicola Mometto [ 14/Apr/14 2:10 PM ]

Added result of (pst *e) in the description





[CLJ-1402] sort-by calls keyfn more times than is necessary Created: 11/Apr/14  Updated: 11/Apr/14

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

Type: Enhancement Priority: Minor
Reporter: Steve Kim Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance


 Description   

clojure.core/sort-by evaluates keyfn for every pairwise comparison. This is wasteful when keyfn is expensive to compute.

user=> (def keyfn-calls (atom 0))
#'user/keyfn-calls
user=> (defn keyfn [x] (do (swap! keyfn-calls inc) x))
#'user/keyfn
user=> @keyfn-calls
0
user=> (sort-by keyfn (repeatedly 10 rand))
(0.1647483850582695 0.2836687590331822 0.3222305842748623 0.3850390922996001 0.41965440953966326 0.4777580378736771 0.6051704988802923 0.659376178201709 0.8459820304223701 0.938863131161208)
user=> @keyfn-calls
44


 Comments   
Comment by Steve Kim [ 11/Apr/14 11:46 AM ]

CLJ-99 is a similar issue





[CLJ-1401] CompilerException / IllegalStateException when reloading namespaces Created: 10/Apr/14  Updated: 12/Apr/14

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

Type: Defect Priority: Minor
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, errormsgs


 Description   
user> (ns op)
nil
op> (defn * [a b] (clojure.core/* a b))
WARNING: * already refers to: #'clojure.core/* in namespace: op, being replaced by: #'op/*
#'op/*
op> (ns use-op (:require [op :refer :all]))
WARNING: * already refers to: #'clojure.core/* in namespace: use-op, being replaced by: #'op/*
nil
use-op> (ns use-op (:require [op :refer :all]))
IllegalStateException * already refers to: #'op/* in namespace: use-op  clojure.lang.Namespace.warnOrFailOnReplace (Namespace.java:88)
use-op> (clojure.repl/pst *e)
IllegalStateException * already refers to: #'op/* in namespace: use-op
	clojure.lang.Namespace.warnOrFailOnReplace (Namespace.java:88)
	clojure.lang.Namespace.reference (Namespace.java:110)
	clojure.lang.Namespace.refer (Namespace.java:168)
	clojure.core/refer (core.clj:3920)
	use-op/eval2402/loading--4958--auto----2403 (NO_SOURCE_FILE:1)
	use-op/eval2402 (NO_SOURCE_FILE:1)
	clojure.lang.Compiler.eval (Compiler.java:6703)
	clojure.lang.Compiler.eval (Compiler.java:6692)
	clojure.lang.Compiler.eval (Compiler.java:6666)
	clojure.core/eval (core.clj:2927)
	clojure.main/repl/read-eval-print--6625/fn--6628 (main.clj:239)
	clojure.main/repl/read-eval-print--6625 (main.clj:239)

I would expect (at worst) a similar warning to the initial namespace loading, rather than an exception here.



 Comments   
Comment by Alex Miller [ 11/Apr/14 8:26 AM ]

Could you put together a better reproducible test case for this that does not depend on core.matrix? Also, please include the (pst *e) when it occurs.

Comment by Andy Fingerhut [ 11/Apr/14 10:19 AM ]

I have tried the smallest possible Leiningen project I could think of that would cause the warnings about redefinitions, to see if I could get the exception to occur. 'lein new try1' to create the skeleton project, then edit src/try1/core.clj to contain only the following function definitions:

(defn merge
  "This definition of merge replaces clojure.core/merge"
  [x y]
  (- x y))

(defn *
  [x y]
  (* x y))

Then start a REPL with 'lein repl', and I see this behavior:

user=> (require '[try1.core :as c])
WARNING: merge already refers to: #'clojure.core/merge in namespace: try1.core, being replaced by: #'try1.core/merge
WARNING: * already refers to: #'clojure.core/* in namespace: try1.core, being replaced by: #'try1.core/*
nil
user=> (require '[try1.core :as c] )
nil
user=> (require '[try1.core :as c] :reload)
WARNING: merge already refers to: #'clojure.core/merge in namespace: try1.core, being replaced by: #'try1.core/merge
WARNING: * already refers to: #'clojure.core/* in namespace: try1.core, being replaced by: #'try1.core/*
nil

Ths all looks like behavior as I would expect, and I did not see the exception that Mike reports.

It seems that either Ctrl+Alt+L in Counterclockwise does something different than (require ... :reload), or there is something different about Mike's namespace in addition to redefining names in clojure.core that is causing the problem.

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

Marking this as NR for now - would be happy to see it reopened with an easily reproducible test case.

Comment by Mike Anderson [ 12/Apr/14 12:41 AM ]

To reproduce:

(ns op)
(defn * [a b] (clojure.core/* a b)) ;; gives warning
(ns use-op (:require [op :refer :all])) ;; gives warning
(ns use-op (:require [op :refer :all])) ;; gives error!

I believe Counterclockwise is simply loading the namespace again with CTRL-Alt+L, which is causing the ns form to be re-executed.

The docstring implies that ns can be used multiple times ("Sets ns to the namespace named by name (unevaluated), creating it if needed") so I would certainly expect multiple invocations of ns to be a no-op





[CLJ-1400] Error "Can't refer to qualified var that doesn't exist" should name the bad symbol Created: 09/Apr/14  Updated: 26/Jun/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: Howard Lewis Ship Assignee: Scott Bale
Resolution: Unresolved Votes: 0
Labels: Compiler, errormsgs
Environment:

OS X


Attachments: File clj-1400-1.diff    
Patch: Code and Test
Approval: Vetted

 Description   

Def of var with a ns that doesn't exist will yield this error:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Can't refer to qualified var that doesn't exist, compiling:(NO_SOURCE_PATH:1:1)

Cause: Compiler.lookupVar() returns null if the ns in a qualified var does not exist yet.

Proposed: The error message would be improved by naming the symbol and throwing a CompilerException with file/line/col info. It's not obvious, but this may be the only case where this error occurs. If so, the error message could be more specific that the ns is the part that doesn't exist.

Patch:

Screened by:



 Comments   
Comment by Scott Bale [ 25/Jun/14 9:58 AM ]

This looks to me like relatively low hanging fruit unless I'm missing something; assigning to myself.

Comment by Scott Bale [ 26/Jun/14 11:23 PM ]

Patch clj-1400-1.diff to Compiler.java.

With this patch the example would now look like:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

I'm not sure the if(namesStaticMember(sym)) [see below], and the 2nd branch, is even necessary. Just by inspection I suspect it is not.

[footnote]

public static boolean namesStaticMember(Symbol sym){
	return sym.ns != null && namespaceFor(sym) == null;
}
Comment by Scott Bale [ 26/Jun/14 11:24 PM ]

patch: code and test

Comment by Scott Bale [ 26/Jun/14 11:27 PM ]

I tested on an actual source file, and the exception message included the file/line/col info as desired:

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)




[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-1398] Update URLs in javadoc.clj Created: 02/Apr/14  Updated: 09/May/14

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

Type: Enhancement Priority: Trivial
Reporter: Eli Lindsey Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0001-update-apache-commons-javadoc-location.patch     Text File 0002-add-javadoc-lookup-for-guava-and-apache-commons-lang.patch     Text File 0003-add-javadoc-lookup-for-jdk8.patch    
Patch: Code

 Description   

Three minor fixes/enhancements to javadoc.clj:

0001 corrects the URLs for apache commons javadoc (the ones used in javadoc.clj no longer resolve).
0002 adds javadoc lookup for guava and apache commons lang3.
0003 adds javadoc lookup for jdk8.

(Note: contributor agreement is in the mail)



 Comments   
Comment by Andy Fingerhut [ 04/Apr/14 11:22 AM ]

Eli, thanks for the patches. It appears that you are not currently on the list of Clojure contributors here: http://clojure.org/contributing

It is the policy of the Clojure team only to incorporate patches submitted by people who have signed and submitted a Clojure CA. Were you interested in doing that?

Comment by Eli Lindsey [ 04/Apr/14 11:27 AM ]

> It is the policy of the Clojure team only to incorporate patches submitted by people who have signed and submitted a Clojure CA. Were you interested in doing that?

Yup! I mailed off the CA to Rich on Wednesday when this was filed; should be arriving shortly.

Comment by Eli Lindsey [ 09/May/14 8:18 PM ]

Just to note - Clojure CA went through and I'm listed on the contributors page now.





[CLJ-1397] exception testing broken over map Created: 01/Apr/14  Updated: 01/Apr/14  Resolved: 01/Apr/14

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

Type: Defect Priority: Major
Reporter: MG Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: test
Environment:

Linux ... 3.2.0-56-generic-pae #86-Ubuntu SMP ... i686 i686 i386 GNU/Linux



 Description   

Expected: Tests pass
Actual: Two tests fail
To reproduce, run the following test file:

(ns pe.test-test
(:require [clojure.test :refer :all]))
(defn throwexc [m] (throw (Exception. m)))
(defn throwass [m] (assert false m))
(defn nestexc [] (throwexc "exc"))
(defn nestass [] (throwass "ass"))
(defn nestmapexc [] (map throwexc '("a" "b" "c")))
(defn nestmapass [] (map throwass '("a" "b" "c")))
(deftest exceptions-and-assertions-test
(testing "throwing"
(is (thrown? Exception (throwexc "exc")))
(is (thrown? AssertionError (throwass "ass"))))
(testing "nesting"
(is (thrown? Exception (nestexc)))
(is (thrown? AssertionError (nestass))))
(testing "nesting over map"
(is (thrown? Exception (nestmapexc)))
(is (thrown? AssertionError (nestmapass)))))



 Comments   
Comment by MG [ 01/Apr/14 7:25 AM ]

Clarification: The two assertions in "nesting over map" fails, the other four assertions succeed.

Comment by Nicola Mometto [ 01/Apr/14 7:30 AM ]

map is lazy, the exception is never thrown because the sequence is never realized.
either wrap the map in a dorun or use a doseq instead of a map.
map should not be used for side-effecting





[CLJ-1396] Bad link Created: 01/Apr/14  Updated: 01/Apr/14  Resolved: 01/Apr/14

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

Type: Defect Priority: Trivial
Reporter: MG Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

http://richhickey.github.io/clojure/



 Description   

http://richhickey.github.io/clojure/
The issue tracker link points to Assembla.



 Comments   
Comment by Nicola Mometto [ 01/Apr/14 7:31 AM ]

That site is deprecated and so is the repo that's hosting it.
This is the updated one http://clojure.github.io/clojure/

Comment by Alex Miller [ 01/Apr/14 7:45 AM ]

Nicola, I agree with your assessment here but only screeners should be closing tickets, thanks.

Comment by Nicola Mometto [ 01/Apr/14 7:50 AM ]

Alex, sorry, I didn't know this, I will refrain from doing so in the future.





[CLJ-1395] get should deref delay, refs atoms etc Created: 01/Apr/14  Updated: 01/Apr/14  Resolved: 01/Apr/14

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

Type: Enhancement Priority: Minor
Reporter: Gerrit Jansen van Vuuren Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

(get (delay {:a 1}) :a)
;; nil

(get {:a 1} :a)
;; 1

The above situation can happen easily and there is no way to refactor or check that all code doing gets is in fact doing deref before doing get. Given that everything is dynamic typing, changing a single value from a map to delay or ref can make large parts of the code fail silently on nil for gets.

get would be more consistent (with what is expected of it) if it was to check for refs, atoms and delays and do a deref.



 Comments   
Comment by Alex Miller [ 01/Apr/14 7:43 AM ]

I think this goes beyond what get should do. In regards to silent failure, that is covered by CLJ-1107 which would cause this to throw an exception instead.





[CLJ-1394] Print multi method dispatch values in the exception messages. Created: 31/Mar/14  Updated: 01/Apr/14

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

Type: Enhancement Priority: Trivial
Reporter: Roman Scherer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs

Attachments: File pr-str-dispatch-value-safe.diff    
Patch: Code and Test

 Description   

The error messages of defmulti are at the moment not as helpful as they could be under certain circumstances. Calling this multi method with a lazy seq as it's dispatch argument raises the following exception:

(defmulti test-multi identity)
(defmethod test-multi 1 [arg] true)

(test-multi (map identity [:x]))
;=> java.lang.IllegalArgumentException: No method in multimethod 'test-multi' for dispatch value: clojure.lang.LazySeq@3c6f1187

Sometimes it would be useful to actually see which values are in the lazy seq being dispatched on. A better error message could look like
this for example:

(test-multi (map identity [:x]))
;=> java.lang.IllegalArgumentException: No method in multimethod 'test-multi' for dispatch value (:x) of class clojure.lang.LazySeq

This patch addresses this issue by formatting the dispatch value via `pr-str` and printing the class before it is passed to the exception constructor. The same is also done for the methods in MultiFn.java that throw a dispatch value as part of their exception message.



 Comments   
Comment by Nicola Mometto [ 31/Mar/14 8:22 PM ]

What if the value is infinite lazy-seq?

Comment by Roman Scherer [ 01/Apr/14 2:50 AM ]

Nicola, I forgot those. But I think infinite sequences could be handled with:

(set! *print-length* 10)

I'll try it out and will update the patch later.

Any other edge cases in mind?

Comment by Roman Scherer [ 01/Apr/14 2:28 PM ]

After having read "Controlling run-away trains, onions, and exercise
bikes" [1] I now bind print-length and print-size when building
the error message. This helps when not being able to dispatch on this
for example:

(test-multi (let [x (atom 0)] (reset! x {:deeper x})))

However I'm not sure if this helps in the following case, where
dispatching would fail on an infinite seq.

(test-multi (iterate inc 0))

The above doesn't terminate in Clojure 1.6.0, nor does it when binding
print-length like the attached patch does.

[1] http://blog.n01se.net/blog-n01se-net-p-85.html





[CLJ-1393] clojure.string/trim doesn't trim null character Created: 28/Mar/14  Updated: 28/Mar/14  Resolved: 28/Mar/14

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

Type: Defect Priority: Minor
Reporter: Ryan Fowler Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: string


 Description   

CLJ-935 changed clojure.string/trim to not trim all the characters less than or equal to \u0020 as Java does.

I noticed this because base64 uses null characters to pad the end of encoding blocks.

Clojure 1.6.0's trim leaves the null character in:
user=> (.length (clojure.string/trim "\u0000"))
1

java.lang.String's trim takes it out:
user=> (.length (.trim "\u0000"))
0

Here are the first 21 unicode characters and what Character/isWhitespace says about them.

(dotimes [n 0x20] (printf "
u%04x - %b\n" n (Character/isWhitespace n)))
\u0000 - false
\u0001 - false
\u0002 - false
\u0003 - false
\u0004 - false
\u0005 - false
\u0006 - false
\u0007 - false
\u0008 - false
\u0009 - true
\u000a - true
\u000b - true
\u000c - true
\u000d - true
\u000e - false
\u000f - false
\u0010 - false
\u0011 - false
\u0012 - false
\u0013 - false
\u0014 - false
\u0015 - false
\u0016 - false
\u0017 - false
\u0018 - false
\u0019 - false
\u001a - false
\u001b - false
\u001c - true
\u001d - true
\u001e - true
\u001f - true



 Comments   
Comment by Alex Miller [ 28/Mar/14 12:27 PM ]

The choice was made in CLJ-935 to consistently define whitespace as Character.isWhitespace() across trim, triml, and trimr. There are many possible ways to define "space" (at least two as we see here). If your trimming needs differ from the standard library, then you'll probably need to define your own functions to trim your data. You can still use Java interop to call String.trim() directly if that happens to match your needs.

Comment by Ryan Fowler [ 28/Mar/14 1:03 PM ]

Indeed, it's an easy workaround to use Java interop once you figure out what your problem is.

It's just unintuitive that the character generally used for string termination isn't trimmed by clojure.string/trim.





[CLJ-1392] AOT vs "var already exists warning" results in NPE Created: 26/Mar/14  Updated: 26/Mar/14  Resolved: 26/Mar/14

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

Type: Defect Priority: Major
Reporter: Aaron Cohen Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: aot, compiler


 Description   

I just saw this thread on the cascalog list: https://groups.google.com/forum/#!topic/cascalog-user/Pe5QIpmU0vA

Apparently the "WARNING: some? already refers to: #'clojure.core/some? in namespace: jackknife.seq, being replaced by: #'jackknife.seq/some?" results in a NullPointerException when trying to AOT a namespace that results in the message being output.

Reproducer:

1) lein new aotFail
2) Edit project.clj
;add as appropriate
:aot :all
:dependencies [[org.clojure/clojure "1.6.0"]
[cascalog "2.0.0"]]
2) Add "(:use cascalog.api)" to the ns block of src/aotFail/core.clj
3) lein compile

Output:

Compiling aotFail.core
WARNING: some? already refers to: #'clojure.core/some? in namespace: jackknife.seq, being replaced by: #'jackknife.seq/some?
Exception in thread "main" java.lang.NullPointerException, compiling:(api.clj:1:1)
at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3558)
at clojure.lang.Compiler.compile1(Compiler.java:7226)
at clojure.lang.Compiler.compile1(Compiler.java:7216)
at clojure.lang.Compiler.compile(Compiler.java:7292)
at clojure.lang.RT.compile(RT.java:398)
at clojure.lang.RT.load(RT.java:438)
at clojure.lang.RT.load(RT.java:411)
at clojure.core$load$fn__5066.invoke(core.clj:5641)
at clojure.core$load.doInvoke(core.clj:5640)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.core$load_one.invoke(core.clj:5446)
at clojure.core$load_lib$fn__5015.invoke(core.clj:5486)
at clojure.core$load_lib.doInvoke(core.clj:5485)
at clojure.lang.RestFn.applyTo(RestFn.java:142)
at clojure.core$apply.invoke(core.clj:626)
at clojure.core$load_libs.doInvoke(core.clj:5528)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:628)
at clojure.core$use.doInvoke(core.clj:5618)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at cascalog2.core$loading_4958auto_.invoke(core.clj:1)
at clojure.lang.AFn.applyToHelper(AFn.java:152)
at clojure.lang.AFn.applyTo(AFn.java:144)
at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3553)
at clojure.lang.Compiler.compile1(Compiler.java:7226)
at clojure.lang.Compiler.compile1(Compiler.java:7216)
at clojure.lang.Compiler.compile(Compiler.java:7292)
at clojure.lang.RT.compile(RT.java:398)
at clojure.lang.RT.load(RT.java:438)
at clojure.lang.RT.load(RT.java:411)
at clojure.core$load$fn__5066.invoke(core.clj:5641)
at clojure.core$load.doInvoke(core.clj:5640)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.core$load_one.invoke(core.clj:5446)
at clojure.core$compile$fn__5071.invoke(core.clj:5652)
at clojure.core$compile.invoke(core.clj:5651)
at user$eval19.invoke(form-init2092370125048380878.clj:1)
at clojure.lang.Compiler.eval(Compiler.java:6703)
at clojure.lang.Compiler.eval(Compiler.java:6693)
at clojure.lang.Compiler.load(Compiler.java:7130)
at clojure.lang.Compiler.loadFile(Compiler.java:7086)
at clojure.main$load_script.invoke(main.clj:274)
at clojure.main$init_opt.invoke(main.clj:279)
at clojure.main$initialize.invoke(main.clj:307)
at clojure.main$null_opt.invoke(main.clj:342)
at clojure.main$main.doInvoke(main.clj:420)
at clojure.lang.RestFn.invoke(RestFn.java:421)
at clojure.lang.Var.invoke(Var.java:383)
at clojure.lang.AFn.applyToHelper(AFn.java:156)
at clojure.lang.Var.applyTo(Var.java:700)
at clojure.main.main(main.java:37)
Caused by: java.lang.NullPointerException
at clojure.lang.Compiler$ObjExpr.emitVar(Compiler.java:4944)
at clojure.lang.Compiler$DefExpr.emit(Compiler.java:437)
at clojure.lang.Compiler.compile1(Compiler.java:7225)
at clojure.lang.Compiler.compile(Compiler.java:7292)
at clojure.lang.RT.compile(RT.java:398)
at clojure.lang.RT.load(RT.java:438)
at clojure.lang.RT.load(RT.java:411)
at clojure.core$load$fn__5066.invoke(core.clj:5641)
at clojure.core$load.doInvoke(core.clj:5640)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.core$load_one.invoke(core.clj:5446)
at clojure.core$load_lib$fn__5015.invoke(core.clj:5486)
at clojure.core$load_lib.doInvoke(core.clj:5485)
at clojure.lang.RestFn.applyTo(RestFn.java:142)
at clojure.core$apply.invoke(core.clj:626)
at clojure.core$load_libs.doInvoke(core.clj:5524)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:628)
at clojure.core$use.doInvoke(core.clj:5618)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at cascalog.api$loading_4958auto_.invoke(api.clj:1)
at clojure.lang.AFn.applyToHelper(AFn.java:152)
at clojure.lang.AFn.applyTo(AFn.java:144)
at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3553)



 Comments   
Comment by Nicola Mometto [ 26/Mar/14 12:42 PM ]

Duplicate of http://dev.clojure.org/jira/browse/CLJ-1241





[CLJ-1391] Allow logical operators on assert expressions Created: 26/Mar/14  Updated: 26/Mar/14

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

Type: Defect Priority: Minor
Reporter: Sanel Zukan Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: clojure.test


 Description   

With current code, it is not possible to express logical operators on some clojure.test assert expressions. For example, this will work:

(is (thrown? Exception <some-expression>))

however, here will fail:

(is (not (thrown? Exception <some-expression>)))

since '(thrown?)' is not an ordinary function, but looks like. This also adds confusion which is hard to explain to others unless '(is)' code was shown first.

Also, if the one would like to implement macro (e.g. 'is-not-thrown?') in form:

(defmacro is-not-thrown? [e expr]
  `(is (not ('thrown? ~e ~expr))))

which could be even more confusing for a person not knowing how 'thrown?' is implemented.






[CLJ-1390] pprint a GregorianCalendar results in Arity exception Created: 25/Mar/14  Updated: 10/Apr/14

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

Type: Defect Priority: Minor
Reporter: Steve Suehs Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: print

Attachments: Text File 0001-CLJ-1390.patch     Text File 0002-CLJ-1390-test2.patch     Text File 0002-CLJ-1390-test.patch     Text File CLJ-1390-pprint-GregorianCalendar.patch    
Patch: Code

 Description   

What I was doing: attempting to pretty-print nested structures from Things mac app, which include instances of java.util.GregorianCalendar.
What I expected to happen: output should have an #inst very much like printing java.util.Date.
What happened instead: ArityException Wrong number of args (4) passed to: pprint$pretty-writer$fn

thingsplay.core=> (def nowish (java.util.GregorianCalendar.))
#'thingsplay.core/nowish
thingsplay.core=> nowish
#inst "2014-03-25T22:43:29.240-05:00"
thingsplay.core=> (require 'clojure.pprint)
nil
thingsplay.core=> (pprint nowish)
ArityException Wrong number of args (4) passed to: pprint$pretty-writer$fn  clojure.lang.AFn.throwArity (AFn.java:437)
#inst "
thingsplay.core=> (simple-dispatch nowish)
#inst "2014-03-25T22:43:29.240-05:00"nil
thingsplay.core=> nowish
#inst "2014-03-25T22:43:29.240-05:00"
thingsplay.core=> (write nowish)
ArityException Wrong number of args (4) passed to: pprint$pretty-writer$fn  clojure.lang.AFn.throwArity (AFn.java:437)
#inst "


 Comments   
Comment by Norman Richards [ 29/Mar/14 4:15 PM ]

The print-calendar function introduced in CLJ-928 doesn't work with clojure.pprint/pretty-writer since pretty-writer does not correctly implement the java.io.Writer interface. It only implements write(String) but print-calendar wants write(String,int,int) among others. Although pretty-writer should probably correctly implement java.io.Writer, it's pretty easy to rewrite print-calendar to use the supported subset of java.io.Writer that is implemented.

Comment by Steve Suehs [ 29/Mar/14 4:20 PM ]

Thank you, "random person at the Austin Clojure Hack Day" "that I don't know" that has a CA in place. You are awesome!

See you at the next Austin Clojure Meetup.

-s

Comment by Andy Fingerhut [ 04/Apr/14 2:51 PM ]

Norman, it would be good if the patch included a few test cases, especially ones that fail without the patch, and succeed with the patch.

Comment by Norman Richards [ 04/Apr/14 3:09 PM ]

Absolutely. I have no idea how test cases work on Clojure core, but I assume it should be easy enough to do.

Comment by Andy Fingerhut [ 04/Apr/14 3:15 PM ]

I would recommend looking at the following file in the Clojure repo, which already contains other pprint tests. It shouldn't be too difficult to get an idea from one or more of the tests there. Actually those might be slightly unusual in that many of them use a simple-tests macro defined in file test_helper.clj that most of the Clojure tests do not use, but ask questions if you have trouble, e.g. on the Clojure Google group or IRC channel.

test/clojure/test_clojure/pprint/test_pretty.clj

Comment by Steve Suehs [ 04/Apr/14 7:50 PM ]

Alright...you two are inspiring me to go work on getting my CA in.

Comment by Norman Richards [ 04/Apr/14 8:31 PM ]

Test case attached. Apply the test patch, "mvn test" fails. Apply the fix, test passes.

Comment by Andy Fingerhut [ 06/Apr/14 3:32 PM ]

It would be better if the "is" were of the form:

(is (= calculated-value "constant string to compare against") "string to show if test fails")

rather than just (is calculated-value "string to show if test fails"). The second form will fail if calculating the value throws an exception, but only the first form will calculate it, and then verify that the value is the expected one (and fail if it is not the expected one).

Comment by Norman Richards [ 07/Apr/14 10:49 AM ]

Ok - here's an alternative test case. I'm less happy with this test case, since I have to add the TimeZone and make assumptions about how the specifics of how the pretty printer formats. But, it does test the fix adequately, so if you like the test2 patch better, that's perfectly fine with me.

Comment by Steve Miner [ 10/Apr/14 4:23 PM ]

I would rather fix the actual bug in pretty_writer.clj. The proxy needs to support more of the java.io.Writer interface. I think adding another arity to the write method would work. Something like:

(write 
   ...
  ([x off len]
      (.write this (subs (str x) off (+ off len)))))
Comment by Steve Miner [ 10/Apr/14 4:38 PM ]

CLJ-1390-pprint-GregorianCalendar.patch fixes the pretty_writer.clj proxy to support the missing version of the write method. Includes the same test as the previous patch.





[CLJ-1389] Re-loading a namespace ignores metadata specified for the namespace Created: 20/Mar/14  Updated: 20/Mar/14

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

Type: Defect Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: metadata, namespace, repl


 Description   

Using the REPL I added some metadata to a namespace and reloaded it.

(ns io.aviso.rook-test5)

to

(ns io.aviso.rook-test5
  "A testing namespace"
  {:inherted   :namespace
   :overridden :namespace})

But requesting the meta data yields nil:

(-> 'io.aviso.rook-test5 find-ns meta)
=> nil

I have tested a few variations, such as putting the metadata on the symbol instead of providing an attribute map. In all cases, the metadata from before the load persists.

Using remove-ns before re-loading the namespace does the right thing ... the metadata shows up as expected.






[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-1387] reduce-kv on hash map ignores reduced objects in large maps Created: 18/Mar/14  Updated: 22/Mar/14  Resolved: 22/Mar/14

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

Type: Defect Priority: Major
Reporter: Jozef Wagner Assignee: Jozef Wagner
Resolution: Completed Votes: 0
Labels: None
Environment:

JRE 7


Attachments: File clj-1387.diff     File clj-1387-v2.diff     File clj-1387-v3.diff    
Patch: Code and Test
Approval: Ok

 Description   

Larger hash maps have nested INodes. As kvreduce implementations in INodes dereference reduced objects, parent INodes continue to reduce.

user=> (defn test-reduce-kv [m] (reduce-kv (fn [_ k v] (when (== 1 k) (reduced :foo))) nil m))
#'user/test-reduce-kv
user=> (test-reduce-kv (zipmap (range 3) (range 3)))
:foo
user=> (test-reduce-kv (zipmap (range 300) (range 300)))
nil

Dereferencing reduced objects should happen only PersistentHashMap/kvreduce - intermediate nodes should pass the Reduced object along.

Patch: clj-1387-v3.diff
Screened-by:



 Comments   
Comment by Alex Miller [ 18/Mar/14 5:11 PM ]

I updated the patch to use a generative test that will try many combinations of map size and the reduced index to bail out on. This test failed before applying the source patch and passes with it.

Comment by Rich Hickey [ 21/Mar/14 7:33 AM ]

if(root != null){ - return root.kvreduce(f,init); + init = root.kvreduce(f,init); + if(RT.isReduced(init)) + return ((IDeref)init).deref(); }

Turns code that always had a return into code that sometimes does.

Comment by Alex Miller [ 21/Mar/14 9:07 AM ]

Added new version of patch that retains the return flow and doesn't fall through.





[CLJ-1386] Add transient? predicate Created: 17/Mar/14  Updated: 23/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Devin Walters Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, transient
Environment:

N/A


Attachments: Text File 0001-Add-transient-predicate.patch     Text File 0002-Add-transient-predicate.patch    
Patch: Code and Test
Approval: Triaged

 Description   

I've encountered situations where I wanted to check whether something was transient in order to know whether I should call assoc! or assoc, conj! or conj, etc.

This patch adds `transient?` as a predicate fn.



 Comments   
Comment by Alex Miller [ 17/Mar/14 10:21 AM ]

Patch needs a docstring and a test.

Comment by Devin Walters [ 17/Mar/14 4:42 PM ]

Alex: I figured that would be the case! Sorry about that. I've updated the patch. It now includes a docstring and has tests of `transient?` for #{}, [], and {}.

Thanks!

Comment by Alex Miller [ 17/Mar/14 9:48 PM ]

Thanks - please don't use the labels "patch" or "test" - those are covered by the Patch field.

Comment by Devin Walters [ 18/Mar/14 9:17 AM ]

Ah, sorry for the mixup Alex. I assumed you removed "patch" as a label the first time around to flag this ticket as still needing a vetted patch. My mistake.

Comment by Andy Fingerhut [ 21/Mar/14 1:42 PM ]

Patch 0001-Add-transient-predicate.patch dated Mar 17, 2014 applies cleanly to latest Clojure master, but fails a test because the new function transient? has no :added metadata. See most other Clojure functions in clojure.core for examples.

It also generates a new warning while running tests:

WARNING: transient? already refers to: #'clojure.core/transient? in namespace: clojure.test-clojure.data-structures, being replaced by: #'clojure.test-clojure.data-structures/transient?

There is an older (but equivalent) definition of transient? in test file data_structures.clj that should be removed when adding it to clojure.core

Comment by Devin Walters [ 22/Mar/14 11:29 PM ]

@Andy, the reason I did not add :added metadata is because I do not know if/when this patch will be accepted, and as a result, I don't really know if it will sneak into 1.6.X or 1.7. For now, I've put it in as 1.7. If it's in the running to be added sooner than that, let me know and I'll adjust it.

RE: The warning. Good catch. I've submitted a new patch which removes the private version of transient? from data_structures.clj. All tests pass.

Edit to Add: The latest patch as of this comment is now 0002-Add-transient-predicate.patch.





[CLJ-1385] Docstrings for `conj!` and `assoc!` should suggest using the return value; effect not always in-place Created: 16/Mar/14  Updated: 11/May/14

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

Type: Enhancement Priority: Minor
Reporter: Pyry Jahkola Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: collections, docstring

Attachments: Text File CLJ-1385-reword-docstrings-on-transient-update-funct.patch    
Patch: Code

 Description   

The docstrings of both `assoc!` and `conj!` say "Returns coll.", suggesting the transient edit happens always in-place, `coll` being the first argument.

However, the fact that the following example omits the key `8` in its result proves that in-place edits aren't always the case:

(let [a (transient {})]
      (dotimes [x 9]
        (assoc! a x :ok))
      (persistent! a))
    ;;=> {0 :ok, 1 :ok, 2 :ok, 3 :ok, 4 :ok, 5 :ok, 6 :ok, 7 :ok}

Instead, programmers should be guided towards using constructs like `reduce` with transients:

(persistent! (reduce #(assoc! %1 %2 :ok)
                 (transient {})
                 (range 9)))
    ;;=> {0 :ok, 1 :ok, 2 :ok, 3 :ok, 4 :ok, 5 :ok, 6 :ok, 7 :ok, 8 :ok}

The easiest way to achieve this is by changing the docstrings of (at least) `conj!` and `assoc!` to not read "Returns coll." but instead tell that the change is destructive.



 Comments   
Comment by Alex Miller [ 16/Mar/14 8:49 AM ]

When modifying transient collections, it is required to use the collection returned from functions like assoc!. The ! here indicates its destructive nature. The transients page (http://clojure.org/transients) describes the calling pattern pretty explicitly: "You must capture and use the return value in the next call."

I do not agree that we should be guiding programmers away from using functions like assoc! – transients are used as a performance optimization and using assoc! or conj! in a loop is often the fastest version of that. However I do think it would be helpful to make the docstring more explicit.

Comment by Gary Fredericks [ 05/Apr/14 10:23 AM ]

Alex I think you must have misread the ticket – the OP is suggesting guiding toward using the return value of assoc!, not avoiding assoc! altogether.

And the docstring is not simply inexplicit, it's actually incorrect specifically in the case that the OP pointed out. conj! and assoc do not return coll at the point where array-maps transition to hash-maps, and the fact that they do otherwise is supposed to be an implementation detail as far as I understand it.

Comment by Alex Miller [ 05/Apr/14 11:55 AM ]

@Gary - you're right, I did misread that.

assoc and conj both explicitly say "return a new collection" whereas assoc! and conj! say "Returns coll." I read that as "returns the modified collection" without regard to whether it's the identical instance, but I can read it your way too.

Would saying "Returns updated collection." transmit the right idea? Using "collection" instead of "coll" removes the concrete tie to the variable and "updated" hints more strongly that you should use the return value.

Comment by Pyry Jahkola [ 05/Apr/14 12:47 PM ]

@Alex, that update makes it sound right to me, FWIW.

Comment by Gary Fredericks [ 05/Apr/14 2:37 PM ]

Yeah, I think that's better. Thanks Alex. I'd be happy to submit a patch for that but I'm assuming patches are too heavy for this kind of change?

Comment by Andy Fingerhut [ 06/Apr/14 3:35 PM ]

Patches are exactly what has been done in the past for this kind of change, if it is in a doc string and not on the clojure.org web page.

Comment by Alex Miller [ 06/Apr/14 4:13 PM ]

Yup, patch desired.

Comment by Gary Fredericks [ 06/Apr/14 5:32 PM ]

Glad I asked.

Patch is attached that also updates the docstring for pop! which had the same issue, though arguably it's less important since afaik pop! does always return the identical collection (but I don't think this is part of the contract).





[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-1383] Should name throw on nil? Created: 14/Mar/14  Updated: 15/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: John Chijioke Assignee: Jozef Wagner
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File clj-1383.diff    
Patch: Code and Test

 Description   

The name function throws NullPointerException on nil. Since the name function is about obtaining the string form of a specific object it should not throw on nil. It should just return the nil object as the str fn does.



 Comments   
Comment by Jozef Wagner [ 15/Mar/14 3:23 AM ]

added patch with test





[CLJ-1382] Vector sort order should be specified sufficiently to embrace sort-by-juxt Created: 13/Mar/14  Updated: 15/Mar/14  Resolved: 15/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: data-structures, documentation, idiom


 Description   

Vectors of equal length sort in a way that seems natural – by
comparing their 0th elements, then their 1st, etc., until something
is different:

user> (def vv '(["c" 9] ["a" 100] ["a" 33] ["b" 8]))
#'user/vv
user> (sort vv)
(["a" 33] ["a" 100] ["b" 8] ["c" 9])

This property enables a blisteringly wonderful idiom for sorting
records by multiple keys:

user> (def mm (->> vv (map (fn [[p q]] {:p p :q q}))))
#'user/mm
user> (sort-by (juxt :p :q) mm)
({:p "a", :q 33} {:p "a", :q 100} {:p "b", :q 8} {:p "c", :q 9})

The sort-by-juxt idiom was described on briancarper.net[1], where it
was refined for Clojure 1.1 by Malcolm Sparks. Andy Fingerhut has
also written about it, thoroughly.[2]

Such lore gives it the odor of respectability, but the sort-by-juxt
idiom takes liberties beyond the documented behavior ("contract") of
vectors. APersistentVector indulges the idiom, but the clojure.org
Data Structures page does not say how vectors should compare.

The vector specification should be bolstered with enough traits of
vectors' sorting behavior to make the sort-by-juxt idiom safe to use
wherever Clojure might be implemented.

[1] http://briancarper.net/blog/488/sort-a-clojure-map-by-two-or-more-keys

[2] https://github.com/jafingerhut/thalia/blob/master/doc/other-topics/comparators.md



 Comments   
Comment by Alex Miller [ 13/Mar/14 9:52 PM ]

I don't understand what this ticket is asking for.

Comment by Andy Fingerhut [ 13/Mar/14 10:32 PM ]

It sounds like he is asking that the doc of clojure.core/compare say that vectors of equal length are compared in lexicographic order.

Comment by Phill Wolf [ 15/Mar/14 1:07 PM ]

"(sort-by (juxt" relies on a feature of vectors that the Clojure documentation does not guarantee. But using juxt in this way is part of the cultural fabric and helps make concise programs that "read like a definition" of the problem, to quote Halloway in "Programming Clojure". Therefore, let's document the sort order of equal-length vectors. I looked for this information on the Data Structures page, which has a section on vectors.

Comment by Alex Miller [ 15/Mar/14 11:11 PM ]

I added a line to the Vectors section on the Data Structures (http://clojure.org/data_structures) page: "Vectors are compared first by length, then each element is compared in order."





[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-1380] Three-arg ExceptionInfo constructor permits nil data Created: 13/Mar/14  Updated: 25/Mar/14

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

Type: Defect Priority: Minor
Reporter: Gordon Syme Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File clj-1380.patch    
Patch: Code and Test

 Description   

The argument check in the two-arg clojure.lang.ExceptionInfo constructor isn't present in the three-arg constructor so it's possible to create an ExceptionInfo with arbitrary (or nil) data.

E.g.:

user=> (clojure-version)
"1.5.1"

user=> (ex-info "hi" nil)
IllegalArgumentException Additional data must be a persistent map: null  clojure.lang.ExceptionInfo.<init> (ExceptionInfo.java:26)

user=> (ex-info "hi" nil (Throwable.))
NullPointerException   clojure.lang.ExceptionInfo.toString (ExceptionInfo.java:40)


 Comments   
Comment by Gordon Syme [ 13/Mar/14 10:47 AM ]

Sorry, didn't meant to classify as "major" and I don't have permissions to edit.

Comment by Gordon Syme [ 13/Mar/14 11:11 AM ]

Patch + tests

I'm not at all familiar with the project so may have put tests in the wrong language and/or wrong place.

The ex-info-works test is a bit dorky but shows that both constructors are equivalent (and passes without the patch to ExceptionInfo).

Comment by Alex Miller [ 13/Mar/14 12:18 PM ]

No worries on the classification - I adjust most incoming tickets in some way or another.

Thanks for the patch, however it cannot be considered unless you complete the Clojure Contributor's Agreement - http://clojure.org/contributing. This is an important step in the process that keeps the Clojure codebase on a sound legal basis.

Someone else could develop a clean room patch implementation for this ticket later, but of course it would be ideal if you could become a contributor!

Comment by Gordon Syme [ 13/Mar/14 1:15 PM ]

Hi Alex,

sure, that makes sense. I'll get the contributor's agreement in the post. It may take a while to arrive since I'm based in Europe.

Comment by Gordon Syme [ 25/Mar/14 10:03 AM ]

I just checked http://clojure.org/contributing, looks like my CCA made it through





[CLJ-1379] Quoting of :actual form is incorrect in clojure.test :pass type maps Created: 12/Mar/14  Updated: 12/Mar/14

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

Type: Defect Priority: Minor
Reporter: Hugo Duncan Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: test
Environment:

All clojure versions


Attachments: File fix-quoting-in-pass-case.diff    
Patch: Code

 Description   

The function symbol is not correctly quoted in the construction of the :actual value in a :pass type map for clojure.test.

It currently produces (list = 1 1) instead of (list '= 1 1) for an (is (= 1 1)) test.

I haven't been able to come up with a workaround for this.






[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-1377] java.lang.IllegalArgumentException: More than one matching method found on calling ExecutorService.submit from let instead of using Var. Created: 11/Mar/14  Updated: 12/Mar/14  Resolved: 11/Mar/14

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

Type: Defect Priority: Major
Reporter: Roy Varghese Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Example from Joy Of Clojure doesn't work when Executor service pool is used in (let )

https://groups.google.com/forum/#!msg/clojure/asEXM2uHyxw/aK4rrKOKs1YJ



 Comments   
Comment by Alex Miller [ 11/Mar/14 3:47 PM ]

In the let case, the pool will be tagged with the proper type so the ambiguity is detected.

In the def case, the pool will be seen as an object and the compiler is just deferring to reflection at runtime to figure it out. If you turn on warn-on-reflection, you'll see a reflection warning in this case. Reflection is just picking the first one that matches in that case. If you type hinted the def case, you'd see the same error.

I don't think there is a bug here.

Comment by Roy Varghese [ 11/Mar/14 5:08 PM ]

Actually, I did turn on warn-on-reflection, but didn't see any messages.

I think this creates an element of surprise and uncertainty.

(def A (let [x (java.util.concurrent.Executors/newFixedThreadPool 5)] x))

How would (.submit A ...) behave?

Comment by Alex Miller [ 11/Mar/14 11:15 PM ]

When I ran the prior example, I saw a reflection warning.

In your new example, I would expect x to be typed as a ThreadExecutorPool, A to be typed as an Object, and .submit to get called reflectively (successfully) and produce a reflection warning. It's effectively no different than the def example in the post.

What are you suggesting is the bug and what should happen? Reflector could throw an error when it finds multiple matches but that would certainly break code that is currently working like the example you gave, so I doubt we would consider such a change at this point. There really is an ambiguity here, so I think a type hint is required to make it work unambiguously in either case.

Comment by Roy Varghese [ 12/Mar/14 10:49 AM ]

The bug is that type hints are required in one case, and not required in the other case because of implementation details, unless its documented in the language that (def) and (let) hold different types of Vars.

I the example above, I would expect A and x to refer to the same object, and thus contain the type information. Or not. Without reading the implementation, either case seems possible.

Agree, its a breaking change, but that's a different consideration.

Comment by Roy Varghese [ 12/Mar/14 10:50 AM ]

BTW..trying to fix with hints is what led to CLJ-1378.

Comment by Alex Miller [ 12/Mar/14 11:39 AM ]

x is locally bound to an object (there is no Var here) - the type information is on the local binding, inferred from the type of the expression. In the compiled form, the let expression does know the return type of the evaluated let (if there were multiple branches, there might be many possible return types). The def creates a Var that holds the result of evaluating the let. At compilation time, the type of the result of the let is unknown so is assumed to be Object.

So, x is a local binding and A is a Var. Even if they both refer to the same object, they do so in different contexts using different information. The type hinting and binding behavior in let is described on the special forms page http://clojure.org/special_forms. Remember too that Clojure is a dynamic language - while the Var A might hold a ThreadPoolExecutor instance now, it might hold something else later.

CLJ-1378 is useful - that's phrased more as a problem we can solve and there's a reasonable patch there.

Comment by Alex Miller [ 12/Mar/14 11:55 AM ]

"there is no Var here" == "there is no Var at this point"
"the let expression does know the return type" should have been "does NOT know"





[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-1375] Remove Util.pcequiv() and stop pretending Java colls are equiv to Clojure colls Created: 11/Mar/14  Updated: 11/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Attachments: Text File clj-1375-v1.patch    
Patch: Code

 Description   

The Util.pcequiv() method

static public boolean pcequiv(Object k1, Object k2){
	if(k1 instanceof IPersistentCollection)
		return ((IPersistentCollection)k1).equiv(k2);
	return ((IPersistentCollection)k2).equiv(k1);
}

tries to get equiv semantics (cross-class number equality) for cases of mixed Clojure/Java collection comparison. However, this is not a sustainable direction and we would like to stop doing this.

Attached patch removes this and changes calling code to only call equiv when both collections are IPersistentCollection.






[CLJ-1374] Make PersistentQueue implement List Created: 09/Mar/14  Updated: 31/Mar/14  Resolved: 31/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None

Attachments: File clj-1374-1.diff    
Patch: Code and Test

 Description   

Most ordered Clojure collections like lists, vectors, and lazy seqs implement java.util.List, and thus .equals can be true between values of those types and other collections implementing java.util.List, like java.util.Vector and java.util.ArrayList.

Clojure PersistentQueue seems to be the odd man out here, in that it implements Collection but not List, and thus while it can be .equals to Clojure lists, vectors, and lazy seqs, it cannot be .equals to other collections implementing java.util.List.

user=> (instance? java.util.List '())
true
user=> (instance? java.util.List (lazy-seq))
true
user=> (instance? java.util.List [])
true
user=> (instance? java.util.List (vector-of :long))
true
user=> (instance? java.util.List clojure.lang.PersistentQueue/EMPTY)
false

user=> (= '() (java.util.ArrayList.))
true
user=> (= (lazy-seq) (java.util.ArrayList.))
true
user=> (= [] (java.util.ArrayList.))
true
user=> (= (vector-of :long) (java.util.ArrayList.))
true
user=> (= clojure.lang.PersistentQueue/EMPTY (java.util.ArrayList.))
false


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

Patch clj-1374-1.diff is written assuming that CLJ-1372 patch clj-1372-2.diff or very similar has been committed, because of the tests modified, not because of the change of PersistentQueue to implement the java.util.List interface. I can update this patch as desired if that change does not go in.

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

Ugh. The subject is definitely a duplicate of CLJ-1059, which I should have checked for before creating this ticket. I will compare patches to see how the approaches compare. Mine is probably a poor substitute for that one, but the tests I add may still be useful to keep in a patch for CLJ-1059.

Comment by Andy Fingerhut [ 31/Mar/14 5:29 PM ]

Problem description is a duplicate of CLJ-1059. Even the patch (independently developed) is nearly the same as the patch with a name beginning with "001" attached to CLJ-1059.





[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-1371] divide(Object, Object) with (NaN, 0) does not return NaN Created: 07/Mar/14  Updated: 07/Mar/14

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

Type: Defect Priority: Trivial
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: math


 Description   

user=> (def x Double/NaN)
#'user/x
user=> (/ x 0)

ArithmeticException Divide by zero clojure.lang.Numbers.divide (Numbers.java:156)
user=> (/ Double/NaN 0)
Double/NaN



 Comments   
Comment by Alex Miller [ 07/Mar/14 7:50 AM ]

As per the Java Language Specification (http://docs.oracle.com/javase/specs/jls/se7/html/jls-4.html#jls-4.2.4),

"All numeric operations with NaN as an operand produce NaN as a result."

Comment by Yongqian Li [ 07/Mar/14 7:54 AM ]

But in the first example it produces an ArithmeticException.

Comment by Alex Miller [ 07/Mar/14 9:27 AM ]

Ah, I see the question now.

Here we are dividing a double by a long. In the first case, this is parsed as divide(Object, long) which then calls divide(Object, Object), which throws ArithmeticException if the second arg is 0 (regardless of the first arg).

In the second case it's parsed as divide(double, long) which just relies on Java to properly upcast the primitive long to a double to do the divide.

Note that making this call with 2 doubles does return NaN:

user=> (def x Double/NaN)
#'user/x
user=> (/ x 0.0)
NaN

or type hinting x to a double works as well:

user=> (def x Double/NaN)
#'user/x
user=> (/ ^double x 0.0)
NaN

I think one option to "fix" this behavior would be to add checks in divide(Object, Object) to check whether x is NaN and instead return NaN.





[CLJ-1370] ((println)) should throw CompilerException instead of NPE? Created: 06/Mar/14  Updated: 06/Mar/14  Resolved: 06/Mar/14

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

Type: Defect Priority: Minor
Reporter: The Alchemist Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug


 Description   

How to Reproduce

=> ((println))

NullPointerException   hello.core/eval4117 (NO_SOURCE_FILE:1)

What's Wrong?

I might be completely wrong, in which case don't hesitate to close this defect, but I wish this would throw a CompilerException with an IllegalArgumentException, just like ((nil)):

=> ((nil))
CompilerException java.lang.IllegalArgumentException: Can't call nil, compiling:(NO_SOURCE_PATH:1:2)


 Comments   
Comment by Alex Miller [ 06/Mar/14 12:37 PM ]

There is no compilation error here. The error occurs during evaluation.

user> (defn x [] ((println)))  ;; compiles just fine
#'user/x
user> (x)   ;; fails in evaluation
NullPointerException   user/x (NO_SOURCE_FILE:1)

The error is thrown when trying to evaluate (nil) where NullPointerException is a perfectly valid error.





[CLJ-1369] CLJ-738 is marked Closed is not implemented Created: 04/Mar/14  Updated: 04/Mar/14  Resolved: 04/Mar/14

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

Type: Defect Priority: Minor
Reporter: David Welte Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

OS X, Java 6



 Description   

CLJ-738 is marked Closed but the attached patch is has not been applied and both Clojure 1.5.1 and 1.6.0-beta2 exhibit the bad behavior listed in CLJ-738. The issue that CLJ-738 is that (<= (Double. Double/NaN) 1) evaluates to true while (<= Double/NaN 1) evaluates to false. See CLJ-738 for many details.



 Comments   
Comment by Alex Miller [ 04/Mar/14 3:20 PM ]

Thanks for letting us know about this - I concur that 738 was incorrectly closed without being applied and I have resurrected that ticket. I am closing this one. In the future, feel free to just comment on a ticket directly, or better (for a closed ticket), comment on one of the mailing lists.





[CLJ-1368] Document usage for case with non-readable constants Created: 02/Mar/14  Updated: 02/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docs, interop


 Description   

Problem

It is pretty obscure how to get constant-time dispatch for e.g. Enums, even if user knows about case.

Proposal

The possibility to dispatch to arbitrary constants with case, by wrapper macro, should be documented.

Wording

  • Should it warn against doing that with unstable values?
  • Should it mention anything else than java Enums?

Case Techniques

Case is documented for accepting all readable forms as test-constants. However, it can also be made to use any compile-time-known constants as test-constants, by wrapping it in another macro.

Sometimes this is appropriate, e.g. when dispatching on a java Enum.
Other times, less so, e.g. when dispatching on objects whose hash changes when the vm is restarted (breaks AOT).

Implications

This technique is an application of a more general technique: Passing non-literals to a macro from another macro.
Are there other macros that have use cases like this?

References

https://groups.google.com/d/topic/clojure/3yGjDO2YnjQ/discussion



 Comments   
Comment by Herwig Hochleitner [ 02/Mar/14 11:25 AM ]

This is a duplicate of http://dev.clojure.org/jira/browse/CLJ-1367

Actually, it's an alternate solution





[CLJ-1367] Allow case statement to compare java constants Created: 02/Mar/14  Updated: 02/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Adam Clements Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: interop


 Description   

As raised on the mailing list: https://groups.google.com/forum/#!topic/clojure/3yGjDO2YnjQ

It's not possible to use java constants in a case statement. condp = could be used in this case but these are things which could be used in a java switch statement and so it's annoying to give up constant time dispatch. For example:

(case (.getActionMasked event)
MotionEvent/ACTION_POINTER_DOWN :down
MotionEvent/ACTION_UP :up
MotionEvent/ACTION_POINTER_UP :up
MotionEvent/ACTION_MOVE :move
MotionEvent/ACTION_CANCEL :cancel
MotionEvent/ACTION_OUTSIDE :outside
:none))

Doesn't work, but there is no reason this couldn't be resolved at compile time and dispatched in constant time.



 Comments   
Comment by Herwig Hochleitner [ 02/Mar/14 11:32 AM ]

Another solution for this problem: http://dev.clojure.org/jira/browse/CLJ-1368





[CLJ-1366] The empty map literal is read as a different map each time Created: 01/Mar/14  Updated: 02/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: OHTA Shogo Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: memory, reader

Attachments: Text File 0001-make-the-reader-return-the-same-empty-map-when-it-re.patch     Text File 0002-make-the-reader-return-the-same-empty-map-when-it-re.patch    
Patch: Code

 Description   

As reported here (https://groups.google.com/forum/?hl=en#!topic/clojure-dev/n83hlRFsfHg), the empty map literal is read as a different map each time.

user=> (identical? (read-string "{}") (read-string "{}"))
false

Making the reader return the same empty map when it reads an empty map is expected to improve some memory efficiency, and also lead to consistency with the way other collection literals are read in.

user=> (identical? (read-string "()") (read-string "()"))
true
user=> (identical? (read-string "[]") (read-string "[]"))
true
user=> (identical? (read-string "#{}") (read-string "#{}"))
true

Cause: LispReader calls RT.map() with an empty array when it reads an empty map, and RT.map() in turn makes a new map unless its argument given is null.

Approach: make RT.map() return the same empty map when the argument is an empty array as well, not only when null



 Comments   
Comment by OHTA Shogo [ 01/Mar/14 2:59 AM ]

Sorry, the patch 0001-make-the-reader-return-the-same-empty-map-when-it-re.patch didn't work.

The updated patch 0002-make-the-reader-return-the-same-empty-map-when-it-re.patch works, but I'm afraid it'd be beyond the scope of this ticket since it modifies RT.map() behavior a bit.





[CLJ-1365] New collection hash functions are too slow Created: 20/Feb/14  Updated: 11/Mar/14  Resolved: 11/Mar/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: collections

Attachments: Text File clj-1365-v1.patch     Text File clj-1365-v2.patch     Zip Archive testclj1365.zip    
Patch: Code
Approval: Ok

 Description   

As reported ( https://groups.google.com/d/msg/clojure-dev/t6LAmVe-RLM/ekLTKxYfU5UJ ) by Mark Engelberg, the new collection hashing functions are slower than invoking the Murmur3 functions directly. See the attached zip for performance tests.

Approach: Made mix-collection-hash, hash-ordered-coll, and hash-unordered-coll use primitive type hints to avoid the bulk of the time.

Patch: clj-1365-v2.patch

Screened by:



 Comments   
Comment by Alex Miller [ 20/Feb/14 11:26 AM ]

Added to 1.6 release.

Comment by Alex Miller [ 20/Feb/14 12:40 PM ]

Made hash functions inline for performance.

Comment by Rich Hickey [ 20/Feb/14 7:55 PM ]

Reported where?

This looks like bad benchmarking.

(dotimes [_ 10] (let [x 1 y 1] (time (dotimes [n 1000000000] (clojure.lang.Murmur3/mixCollHash x y)))))

and

(dotimes [_ 10] (let [x 1 y 1] (time (dotimes [n 1000000000] #_(clojure.lang.Murmur3/mixCollHash x y)))))

take the same time on my machine.

I'd need to see tests where the return was definitely used, it seems this is just more easily ignored by hotspot when not used.

We probably only need to hint count and the return for decent results.

Comment by Alex Miller [ 20/Feb/14 8:55 PM ]

It was reported by Mark Engelberg in his Instaparse rework - he observed these calls taking noticeably longer and overall times 10-20% down. I will ask him to chime in here.

Comment by Rich Hickey [ 04/Mar/14 8:44 AM ]

Could someone please test hinting hint count and the return? I'd hate for the answer to anyone's perf issues be inlining.

Comment by Alex Miller [ 04/Mar/14 9:06 AM ]

I will provide some more data for consideration of the options.

Comment by Alex Miller [ 04/Mar/14 11:07 AM ]

Test project for different variants

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

Attached a test project with different variants for testing and better benchmarking. To run:

unzip testclj1365.zip
cd clj1365
lein uberjar
java -server -cp target/clj1365-0.1.0-SNAPSHOT-standalone.jar clj1365.core

Results:

mix-collection-hash original
"Elapsed time: 57.777 msecs"
"Elapsed time: 18.034 msecs"
"Elapsed time: 20.591 msecs"
"Elapsed time: 25.179 msecs"
"Elapsed time: 21.781 msecs"
mix-collection-hash hints
"Elapsed time: 14.983 msecs"
"Elapsed time: 8.871 msecs"
"Elapsed time: 8.793 msecs"
"Elapsed time: 8.92 msecs"
"Elapsed time: 8.873 msecs"
mix-collection-hash inline
"Elapsed time: 10.04 msecs"
"Elapsed time: 7.117 msecs"
"Elapsed time: 7.306 msecs"
"Elapsed time: 7.324 msecs"
"Elapsed time: 7.175 msecs"
Murmur3/mixCollHash
"Elapsed time: 9.522 msecs"
"Elapsed time: 7.288 msecs"
"Elapsed time: 7.397 msecs"
"Elapsed time: 7.364 msecs"
"Elapsed time: 7.345 msecs"

From these results, I infer that the unhinted version is slower (21 ms) than a static call (7 ms). Inlining gives you same perf as static. Hinting inputs and return gives almost the same perf (9 ms).





[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-1363] Field access via .- in reflective case does not work Created: 18/Feb/14  Updated: 28/Feb/14  Resolved: 28/Feb/14

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: interop

Attachments: Text File clj-1363-v1.patch     Text File clj-1363-v2.patch     Text File clj-1363-v3.patch    
Patch: Code and Test
Approval: Ok

 Description   

The (.-foo instance) syntax will properly resolve to either a field or a no-arg method if the type of instance is known. However, in the reflective case, it will only resolve to a method. This behavior should match the non-reflective case. The method case always be forced by using (. foo (method)).

user> (definterface I (a []))
user.I
user> (deftype T [a] I (a [_] "method"))
user.T
user> (def t (->T "field"))
#'user/t
user=> (. ^T t a)  ;; as expected (prefer method)
"method"
user=> (. ^T t -a) ;; as expected (prefer field)
"field"
user> (. t a)      ;; as expected (prefer method)
"method"
user> (. t -a)     ;; WRONG - should return "field"
"method"

Approach: This case falls into Reflector.invokeNoArgInstanceMember() (this is the only place this method is used). InstanceFieldExpr now takes another flag (requireField) which will be set to true if "-field" and false if "field". InstanceFieldExpr will invoke (or emit) a call to Reflector.invokeNoArgInstanceMember() which now takes the same flag. If the flag is set to true, it first looks only for a field, otherwise it looks for a method and falls back to field which throws an error if necessary. I added a new invokeNoArgInstanceMember() with an arity to match the old arity - existing bytecode compiled on older Clojure versions will be trying to call this arity.

Patch: clj-1363-v3.patch

Screened by:



 Comments   
Comment by Rich Hickey [ 20/Feb/14 7:24 PM ]

You can't change the semantics of invokeNoArgInstanceMember - they are correct when not using '-'. We need to feed the info that '-' was used through InstanceFieldExpr and make field-first conditional on that.

Comment by Alex Miller [ 21/Feb/14 5:42 AM ]

Updated with new patch to thread this case through InstanceFieldExpr.

Comment by