<< Back to previous view

[CLJ-1663] DynamicClassLoader delegates to parent classloader before checking in its URL list Created: 18/Feb/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: classloader, regression

Attachments: Text File 0001-CLJ-1663-delegate-loadClass-to-super-classloader-bef.patch    
Patch: Code
Approval: Ok

 Description   

See Cursive #748. Cursive calls into Leiningen in-process, and before doing that it creates a new DynamicClassLoader which uses an IntelliJ PluginClassLoader as its parent. This is throwing a CNFE, although the URL containing the class is present in the DynamicClassLoader URL list.

Cause: The patch for CLJ-979 added an implementation of loadClass that delegates to "getParent().loadClass()", which in this case delegates to PluginClassLoader.loadClass(). This was incorrect as the current implementation should have been to delegate to the loadClass method of the superclass, which will take care of delegating to the loadClass method of the parent class loader if necessary.

Approach: The proposed patch replaces the call to "getParent().loadClass()" with a call "super.loadClass()" fixing this issue.

Screened by: Alex Miller



 Comments   
Comment by Colin Fleming [ 18/Feb/15 6:25 AM ]

Unfortunately getClassLoadingLock(name) is only available from Java 1.7+ and I am targeting Java 1.6. However reverting that part of the patch to synchronize on "this" as previously does indeed fix the original problem.

Comment by Nicola Mometto [ 18/Feb/15 7:22 AM ]

I'll revert the getClassLoadingLock change then, it was actually out of scope for this ticket.





[CLJ-1642] Add mention of new :warn-on-boxed option to doc string of unchecked-math Var Created: 15/Jan/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring

Attachments: Text File clj-1642-v1.patch    
Patch: Code
Approval: Ok

 Description   

A small doc string enhancement about the new compiler behavior in Clojure 1.7 when *unchecked-math* is bound to :warn-on-boxed

https://github.com/clojure/clojure/blob/master/changes.md#13-warn-on-boxed-math

Patch: clj-1642-v1.patch
Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 15/Jan/15 11:51 AM ]

Patch clj-1642-v1.patch dated Jan 15 2014 is one way to document the new :warn-on-boxed behavior.





[CLJ-1637] vec fails on MapEntry Created: 11/Jan/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

Type: Defect Priority: Critical
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: regression
Environment:

1.7.0-alpha5


Attachments: Text File clj-1637-jdevuyst.patch     Text File clj-1637.patch     Text File clj-1637-with-test.patch    
Patch: Code
Approval: Ok

 Description   

After CLJ-1546:

(vec (first {1 2}))

Cause: (if (vector? coll) (with-meta coll nil) ...) checks that something is IPersistentVector, then sends it to something that takes IObj, so anything that is one but not both throws an error. In Clojure itself, this is the set of classes extending from AMapEntry.

Alternatives:

1. Make AMapEntry implement IObj - this fixes everything in Clojure and keeps the vec code as is but still leaves open this gap for any external implementation of IPersistentVector.
2. Check for this case explicitly in vec. (if (vector? coll) (if (instance? clojure.lang.IObj coll) (with-meta coll nil) (clojure.lang.LazilyPersistentVector/create coll)) ...). Perf testing shows no significant difference in performance with the change.
3. Pull the special check for vector? in vec.
4. Check for this case explicitly in vec and return the same instance if it's not an IObj. See clj-1637-jdevuyst.patch.

Approach: patch takes approach #2

Patch: clj-1637-with-test.patch

Screened by: Stu (also added test)



 Comments   
Comment by Nicola Mometto [ 11/Jan/15 8:19 AM ]

The correct fix is to probably make MapEntry an IObj

Comment by Nicola Mometto [ 11/Jan/15 8:21 AM ]

Actually, making AMapEntry an IObj rather than MapEntry would fix the issue for sorted-map kv-pairs too.

user=> (vec (first (sorted-map 1 1)))
ClassCastException clojure.lang.PersistentTreeMap$BlackVal cannot be cast to clojure.lang.IObj  clojure.core/with-meta--4121 (core.clj:216)
Comment by Alex Miller [ 11/Jan/15 8:35 AM ]

There are potentially a couple ways to fix this. I'll look at it next week.

Comment by Jonas De Vuyst [ 14/Jan/15 6:14 AM ]

Slightly modified patch. In the case where coll is a vector but not an IObj, simply return coll.

Comment by Jonas De Vuyst [ 14/Jan/15 7:17 AM ]

If desired I could also update the patch to fix an analogous—albeit somewhat theoretic—bug in `set`.

It does make me wonder if perhaps a `without-meta` function should be added to `clojure.core`.

I think making AMapEntry an IObj might make sense even after applying the above patches. In `AMapEntry`, perhaps `withMeta(m)` could be implemented as `asVector().withMeta(m)`. This, however, would require changing `asVector()` to return some `IObj ∩ IPersistentVector` type (e.g. `PersistentVector`). This would be straightforward to do, but requires deciding if this change in signature may be propagated to the static methods of `LazilyPersistentVector`.

Comment by Alex Miller [ 14/Jan/15 9:28 AM ]

This is a point of some debate, but the intention with the change in the implementation is to retain current behavior, which always gives you a new vector instance. It's not clear to me that there is any point in attaching meta to map entries (which also does not solve the problem for external IPersistentVector, non-IObj instances outside Clojure).

In any case, I'm going to update the description a bit to add this as an alternative.





[CLJ-1636] SeqIterator can return incorrect results Created: 10/Jan/15  Updated: 18/Jan/15  Resolved: 16/Jan/15

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

Type: Defect Priority: Blocker
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: regression
Environment:

clojure-1.7.0-alpha5


Attachments: Text File 0001-CLJ-1636-don-t-use-this-as-a-sentinel-in-SeqIterator.patch     Text File 0001-fix-for-CLJ-1636.patch    
Patch: Code
Approval: Ok

 Description   

As of 1.7.0-alpha5, we are seeing SeqIterator return iterated results that do reflect the values of the underlying seq, in particular acting as if the seq contains a nil value when it does not. This problem is intermittent but has at times caused clojure master to fail in compilation (which is why this is marked as a blocker).

Two recent changes during 1.7 have created and exposed this problem:

1) This commit https://github.com/clojure/clojure/commit/43cc1854508d655e58e377f84836ba128971f90c changed the SeqIterator implementation to be lazier and to use "this" as a sentinel object in SeqIterator. (1.7.0-alpha2)
2) CLJ-1546 changed the implementation of vec such that PersistentHashMap and PersistentHashSet are now converted using iterator() rather than seq(). PHS/PHM use SeqIterator for their Iterator implementation. (1.7.0-alpha5)

Because of #2, we are now stressing #1 much more than before. In particular, things like defining defrecords rely heavily on vec (and set) of PHS and PHM.

Example stack trace: https://gist.github.com/puredanger/f56e3253f0668a515ec5 (seen compiling Clojure itself)

Cause: Setting seq==this; in the constructor of SeqIterator is allowing unsafe publication of the partially constructed "this" object, which can cause subtle problems in the hasNext() implementation. In particular, it seems that after inlining, on the first call, the seq==this condition when comparing the cached partially constructed instance in seq and the fully constructed version in this will return false, even though these have the same object identity. This causes the wrong path to be executed in hasNext().

Approach: Do not use this as a sentinel value.

Patch: 0001-CLJ-1636-don-t-use-this-as-a-sentinel-in-SeqIterator.patch

Screened by: Alex Miller



 Comments   
Comment by Colin Jones [ 11/Jan/15 12:40 AM ]

I was able to reproduce this (intermittently) earlier, but I've seen periods of many successful runs in a row (both with that patch reverted and with it in place), so it's been hard for me to trust what I'm seeing locally when it passes. I didn't see any evidence of AOT compilation happening (e.g. no classfiles under `target/`), so I'd have expected the new function `already-compiled?` in CLJ-1544 never to actually run.

It looks like the Cause section of the stacktrace is implicating an error in trying to `(resolve nil)`, where `nil` is an entry in an interfaces collection that should actually be empty. That's based on these two lines (along with the lines higher up in the cause):

...snip...
at clojure.core$set.invoke(core.clj:3944)
at clojure.core$emit_defrecord.invoke(core_deftype.clj:154)
...snip...

(https://github.com/clojure/clojure/blob/3e7cb1a5c840612ad41cf6e0be92480f798bc05d/src/clj/clojure/core_deftype.clj#L154)

The defrecord here in question looks like

(defrecord Foo [x y])

So the `opts+specs` var-arg argument to `defrecord` should be `nil` since there are no entries, which should mean the `interfaces` piece of the `parse-opts+specs` call should return an empty vector.

But that stacktrace confuses me, because it suggests that the `interfaces` vector, instead of being empty, contains a `nil` element. How can this be? Or what misstep have I made in tracing through this?

Comment by Alex Miller [ 11/Jan/15 12:54 AM ]

If the error is intermittent, then my pegging of CLJ-1544 may be wrong. For me, it was repeatable as of clojure commit e5a104e894ed82f244d69513918d570cee5df67d (when CLJ-1544 was applied) and I have not reproduced it prior.

Comment by Nicola Mometto [ 11/Jan/15 4:50 AM ]

Alex, just to be sure – you were able to reproduce this bug with clojure at e5a104e894ed82f244d69513918d570cee5df67d (CLJ-1544) ? I'd like to have confirmation so 9f277c80258b3d2951128ce26a07c30ad0b47af0 (CLJ-979) can be excluded as the culprit

Comment by Alex Miller [ 11/Jan/15 8:09 AM ]

Correct.

Comment by Nicola Mometto [ 11/Jan/15 8:15 AM ]

Well this is weird then.
The only way I can think of that would produce that exception is if this returned nil: https://github.com/clojure/clojure/blob/3e7cb1a5c840612ad41cf6e0be92480f798bc05d/src/clj/clojure/core_deftype.clj#L57

This means a scenario like this:

user=> (defrecord x [] +)
NullPointerException   clojure.lang.Compiler.maybeResolveIn (Compiler.java:7015)
user=> (.printStackTrace *e)
java.lang.NullPointerException, compiling:(NO_SOURCE_FILE:3:1)
	at clojure.lang.Compiler.macroexpand1(Compiler.java:6620)
	at clojure.lang.Compiler.macroexpand(Compiler.java:6678)
	at clojure.lang.Compiler.eval(Compiler.java:6752)
	at clojure.lang.Compiler.eval(Compiler.java:6731)
	at clojure.core$eval.invoke(core.clj:3076)
	at clojure.main$repl$read_eval_print__7044$fn__7047.invoke(main.clj:239)
	at clojure.main$repl$read_eval_print__7044.invoke(main.clj:239)
	at clojure.main$repl$fn__7053.invoke(main.clj:257)
	at clojure.main$repl.doInvoke(main.clj:257)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.main$repl_opt.invoke(main.clj:323)
	at clojure.main$main.doInvoke(main.clj:421)
	at clojure.lang.RestFn.invoke(RestFn.java:397)
	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.main.main(main.java:37)
Caused by: java.lang.NullPointerException
	at clojure.lang.Compiler.maybeResolveIn(Compiler.java:7015)
	at clojure.core$ns_resolve.invoke(core.clj:4200)
	at clojure.core$ns_resolve.invoke(core.clj:4197)
	at clojure.core$resolve.invoke(core.clj:4206)
	at clojure.core$map$fn__4523.invoke(core.clj:2612)
	at clojure.lang.LazySeq.sval(LazySeq.java:40)
	at clojure.lang.LazySeq.seq(LazySeq.java:49)
	at clojure.lang.RT.seq(RT.java:485)
	at clojure.core$seq__4103.invoke(core.clj:135)
	at clojure.core$reduce1.invoke(core.clj:899)
	at clojure.core$set.invoke(core.clj:3944)
	at clojure.core$emit_defrecord.invoke(core_deftype.clj:154)
	at clojure.core$defrecord.doInvoke(core_deftype.clj:374)
	at clojure.lang.RestFn.invoke(RestFn.java:497)
	at clojure.lang.Var.invoke(Var.java:401)
	at clojure.lang.AFn.applyToHelper(AFn.java:171)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.lang.Compiler.macroexpand1(Compiler.java:6607)
	... 16 more
nil

where a var is used as a protocol but no interface is present.

Comment by Nicola Mometto [ 11/Jan/15 8:18 AM ]

Alex, since I cannot reproduce, can you try getting the exception with a patched version of clojure that replaces https://github.com/clojure/clojure/blob/3e7cb1a5c840612ad41cf6e0be92480f798bc05d/src/clj/clojure/core_deftype.clj#L57
with something like

(or (:on (deref (resolve %))) 
    (println % @(resolve %)))

so we can get an idea of what's going on?

Comment by Nicola Mometto [ 11/Jan/15 11:38 AM ]

I was just able to reproduce this issue using clojure at commit 4afd4a7c14c48b5baf3c03196053066483cb4223

This means that CLJ-1544 is not responsable for this bug.

I can also confirm that this bug is intermittent, which makes figuring out what's going on really hard.

Comment by Nicola Mometto [ 11/Jan/15 12:10 PM ]

I still have absolutely no idea how this can happen but adding a bunch of printlns it turned out that for some reason in this binding of the deftype macro:

[interfaces methods opts] (parse-opts+specs opts+specs)

when opts+specs is nil, interfaces is sometimes [nil] as opposed to [].

This makes me think that there's some concurrency bug in the recent changes around the handling of vec, but this is just a guess.

Comment by Nicola Mometto [ 11/Jan/15 12:13 PM ]

I've restricted it down a bit and it looks like this part of opts+spec can bind interfaces to [nil] when impls is {}

interfaces (→ (map #(if (var? (resolve %))
                      (:on (deref (resolve %)))
                      %)
                   (keys impls))
             set
             (disj 'Object 'java.lang.Object)
             vec)
Comment by Nicola Mometto [ 11/Jan/15 12:25 PM ]

Here's an example output from my debugging tests, with the following patch applied:

diff --git a/src/clj/clojure/core_deftype.clj b/src/clj/clojure/core_deftype.clj
index 97e14cc..8f521eb 100644
--- a/src/clj/clojure/core_deftype.clj
+++ b/src/clj/clojure/core_deftype.clj
@@ -60,6 +60,8 @@ (defn- parse-opts+specs [opts+specs]
                        set
                        (disj 'Object 'java.lang.Object)
                        vec)
+        _ (when (nil? opts+specs)
+            (println impls interfaces))
         methods (map (fn [[name params & body]]
                        (cons name (maybe-destructured params body)))
                      (apply concat (vals impls)))]
{} [nil]
Exception in thread "main" java.lang.NullPointerException, compiling:(schema/utils.clj:68:1)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6716)
	at clojure.lang.Compiler.analyze(Compiler.java:6500)
	at clojure.lang.Compiler.analyze(Compiler.java:6461)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5837)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6155)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6709)
	at clojure.lang.Compiler.analyze(Compiler.java:6500)
	at clojure.lang.Compiler.analyze(Compiler.java:6461)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5837)
	at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5272)
	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3901)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6707)
	at clojure.lang.Compiler.analyze(Compiler.java:6500)
	at clojure.lang.Compiler.eval(Compiler.java:6765)
	at clojure.lang.Compiler.load(Compiler.java:7195)
	at clojure.lang.RT.loadResourceScript(RT.java:370)
	at clojure.lang.RT.loadResourceScript(RT.java:361)
	at clojure.lang.RT.load(RT.java:440)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5424.invoke(core.clj:5848)
	at clojure.core$load.doInvoke(core.clj:5847)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5653)
	at clojure.core$load_lib$fn__5373.invoke(core.clj:5693)
	at clojure.core$load_lib.doInvoke(core.clj:5692)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5731)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5814)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at plumbing.core$eval13998$loading__5316__auto____13999.invoke(core.clj:1)
	at plumbing.core$eval13998.invoke(core.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6768)
	at clojure.lang.Compiler.eval(Compiler.java:6757)
	at clojure.lang.Compiler.load(Compiler.java:7195)
	at clojure.lang.RT.loadResourceScript(RT.java:370)
	at clojure.lang.RT.loadResourceScript(RT.java:361)
	at clojure.lang.RT.load(RT.java:440)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5424.invoke(core.clj:5848)
	at clojure.core$load.doInvoke(core.clj:5847)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5653)
	at clojure.core$load_lib$fn__5373.invoke(core.clj:5693)
	at clojure.core$load_lib.doInvoke(core.clj:5692)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5731)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5814)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at user.fus_threading$eval13994.invoke(fus_threading.clj:6)
	at clojure.lang.Compiler.eval(Compiler.java:6768)
	at clojure.lang.Compiler.load(Compiler.java:7195)
	at clojure.lang.RT.loadResourceScript(RT.java:370)
	at clojure.lang.RT.loadResourceScript(RT.java:361)
	at clojure.lang.RT.load(RT.java:440)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5424.invoke(core.clj:5848)
	at clojure.core$load.doInvoke(core.clj:5847)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5653)
	at clojure.core$load_lib$fn__5373.invoke(core.clj:5693)
	at clojure.core$load_lib.doInvoke(core.clj:5692)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5731)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5814)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at midje.repl$load_facts$fn__6148.invoke(repl.clj:206)
	at midje.repl$load_facts.doInvoke(repl.clj:192)
	at clojure.lang.RestFn.invoke(RestFn.java:397)
	at user$eval6211.invoke(form-init7109545842773565024.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6768)
	at clojure.lang.Compiler.eval(Compiler.java:6758)
	at clojure.lang.Compiler.load(Compiler.java:7195)
	at clojure.lang.Compiler.loadFile(Compiler.java:7151)
	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.resolveIn(Compiler.java:6971)
	at clojure.lang.Compiler.resolve(Compiler.java:6949)
	at clojure.lang.Compiler$NewInstanceExpr.build(Compiler.java:7565)
	at clojure.lang.Compiler$NewInstanceExpr$DeftypeParser.parse(Compiler.java:7490)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6709)
	... 91 more
Comment by Nicola Mometto [ 11/Jan/15 12:32 PM ]

Further debugging is convincing me further that some of the recent changes around `vec` are causing this bug.

With the following patch:

diff --git a/src/clj/clojure/core_deftype.clj b/src/clj/clojure/core_deftype.clj
index 97e14cc..9478b04 100644
--- a/src/clj/clojure/core_deftype.clj
+++ b/src/clj/clojure/core_deftype.clj
@@ -53,13 +53,16 @@ (defn- parse-impls [specs]
 (defn- parse-opts+specs [opts+specs]
   (let [[opts specs] (parse-opts opts+specs)
         impls (parse-impls specs)
-        interfaces (-> (map #(if (var? (resolve %)) 
-                               (:on (deref (resolve %)))
-                               %)
-                            (keys impls))
-                       set
-                       (disj 'Object 'java.lang.Object)
-                       vec)
+        ks (keys impls)
+        interfaces' (map #(if (var? (resolve %))
+                            (:on (deref (resolve %)))
+                            %)
+                         ks)
+        interfaces'' (set interfaces')
+        interfaces''' (disj interfaces'' 'Object 'java.lang.Object)
+        interfaces (vec interfaces''')
+        _ (when (nil? opts+specs)
+            (println impls ks interfaces' interfaces'' interfaces''' interfaces))
         methods (map (fn [[name params & body]]
                        (cons name (maybe-destructured params body)))
                      (apply concat (vals impls)))]

I get this println when the NPE occurs:

{} nil () #{} #{} [nil]

Meaning that for some reson, `vec` of `#{}` returns `[nil]` rather than `[]`

Comment by Nicola Mometto [ 11/Jan/15 12:42 PM ]

I confirmed that the bug is in the vec function.
With the following patch, when the NPE occurs, the debug println is triggered:

diff --git a/src/jvm/clojure/lang/PersistentVector.java b/src/jvm/clojure/lang/PersistentVector.java
index 9804a0b..a460b6f 100644
--- a/src/jvm/clojure/lang/PersistentVector.java
+++ b/src/jvm/clojure/lang/PersistentVector.java
@@ -96,19 +96,22 @@ static public PersistentVector create(ArrayList list){
 static public PersistentVector create(Iterable items){
     // optimize common case
     if(items instanceof ArrayList)
         return create((ArrayList)items);

     Iterator iter = items.iterator();
     TransientVector ret = EMPTY.asTransient();
     while(iter.hasNext())
         ret = ret.conj(iter.next());
-    return ret.persistent();
+    PersistentVector r = ret.persistent();
+    if (RT.seq(r) != null && RT.seq(items) == null)
+        System.out.println("bug");
+    return r;
 }

 static public PersistentVector create(Object... items){
        TransientVector ret = EMPTY.asTransient();
        for(Object item : items)
                ret = ret.conj(item);
        return ret.persistent();
 }
Comment by Alex Miller [ 11/Jan/15 1:47 PM ]

And items is a PersistentSet? I've actually been looking at some weirdness on set iterators in the context of CLJ-1499 in consistency between seq and iterators.

Comment by Nicola Mometto [ 11/Jan/15 1:52 PM ]

I believe I have identified the bug, but I cannot make any sense out of it.

The bug apperas to be in SeqIterator.hasNext(), when the NPE occurs, after applying the following patch:

diff --git a/src/jvm/clojure/lang/SeqIterator.java b/src/jvm/clojure/lang/SeqIterator.java
index e6ad481..031fbc8 100644
--- a/src/jvm/clojure/lang/SeqIterator.java
+++ b/src/jvm/clojure/lang/SeqIterator.java
@@ -35,14 +35,18 @@ public SeqIterator(ISeq o){
 public boolean hasNext(){
        if(seq == this){
                seq = START;
                next = RT.seq(next);
                }
        else if(seq == next)
                next = RT.next(seq);
+    else if (RT.seq(next) == null)
+        System.out.println("this shouldn't happen: " + (this == seq));
+    if (RT.seq(next) == null && next != null)
+        System.out.println("bug: " + next);
        return next != null;
 }

 public Object next() throws NoSuchElementException {
        if(!hasNext())
                throw new NoSuchElementException();
        seq = next;

I get the following output:

this shouldn't happen: true
bug: #{}

I have absolutely no idea how it is possible that the last branch gets executed since it is true that seq == this thus the first branch should have been executed.

Comment by Nicola Mometto [ 11/Jan/15 1:58 PM ]

I don't know why, but with the attached patch the bug seems to go away.
This is probably just by accident though as I have no idea what changes between the code pre patch and the code post patch.

Comment by Alex Miller [ 11/Jan/15 3:21 PM ]

Without looking at the patch Id say that non deterministic bug plus impossible state smells like a concurrency / race condition problem.

Comment by Michael Blume [ 11/Jan/15 4:18 PM ]

This isn't the bug, per se, the thing I'm describing should not break anything, but why is the PersistentVector(Iterable) constructor being called on a PersistentHashSet? It looks like we could very easily do

--- i/src/jvm/clojure/lang/LazilyPersistentVector.java
+++ w/src/jvm/clojure/lang/LazilyPersistentVector.java
@@ -26,7 +26,7 @@ static public IPersistentVector createOwning(Object... items){
 static public IPersistentVector create(Object obj){
    if(obj instanceof IReduceInit)
        return PersistentVector.create((IReduceInit) obj);
-   else if(obj instanceof ISeq)
+   else if(obj instanceof Seqable)
        return PersistentVector.create(RT.seq(obj));
    else if(obj instanceof Iterable)
        return PersistentVector.create((Iterable)obj);

and treat the set directly as a seq. Is there some way that would be slower?

Comment by Michael Blume [ 11/Jan/15 4:45 PM ]

It may be that I've just tried it an insufficient number of times, but simply adding 'synchronized' to SeqIterator.hasNext appears to solve the problem. Again, this doesn't really tell us what the problem is.

ETA: Nope, fails sometimes even with synchronized.

Comment by Alex Miller [ 12/Jan/15 8:57 AM ]

The use of ISeq and not Seqable in LazilyPersistentVector is quite intentional. The idea here is that if something is already a seq (effectively a linked list), the best we're going to do is walk that chain. However, many things are Seqable that may have more efficient Iterable implementations which can (statefully) walk a data structure without creating all the intermediate objects required by seq. In particular, via CLJ-1499, map and set will soon be gaining direct Iterable implementations that walk the persistent tree without instantiating a seq object for every element. However, at the moment set and map will create a SeqIterator wrapped around the seq.

CLJ-1546 changed this path - it was walking the seq but is now walking it via SeqIterator. My working theory is that that switch has uncovered a latent race condition in SeqIterator that was never noticed before as the path wasn't exercised.

Note that because CLJ-1499 removes the reliance on SeqIterator, it would have avoided the bug in a different way! However, I have been seeing a number of weird things while doing dev on CLJ-1499 specifically around iterating over sets - the OO around iterator() and seq() in the APersistentSet/PersistentHashSet/PersistentTreeSet has some weird interactions.

I'm going to look a little closer at the suggested patch.

Comment by Nicola Mometto [ 12/Jan/15 9:01 AM ]

Alex, before you waste your time on my patch I want to clarify that I don't think that patch fixes the issue in any way, it's just a random change I made that happens to make the symptoms disappear on my system, I just attached it for debugging purposed.

Maybe there's a reason why the patch solves the bug or maybe it's just masking it, I still can't figure out why this apparent race condition happens.

Comment by Alex Miller [ 12/Jan/15 9:05 AM ]

Gotcha, thx.

Comment by Michael Blume [ 12/Jan/15 11:54 AM ]

Aha, makes sense, thanks =)

Comment by Andy Fingerhut [ 12/Jan/15 12:32 PM ]

I haven't dug into this, and don't have a solution, but SeqIterator's fields are not final, so there is no guarantee that the values assigned to a new instance's fields in its constructors will be visible to other threads, yes? And I believe that if those writes to the fields do eventually become visible, they need not become visible in the order that the assignments occur in the source code, but can become visible in any order.

Comment by Nicola Mometto [ 12/Jan/15 5:13 PM ]

By the way, I've been able to reproduce this bug using jdk 1.8 so it's not just with 1.7

Comment by Michael Blume [ 12/Jan/15 9:10 PM ]

SeqIterator seems to use 'this' as a sentinel value. If I replace it with an explicit 'new Object' sentinel, the problem appears to go away (~40 compilations without an NPE).

Making seq and next volatile doesn't help.

Interestingly, when I synchronize the entire SeqIterator class (both hasNext and next synchronized on this), the problem doesn't go away, so if this is a race condition, it's kind of a weird one.

I can insert a call to seq before the call to set here https://github.com/clojure/clojure/blob/clojure-1.7.0-alpha5/src/clj/clojure/core_deftype.clj#L60 and the problem doesn't go away. I can then print the result of seq before it's passed to set, and, of course, it's a nil.

So somehow, we're basically evaluating (-> nil set (disj 'Object 'java.lang.Object) vec) and getting [nil] instead of []

But when I actually evaluate that expression in a REPL (from 20 threads at once, 1M times in a row) it evaluates to empty vector every time.

So I'm confused.

Debug patch here if anyone wants to check my work: https://gist.githubusercontent.com/MichaelBlume/735c8f601210cfa1ecaf/raw/814f21e5e4abb2ca9d1d5330d0b4cc2b3a4424e6/gistfile1.txt

Comment by Alex Miller [ 12/Jan/15 9:53 PM ]

I'm with you. I'm starting to suspect that this is involved:

https://github.com/clojure/clojure/commit/43cc1854508d655e58e377f84836ba128971f90c

Comment by Andy Fingerhut [ 12/Jan/15 11:57 PM ]

Out of curiosity, I tried adding a field to the SeqIterator class that remembers the thread that constructed each instance, and then checks in the call to hasNext if the calling thread is the same as the creator thread, printing a message if they are ever different. I never saw that while building Clojure, nor running the command on the Midje project. That seems to rule out the possibility of the SeqIterator getting passed from one thread to another.

If that is always true, then I'm with Nicola: I would love an explanation of what is going on here to cause the debug print's he mentions in a comment above to print what they do when a failure occurs. It looks really, really wrong, as in wrong single-threaded program behavior.

Comment by Nicola Mometto [ 13/Jan/15 5:09 AM ]

Michael's and Andy's findings agree with what I observed. There's no multithreading involved yet somehow there's what appears to be a data race condition in SeqIterator.

Also, as Michael observed in his own testings, changing the sentinel from this to an Object instance (START) makes the issue go away, this is exactly what the patch I attached does.

I am also unable to reproduce this issue by repeatedly invoking vec on an empty set.

Comment by Nicola Mometto [ 13/Jan/15 5:15 AM ]

This just happened:

public boolean hasNext(){
    Object a = seq;
    Object b = this;
	if(seq == this){
		seq = START;
		next = RT.seq(next);
		}
	else {
        if (RT.seq(next) == null) {
            System.out.println (a);
            System.out.println (b);
        }
        if(seq == next)
            next = RT.next(seq);
    }
	return next != null;
}
clojure.lang.SeqIterator@3ddc6873
clojure.lang.SeqIterator@3ddc6873
Exception in thread "main" java.util.NoSuchElementException, compiling:(utils.clj:68:1)
Comment by Nicola Mometto [ 13/Jan/15 5:45 AM ]

I'm taking a shot in the dark but I tried reproducing this bug using -Xint (using the jvm interpreter) and I can't seem to be able to reproduce it after many runs.
As absurd as this sounds, I'm starting to think that some hotspot optimization is responsible for this nonsensical behaviour – this would explain the nondeterministic behaviour in the absence of multithreading.

Comment by Alex Miller [ 13/Jan/15 8:50 AM ]

I did some experiments myself yesterday and found many of the same things listed here - single-threaded usage with seemingly impossible results.

I am currently thinking that setting seq = this; in the constructor is unsafe publication of this. "this" is not valid until after the constructor completes, yet we have saved away a reference to it in a field.

Thus seq==this may possibly return false in hasNext() because it is comparing a partially constructed object with the fully constructed object (same object identity!). It may be that this doesn't happen until after hot spot/inlining. By turning on the inline diagnostics, I do see these SeqIterator methods as a hot spot that gets inlined at some point soon before I see the error manifest.

This is my best explanation of the results we're seeing and seems sufficient justification to change the implementation of SeqIterator to me.

Comment by Nicola Mometto [ 13/Jan/15 8:59 AM ]

From the Java Language Spec, section 15.8.3:
"The keyword this may be used only in the body of an instance method or default method, or in the body of a constructor of a class, or in an instance initializer of a class, or in the initializer of an instance variable of a class"

So I don't believe that the current usage of this is incorrect for this reason, however I agree that it's likely caused by some interaction of doing identity check with this and hot spot/inlining issue so I'm more confident that the patch I posted is a right fix for this bug.

Comment by Andy Fingerhut [ 13/Jan/15 10:30 AM ]

The only other scenario of multiple calls to next / hasNext that I could think of with a single thread is if some method call inside of hasNext causes next / hasNext to be called, nested, on the same SeqIterator instance. I did some instrumentation to check for nested calls, unlikely as that would seem from the source code, and saw a failure with no nested call to hasNext.

Comment by Alex Miller [ 13/Jan/15 4:28 PM ]

Midje stack trace (removed from original ticket, left here for safe-keeping). To reproduce:

// set your JAVA_HOME and PATH to JDK 1.7
git clone git@github.com:marick/Midje.git
cd Midje
git co e98cf87
lein with-profile 1.7 midje

Error:

Exception in thread "main" java.lang.NullPointerException, compiling:(fim_collection_diffs.clj:7:1)
	at clojure.lang.Compiler.macroexpand1(Compiler.java:6619)
	at clojure.lang.Compiler.macroexpand(Compiler.java:6677)
	at clojure.lang.Compiler.eval(Compiler.java:6751)
	at clojure.lang.Compiler.load(Compiler.java:7194)
	at clojure.lang.RT.loadResourceScript(RT.java:370)
	at clojure.lang.RT.loadResourceScript(RT.java:361)
	at clojure.lang.RT.load(RT.java:440)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5436.invoke(core.clj:5863)
	at clojure.core$load.doInvoke(core.clj:5862)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5653)
	at clojure.core$load_lib$fn__5383.invoke(core.clj:5708)
	at clojure.core$load_lib.doInvoke(core.clj:5707)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5746)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5829)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at midje.repl$load_facts$fn__6148.invoke(repl.clj:206)
	at midje.repl$load_facts.doInvoke(repl.clj:192)
	at clojure.lang.RestFn.invoke(RestFn.java:397)
	at user$eval6211.invoke(form-init3965655274254111851.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6767)
	at clojure.lang.Compiler.eval(Compiler.java:6757)
	at clojure.lang.Compiler.load(Compiler.java:7194)
	at clojure.lang.Compiler.loadFile(Compiler.java:7150)
	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.maybeResolveIn(Compiler.java:7014)
	at clojure.core$ns_resolve.invoke(core.clj:4200)
	at clojure.core$ns_resolve.invoke(core.clj:4197)
	at clojure.core$resolve.invoke(core.clj:4206)
	at clojure.core$map$fn__4529.invoke(core.clj:2612)
	at clojure.lang.LazySeq.sval(LazySeq.java:40)
	at clojure.lang.LazySeq.seq(LazySeq.java:49)
	at clojure.lang.RT.seq(RT.java:485)
	at clojure.core$seq__4109.invoke(core.clj:135)
	at clojure.core$reduce1.invoke(core.clj:899)
	at clojure.core$set.invoke(core.clj:3944)
	at clojure.core$emit_defrecord.invoke(core_deftype.clj:154)
	at clojure.core$defrecord.doInvoke(core_deftype.clj:374)
	at clojure.lang.RestFn.invoke(RestFn.java:470)
	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.lang.Compiler.macroexpand1(Compiler.java:6606)
Comment by Ghadi Shayban [ 13/Jan/15 4:32 PM ]

Not sure I fully understand the sad path that causes this bug, but START can safely be marked static final in the patch.

Comment by Andy Fingerhut [ 13/Jan/15 4:54 PM ]

Does this seem like it may be a bug in JIT compilation to anyone? I ask because as far as we can tell, this bug occurs completely within a single thread, and as far as I've read, the Java memory model should guarantee that operations in a single thread appear to occur in the order they happen in the source code.

Independent question: It seems that the assumption is that a SeqIterator is only ever accessed from 1 thread. Is it considered an internal implementation detail, and thus on documentation is needed about this assumption?

Comment by Alex Miller [ 13/Jan/15 5:22 PM ]

Re JIT - I wouldn't rule that out, but if it is, that doesn't help us make Clojure work again for everyone with existing JVMs. JCiP section 3.2.1 says "If the this reference escapes during construction, the object is considered not properly constructed." which sounds like what we're doing.

Re threading - I think that the use of iterators inside Clojure itself has (until recently) been unusual. Virtually everything is written to leverage the seq model and iterators were largely provided for Java compliance. With the creation of LazyTransformer and extension of reduce to iterators, this orientation has changed somewhat. However, I'd say that iterators are dirty stateful things and they should be consumed in localized contexts by no more than one thread at a time. If they are used in a thread-confined way and safely published between threads, SeqIterator seems ok.

Comment by Nicola Mometto [ 13/Jan/15 5:32 PM ]

0001-CLJ-1636-don-t-use-this-as-a-sentinel-in-SeqIterator.patch is the same as 0001-fix-for-CLJ-1636.patch except it makes START final as suggested by Ghadi

Comment by Ghadi Shayban [ 15/Jan/15 12:27 PM ]

Alex has screened this – which probably implies that the patch fixes the issue. Just to confirm does the patch clear up the issue for everyone else?

Comment by Michael Blume [ 15/Jan/15 12:48 PM ]

Does for me, yes =)

Comment by Andy Fingerhut [ 15/Jan/15 1:17 PM ]

If you want to collect test results, seems like it would be good for people to respond with the OS version and JVM version they tested on, and whether it was the Midje test in the description of this ticket that they tried, or some other test.

Comment by Andy Fingerhut [ 16/Jan/15 4:59 PM ]

Out of curiosity, does anyone have a smaller test case that causes this incorrect return value from the SeqIterator, without running the 'lein with-profile 1.7 midje' command on Midje? e.g. running a page or less worth of Clojure code?

Comment by Alex Miller [ 16/Jan/15 6:01 PM ]

While we've applied the patch, I would still love to understand wtf is happening here and would love to see that too. For me, I can quite reliably reproduce it building Clojure itself. To support the theory of it happening during an inlining transition, it's unlikely to reproduce outside the context of other code however. I see it get embedded inside a big wad of calling code when I watching inlining.

Comment by Nicola Mometto [ 18/Jan/15 11:36 AM ]

I've spent some time reading through both the jvm and the java specs and I can't find a reasonable explaination for what was happening, I can only think this is a bug in some hotspot inlining optimization.





[CLJ-1635] Make distinct/dedupe/interpose transducer tests play nicely with new reporting Created: 09/Jan/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: Michael Blume Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: test, transducers

Attachments: Text File clj-1635.patch    
Patch: Code and Test
Approval: Ok

 Description   

Fix interaction between CLJ-1601 (which introduced new transducers) and CLJ-1621 (which improved transducer tests) to improve test reporting for these new transducer arities as well.

Note from Alex M: I goofed these while rebasing CLJ-1601 after CLJ-1621 went in.

Patch: clj-1635.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 09/Jan/15 2:31 PM ]

My fault in the integration process! We'll try to get it fixed in 1.7. Thanks...





[CLJ-1633] PersistentList/creator doesn't handle ArraySeqs correctly Created: 07/Jan/15  Updated: 10/Jan/15  Resolved: 10/Jan/15

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: collections

Attachments: Text File 0001-CLJ-1633-fix-PersistentList-creator-handling-of-Arra.patch     Text File CLJ-1633-v2.patch     Text File CLJ-1633-v3.patch     Text File generative-seq-tests.patch     Text File generative-seq-tests-v2.patch    
Patch: Code and Test
Approval: Ok

 Description   

This should return '(2 3) but returns '(1 2 3) instead:

user=> ((fn [& args] (apply (fn [a & b] (println a b) (apply list b)) args)) 1 2 3)
1 (2 3)
(1 2 3)

Note that using vector rather than list returns the correct values:

user=> ((fn [& args] (apply (fn [a & b] (println a b) (apply vector b)) args)) 1 2 3)
1 (2 3)
[2 3]

The bug was reported in this stackoverflow question: https://stackoverflow.com/questions/27819418/strange-behaviour-of-clojure-trampoline and the bug identified in this comment: https://stackoverflow.com/questions/27819418/strange-behaviour-of-clojure-trampoline#comments-27821793

A simpler example of this bug:

user=> (apply list (next (clojure.lang.ArraySeq/create (object-array [1 2 3]))))
(1 2 3)

Patch: CLJ-1633-v3.patch



 Comments   
Comment by Michael Blume [ 07/Jan/15 12:28 PM ]

Very nice catch.

This makes me wonder if there's more we can do with generative testing to catch this class of bugs, maybe along the lines of zach tellman's collection-check

Comment by Alex Miller [ 07/Jan/15 2:27 PM ]

There's definitely more we can do. collection-check is great and I've started to integrate some of those ideas into Clojure's tests (see for example the new transducer tests that generate random chains of sequence operations and compare seq and transducer executions). If you have ideas about specific test areas, would be happy to see a jira/patch.

Comment by Michael Blume [ 07/Jan/15 4:59 PM ]

Updated test to get expected list inside the (= ...) form

Comment by Michael Blume [ 07/Jan/15 5:03 PM ]

Oops, rerolling again to apply cleanly to master

Comment by Nicola Mometto [ 07/Jan/15 5:04 PM ]

Thanks for the fix!

Comment by Michael Blume [ 07/Jan/15 5:40 PM ]

No problem =)

Comment by Michael Blume [ 07/Jan/15 5:42 PM ]

Ok, this is kind of crude, and mostly a proof of concept, but this does catch the bug.

Output:

[java] FAIL in (seq-gentest) (sequences.clj:105)
     [java] {:acts1 (-> [] next (->> (cons :foo)) (->> (cons :foo)) next),
     [java]  :acts2
     [java]  (->
     [java]   []
     [java]   next
     [java]   (->> (cons :foo))
     [java]   (->> (cons :foo))
     [java]   into-array-seq
     [java]   next
     [java]   (->> (apply list))),
     [java]  :result1 (:foo),
     [java]  :result2 (:foo :foo),
     [java]  :pass false}
     [java]
     [java] expected: (:result res)
     [java]   actual: false
Comment by Alex Miller [ 09/Jan/15 9:12 AM ]

Rich said to move this forward.

Comment by Nicola Mometto [ 09/Jan/15 5:51 PM ]

This ticket has been closed but no patch has been committed

Comment by Alex Miller [ 09/Jan/15 6:22 PM ]

Stu, doesn't look like this patch was applied but it was closed?





[CLJ-1621] Improve reporting in transducers generative test. Created: 21/Dec/14  Updated: 09/Jan/15  Resolved: 09/Jan/15

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

Type: Enhancement Priority: Minor
Reporter: Michael Blume Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: transducers

Attachments: Text File transducer-reporting-v1.patch    
Patch: Code and Test
Approval: Ok

 Description   

If the transducers generative test breaks, you get output like this:

[java] {:test-var seq-and-transducer, :result #<ExceptionInfo clojure.lang.ExceptionInfo: Applied actions to coll as seq, sequence transducer, and into transducer and got different results. {:coll [-16 10 -8 8 -5], :actions map clojure.core$dec@782a4056,take 5,partition-by clojure.core$even_QMARK_@2200d281,partition-all 9,map clojure.core$inc@643b798d,drop 9,remove clojure.core$empty_QMARK_@4600f352,remove clojure.core$odd_QMARK_@32dd05af, :s #<ClassCastException java.lang.ClassCastException: clojure.lang.LazySeq cannot be cast to java.lang.Number>, :xs (), :xi [], :xt []}>, :seed 1419199634890, :failing-size 21, :num-tests 22, :fail [[-16 10 -8 8 -5] [{:desc map clojure.core$dec@782a4056, :xf #<core$map$fn__3669 clojure.core$map$fn__3669@28449652>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@506b8505>} {:desc take 5, :xf #<core$take$fn__3712 clojure.core$take$fn__3712@38934406>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@27ce0b6d>} {:desc partition-by clojure.core$even_QMARK_@2200d281, :xf #<core$partition_by$fn__5568 clojure.core$partition_by$fn__5568@5287c159>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@70961c7b>} {:desc partition-all 9, :xf #<core$partition_all$fn__5590 clojure.core$partition_all$fn__5590@3f869b0>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@6f99ed9f>} {:desc map clojure.core$inc@643b798d, :xf #<core$map$fn__3669 clojure.core$map$fn__3669@2f2c41d3>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@2fdbef8d>} {:desc drop 9, :xf #<core$drop$fn__3728 clojure.core$drop$fn__3728@4f7b4b50>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@214b9b5>} {:desc remove clojure.core$empty_QMARK_@4600f352, :xf #<core$filter$fn__3696 clojure.core$filter$fn__3696@6846d654>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@7df231c7>} {:desc remove clojure.core$odd_QMARK_@32dd05af, :xf #<core$filter$fn__3696 clojure.core$filter$fn__3696@5a8ce6dd>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@34ee9000>}]], :shrunk {:total-nodes-visited 32, :depth 12, :result #<ExceptionInfo clojure.lang.ExceptionInfo: Applied actions to coll as seq, sequence transducer, and into transducer and got different results. {:coll [0], :actions map clojure.core$inc@643b798d, :s (1), :xs (0), :xi [0], :xt [0]}>, :smallest [[0] [{:desc map clojure.core$inc@643b798d, :xf #<core$map$fn__3669 clojure.core$map$fn__3669@2f2c41d3>, :seq #<core$partial$fn__3652 clojure.core$partial$fn__3652@2fdbef8d>}]]}}
     [java]
     [java] ERROR in (seq-and-transducer) (core.clj:4566)
     [java] Uncaught exception, not in assertion.
     [java] expected: nil
     [java]   actual: clojure.lang.ExceptionInfo: Applied actions to coll as seq, sequence transducer, and into transducer and got different results.
     [java]  at clojure.core$ex_info.invoke (core.clj:4566)
     [java]     clojure.test_clojure.transducers$seq_and_transducer_same_result.invoke (transducers.clj:103)
     [java]     clojure.lang.AFn.applyToHelper (AFn.java:156)
...etc etc

This has a few problems:

  • when clojure functions are given as arguments, they're full object printouts, with classnames and memory addresses, this is kind of hard to read
  • the combination of the first problem found with the shrunk version means there's a lot of content to read
  • lack of pretty printing makes that content very hard to read
  • the traceback isn't actually that helpful – we know what failed already.

Approach: The attached patch encodes more descriptive info in the actions and does a better job of reporting the difference in an understandable manner:

[java] FAIL in (seq-and-transducer) (transducers.clj:135)
     [java] {:coll [0],
     [java]  :actions (->> coll (map inc)),
     [java]  :s (1),
     [java]  :xs (0),
     [java]  :xi [0],
     [java]  :xt [0]}

Patch: transducer-reporting-v1.patch

Screened by: Alex Miller






[CLJ-1619] PersistentVector implements IReduce but the no init arity throws Created: 17/Dec/14  Updated: 10/Jan/15  Resolved: 10/Jan/15

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-1619-Implement-no-init-arity-of-reduce-for-Persi.patch     Text File 0001-Implement-no-init-arity-of-reduce-for-PersistentVect.patch    
Patch: Code and Test
Approval: Ok

 Description   

The reduce arity of IReduce in PersistentVector is implemented as: "throw new UnsupportedOperationException()".

After the CLJ-1572 patch is applied the following code will throw:

(reduce + [1 2])

Approach taken: Implement reduce(f) in PersistentVector.

Alternative: An alternate would be to change PersistentVector from IReduce to IReduceInit and remove the reduce without init function. In this case, reducing a vector would fall back to seqs.

Patch: 0001-CLJ-1619-Implement-no-init-arity-of-reduce-for-Persi.patch

Screened by: Stu



 Comments   
Comment by Alex Miller [ 18/Dec/14 10:59 AM ]

Is that return null there right? In the case of no elements, you should invoke f with no args right?

Comment by Nicola Mometto [ 18/Dec/14 11:04 AM ]

you're right, I didn't know that detail about the behaviour of reduce. Updated the patch to invoke (f) rather than returning nil when the coll is empty

Comment by Alex Miller [ 09/Jan/15 8:14 AM ]

Needs tests

Comment by Nicola Mometto [ 09/Jan/15 8:33 AM ]

Updated patch adding testcases for PersistentVector.reduce in the already existing reduce test

Comment by Stuart Halloway [ 09/Jan/15 11:50 AM ]

I don't understand the problem here. coll-reduce appears to cut off ever hitting this path (the tests call underlying interfaces directly).

  • Need a public API example showing the failure
  • Need tests covering main branches (i.e. the empty case)
Comment by Stuart Halloway [ 09/Jan/15 11:52 AM ]

nevermind, screening 1572





[CLJ-1618] Widen set to take Iterable/IReduceInit Created: 17/Dec/14  Updated: 09/Jan/15  Resolved: 09/Jan/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1618.patch    
Patch: Code
Approval: Ok

 Description   

Similar to CLJ-1546 (same thing on vec), set should work on IReducibleInit or Iterable. Currently eduction will work via Iterable but through SeqIterator. set on an IReduceInit will throw an error.

user=> (set (eduction (map inc) (range 100)))  ;; works, but slower path
user=> (set (reify clojure.lang.IReduceInit  
       (reduce [_ f start]
         (reduce f start (range 10)))))
IllegalArgumentException Don't know how to create ISeq from: user$eval1198$reify__1199  clojure.lang.RT.seqFrom (RT.java:506)

Approach: Check for and use IReduceInit path if available, otherwise fallback to seq. Additionally, the patch adds a modification to return a set without it's meta (same approach as CLJ-1546) if a set is passed, which is fast constant time with no change in effective behavior.

Performance: (using Criterium quick-bench)

Timings done with either (count (set coll)) or (count (into #{} coll)):

coll 1.6.0 into 1.6.0 set 1.7.0-alpha4 set 1.7.0-alpha4+patch set
(set (range 100)) 15.4 µs 17.0 µs 11.4 µs 0.0 µs
(vec (range 1000000)) 360.7 ms 702.5 ms 391.1 ms 358.6 ms
(doall (range 1000000)) 363.6 ms 736.9 ms 387.5 ms 371.0 ms
(doall (range 5)) 404.9 ns 612.3 ns 481.9 ns 445.9 ns
(eduction (map identity) (vec (range 100))) n/a n/a 11.3 µs 8.7 µs

See also: CLJ-1546, CLJ-1384

Patch: clj-1618.patch

Screened by:






[CLJ-1606] Transducing an eduction finishes twice Created: 27/Nov/14  Updated: 09/Jan/15  Resolved: 09/Jan/15

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

Type: Defect Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: transducers
Environment:

1.7.0-alpha4


Attachments: Text File CLJ-1606-2.patch     Text File CLJ-1606-3.patch     Text File CLJ-1606-4.patch     Text File CLJ-1606-5.patch     Text File CLJ-1606.patch    
Patch: Code and Test
Approval: Ok

 Description   
> (transduce (map identity)
             (fn
               ([s] (println "Finishing") s)
               ([s i] s))
             nil
             (eduction (map identity) []))
Finishing
Finishing
nil

Cause: transduce passes (xf f) into .reduce of Eduction, which calls transduce, causing completing xf to be called more than once.

Proposed: Eduction reduce should use (completing f) instead of f to isolate completion of inner xf from outer xf.

Patch: CLJ-1606-5.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 27/Nov/14 11:01 PM ]

identity is not a valid xf - changed to (map identity)

Comment by Ghadi Shayban [ 27/Nov/14 11:34 PM ]

identity is a valid though nonsensical transducer. fix & test added.

Comment by Ghadi Shayban [ 28/Nov/14 12:06 AM ]

Simple reproduction similar to into:

(transduce (map dec)
           (completing conj! persistent!)
           (transient [])
           (eduction (map inc) (range 6)))

;; ClassCastException clojure.lang.PersistentVector cannot be cast to clojure.lang.ITransientCollection

into doesn't use completing, and conj! has an arity that hides the problem.

Comment by Alex Miller [ 28/Nov/14 8:54 AM ]

I removed trailing whitespace in the patch so it applies cleanly.

Comment by Ghadi Shayban [ 14/Dec/14 11:16 PM ]

This patch is a little more subtle than I thought. Completion of the eduction's rfn needs to be handled separately from the "outer" transduce's xform. Patch coming.

Comment by Ghadi Shayban [ 14/Dec/14 11:32 PM ]

New patch with tests that completes the inner xform without completing the passed in rfn

Comment by Ghadi Shayban [ 15/Dec/14 1:19 AM ]

both -3 and -2 are equivalent. -3 is probably better stylistically.

Comment by Alex Miller [ 15/Dec/14 8:37 AM ]

Added CLJ-1606-4.patch - identical to -3, just fixed whitespace error.

Comment by Andy Fingerhut [ 08/Jan/15 6:09 PM ]

There are two identically named attachments here (containing -2). It looks like it isn't the one under consideration, but it might be nice to remove or rename to avoid the name conflict.

Comment by Ghadi Shayban [ 08/Jan/15 6:24 PM ]

Andy, not sure how to do that, but in any case I just added -5 clarifying language in the comment

Comment by Alex Miller [ 08/Jan/15 6:41 PM ]

Ghadi, that was super confusing. Did you just add a new -5 patch? The -4 patch has already been screened, and you have not removed the duplicate -2 patch so I don't get what the -5 is. Can we just delete the -5 and older -2 patches?

Comment by Andy Fingerhut [ 08/Jan/15 6:44 PM ]

Sorry for adding to the confusion. Ghadi, instructions for deleting patches are in the "Removing patches" section on this wiki page: http://dev.clojure.org/display/community/Developing+Patches

Comment by Ghadi Shayban [ 08/Jan/15 6:50 PM ]

Sorry. Fine by me, though permissions prevent me from deleting one of the patches.

As I read through the screened patch I just tried to clarify the wording. This:

;; NB (completing f) isolates completion of inner xfns from outer xfns
became:
;; NB (completing f) isolates completion of inner rf from outer rf

Feel free to nix that -5 patch if that's worthless

Comment by Alex Miller [ 08/Jan/15 7:12 PM ]

Gotcha. I will take care of the further changes later tonight.

In the future, please don't modify screened patches without letting me know.





[CLJ-1604] AOT'ed code that defs a var with clojure.core symbol name causes IllegalStateException Created: 25/Nov/14  Updated: 21/Feb/15  Resolved: 20/Feb/15

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 3
Labels: aot, compiler

Attachments: Text File 0001-fix-AOT-bug-preventing-overriding-of-clojure.core-fu.patch     Text File 0001-fix-AOT-bug-preventing-overriding-of-clojure.core-fu-v2.patch     File 1604-context.diff    
Patch: Code
Approval: Ok

 Description   

AOT'ed code that defs a var that is also a symbol in clojure.core results in an exception at runtime. This problem can be avoided with (:refer-clojure :exclude ...) but this requires a library author to update and release a new version. AOT'ed applications must then wait for all transitive dependencies to update before they can update to a new Clojure version. For some users, this problem prevents them from trying or adopting new releases.

For example, the contrib library data.int-map defines an update function. clojure.core will also have a new update function as of 1.7.0. If this library is AOT'ed, then users of the clojure.data.int-map/update function will see the exception below. This situation can commonly occur when an application uses lein uberjar to compile all of the project+libs. In this case, applications or libraries that use data.int-map (either directly or indirectly) are affected.

java.lang.IllegalStateException: Attempting to call unbound fn: #'clojure.data.int-map/update
 at clojure.lang.Var$Unbound.throwArity (Var.java:43)
    clojure.lang.AFn.invoke (AFn.java:40)
    compiler_update_not_referenced_bug.core$foo.invoke (core.clj:5)

Reproduce with this sample project: https://github.com/yeller/compiler_update_not_referenced_bug

Cause: When AOT compiling a namespace, the def forms are hoisted into the ns__init class (in the example here, clojure.data.int_map__init). The static initializer in this class creates each var in the ns via a call to RT.var(ns, name). For data.int-map the static initializer will properly create the var for clojure.data.int-map/update. But when the ns is loaded (via the clojure.data.int_map.load() method), (refer-clojure) will be called, which will remap clojure.data.int-map/update to point to clojure.core/update.

This problem does not affect non-AOT loading (which doesn't use the ns__init class) and does not affect collisions from any other namespace. Only collisions from clojure.core create this possibility.

Proposed: The proposed patch explicitly refers the Var during ns__init.load() (after Clojure symbols are referred) rather than implicitly during ns__init static {}.

This change in behavior only happens during AOT in the specific case where a core symbol is being shadowed. In that case, clojure.core has already been loaded and v (the looked up var) will have ns=clojure.core. The currentNS will be (for example) data.int-map. If that's the case, and the sym has no ns, then the new logic will be emitted.

In the case of clojure.core itself, NO new bytecode is emitted. From tests on several projects, only shadowed vars during AOT get this additional bytecode.

Patch: 0001-fix-AOT-bug-preventing-overriding-of-clojure.core-fu-v2.patch

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 25/Nov/14 11:28 PM ]

When I try latest Clojure master plus patch CLJ-1604-only-core.patch with the small test project created by Tom Crayford to demonstrate this issue: https://github.com/yeller/compiler_update_not_referenced_bug

In that project, I get the same exception thrown when attempting 'lein do clean, uberjar, test' using this patch, as without it. It is because int-map/update in namespace compiler-update-not-referenced-bug.core is an unbound var.

Comment by Nicola Mometto [ 26/Nov/14 4:25 AM ]

Andy, you're right. For some reason I attached the wrong patch to the ticket, this is the correct one

Comment by Nicola Mometto [ 26/Nov/14 5:21 AM ]

I wasn't able to write a test for this, so here's a repl session using the clojure jar demonstrating this issue:

[˷/test]> ls
classes  clojure.jar  test.clj
[˷/test]> cat test.clj
(in-ns 'test)
(clojure.core/refer 'clojure.core)
(def foo "bar")
(def update "foo")
[˷/test]> java -cp classes:clojure.jar:. clojure.main
Clojure 1.7.0-master-SNAPSHOT
user=> (binding [*compile-files* true] (load "test"))
WARNING: update already refers to: #'clojure.core/update in namespace: test, being replaced by: #'test/update
nil
user=> test/foo
"bar"
user=> test/update
"foo"
user=>
[˷/test]> java -cp classes:clojure.jar:. clojure.main
Clojure 1.7.0-master-SNAPSHOT
user=> (load "test")
nil
user=> test/foo
"bar"
user=> test/update
CompilerException java.lang.RuntimeException: No such var: test/update, compiling: (NO_SOURCE_PATH:0:0)
user=>
Comment by Andy Fingerhut [ 26/Nov/14 10:39 AM ]

Thanks. I have not tried to assess the details of the change, other than to say that patch 0001-fix-AOT-bug-preventing-overriding-of-clojure.core-fu.patch dated 26 Nov 2014, when applied to latest Clojure master as of today, enables both 'lein do clean, test' and 'lein do clean, uberjar, test' to work as expected with Tom Crayford's test project, linked above, whereas 'lein do clean, uberjar, test' fails without this patch, due to a var being unbound that should have a value.

Comment by Andy Fingerhut [ 27/Nov/14 10:53 AM ]

Copying a comment here from CLJ-1591, since it is more appropriate here. It is responding to Tom Crayford's posting of his example project to demonstrate the issue: https://github.com/yeller/compiler_update_not_referenced_bug

Tom, looked at your project. Thanks for that. It appears not to have anything like (def inc inc) in it. It throws exception during test step of 'lein do clean, uberjar, test' consistently for me, too, but compiles with only warnings and passes tests with 'lein do clean, test'. I have more test results showing in which Clojure versions these results change. To summarize, the changes to Clojure that appear to make the biggest difference in the results are below (these should be added to the new ticket you create – you are welcome to do so):

Clojure 1.6.0, 1.7.0-alpha1, and later changes up through the commit with description "CLJ-1378: Allows FnExpr to override its reported class with a type hint": No errors or warnings for either lein command above.

Next commit with description "Add clojure.core/update, like update-in but takes a single key" that adds clojure.core/update: 'lein do clean, test' is fine, but 'lein do clean, uberjar' throws exception during compilation, probably due to CLJ-1241.

Next commit with description "fix CLJ-1241": 'lein do clean, test' and 'lein do clean, uberjar' give warnings about clojure.core/update, but no errors or exceptions. 'lein do clean, uberjar, test' throws exception during test step that is same as the one I see with Clojure 1.7.0-alpha4. Debug prints of values of clojure.core/update and int-map/update (in data.int-map and in Tom's namespace compiler-update-not-referenced-bug.core) show things look fine when printed inside data.int-map, and in Tom's namespace when not doing the uberjar, but when doing the uberjar, test, int-map/update is unbound in Tom's namespace.

In case it makes a difference, my testing was done with Mac OS X 10.9.5, Leiningen 2.5.0 on Java 1.7.0_45 Java HotSpot(TM) 64-Bit Server VM

Comment by Nicola Mometto [ 02/Dec/14 9:04 AM ]

The updated patch only emits the interning bytecode when necessary, avoiding the emission when a clojure.core var with the same name exists but is not mapped to the current namespace

Comment by Alex Miller [ 09/Jan/15 9:27 AM ]

Attached 1604-context.diff for purely informational purposes - same diff just more context in it for easier reading.

Comment by Tom Crayford [ 10/Jan/15 4:52 PM ]

Thought I'd add a minor note in here to say I tried testing this patch out on my app (which is where I discovered this AOT bug), and the bug doesn't turn up with this patch applied to clojure (tested by applying 0001-fix-AOT-bug-preventing-overriding-of-clojure.core-fu-v2.patch to 1.7-alpha5)

Comment by Adam Krieg [ 21/Feb/15 12:28 PM ]

I ran into this issue with Korma 0.4.0. I'm still running into it, but there is a twist.

My project depends on an artifact that was built with Clojure 1.7.0-alpha1. If I remove this dependency, everything is fine. However, with this dependency, I run into this issue, even if I declare a dependency on 1.7.0-master-SNAPSHOT in my project and exclude any dependency on clojure-1.7.0-alpha1.

I'm not sure if this is a Maven issue or a Clojure issue. Running Maven with debug on seems to show that it's using the correct version of Clojure.

I have created a dummy project that reproduces this issue if you are interested.

https://github.com/deaddowney/UpdateProblem

Check it out, run "mvn install", and you will get
java.lang.RuntimeException: No such var: korma.core/update
.





[CLJ-1601] transducer arities for map-indexed, distinct, and interpose Created: 25/Nov/14  Updated: 09/Jan/15  Resolved: 09/Jan/15

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: transducers

Attachments: Text File clj-1601-2.patch     Text File clj-1601-3.patch     Text File clj-1601-4.patch     Text File clj-1601.patch     Text File clj-1601-transient-distinct.patch    
Patch: Code and Test
Approval: Ok

 Description   
  • with generative tests
  • with examples demonstrating performance

Performance: Details in comments, summary:

(def v (vec (concat (range 1000) (range 1000))))
(into [] (distinct v))            ;; 821.3 µs
(into [] (distinct) v)            ;; 388.2 µs
(into [] (interpose nil v))       ;; 316.0 µs
(into [] (interpose nil) v)       ;; 35.5 µs
(into [] (map-indexed vector v))  ;; 76.8 µs
(into [] (map-indexed vector) v)  ;; 49.4 µs

Patch: clj-1601-4.patch

Screening note: We could use transients to improve performance of the distinct impl, except checking containment in a transient set is broken per CLJ-700 (which is not currently in 1.7). I have a new patch and direction on CLJ-700 that could provide a way to solve that if we want to move it back and push this further. Or we could just wait and refactor when CLJ-700 does go in.

Screened by:



 Comments   
Comment by Alex Miller [ 25/Nov/14 11:54 AM ]

working on this

Comment by Alex Miller [ 25/Nov/14 4:22 PM ]

Initial patch with impls. Tests and perf still to do.

Comment by Alex Miller [ 27/Nov/14 7:09 AM ]

Perf tests, summarized in description:

user=> (use 'criterium.core)
nil
user=> (def v (vec (concat (range 1000) (range 1000))))
#'user/v
user=> (quick-bench (into [] (distinct v)))
WARNING: Final GC required 10.433088780213309 % of runtime
Evaluation count : 744 in 6 samples of 124 calls.
             Execution time mean : 821.339608 µs
    Execution time std-deviation : 11.351053 µs
   Execution time lower quantile : 811.901435 µs ( 2.5%)
   Execution time upper quantile : 837.972000 µs (97.5%)
                   Overhead used : 1.794010 ns
nil
user=> (quick-bench (into [] (distinct) v))
WARNING: Final GC required 10.78492057474076 % of runtime
Evaluation count : 14028 in 6 samples of 2338 calls.
             Execution time mean : 43.630656 µs
    Execution time std-deviation : 170.185825 ns
   Execution time lower quantile : 43.433193 µs ( 2.5%)
   Execution time upper quantile : 43.853959 µs (97.5%)
                   Overhead used : 1.794010 ns
				   
user=> (quick-bench (into [] (interpose nil v)))
WARNING: Final GC required 10.79555726490133 % of runtime
Evaluation count : 1914 in 6 samples of 319 calls.
             Execution time mean : 316.024853 µs
    Execution time std-deviation : 9.077484 µs
   Execution time lower quantile : 310.139273 µs ( 2.5%)
   Execution time upper quantile : 330.917486 µs (97.5%)
                   Overhead used : 1.794010 ns

Found 1 outliers in 6 samples (16.6667 %)
	low-severe	 1 (16.6667 %)
 Variance from outliers : 13.8889 % Variance is moderately inflated by outliers
nil
user=> (quick-bench (into [] (interpose nil) v))
WARNING: Final GC required 10.70401297525592 % of runtime
Evaluation count : 17022 in 6 samples of 2837 calls.
             Execution time mean : 35.592672 µs
    Execution time std-deviation : 560.066138 ns
   Execution time lower quantile : 35.252348 µs ( 2.5%)
   Execution time upper quantile : 36.553414 µs (97.5%)
                   Overhead used : 1.794010 ns

Found 1 outliers in 6 samples (16.6667 %)
	low-severe	 1 (16.6667 %)
 Variance from outliers : 13.8889 % Variance is moderately inflated by outliers
nil

user=> (quick-bench (into [] (map-indexed vector v)))
WARNING: Final GC required 12.45755646853723 % of runtime
Evaluation count : 7338 in 6 samples of 1223 calls.
             Execution time mean : 76.807691 µs
    Execution time std-deviation : 381.019170 ns
   Execution time lower quantile : 76.433202 µs ( 2.5%)
   Execution time upper quantile : 77.170733 µs (97.5%)
                   Overhead used : 1.794010 ns
nil
user=> (quick-bench (into [] (map-indexed vector) v))
WARNING: Final GC required 11.38700971837483 % of runtime
Evaluation count : 12474 in 6 samples of 2079 calls.
             Execution time mean : 49.458043 µs
    Execution time std-deviation : 620.716737 ns
   Execution time lower quantile : 48.995801 µs ( 2.5%)
   Execution time upper quantile : 50.229507 µs (97.5%)
                   Overhead used : 1.794010 ns
Comment by Alex Miller [ 17/Dec/14 1:50 PM ]

Updated based on comment from Christophe Grand that java.util.HashSet used in distinct impl had different hash/equality semantics than the set used with sequences.

Comment by Nikita Prokopov [ 21/Dec/14 6:13 AM ]

This can be further improved by using transient set instead of persistent one in distinct:

distinct with persistent set, w/o transducers:  904.932406 µs
distinct with transient set,  w/o transducers:  755.338598 µs
distinct with persistent set, with transducers: 452.170600 µs
distinct with transient set,  with transducers: 293.258473 µs

Only caveat is that transient sets do not support contains? for now (see CLJ-700). This can be solved by using (.contains ^clojure.lang.ITransientSet set key)

I’m not sure what’s the best way to attach patch to this, for now attaching a patch that can be applied on top of Alex changes (clj-1601-transient-distinct.patch).

Comment by Alex Miller [ 22/Dec/14 8:32 AM ]

Hey Nikita, I'd rather fix CLJ-700 and use the normal functions rather than what you've done in the patch, which is why I hadn't done this before. I'm waiting to check with Rich whether we'll do that in 1.7 or wait till next release.

Comment by Michael Blume [ 09/Jan/15 10:23 AM ]

1601-3 no longer applies cleanly to master, I've got a reroll that does, is it ok to attach it even though the ticket is marked 'screened'?

Comment by Alex Miller [ 09/Jan/15 10:27 AM ]

Updated tests to apply cleanly to current master in -4 patch.

Comment by Alex Miller [ 09/Jan/15 10:33 AM ]

Ha, didn't see your comment Michael! I was working on the same thing.





[CLJ-1600] calling hashCode on clojure.lang.LazyTransformer causes a StackOverflowError Created: 24/Nov/14  Updated: 09/Jan/15  Resolved: 09/Jan/15

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

Type: Defect Priority: Major
Reporter: Sam Ritchie Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: transducers
Environment:

OS X 10.10.1, Macbook Pro,, Java 1.8.0_11, Clojure 1.7.0-alpha4


Attachments: Text File CLJ-1600-2.patch     Text File CLJ-1600.patch    
Patch: Code and Test
Approval: Ok

 Description   

Calling .hashCode on a an instance of clojure.lang.LazyTransformer causes a StackOverflowError:

user> (.hashCode (sequence (map identity) ["s"]))
StackOverflowError   clojure.lang.Util.hash (Util.java:161)

The trace is

Util.java:  161  clojure.lang.Util/hash
          LazyTransformer.java:  216  clojure.lang.LazyTransformer/hashCode
                     Util.java:  161  clojure.lang.Util/hash
          LazyTransformer.java:  216  clojure.lang.LazyTransformer/hashCode
                     Util.java:  161  clojure.lang.Util/hash
          LazyTransformer.java:  216  clojure.lang.LazyTransformer/hashCode
                     Util.java:  161  clojure.lang.Util/hash
          LazyTransformer.java:  216  clojure.lang.LazyTransformer/hashCode

Relevant lines:

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/LazyTransformer.java#L212
https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Util.java#L161

Cause: Looks like "seq" returns "this":

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/LazyTransformer.java#L55

This does NOT occur on an empty sequence, as clojure.core/sequence short-circuits.

Proposal: compute and cache hash and hasheq using same algorithm used in other seqs

Patch: CLJ-1600-2.patch

Screened by: Alex Miller



 Comments   
Comment by Ghadi Shayban [ 25/Nov/14 1:18 AM ]

Patch with hashcode calculation and caching similar to ASeq. Might be worthwhile hoisting that into its own hashSeq method.

Comment by Alex Miller [ 25/Nov/14 10:13 AM ]

What's here looks good. Can we hook into existing tests that verify equals/hashcode and equiv/hasheq equivalence?

Comment by Ghadi Shayban [ 25/Nov/14 1:24 PM ]

Test case added. Verified case was failing with SO prior to patch.





[CLJ-1590] Some IReduce/IReduceInit implementors don't respect reduced Created: 14/Nov/14  Updated: 09/Jan/15  Resolved: 09/Jan/15

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-ensure-IReduce-IReduceInit-implementors-respect-redu.patch     Text File clj-1537-gvec-ArraySeq.patch    
Patch: Code and Test
Approval: Ok

 Description   

Several reduce implementations don't properly respect reduced:

  • clojure.core.ArrayChunk's implementation of IChunk/reduce
  • VecSeq's impl of InternalReduce/reduce
  • APersistentVector's reduce with init doesn't unwrap reduced on last value
  • seqs of primitive arrays don't unwrap reduced on last value
  • PersistentList doesn't unwrap reduced on last value

Some examples:

user=> (transduce (take 1) conj (seq (long-array [1 2 3 4])))
#<Reduced@38f774f8: [1]>
user=> (.reduce (list 1 2 3 4 5) (fn [_ a] (if (= a 5) (reduced "foo"))) 1)
#<Reduced@753d01cc: "foo">

Patch: 0001-ensure-IReduce-IReduceInit-implementors-respect-redu.patch
Screened by: Alex Miller
See also: http://dev.clojure.org/jira/browse/CLJ-1537



 Comments   
Comment by Alex Miller [ 17/Nov/14 12:35 PM ]

The patch should only be considering the result of calling the reducing function, not checking the init value (this matches what we do elsewhere).

Also, needs at least some simple example tests.

Comment by Nicola Mometto [ 17/Nov/14 1:36 PM ]

While reworking my patch to address your comment, I discovered that PersistentList and APersistentList's IReduceInit/reduce implementation aren't handling correctly reduced when the reducing function returns one on the last iteration.

The attached patch fixes those too and contains testcases demonstrating the issues.

Comment by Nicola Mometto [ 17/Nov/14 1:39 PM ]

I haven't fixed the IReduce/IReduceInit implementations for range as that's in scope for http://dev.clojure.org/jira/browse/CLJ-1515

Comment by Nicola Mometto [ 17/Nov/14 1:59 PM ]

As Ghadi Shayban noticed, while reduce doesn't use IReduceInit's reduce impl for PersistentList, transduce does so this might be cause of serious bugs even from clojure code, not only when using `.reduce` calls

Comment by Alex Miller [ 17/Nov/14 2:47 PM ]

reduce will use IReduceInit's reduce impl for PersistentList, after CLJ-1572.





[CLJ-1589] Cleanup internal-reduce implementation Created: 14/Nov/14  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-cleanup-internal-reduce-impl.patch    
Patch: Code
Approval: Ok

 Description   

Currently internal-reduce provides an implementation for ArraySeq and the ArraySeq_* prim classes.
Since those classes implement IReduce the current patch makes instances of those classes fallback on coll-reduce's IReduce impl (that simply invokes .reduce)

This change is desiderable because it removes unnecessary duplicated code, reducing the implementation surface and making it easier to follow reduce's code path. In addition to ArraySeq there will be (based on other tickets) more seq impls that also IReduce, so it would be good to re-route back through coll-reduce when we get combinations of potentially reducible sub-seqs.

Patch: 0001-cleanup-internal-reduce-impl.patch

  • This patch depends on the patch for CLJ-1590 since the current IReduce impl for those ArraySeq classes doesn't properly handle Reduced

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 14/Nov/14 10:28 PM ]

I'm not sure whether this should be in 1.7 or not, but I'm adding it there so we can have a discussion on it regardless.





[CLJ-1580] Transient collections should guarantee thread visibility Created: 05/Nov/14  Updated: 09/Jan/15  Resolved: 09/Jan/15

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: transient

Attachments: Text File clj-1580-2.patch     Text File clj-1580.patch    
Patch: Code
Approval: Ok

 Description   

With changes from CLJ-1498, transients are still thread isolated but may move between threads during their lifetime which introduces new concurrency concerns, namely visibility of changes across threads.

Approach: Make all transient collection fields either final or volatile to ensure visibility across threads.

Patch: clj-1580-2.patch

Screened by:



 Comments   
Comment by Stuart Halloway [ 02/Jan/15 11:39 AM ]

Should ATtransientSet.impl be volatile also?

Comment by Alex Miller [ 02/Jan/15 11:57 AM ]

Added -2 patch that makes ATransientSet.impl volatile.





[CLJ-1578] 1.7.0-alpha3 breakage due to symbol conflicts Created: 31/Oct/14  Updated: 14/Nov/14  Resolved: 14/Nov/14

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

Type: Defect Priority: Critical
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-1578-don-t-throw-when-a-core-Var-replaces-anothe.patch    
Patch: Code
Approval: Ok

 Description   

I've been trying to build core.matrix with 1.7.0-alpha3 and I get a failures due to symbol conflicts with clojure.core (specifically the new update function).

java.lang.IllegalStateException: update already refers to: #'clojure.core.matrix.utils/update in namespace: clojure.core.matrix.impl.ndarray-magic
	at clojure.lang.Namespace.warnOrFailOnReplace(Namespace.java:88)
	at clojure.lang.Namespace.reference(Namespace.java:110)
	at clojure.lang.Namespace.refer(Namespace.java:168)
	at clojure.core$refer.doInvoke(core.clj:4071)
	at clojure.lang.RestFn.invoke(RestFn.java:439)
	at clojure.core.matrix.impl.ndarray_magic$eval9762$loading__5295__auto____9763.invoke(ndarray_magic.clj:1)
	at clojure.core.matrix.impl.ndarray_magic$eval9762.invoke(ndarray_magic.clj:1)

Simpler case to reproduce:

(ns foo)
(def inc dec) ;; gets a warning 
(ns bar (:require [foo :refer :all])) ;; gets another warning
(ns bar (:require [foo :refer :all])) ;; causes the exception (effectively a ns reload)

Cause: In the case of a load, foo/inc is replacing clojure.core/inc and that causes the expected warning. In the case of a reload, clojure.core/inc is replacing foo/inc - this case is not currently handled and falls into the error case.

Approach: In the case of clojure.core/inc replacing foo/inc (should only happen during a reload), ignore and issue neither warning or error.

Patch: 0001-CLJ-1578-don-t-throw-when-a-core-Var-replaces-anothe.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 01/Nov/14 7:12 AM ]

The warnings I would expect / the failures I would not. Can you boil down the reproduction of the exception somehow?

Comment by Nicola Mometto [ 01/Nov/14 7:32 AM ]

I have seen similar failures when re-compiling a namespace that shadows a core Var:

  • ns foo is created
  • ns foo maps 'update to #'clojure.core/update
  • ns foo interns 'update, the compiler emits a warning
  • ns foo now maps 'update to #'foo/update
  • ns foo is reloaded
  • ns foo tries to map 'update to #'clojure.core/update but it's already mapped to #'foo/update

The logic in clojure.lang.Namespace/warnOnReplace makes it so that shadowing a clojure.core Var produces a warning while shadowing a Var from another namespace produces an error, this is what happening after reloading the namespace.

I haven't looked into the core.matrix code but I highly suspect that's what's going on there.

Comment by Alex Miller [ 01/Nov/14 11:27 AM ]

Definitely interested in a patch for this for the special case of clojure.core.

Comment by Nicola Mometto [ 01/Nov/14 11:41 AM ]

The attached patch fixes this issue by making warnOrFailOnReplace silently ignore when a clojure.core Var shadows another Var, which should only happen on namespace reload.

Comment by Mike Anderson [ 03/Nov/14 12:29 AM ]

The simplest way I can find to reproduce the general issue at the REPL is as follows:

(ns foo)
(def inc dec) ;; gets a warning
(ns bar (:require [foo :refer :all])) ;; gets another warning
(ns bar (:require [foo :refer :all])) ;; causes the exception (effectively a ns reload)

Preventing the exception is the biggest priority, it would be really nice to be also suppress the warnings. There are often good reasons to re-use names in clojure.core so it shouldn't cause a non-suppressible warning.

Note that the Clojure library coding standards say "Use good names, and don't be afraid to collide with names in other namespaces" so it is very inconsistent to trigger warnings / exceptions when people do exactly this.





[CLJ-1572] into does not work with IReduceInit Created: 24/Oct/14  Updated: 10/Jan/15  Resolved: 10/Jan/15

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: transducers

Attachments: Text File clj-1572-2.patch     Text File clj-1572-3.patch     Text File clj-1572-4.patch     Text File CLJ-1572-alternative-POC.patch     Text File clj-1572.patch    
Patch: Code and Test
Approval: Ok

 Description   

This should work:

(into []
  (reify clojure.lang.IReduceInit
    (reduce [_ f start]
      (reduce f start (range 10)))))
IllegalArgumentException Don't know how to create ISeq from: user$eval5$reify__6
	clojure.lang.RT.seqFrom (RT.java:506)
	clojure.lang.RT.seq (RT.java:487)
	clojure.core/seq--seq--4091 (core.clj:135)
	clojure.core.protocols/seq-reduce (protocols.clj:30)
	clojure.core.protocols/fn--6422 (protocols.clj:42)
	clojure.core.protocols/fn--6369/f--6255--auto----G--6364--6382 (protocols.clj:13)
	clojure.core/reduce (core.clj:6469)
	clojure.core/into (core.clj:6550)

Cause: CollReduce only supports IReduce, not IReduceInit so when reduce calls into it, it falls back to trying to obtain a seq representation which fails.

Proposed: Extend CollReduce to IReduceInit and in the non-init arity, cast to IReduce. Also, now that CollReduce supports both IReduceInit and Iterable, a coll that implements both makes the path through CollReduce nondeterministic. transduce does an explicit check that prefers IReduceInit - the patch copies that approach to reduce as well.

Another consequence of this change is that since PersistentVector implements IReduce but throws on the non-init path, there are some test breakages. To address this, CLJ-1619 (which implements the non-init reduce) must be applied first.

Patch: clj-1572-4.patch
Depends on: CLJ-1619 being applied first



 Comments   
Comment by Alex Miller [ 24/Oct/14 10:40 AM ]

into calls reduce which calls into CollReduce. CollReduce extends to IReduce, but not to IReduceInit. If CollReduce were extended to IReduceInit for the arity it can support, into work as expected in the given example. Patch clj-1572.patch does this.

Comment by Ghadi Shayban [ 08/Nov/14 4:34 PM ]

It is also possible that core/reduce needs the same special casing of IReduceInit that transduce has to allow for a deterministic dispatch when transduce is called with (mapcat f), as mapcat calls reduce.

Comment by Stuart Halloway [ 10/Nov/14 11:02 AM ]

Can someone please expand on Ghadi's comment with an example of the problem?

Comment by Ghadi Shayban [ 10/Nov/14 11:14 AM ]

Example of something that is Iterable & ReduceInit:
https://github.com/ghadishayban/reducers/blob/master/src/ghadi/reducers.clj#L122-L128

Let's call that r/range in this example:
(transduce (mapcat r/range) + 0 [5 5 5 5 5])

The when the mapcat transducer encounters r/range, the inner reduce call will dispatch through CollReduce upon Iterable, rather than IReduceInit.

the inner call to reduce within cat:
https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L7243

Comment by Alex Miller [ 12/Nov/14 12:55 PM ]

To restate the issue from Ghadi for my own sake:

The CollReduce protocol extends to IReduce, IReduceInit and Iterable. Because these are all interfaces, its possible for a custom coll to implement two or more of them. In that case, Clojure will arbitrarily pick which protocol impl is called - this can result in the Iterable version being called instead of IReduce/IReduceInit (which should be preferred).

transduce avoids this by explicitly checking for IReduceInit and preferring it over CollReduce.

Ghadi is suggesting that reduce should also make this preference (currently it does not).

Comment by Nicola Mometto [ 17/Nov/14 3:06 PM ]

If CollReduce could be direcly backed by the IReduce interface, this would remove the need for explicit IReduceInit checking at the callsite.

It's already possible to (defprotocol CollReduce :on-interface clojure.lang.IReduce ..), I'm proposing adding the ability to map the "reduce" method to the coll-reduce protocol-fn aswell and go with this solution

Comment by Alex Miller [ 17/Nov/14 3:21 PM ]

CollReduce extends to two interfaces (IReduceInit and Iterable) and for some impls this is ambiguous under the CollReduce protocol. The check in reduce and transduce is to force the choice of IReduceInit so it is not ambiguous. I think your suggestion re-introduces that issue? Or maybe I'm just not understanding what you mean.

Comment by Nicola Mometto [ 17/Nov/14 3:46 PM ]

Turns out defprotocol already has that capability via :on metadata field.

The attached patch is a proof of concept of my proposal, if there's interest in this approach I can fix the deftype/record/reify method parser to automatically pick the var name rather than having to specify the method name.

Comment by Nicola Mometto [ 17/Nov/14 3:52 PM ]

Ah, I see now the issue. Disregard my patch then.

Comment by Ghadi Shayban [ 14/Dec/14 11:58 AM ]

Note that unless this patch is applied, a plain reduce over an Eduction goes through the seq/iterator path of CollReduce, and not eduction's native IReduceInit path.

Comment by Ghadi Shayban [ 17/Dec/14 5:03 PM ]

with this patch + CLJ-1546

(reduce + [1 2 3]) doesn't work anymore, breaking a few tests.

Comment by Ghadi Shayban [ 17/Dec/14 5:16 PM ]

Should have left a bit more detail.
https://github.com/clojure/clojure/commit/ad7d9c46992cac0e812ce3dd47584c9bb2fda11f

This might not have anything to do with CLJ-1546, just happened to have them both applied. Seems like vectors are both IReduce+IReduceInit, but throw on the IReduce impl.

Vectors were made IReduce before IReduce was split into IReduceInit.

Comment by Nicola Mometto [ 17/Dec/14 5:19 PM ]

I've opened CLJ-1619 with a patch implementing the no-init arity of reduce for PersistentVector

Comment by Nicola Mometto [ 17/Dec/14 5:20 PM ]

An alternative fix would be to just make PersistentVectors IReduceInit rather than IReduce but I don't see the point in doing that since the implementation is trivial.

Comment by Ghadi Shayban [ 22/Dec/14 3:04 PM ]

Nicola, that is my impression, that Rich intended PersistentVector to be IReduceInit but not IReduce. But he changed it before that interface was split. Would still need some sort of way to handle the existing no-init case, which he mentioned was unfortunate at the conj.

Comment by Nicola Mometto [ 22/Dec/14 3:11 PM ]

Ghadi, what would the rationale be for PV not supporting the no-init arity? I'm not aware of any technical issues caused by my patch for CLJ-1619 but maybe you know about one?

Comment by Ghadi Shayban [ 22/Dec/14 10:58 PM ]

No I may just be confused.

Rubber-ducking aloud so that I can be corrected:

A call to init reduce with an 'init' supplied is unambiguous.

It's the responsibility of an IReduce to do what is appropriate with 'f', whereas with "improved" reduce and transduce (f) becomes the init. (c.c.reducers/reduce being the improved reduce)

IReduce implementations must preserve compatibility with core/reduce's docstring in the 0 and 1 arity cases.

If coll contains no
items, f must accept no arguments as well, and reduce returns the
result of calling f with no arguments. If coll has only 1 item, it
is returned and f is not called. If val is supplied, returns the
result of applying f to val and the first item in coll, then
applying f to that result and the 2nd item, etc. If coll contains no
items, returns val and f is not called.

A protocol's dispatch is non-deterministic when invoked upon things with multiple paths. One way to resolve the ambiguities in CollReduce is to extend CollReduce directly to any IReduce/IReduceInit impl and not rely upon friends like Iterable.

For example CLJ-1515 needs the same CollReduce extension as CLJ-1603's Iterable/Repeat/Cycle got.

(extend-protocol p/CollReduce
  clojure.lang.LongRange
  (coll-reduce [this f] (.reduce this f)
  ..

  clojure.lang.Cycle
  ...)
;; etc.
Comment by Nicola Mometto [ 23/Dec/14 4:36 AM ]

I feel like all those issues introduced by the non-deterministic dispatch of protocol functions in case of multiple available implementations, could (and should?) be solved by a prefer-method-like capability for protocols.
This way we could say have a bunch of hints like (prefer-dispatch CollReduce IReduce Iterable) and be done with it.





[CLJ-1571] Transducer of partition-by over take gives wrong answer Created: 20/Oct/14  Updated: 21/Oct/14  Resolved: 21/Oct/14

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

Type: Defect Priority: Critical
Reporter: Alex Miller Assignee: Rich Hickey
Resolution: Completed Votes: 1
Labels: transducers

Attachments: Text File 0001-CLJ-1571-fix-regression-introduced-by-43cc1854508d65.patch     Text File CLJ-1571.patch    
Approval: Ok

 Description   
(partition-by pos? (take 2 [-1 1]))
=> ((-1) (1))
(sequence (comp (take 2) (partition-by pos?)) [-1 1])
=> ([-1])


 Comments   
Comment by Nicola Mometto [ 21/Oct/14 7:49 AM ]

Given that it works fine when using transduce instead of sequence, the bug might be in LazyTransformer rather than in partition-by.

(into [] (comp (take 2) (partition-by pos?)) [-1 1])
=> [[-1] [1]]
Comment by Ghadi Shayban [ 21/Oct/14 9:21 AM ]

Patch fixes the test case, but needs eyes, I certainly may have broken something. This highlights the importance of CLJ-1554, something similar to the existing defequiv tests for reducers, but between #'into and #'sequence, also covering edge cases in reduced unwrapping.

Comment by Alex Miller [ 21/Oct/14 9:41 AM ]

Thanks Ghadi. This bug was found by the tests I wrote for CLJ-1554, so yes.

Comment by Nicola Mometto [ 21/Oct/14 9:53 AM ]

Applying this patch causes a regression in the lazyiness of sequence.
The lines that Ghadi removed for this patch were added by Rich in this commit https://github.com/clojure/clojure/commit/43cc1854508d655e58e377f84836ba128971f90c to address http://dev.clojure.org/jira/browse/CLJ-1497

Example of the regression:
current master:

user=>  (sequence (take 2) (map #(do (println (str "~" %)) %) (iterate inc 1)))
~1
~2
(1 2)

with this patch:

user=>  (sequence (take 2) (map #(do (println (str "~" %)) %) (iterate inc 1)))
~1
~2
~3
(1 2)
Comment by Nicola Mometto [ 21/Oct/14 10:03 AM ]

Patch 0001-CLJ-1571-fix-regression-introduced-by-43cc1854508d65.patch addresses this issue while preserving the current lazyness factor of `sequence`

Comment by Alex Miller [ 21/Oct/14 11:09 AM ]

Rich has a (different) patch for this on the way.

Comment by Alex Miller [ 21/Oct/14 1:16 PM ]

Fixed directly by Rich in commit https://github.com/clojure/clojure/commit/38d7572e4254afdd7f02b78095ccdb27065754d2





[CLJ-1568] Incorrect error locations reported in the stacktrace Created: 19/Oct/14  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

Type: Defect Priority: Major
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 19
Labels: errormsgs, ft

Attachments: Text File 0001-CLJ-1568-fix-incorrect-error-locations.patch     Text File 0001-CLJ-1568-fix-incorrect-error-locations-v2.patch     Text File clj-1568.patch    
Patch: Code and Test
Approval: Ok

 Description   

The following code produces an incorrect stacktrace:

(ns clojure-demo.core)

(defn foo
  "I don't do a whole lot."
  [x]
  (println x "Hello, World!"))

(/ 1 0)
Exception in thread "main" java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:6:31)

The problem is actually on the 8th line. As a matter of fact - there's nothing at location 6:31.
This is a pretty serious problem as many tools parse stacktraces for error locations.
Here's a related discussion in cider's issue tracker.

Patch: clj-1568.patch
Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 19/Oct/14 1:39 PM ]

Maybe a dupe of CLJ-1561 ?

Comment by Andy Fingerhut [ 19/Oct/14 4:16 PM ]

I tried out the example given in the description, with the latest Clojure master as of today plus the patch for CLJ-1561 called 0002-Mark-line-number-after-emitting-children.patch, dated Oct 10 2014.

The line:column number 6:31 is the same for that patched version as it is in the ticket description, which is for Clojure 1.6.0.

The issue of misleading line:column numbers is common between the two tickets, but at least the proposed improvement in CLJ-1561's patch is not effective for improving this issue.

Comment by Bozhidar Batsov [ 20/Oct/14 1:36 AM ]

I know that the issue list for 1.7 is pretty much finalised, but I think that this issue and and CLJ-1561 should be fixed as soon as possible.
Correct error reporting is extremely important IMO.

Comment by Nicola Mometto [ 20/Oct/14 8:28 AM ]

Attached a patch that fixes the issue by consuming all the whitespaces before retrieving line/column info for the next form.

Comment by Alex Miller [ 20/Oct/14 8:39 AM ]

Are there possible downsides to more eagerly consuming whitespace as done in the patch?

Comment by Nicola Mometto [ 20/Oct/14 8:44 AM ]

I can't think of any

Comment by Paul Stadig [ 22/Oct/14 2:59 PM ]

The defect on master does not have effect when using compile:

user=> (require 'clojure-demo.core)

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:6:31) 
user=> (load "/clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:6:31) 
user=> (compile "clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(core.clj:8:1) 

With the patch applied all the line numbers are the same in all cases:

user=> (require 'clojure-demo.core)

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:8:1) 
user=> (load "/clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:8:1) 
user=> (compile "clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(core.clj:8:1) 

Agreed that this seems to be orthogonal to CLJ-1561.

Comment by Bozhidar Batsov [ 10/Jan/15 3:08 AM ]

Seems we need to add tests before this can be merged - https://groups.google.com/forum/#!topic/clojure-dev/7pFhG8LMvGo

Comment by Daniel Solano Gómez [ 11/Jan/15 12:20 AM ]

Well, I've been looking into adding tests to this patch, and I've made some interesting discoveries. The additions to Compiler.load() seem to work just fine. However, I'm not seeing much coming out of the changes to Compiler.compile. You can see in Paul's comment above that the compile call actually reports the correct location. I created a test that throws a NullPointerException during compilation, and in the case of compile, it is never wrapped in a CompilerException. I'll attach my test patch that contains the example code I have been working with.

Comment by Daniel Solano Gómez [ 11/Jan/15 12:26 AM ]

Patch with tests for code paths in the fix. The tests for the Compiler.compile changes are not showing what I had expected.

Comment by Alex Miller [ 12/Jan/15 8:07 AM ]

Whenever people feel this is ready for screening, just switch the Approval from Incomplete to Vetted.

Comment by Nicola Mometto [ 12/Jan/15 8:39 AM ]

Updated patch removing the changes to Compiler.compile as they seem to be useless, by Daniel's tests

Comment by Daniel Solano Gómez [ 14/Jan/15 10:26 AM ]

I have cleaned up my test code a bit and put together a combined patch that includes both the fix and the tests.

Comment by Bozhidar Batsov [ 14/Jan/15 11:12 AM ]

Great! Looking forward to seeing this merged.

Comment by Bozhidar Batsov [ 20/Feb/15 7:50 AM ]

Seems we can finally merge this!





[CLJ-1557] Nested reduced is broken Created: 09/Oct/14  Updated: 29/Jan/15  Resolved: 10/Oct/14

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

Type: Defect Priority: Critical
Reporter: Christophe Grand Assignee: Rich Hickey
Resolution: Completed Votes: 0
Labels: transducers

Attachments: File re-reduced.diff    
Patch: Code
Approval: Vetted

 Description   

Re-reduced from composed transformation functions:

  • re-wraps the Reduced when it should not (take)
  • forget to unwrap the Reduced (partition-by, partition-all)
; nested reduced
=> (transduce (comp (take 1)) conj [:a])
[:a]
=> (transduce (comp (take 1) (take 1)) conj [:a])
#<Reduced@65979031: [:a]>
=> (transduce (comp (take 1) (take 1) (take 1)) conj [:a])
#<Reduced@fcbc8d1: #<Reduced@60bea99a: [:a]>>
=> (transduce (comp (take 1) (take 1) (take 1) (take 1)) conj [:a])
#<Reduced@6e9915bb: #<Reduced@5c712302: #<Reduced@472b9f70: [:a]>>>
 
; problems not appearing in all contexts
; not ok with transduce
=> (transduce (comp (partition-by keyword?) (take 1)) conj [] [:a])
#<Reduced@5156c42e: [[:a]]>
; but ok with sequence
=> (sequence (comp (partition-by keyword?) (take 1)) [:a])
([:a])
; well, not always
=> (sequence (comp (partition-by keyword?) (take 1)  (partition-by keyword?) (take 1)) [:a])
ClassCastException clojure.lang.Reduced cannot be cast to clojure.lang.LazyTransformer  clojure.lang.LazyTransformer$Stepper$1.invoke (LazyTransformer.java:104)

See also: https://groups.google.com/d/msg/clojure-dev/cWzMS_qqgcM/7IAhzMKzVigJ



 Comments   
Comment by Ghadi Shayban [ 09/Oct/14 11:11 PM ]

Same with partition-all

(transduce (comp (take 1) (partition-all 3) (take 1)) conj [] (range 15))
 #<Reduced@84f8976: [[0]]>
Comment by Christophe Grand [ 10/Oct/14 5:50 AM ]

patch for take, partition-by and partition-all

Comment by Nicola Mometto [ 29/Jan/15 12:09 PM ]

ticket was marked resolved but not closed





[CLJ-1554] Need to expand tests to cover transducers Created: 07/Oct/14  Updated: 14/Nov/14  Resolved: 14/Nov/14

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: transducers

Attachments: Text File clj-1554-2.patch     Text File clj-1554-3.patch     Text File clj-1554-4.patch     Text File clj-1554-5.patch     Text File clj-1554.patch    
Patch: Code and Test
Approval: Ok

 Description   

Attached patch contains both some generative and example tests for transducers. The generative tests build a series of sequence functions (take 5, filter odd?, etc) and apply them to a random vector of numbers as seq transformations, sequence of transducer, into of transducer, and transduce of transducer. The results are compared.

Note: these tests depend on the patch in CLJ-1349 to run as tests.

Patch: clj-1554-5.patch



 Comments   
Comment by Fogus [ 24/Oct/14 1:44 PM ]

I downloaded and applied this patch and its dependent patch (1349) and ran the tests. The coverage is a good start and the approach of verifying results against results gathered from other approaches is important. One note of style is that the use of `doall` is inconsistent in the `apply-as-*` functions. i would recommend that at least one other person screen this patch as my grasp of test.check is tenuous.

Comment by Alex Miller [ 24/Oct/14 2:52 PM ]

Updated patch slightly to clean up the doall stuff.

Comment by Guangyu Zhang [ 01/Nov/14 2:55 PM ]

What is clojure.test.check? You require it but never use it. This namespace doesn't exist, so I can't do individual test by (require 'clojure.test-clojure.transducers).

The error message:
CompilerException java.io.FileNotFoundException: Could not locate clojure/test/check__init.class or clojure/test/check.clj on classpath., compiling:(clojure/test_clojure/transducers.clj:1:1)

The way I used to do individual test is described in http://dev.clojure.org/display/community/Developing+Patches.

But there is no error when I run 'mvn package'.

Comment by Alex Miller [ 01/Nov/14 3:13 PM ]

As noted in the description, this patch depends on CLJ-1349 to be applied first.

Comment by Alex Miller [ 01/Nov/14 3:23 PM ]

After you apply CLJ-1349 you will also need to rerun antsetup.sh as it adds new dependencies.

Comment by Guangyu Zhang [ 02/Nov/14 12:43 AM ]

I did what you say, but the error still exists.
I can pass this test via 'ant test-example', but I can not do individual test.

To reproduce this problem:
Apply CLJ-1349 and CLJ-1554
$ ./antsetup.sh
$ ant
$ java -cp test:clojure-1.7.0-master-SNAPSHOT.jar clojure.main
user=> (require 'clojure.test-clojure.transducers)
CompilerException java.io.FileNotFoundException: Could not locate clojure/test/check__init.class or clojure/test/check.clj on classpath., compiling:(clojure/test_clojure/transducers.clj:1:1)

This should work:
$ java -cp /Users/guangyu/.m2/repository/org/clojure/test.check/0.5.9/test.check-0.5.9.jar:/Users/guangyu/.m2/repository/org/clojure/test.generative/0.5.1/test.generative-0.5.1.jar:test:clojure.jar clojure.main
user=> (require 'clojure.test-clojure.transducers)
nil

Maybe the document (http://dev.clojure.org/display/community/Developing+Patches) needs to be updated.

Comment by Guangyu Zhang [ 02/Nov/14 12:46 AM ]

There is no need to require clojure.test.check . I remove it and nothing happens.

Comment by Alex Miller [ 03/Nov/14 10:46 AM ]

That page is out of date with respect to running tests with either test.generative or test.check (which doesn't actually exist yet until CLJ-1349).

More complete recipe:

1. Apply CLJ-1349 and CLJ-1554 patches
2. ./antsetup.sh
3. ant
4. java -cp `cat maven-classpath`:target/classes:src:test clojure.main
5. (require 'clojure.test-clojure.transducers)
6. (clojure.test/run-tests 'clojure.test-clojure.transducers)

Works for me.

Confusingly, the patch in this test uses test.check, which is a generative test but run in the build (post CLJ-1349) as an example-based test. Stu and I are still talking about the best way to address that. One issue is that test.generative tests are time-based for intensity while test.check is iteration-based.

I will update the patch to remove the require of test.check.

Comment by Alex Miller [ 03/Nov/14 11:14 AM ]

I updated that testing page to cover test.generative as well.

Comment by Stuart Halloway [ 10/Nov/14 12:15 PM ]

Alex, would like to discuss two possible changes

  • make fbind create a symbolic rep of the work to do, so that failure messages are easier to read
  • whitelist the exceptions we expect, and check with a predicate in seq-and-transducer-same-result
Comment by Alex Miller [ 12/Nov/14 12:08 PM ]

Added new patch that whitelists only IllegalArgumentException and ClassCastException as the possible allowed exceptions in the transducer tests (they may vary between the transducer and non-transducer form).

The fbind does build a semantic description already in the :desc key which is used on error. Here's an example error - see the :actions key. That will be a list of the transformations applied (although shrinking often minimizes that list):

[java] Testing clojure.test-clojure.transducers
     [java] {:test-var seq-and-transducer, :result #<ExceptionInfo clojure.lang.ExceptionInfo: Applied actions to coll as seq, sequence transducer, and into transducer and got different results. {:coll [3 5 5 5 -2], :actions take 6, :s (3 5 5 5 -2), :xs (3 5 5), :xi [3 5 5], :xt [3 5 5]}>, :seed 1415806766835, :failing-size 6, :num-tests 7, :fail [[3 5 5 5 -2] [{:desc take 6, :xf #<core$take$fn__4550 clojure.core$take$fn__4550@4d186c57>, :seq #<core$partial$fn__4490 clojure.core$partial$fn__4490@44709ca4>}]], :shrunk {:total-nodes-visited 46, :depth 10, :result #<ExceptionInfo clojure.lang.ExceptionInfo: Applied actions to coll as seq, sequence transducer, and into transducer and got different results. {:coll [0 0], :actions take 2, :s (0 0), :xs (0), :xi [0], :xt [0]}>, :smallest [[0 0] [{:desc take 2, :xf #<core$take$fn__4550 clojure.core$take$fn__4550@5b938615>, :seq #<core$partial$fn__4490 clojure.core$partial$fn__4490@556733e4>}]]}}




[CLJ-1549] split IReduce Created: 06/Oct/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1549-2.patch     Text File clj-1549.patch    
Patch: Code
Approval: Ok

 Description   
  • IReduceInit should take arity-2 version from existing IReduce
  • IReduce should extend IReduceInit and add arity-1
  • new stuff should implement IReduceInit only (audit everything added for 1.7)
  • old stuff should not change or break

Patch: clj-1549-2.patch



 Comments   
Comment by Alex Miller [ 06/Oct/14 4:56 PM ]

Patch does as requested. Did not change the CollReduce extension which currently needs both arities:

(extend-protocol CollReduce
  ...

  clojure.lang.IReduce
  (coll-reduce
   ([coll f] (.reduce coll f))
   ([coll f val] (.reduce coll f val)))
Comment by Rich Hickey [ 07/Oct/14 8:29 AM ]

Can we please use the name IReduceInit instead of ILeftReduce?





[CLJ-1546] Widen vec to take Iterable/IReduce Created: 02/Oct/14  Updated: 11/Jan/15  Resolved: 10/Jan/15

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1546-2.patch     Text File clj-1546-3.patch     Text File clj-1546-4.patch     Text File clj-1546-5.patch     Text File clj-1546-6.patch     Text File clj-1546.patch     PNG File numbers.png    
Patch: Code and Test
Approval: Ok

 Description   

These examples should work but do not:

Something Iterable but not IReduce:

user> (def i (eduction (map inc) (range 100)))
#'user/i
user> (instance? java.util.Collection i)
false
user> (instance? Iterable i)
true
user> (vec i)
RuntimeException Unable to convert: class clojure.core.Iteration to Object[]

Something IReduceInit but not Iterable:

user=> (vec
  (reify clojure.lang.IReduceInit
    (reduce [_ f start]
      (reduce f start (range 10)))))
RuntimeException Unable to convert: class user$reify__15 to Object[]

Proposal: Add PersistentVector.create(Iterable) and PersistentVector.create(IReduceInit) to efficiently create PVs from those. See also the blog post http://insideclojure.org/2015/01/07/vec-perf/.

For performance, vec has several cases:
1) (vec) if vector?: return new vector w/o meta - this matches prior behavior but has a constant cost of a few ns, rather than linear cost. If not a vector, spill to LazilyPersistentVector.create(Object).

2) (LPV) instanceof IReduceInit: Anything reducible can reduce itself fastest. Right now this has a big benefit for PersistentList. on 1.7.0-alpha4 with list of size 1024, into=28 seconds, vec=18 seconds. After patch, vec=7 seconds. As more things become IReduce, they'll take this path as well. This is also the branch that handles the new Eduction and IReduceInit cases.

3) (LPV) instanceof ISeq: If the coll is a sequence already, best to just walk it rather than build an iterator or array from it. This calls into PersistentVector.create(ISeq). That implementation now contains an optimization to build into an array and construct the PersistentVector directly from the array for sequences <= 32 elements (which is common). Once that threshold is reached, it switches to building with transients. The benchmark shows that the patch makes vec substantially faster for all seqs and even faster than into in some cases.

4) (LPV) instanceof Iterable: For all non-Clojure collections (ArrayList) and current non-IReduce Clojure collections (PHM, PHS), this is fastest path. Iterators are preferred to seqs as they do not cache or hold onto the values as they go by. The PV.create() for Iterable uses transients. Due to slightly more overhead, small maps and sets are slightly slower but this is largely fixed by CLJ-1499 which adds direct iterators. ArrayLists with <= 32 are special-cased - we can toArray() and construct the PV with a seeded node in this case. This type and size is particularly common in real code. Even so, very small ArrayLists are a bit slower than they were due to increased number of conditional checks I think.

5) (LPV) otherwise RT.toArray(): catches Map, String, Object[], primitive array, etc. The important ones here are the arrays - they are slightly slower on small arrays due to overhead of checking more cases above, but big arrays are significantly faster than they were.

In addition, there was one hard-coded path in the Compiler into PersistentVector.create() and I re-routed that through LazilyPersistentVector instead as that code is now the place to choose the fastest path logic.

Patch: clj-1546-6.patch, see numbers.png for perf comparison

Screened by: Stu



 Comments   
Comment by Timothy Baldridge [ 02/Oct/14 9:44 AM ]

Is there a reason the final case for (vec something) can't just be a call to (into [] coll)? It seems a bit odd to do (to-array) on anything thats not a java collection or Iterable, when we have IReduce.

Comment by Rich Hickey [ 02/Oct/14 10:02 AM ]

re: Tim - yes, this needs to support IReduce (and thereby educe) as well

Comment by Alex Miller [ 14/Oct/14 9:56 AM ]

Added new patch that handles Iterable and IReduceInit in vec. It also makes calling with a vector much faster due to the first check. into is still faster for chunked seqs (due to special InternalReduce handling of chunking).

It would be possible to move more of the variant checking into LazilyPersistentVector or PersistentVector so it could be used in more contexts. I'm not sure how much to do with that.

It would also be possible to instead lean on reduce more from the Java side if there was a Java version of reduce (as defined in mikera's branch for http://dev.clojure.org/jira/browse/CLJ-1192 at https://github.com/mikera/clojure/compare/clj-1192-vec-performance. Something like that is the only way I can see of leveraging that same InternalReduce logic that makes into faster than vec.

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

Prior comments from Stu removed from description: "Open Question: Which branch should come first, Collection or IReduceInit? Collection reaches the fast path for small collections through LazilyPersistentVector, but IReduceInit should be faster for larger things. Related: Shouldn't the item count in LazilyPersistentVector be a bounded count?"

I have attached a new patch that simplifies the impl to do it in LazilyPersistentVector instead of in vec, which was easier due to "and" not being able yet when vec is implemented to do the length check.

I have also done a considerable amount of analysis on the matrix of incoming collections and best path to follow and also collected some data on what collections are commonly passed into vec. The current patch reflects those findings. Some highlights:

  • vec is called with PersistentVector in all projects I tested. The instanceof check takes that case from typically 100s of nanos to ~5 ns. So I do think it is worth doing.
  • vec is overwhelmingly called with small collections - in most cases the incoming collection is <10 elements. In cases where the collection is not a sequence, the path of creating the Vector with an owning array is the fastest option, beating even IReduce and transient building (as that path has some checks involved).
  • PersistentList is the only IReduce likely to be encountered by vec right now and adding that branch is a significant performance boost from prior impl and vs into. If maps and sets were IReduce, they would gain this as well.
  • chunked seqs will be significantly faster with into than vec as into goes through CollReduce and can leverage many optimizations on reducing through chunks that are not available to vec.
  • seqs in general though are now faster with vec than they were due to leveraging transients.
  • eduction results support IReduce and are also faster with vec than into.
  • range is currently slower with vec, but when range is IReduce, it will probably be faster with vec

In summary, some new conventional wisdom (after this patch) on (into []) vs vec:

  • vec is faster if passed a vector, an IReduce, or an array
  • into is faster when working with seqs, but even vec is better than it used to be and may even be faster for things like range in the future
Comment by Michael Blume [ 13/Nov/14 7:24 PM ]

Latest patch won't build for me when applied to master

compile-clojure:
     [java] Exception in thread "main" java.lang.ExceptionInInitializerError
     [java] 	at clojure.lang.Compile.<clinit>(Compile.java:29)
     [java] Caused by: java.lang.NoSuchMethodError: clojure.lang.LazilyPersistentVector.create(Ljava/util/Collection;)Lclojure/lang/IPersistentVector;, compiling:(clojure/core.clj:14:23)
     [java] 	at clojure.lang.Compiler.load(Compiler.java:7206)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:370)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:361)
     [java] 	at clojure.lang.RT.load(RT.java:440)
     [java] 	at clojure.lang.RT.load(RT.java:411)
     [java] 	at clojure.lang.RT.doInit(RT.java:448)
     [java] 	at clojure.lang.RT.<clinit>(RT.java:329)
     [java] 	... 1 more
     [java] Caused by: java.lang.NoSuchMethodError: clojure.lang.LazilyPersistentVector.create(Ljava/util/Collection;)Lclojure/lang/IPersistentVector;
     [java] 	at clojure.lang.LispReader$VectorReader.invoke(LispReader.java:1073)
     [java] 	at clojure.lang.LispReader.readDelimitedList(LispReader.java:1138)
     [java] 	at clojure.lang.LispReader$ListReader.invoke(LispReader.java:972)
     [java] 	at clojure.lang.LispReader.read(LispReader.java:183)
     [java] 	at clojure.lang.LispReader$WrappingReader.invoke(LispReader.java:535)
     [java] 	at clojure.lang.LispReader.readDelimitedList(LispReader.java:1138)
     [java] 	at clojure.lang.LispReader$MapReader.invoke(LispReader.java:1081)
     [java] 	at clojure.lang.LispReader.read(LispReader.java:183)
     [java] 	at clojure.lang.LispReader$MetaReader.invoke(LispReader.java:716)
     [java] 	at clojure.lang.LispReader.readDelimitedList(LispReader.java:1138)
     [java] 	at clojure.lang.LispReader$ListReader.invoke(LispReader.java:972)
     [java] 	at clojure.lang.LispReader.read(LispReader.java:183)
     [java] 	at clojure.lang.Compiler.load(Compiler.java:7190)
     [java] 	... 7 more
Comment by Alex Miller [ 13/Nov/14 7:28 PM ]

Did you clean first? I replaced that static method call there with a wider version but if you are cleaning fresh it should be fine.

Comment by Michael Blume [ 13/Nov/14 7:31 PM ]

Apologies, maven just wasn't doing a good job of tracking changes, running mvn clean fixes the build.

Comment by Alex Miller [ 25/Nov/14 9:58 AM ]

Added benchmark.png showing times (in ns), tested with criterium, for into and vec on different types and sizes on 1.7.0-alpha4 and then vec again after the patch.

Comment by Jonas De Vuyst [ 11/Jan/15 6:47 AM ]

This patch breaks (vec (first {1 2}))
; ClassCastException clojure.lang.MapEntry cannot be cast to clojure.lang.IObj

Comment by Alex Miller [ 11/Jan/15 8:12 AM ]

Thanks for the report! I will get that fixed in the next alpha.





[CLJ-1537] Audit IReduce usages for proper Reduced handling Created: 26/Sep/14  Updated: 14/Nov/14  Resolved: 14/Nov/14

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

Type: Defect Priority: Major
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File audit-ireduce.diff     Text File clj-1537-gvec-ArraySeq.patch     File clj-1537-v2.diff     File clj-1537-v3.diff    
Patch: Code

 Description   

Rich asked that we make sure that all usages of IReduce properly handle Reduced semantics.

Approach: I did a "Find Usages" in InteliJ and updated usages of IReduce as needed.

Example: Before the patch:

user=> (transduce (take 1) conj (seq (subvec [1 2 3 4 5] 1)))
#<Reduced@13df2a8c: #<Reduced@1ebea008: #<Reduced@72d6b3ba: #<Reduced@1787f2a0: [2]>>>>

user=> (transduce (take 1) conj '(1 2 3 4))
#<Reduced@51bd8b5c: #<Reduced@7b50df34: #<Reduced@1b410b60: #<Reduced@2462cb01: [1]>>>>

Patch: clj-1537-v3.diff
Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 30/Sep/14 5:59 PM ]

Should be same as audit-ireduce.diff but w/o whitespace diffs.

Comment by Alex Miller [ 30/Sep/14 8:25 PM ]

Should these changes be deref'ing ret?

Also, can you add an example to the description (not sure if it needs to be a test) of where these are an issue?

Comment by Timothy Baldridge [ 03/Oct/14 10:18 AM ]

Following the pattern here: https://github.com/clojure/clojure/blob/master/src/clj/clojure/core/protocols.clj#L85 they should deref the reduced value.

Comment by Timothy Baldridge [ 03/Oct/14 10:29 AM ]

Failure examples from master, are added to the description.

Comment by Timothy Baldridge [ 03/Oct/14 2:23 PM ]

clj-1537-v2.diff didn't properly deref the reduced box. clj-1537-v3.diff does this now.

Comment by Nicola Mometto [ 14/Nov/14 1:00 PM ]

I've reopened this issue as there are still instances of IReduce implementations that don't handle Reduced:

user=> (transduce (take 1) conj (seq (long-array [1 2 3 4])))
#<Reduced@38f774f8: [1]>
Comment by Nicola Mometto [ 14/Nov/14 1:00 PM ]

The attached patch (clj-1537-gvec-ArraySeq.patch) fixes the remaining IReduce impls that don't correctly handle Reduced

Comment by Nicola Mometto [ 14/Nov/14 6:08 PM ]

I'm closing this ticket again and opening a different ticket for the new patch, as asked privately by Alex Miller





[CLJ-1535] Make boxed math warning suppressible Created: 26/Sep/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: math

Attachments: Text File clj-1535-2.patch     Text File clj-1535-3.patch     Text File clj-1535.patch     Text File silence-boxed-patch-10-01-2014.patch    
Patch: Code and Test
Approval: Ok

 Description   

Clojure 1.7.0-alpha2 included a new warning that will notify on use of boxed math when unchecked-math is set to true (CLJ-1325). Based on feedback, would like to make these warnings optional.

Approach: Revert (set! *unchecked-math* true) to prior behavior. Only emit warnings when (set! *unchecked-math* :warn-on-boxed).

Patch: clj-1535-3.patch

Screened by: Stuart Halloway



 Comments   
Comment by Michael Blume [ 01/Oct/14 7:45 PM ]

So I decided to take a shot at writing a patch for this. This is my first Clojure core patch, so I've probably messed up some formatting, but the implementation was pretty simple and the tests pass.

I introduced a variable, clojure.core/silence-boxed which defaults false and, when true, silences boxed math warnings. If the reverse is preferred (warn-boxed or similar) I can do that too.

Comment by Alex Miller [ 01/Oct/14 8:34 PM ]

Hi Michael, we have other plans for how this should be implemented, so will likely not use your patch. In the future, it's always good to check if the ticket is already assigned to someone before working on it.

Comment by Alex Miller [ 07/Oct/14 8:12 AM ]

Added clj-1535-3.patch, which is exactly the same diff as clj-1535-2.patch, but just squashes into a single commit.





[CLJ-1529] Significantly improve compile time by reducing calls to Class.forName Created: 21/Sep/14  Updated: 14/Nov/14  Resolved: 14/Nov/14

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

Type: Enhancement Priority: Critical
Reporter: Zach Tellman Assignee: Unassigned
Resolution: Completed Votes: 28
Labels: compiler, performance

Attachments: File class-for-name.diff     File clj-1529-no-cache-2.diff     File clj-1529-no-cache.diff     PNG File clj-1529.png     File clj-1529-with-cache.diff     Text File maybe-class-cache-2.patch     Text File maybe-class-cache.patch    
Patch: Code
Approval: Ok

 Description   

Compilation speed has been a real problem for a number of my projects, especially Aleph [1], which in 1.6 takes 18 seconds to load. Recently I realized that Class.forName is being called repeatedly on symbols which are lexically bound. Hits on Class.forName are cached, but misses are allowed to go through each time, which translates into tens of thousands of calls after calling `(use 'aleph.http)`.

Proposed: Avoid calling Class.forName() on non-namespaced symbols that do not contain "." or start with "[", don't map to a Class in the ns, and are names in the current local env. Also, adjust the ordering of checks in tagToClass to check for hints before checking for class.

[Note that the latest variant of the patch moves the check from the original patch farther down in the logic to avoid changing the semantics. This still yields virtually all of the performance gains. See comments for details.]

Patch: clj-1529-no-cache-2.diff

Screened by: Stu Halloway. Note that for this change the patch ended up being so small it is easier follow the code than the prose description.

[1] https://github.com/ztellman/aleph



 Comments   
Comment by Ghadi Shayban [ 21/Sep/14 4:30 PM ]

One of our larger projects (not macro-laden) just went from 36 seconds to 23 seconds to start with this patch.

Comment by Ramsey Nasser [ 03/Oct/14 12:34 PM ]

I ported this patch to Clojure-CLR for the Unity integration project and we have seen significant speedups as well. I too agree that this is the behavior I expect as a user.

Comment by Alex Miller [ 06/Oct/14 12:19 PM ]

I ran this on a variety of open-source projects. I didn't find that it produced any unexpected behavior or test errors. Most projects were about 10% faster to run equivalent of "lein test" with a few as high as 35% faster.

Comment by Alex Miller [ 07/Oct/14 12:52 PM ]

We're interested in comparing this and the class caching in fastload branch to get something in for 1.7. Next step is to extract a patch of the stuff in fastload so we can compare them better.

Comment by Alex Miller [ 07/Oct/14 4:06 PM ]

Add maybe class cache patch from fastload branch

Comment by Alex Miller [ 08/Oct/14 8:57 AM ]

Times below to run "time lein test" on a variety of projects with columns:

  • master = current 1.7.0 master
  • maybe-cache = maybe-class-cache.patch extracted from Rich's fastload branch
  • class-for-name = class-for-name.diff from Zach
  • % maybe-cache = % improvement for maybe-cache over master
  • % class-for-name = % improvement for class-for-name patch over master (sorted desc)

project,master,maybe-cache,class-for-name,% maybe-cache,% class-for-name
aleph,25.605,16.572,14.460,35.278,43.527
riemann,40.550,27.656,24.734,31.798,39.004
lamina,37.247,30.072,29.045,19.263,22.021
trapperkeeper,11.979,11.158,10.3,6.854,14.016
plumbing,73.777,68.388,66.922,7.304,9.292
cheshire,5.583,5.089,5.086,8.848,8.902
tools.analyzer,5.411,5.289,5.023,2.255,7.171
core.async,19.161,18.090,17.942,5.589,6.362
tools.reader,4.686,4.435,4.401,5.356,6.082
clara-rules,43.964,42.140,41.542,4.149,5.509
core.typed,158.885,154.954,151.445,2.474,4.683
instaparse,9.286,8.922,8.859,3.920,4.598
schema,45.3,43.914,43.498,3.060,3.978
mandoline,76.295,74.831,74.425,1.919,2.451

The summary is that both patches improve times on all projects. In most cases, the improvement from either is <10% but the first few projects have greater improvements. The class-for-name patch has a bigger improvement in all projects than the maybe-cache patch (but maybe-cache has no change in semantics).

Comment by Nicola Mometto [ 08/Oct/14 9:03 AM ]

Are the two patches mutually exclusive?

Comment by Alex Miller [ 08/Oct/14 9:35 AM ]

They are non-over-lapping. I have not considered whether they could both be applied or whether that makes any sense.

Comment by Alex Miller [ 08/Oct/14 9:53 AM ]

The two patches both essentially cut off the same hot code path, just at different points (class-for-name is earlier), so applying them both effectively should give you about the performance of class-for-name.

Comment by Alex Miller [ 08/Oct/14 2:14 PM ]

Added a picture of the data for easier consumption.

Comment by Deepak Giridharagopal [ 10/Oct/14 4:35 PM ]

One of our bigger projects saw a reduction of startup time of 16% with class-for-name, 14% with maybe-cache, and a whopping 23% with both patches applied. This was actually starting up the program, as opposed to running "lein test", FWIW.

Maybe it's worth re-running the benchmarks with a "both-patches" variant?

Comment by Alex Miller [ 10/Oct/14 5:28 PM ]

Hey Deepak, I did actually run some of them with both patches and saw times similar to class-for-name.

Were your times consistent across restarts? The times in the data above are the best of 3 trials for every data point (although they were relatively consistent).

Comment by Deepak Giridharagopal [ 10/Oct/14 6:08 PM ]

Hi Alex, the tests I ran did 20-iteration loops, and I took the mean (though it was pretty consistent between restarts). I can redo stuff and upload the raw data for you if that will help.

Comment by Deepak Giridharagopal [ 10/Oct/14 6:43 PM ]

So repeating the experiment several times does in fact behave as you suspected...apologies for my previous LOLDATA.

Comment by Alex Miller [ 24/Oct/14 3:01 PM ]

maybe-class-cache-2.patch removes some debugging stuff

Comment by Alex Miller [ 27/Oct/14 4:41 PM ]

I've done more testing and made mods to both patches and moved them closer together.

On the maybe-class-cache patch (new version = clj-1529-with-cache.diff):
1) I found that adding a final else branch that returned null was an improvement - this avoids caching things that will never hit in the future (Cons, PersistentList, Symbols with namespaces, etc). That's both a perf improvement (avoids hashing those things) and a memory improvement.
2) The tagToClass improvement from Zach's patch is orthogonal and also valid here so I added it.
3) I added Zach's check, but moved the placement lower so that it doesn't alter semantics. It helps avoid caching locals that aren't classes.

On the class-for-name patch (new version = clj-1529-no-cache.diff):
1) Same change as #3 above - moved check lower to avoid semantics change.

With these changes, both patches have tagToClass and local checks, neither patch changes semantics, and the only difference is whether to keep or remove the cache.

aleph timings (for "lein test"):

  • 1.7 master = 25.415 s
  • 1.7 + clj-1529-with-cache.diff = 14.329 s
  • 1.7 + clj-1529-no-cache.diff = 14.808 s

lamina timings (for "lein test"):

  • 1.7 master = 37.340 s
  • 1.7 + clj-1529-with-cache.diff = 28.680 s
  • 1.7 + clj-1529-no-cache.diff = 28.759 s

The cache helps slightly in both cases, but it does not seem worth adding the new dynamic var and unbounded memory use inherent in the cache.

Comment by Alex Miller [ 05/Nov/14 11:40 AM ]

Talked to Rich, focusing on no-cache patch. Added new version that fixes tabbing and restores Zach's name to the patch, which seems appropriate.





[CLJ-1524] SeqIterator constructor change broke binary compatibility in 1.7.0-alpha2 Created: 09/Sep/14  Updated: 09/Sep/14  Resolved: 09/Sep/14

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

Type: Defect Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1524.diff    
Patch: Code
Approval: Ok

 Description   

Running code AOT-compiled against Clojure 1.6.0 (or older) with 1.7.0-alpha2 runtime will encounter this error with SeqIterator:

CompilerException java.lang.NoSuchMethodError: clojure.lang.SeqIterator.<init>(Lclojure/lang/ISeq;)V, compiling:(form-init5913779045640355531.clj:1:11)

Cause: This is due to a type change in the constructor of SeqIterator from ISeq to Object (commit: https://github.com/clojure/clojure/commit/43cc1854508d655e58e377f84836ba128971f90c ).

Proposed: Add the ISeq constructor back so that calls into that constructor retain backwards binary compatibility.

Patch: clj-1524.diff

Screened by:

More: From Datomic mailing list - https://groups.google.com/forum/#!topic/datomic/KZqhY6hUHz0



 Comments   
Comment by Alex Miller [ 09/Sep/14 11:06 AM ]

Patch not applied, but similar change applied directly here:

https://github.com/clojure/clojure/commit/ba41f25b6f3f32729c55f7f7ceb179be597acf94





[CLJ-1518] Patch for removing transient thread owner check broke rrb-vector Created: 03/Sep/14  Updated: 04/Sep/14  Resolved: 04/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1518.diff    
Patch: Code
Approval: Ok

 Description   

The patch for CLJ-1498 changed the field types inside the persistent data structures, which inadvertently broke core.rrb-vector, which relies on reusing some of those internals. It is not necessary to change the type to satisfy the patch, so we would like to rollback that aspect of the change to minimize breakage.



 Comments   
Comment by Alex Miller [ 03/Sep/14 2:05 PM ]

In the patch I rolled back the changes in the Persistent*.java from CLJ-1498 and re-applied. The only "real" changes after the rollback are in ensureEditable(). Tests were left of course.





[CLJ-1512] Create volatile box for managing state Created: 25/Aug/14  Updated: 07/Oct/14  Resolved: 03/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Rich Hickey Assignee: Rich Hickey
Resolution: Completed Votes: 0
Labels: transducers

Attachments: File volatile2.diff     File volatile3.diff    
Patch: Code and Test
Approval: Ok

 Description   

Motivation:

Clojure needs a faster variant of Atom for managing state inside transducers. That is, Atoms do the job, but they provide a little too much capability for the purposes of transducers. Specifically the compare and swap semantics of Atoms add too much overhead. Therefore, it was determined that a simple volatile ref type would work to ensure basic propagation of its value to other threads and reads of the latest write from any other thread. While updates are subject to race conditions, access is controlled by JVM guarantees.

Solution overview: Create a concrete type in Java, akin to clojure.lang.Box, but volatile inside supports IDeref, but not watches etc.

API:

(volatile! x) ;;ctor
(vreset! vol newval) ;;like reset
(vswap! vol f args) ;;same shape as swap!, but MACRO over vreset!

Patch: volatile3.diff

Screened by: fogus



 Comments   
Comment by Alex Miller [ 25/Aug/14 9:11 AM ]

Dumb benchmark before/after...

java -cp target/classes -Xmx512m -server clojure.main
(def t (take 1000000))
(def v (doall (range 1000000)))
(defn bench [t v]
  (time (into [] t v)))
(dotimes [_ 30] (bench t v))

before - 29-32 ms after warmup
after - 22-23 ms after warmup

Comment by Alex Miller [ 25/Aug/14 9:12 AM ]

From Stu H elsewhere:

Three questions:
1) Should we keep volatile? in the public API?
2) Should we work in terms of IVolatile interface (guessing no)
3) Do we need a CLJS version of these APIs?

Comment by Alex Miller [ 25/Aug/14 9:13 AM ]

1. We have many tickets requesting predicates over types that are "internal" and generally I find these to be helpful. They also can help in making core more portable to cljs (maybe those fns would fall back to atoms in cljs?).
2. We have tickets requesting the equivalent of this for IAtom (CLJ-803) etc. I don't think an interface adds any value to us here though. There seems to be some requests for this kind of passthrough interface from tooling as a decoupling point. Not putting my finger on those discussions but I know I've heard this, maybe on the mailing list.
3. I think yes if that allows us to be more efficient than whatever is being done now. Not obvious to me.

Comment by Nicola Mometto [ 25/Aug/14 9:40 AM ]

Why is vswap! a macro?

Comment by Ambrose Bonnaire-Sergeant [ 26/Aug/14 8:04 AM ]

An IAtom conversation: https://groups.google.com/forum/#!searchin/clojure-dev/iatom/clojure-dev/y5QoMqd44Lc/y4YmW09blk0J

Comment by Max Penet [ 26/Aug/14 10:28 AM ]

the vswap! macro is probably for performance reasons (the main motivation of this code to begin with), to avoid using apply or unrolling tons of arities

Comment by Nicola Mometto [ 26/Aug/14 1:07 PM ]

If that is the only reason, why can't it be a regular fn + :inline metadata?

Comment by Jozef Wagner [ 27/Aug/14 3:50 AM ]

why the bang in the name of volatile! function? If the reason is to warn users that this is an 'expert only' stuff, I suggest to use a verbose name instead, e.g. volatile-reference. (This will also be consistent with approach chosen in the names of volatile-mutable and unsynchronized-mutable hints.)

Comment by Rich Hickey [ 27/Aug/14 6:37 AM ]

Can you please lift the with-meta stuff out of the syntax-quote?
Actually, if volatile! ctor returned a type-hinted value that extra hinting might not even be needed. Let's do both for now.

Also the type hint on the volatile? arg makes no sense - it's a predicate asking if something is a volatile.

Comment by Alex Miller [ 28/Aug/14 9:05 AM ]

Made changes as requested.

Comment by Fogus [ 29/Aug/14 11:01 AM ]

I downloaded the patch and applied to latest master. I ran the isolated tests and the full test suite and also ensured that the patch didn't add any reflection warnings. I then modified the ticket description to add a little more context and motivation (for future readers). The code is straight-forward and clean.

Comment by Alex Miller [ 29/Aug/14 4:31 PM ]

Updated to volatile3.diff to address offline comment from Rich.





[CLJ-1511] stack overflow when comparing sequence results Created: 24/Aug/14  Updated: 07/Oct/14  Resolved: 27/Aug/14

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

Type: Defect Priority: Critical
Reporter: Chhi'mèd Künzang Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: transducers
Environment:

OS X 10.9.4


Attachments: Text File 0001-provide-working-implementations-for-LazyTransform-eq.patch    
Patch: Code and Test
Approval: Ok

 Description   

Comparing sequences created with sequence causes a stack overflow when used as first argument to =.

Consider this transducer:

user=> (def map-inc (map inc))
#'user/map-inc

When creating a sequence and comparing with expected results, it works fine as the second argument to the comparison:

user=> (= (range 1 11) (sequence map-inc (range 10)))
true

But a stack overflow occurs when the order of arguments is reversed:

user=> (= (sequence map-inc (range 10)) (range 1 11))

StackOverflowError   clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
user=> (clojure.stacktrace/print-stack-trace *e 10)
java.lang.StackOverflowError: null
 at clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)
    clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
    clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)
    clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
    clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)
    clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
    clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)
    clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
    clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)
    clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
nil

The error persists, even if the sequence is forced with doall:

user=> (= (doall (sequence map-inc (range 10))) (doall (range 1 11)))

StackOverflowError   clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)

It does work as expected, however, if the sequence is converted to a vector:

user=> (= (vec (sequence map-inc (range 10))) (range 1 11))
true


 Comments   
Comment by Nicola Mometto [ 25/Aug/14 4:31 AM ]

Patch provides equiv/equals implementations for LazyTransform based on ASeq equiv/equals





[CLJ-1501] LazySeq switches to equiv when using equals Created: 11/Aug/14  Updated: 14/Nov/14  Resolved: 14/Nov/14

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

Type: Defect Priority: Major
Reporter: Jozef Wagner Assignee: Jozef Wagner
Resolution: Completed Votes: 0
Labels: collections, ft, interop, seq

Attachments: File clj-1501.diff    
Patch: Code and Test
Approval: Ok

 Description   

When comparing lazy seqs with java equality operator .equals, the implementation switches to the Clojures .equiv comparison. This switch is not present in any other Seq or ordered collection type.

user> (.equals '(3) '(3N))
false
user> (.equals [3] [3N])
false
user> (.equals (seq [3]) (seq [3N]))
false
user> (.equals (lazy-seq [3]) (lazy-seq [3N]))
true

Screened by: Alex Miller



 Comments   
Comment by Jozef Wagner [ 11/Aug/14 9:32 AM ]

Patch clj-1501.diff with tests added

Comment by Andy Fingerhut [ 08/Oct/14 5:50 PM ]

This ticket has no Fix Version/s, but is Screened, so at least in some code I have it is 'off the JIRA workflow state diagram'. Not sure if it shows up that way in your filters, Alex.

Comment by Alex Miller [ 08/Oct/14 7:45 PM ]

Yup thanks.





[CLJ-1498] Remove birth-thread check from transients Created: 08/Aug/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

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

Type: Enhancement Priority: Major
Reporter: Rich Hickey Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: collections, transient

Attachments: File clj-1498-2.diff     File clj-1498-3.diff     File clj-1498.diff    
Patch: Code
Approval: Ok

 Description   

Transients protect themselves from use by any thread other than the one that creates them. This is good for safety, however it eliminates certain valid usages of transients. For example, usage in a go-block might occur in subsequent invocations across multiple OS threads (but only one logical thread of control).

Current simple test:

user> (def v (transient []))
#'user/v
user> (persistent! @(future (conj! v 1)))
IllegalAccessError Transient used by non-owner thread  clojure.lang.PersistentVector$TransientVector.ensureEditable (PersistentVector.java:464)

Proposal: Remove the owner check from transient collections. (Leave the edit after persistent check as is.) The test above should succeed.

After:

user=> (def v (transient []))
#'user/v
user=> (persistent! @(future (conj! v 1)))
[1]

The clj-1498-3.diff version of the patch also replaces the AtomicReference<Thread> with AtomicBoolean as we can now track just ownership, not who owns it.

Doc update: Various pieces of documentation will need to be updated with this change, namely http://clojure.org/transients

Patch: clj-1498-3.diff

Alternative: Another idea would be to make this check optional with some kind of option on the transient call (transient coll :check-owner true). Not sure whether what the default would be for that.



 Comments   
Comment by Jozef Wagner [ 09/Aug/14 7:08 AM ]

I suggest to add a functionality to pass ownership of a transient to the different thread, or to release the ownership by passing nil.

user=> (def v (pass! (transient []) nil))
#'user/v
user=> (persistent! @(future (conj! v 1)))
[1]

pass! has to be called by current owner thread, or by any thread if the transient is currently released.

Comment by Alex Miller [ 13/Aug/14 1:42 PM ]

New patch that replaces AtomicReference<Thread> with AtomicBoolean.

Comment by Stuart Halloway [ 19/Aug/14 11:05 AM ]

Alex, can you please expand the example test you provided to a generative test that covers the following combinations:

  1. different collection sizes (above and below the ArrayMap size boundary)
  2. different shapes (vector vs. map)
  3. successful use across threads (positive use case this ticket enables)

data_structures.clj has helpers for generating transient interactions that you can build on.

Comment by Alex Miller [ 20/Aug/14 8:59 AM ]

Enhanced existing generative tests to test random actions against sets, vectors, and both PHM and PAM. Added additional actions to do transient modification actions in other threads as well as originating thread.





[CLJ-1497] sequence with transducers realizes n+2 elements Created: 08/Aug/14  Updated: 08/Aug/14  Resolved: 08/Aug/14

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1497.diff     File clj-1497v2.diff    
Approval: Ok

 Description   

The first element is realized at creation time:

user=> (def a (sequence (take 2) (map #(do (println (str "~" %)) %) (iterate inc 1))))
~1
#'user/a

Fully realizing the sequence realizes the other n-1 elements, and 2 more:

user=> a
(~2
~3
1 ~4
2)

Compare with version using seq operations:

user=> (def a (sequence (take 2 (map #(do (println (str "~" %)) %) (iterate inc 1)))))
#'user/a
user=> a
(~1
~2
1 2)

Transduce also doesn't seem to exhibit this issue:

user=> (def a (transduce (take 2) conj [] (map #(do (println (str "~" %)) %) (iterate inc 1))))
~1
~2
#'user/a
user=> a
[1 2]


 Comments   
Comment by Alex Miller [ 08/Aug/14 10:02 AM ]

Patch attached that improves the issue - will now only realize n+1 elements.

Comment by Nicola Mometto [ 08/Aug/14 10:16 AM ]

Nice, I added a commit on top of yours to delay the realization of the first element of the lazyseq to the first .next call instead of on SeqIteration creation

Comment by Alex Miller [ 08/Aug/14 11:12 AM ]

Fixed by Rich directly, not by patch.





[CLJ-1494] remove flatmap in favor of mapcat Created: 07/Aug/14  Updated: 03/Sep/14  Resolved: 03/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File 0001-remove-flatmap-use-1-arity-mapcat-instead.patch    
Patch: Code

 Description   

While all the transducers functions are implemented as an arity in the matching clojure core sequence, for mapcat a new function has been added: flatmap.
The reason for this is, as Rich said in a HN comment, "because mapcat's signature was not amenable to the additional arity".
This patch changes the mapcat signature to take at least one collection so that it's possible to add the 1-arity for the transducer function, eliminating the need for a different function, flatmap.

There has been no loss by removing the 1-arity version of mapcat as a sequence function since trying to use (mapcat f) as currently defined (not as redefined with this patch) would fail before transducers, and after transducers:
Before transducers (mapcat f) would result in a call to (map f) which would fail with an ArityException
After transducers that (map f) call would return a function, which then would be used as an argument to (apply concat the-f), resulting in a IllegalArgumentException since apply expects a sequence but it's been given a fn.



 Comments   
Comment by Alex Miller [ 03/Sep/14 11:02 AM ]

Done as a result of https://github.com/clojure/clojure/commit/7d84a9f6f35a503cddf98487b6544d18937c669e





[CLJ-1481] Typo in type-reflect's docstring Created: 27/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Defect Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, ft

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Ok

 Description   

membrer -> member

Screened by: Alex Miller






[CLJ-1480] Incorrect param name reference in defmulti's docstring Created: 27/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Defect Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, ft

Attachments: Text File 0001-Fix-param-name-reference-in-defmulti-s-docstring.patch    
Patch: Code
Approval: Ok

 Description   

attribute-map should actually be attr-map

Screened by: Alex Miller






[CLJ-1479] Typo in filterv example Created: 27/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: documentation, ft

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Ok

 Description   

filter -> filterv in changes.md

Screened by: Alex Miller






[CLJ-1478] Doc typo Created: 27/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, ft

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Ok

 Description   

Another small typo fix:
from from -> from

Screened by: Alex Miller






[CLJ-1477] Fixed a typo Created: 27/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: documentation, ft

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Ok

 Description   

Just a simple typo fix - "directy" -> "directly".

Screened by: Alex Miller






[CLJ-1466] clojure.core/bean should implement Iterable Created: 16/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Defect Priority: Major
Reporter: Ambrose Bonnaire-Sergeant Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: ft, interop

Attachments: File iterable-bean-v2.diff    
Patch: Code and Test
Approval: Ok

 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.

Screened by: Alex Miller



 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-1439] Reduce keyword cache lookup cost Created: 05/Jun/14  Updated: 07/Oct/14  Resolved: 01/Aug/14

Status: Closed
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: Unassigned
Resolution: Completed Votes: 1
Labels: keywords, performance

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

 Description   

Background: Symbol is composed of name and namespace strings. Symbol construction interns both of these strings - this reduces memory usage and allows for string == checks inside Symbol. Keywords wrap a Symbol and have an additional cache to reuse Keyword instances.

Problem: Certain applications make heavy use of keywords (in particular the case of parsing or transforming JSON, XML, or other data into Clojure maps with keyword keys). Constructing the same keyword from a string over and over again will cause the string to be interned, a symbol constructed, and the lookup to occur in the keyword cache. In the case where the keyword already exists, this is more work than is necessary, making this path slower than it can be.

Reproduce: The following test simulates rounds of creating many keywords - the unique? flag indicates whether to use new or the same set of keywords each rep. unique?=false should be more similar to parsing a similar JSON record format over and over.

(set! *unchecked-math* true)

(defn kw-new [n unique?]
  (let [base (if unique? (str (rand)) "abcdef")]
    (loop [i 0
           kws (transient [])]
      (if (< i n)
        (recur (inc i) (conj! kws (keyword (str base i))))
        (persistent! kws)))))

(defn bench-kw [reps n unique?]
  (dotimes [_ reps]
    (let [begin (System/nanoTime)]
        (kw-new n unique?)
        (let [end (System/nanoTime)
              elapsed (/ (- end begin) 1000000.0)]
          (println elapsed "ms")))))

(bench-kw 50 10000 false)  ;; expected similar to JSON use case
(bench-kw 50 10000 true)   ;; for comparison

On 1.6, we see about 5.5 ms for repeated and 134 ms for unique after warmup.
With the patch, we see about 2.2 ms for repeated and 120 ms for unique after warmup.

Cause: Keyword construction based on a string involves:

  • Interning string(s) in new kw
  • Constructing Symbol with interned strings
  • Clearing Keywords from the Keyword cache if GC has reclaimed them
  • Constructing a new Keyword
  • Wrapping the Keyword in a WeakReference
  • CHM putIfAbsent on the cache
  • If new, return. If exists, get the old one and return.
  • In the event the Keyword is reclaimed by GC between the last 2 steps, retry.

This process involves a fair amount of speculative interning and object creation if the keyword already exist.

Proposal: Streamline the keyword construction process by reworking the cache implementation and the Keyword.intern() process. The patch changes the cache to key by string name instead of symbol, deferring interning and symbol creation on lookup to when we know the keyword construction is needed. The various Keyword.intern() methods are also reworked to take advantage if called with an existing Symbol to avoid re-creating it.

Patch: 0001-Improve-Keyword.intern-performance.patch

Related: CLJ-1415



 Comments   
Comment by Alex Miller [ 01/Aug/14 11:48 AM ]

Alternate changes were committed today to improve both symbol and keyword creation times.

https://github.com/clojure/clojure/commit/c9e70649d2652baf13b498c4c3ebb070118c4573

Comment by Alex Miller [ 07/Oct/14 4:42 PM ]

related patch was applied





[CLJ-1430] Improve performance of partial Created: 23/May/14  Updated: 05/Sep/14  Resolved: 29/Aug/14

Status: Closed
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: Completed Votes: 0
Labels: performance

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

 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.

Comment by Alex Baranosky [ 05/Sep/14 9:11 PM ]

Very nice patch. I've gotten into the habit of not using partial anymore for performance sensitive code. Perhaps this change means I need to rethink that.





[CLJ-1429] Cache unknown multimethod value default dispatch Created: 22/May/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
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: Completed Votes: 2
Labels: performance

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

 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-1417] clojure.java.io/input-stream has incorrect docstring Created: 07/May/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

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

Attachments: File clj-1417.diff    
Patch: Code
Approval: Ok

 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,"

Screened by: Alex Miller






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

Status: Closed
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: Completed Votes: 0
Labels: interop

Attachments: Text File CLJ-1408-2.patch     Text File CLJ-1408-3.patch     Text File CLJ-1408.patch    
Patch: Code and Test
Approval: Ok

 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 or symbol 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)))

;; keyword example

(def k1 (serialize :k))
(println :k)
(def k2 (serialize :k))

(= k1 k2) ;;=> false 

;; symbol example

(def sym 'a)

(def s1 (serialize sym))
(println sym)
(def s2 (serialize sym))

(= s1 s2) ;;=> false

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.

Approach: Add transient keyword to _str field in Keyword and Symbol classes

Patch: CLJ-1408-3.patch

Screened by: Brenton Ashworth



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

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

Tomasz, in addition to Alex's previous comment, it appears that a commit made to Clojure master earlier today causes your patches to no longer apply cleanly. I haven't looked to see whether updating the patches would be easy, but likely it is.

Comment by Alex Miller [ 22/Aug/14 11:00 PM ]

Updated the patch for latest master and added the obvious test.





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

Status: Closed
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: Completed 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: Ok

 Description   

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

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

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

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

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

Screened by: Alex Miller



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

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

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

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

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

Moving out of Screened due to new patch

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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





[CLJ-1384] clojure.core/set should use transients Created: 15/Mar/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
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: Completed Votes: 4
Labels: performance

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

 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-1378] Hints don't work with #() form of function Created: 11/Mar/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
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: Completed Votes: 1
Labels: interop, typehints

Attachments: File clj-1378.diff     File clj-1378-v2.diff    
Patch: Code and Test
Approval: Ok

 Description   

Example showing how a local fn can be hinted but an anonymous function cannot:

;; OK
user> (let [ex (java.util.concurrent.Executors/newFixedThreadPool 1)
            f (fn [])]
        (.submit ex ^Runnable f))
nil
;; ERROR - this should work the same as the previous
user> (let [ex (java.util.concurrent.Executors/newFixedThreadPool 1)]
        (.submit ex #()))
CompilerException java.lang.IllegalArgumentException: More than one matching method found: submit, compiling:(/private/var/folders/7r/_1fj0f517rgcxwx79mn79mfc0000gn/T/form-init7901279404687292754.clj:3:9)

Cause: Functions have metadata, but Compiler does not look in them for type hints. Var expressions and local bindings use :tag metadata to override return of getJavaClass(). Compiler parses #() into a FnExpr, which always return AFunction as its class.

Proposed: Change FnExpr.getJavaClass() to return tag as type if it is available.

Patch: clj-1378-v2.diff

Screened by: Alex Miller



 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-1362] Reduce broken on some primitive vectors Created: 18/Feb/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

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

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

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

 Description   

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

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

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

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

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

Patch: clj-1362-v1.patch

Screened by:



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

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

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

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

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

Screened.





[CLJ-1357] It's a small typo in the gen-class doc-string Created: 17/Feb/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Enhancement Priority: Trivial
Reporter: Steve Miner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, ft

Attachments: Text File CLJ-1357-its-typo.patch    
Patch: Code
Approval: Ok

 Description   

"It's" should be "its" (possessive) in "It's return value is ignored."

Screened by: Alex Miller






[CLJ-1349] update to latest test.generative and prep for test.check Created: 10/Feb/14  Updated: 14/Nov/14  Resolved: 14/Nov/14

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Stuart Halloway
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File clj-1349-1.patch     Text File clj-1349-2.patch     Text File clj-1349-3.patch    
Patch: Code and Test
Approval: Ok

 Description   

This patch updates build to use the latest test.generative, and organizes build.xml so test.check can be dropped in next.

Patch: clj-1349-3

Screeners: note that the elapsed time reported at the end of an Ant build is not wall clock time. Even though the generative tests run for 60 seconds, it will report less. You can see that the tests are running for the correct duration by timing with a stopwatch if you care.



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

1) The (System/setProperty "clojure.test.generative.msec" "60000") was removed from run_tests.clj but not added to the new src/script/run_test_generative.clj. Because of this, the generative tests don't run as long and the overall build time (from generative tests) is shorter. I do not know if that was intentional.

old:

[java] Framework clojure.test.generative
     [java] {:assert/pass 1282219, :test/group 6, :test/test 120, :test/iter 10025545}

new:

[java] Framework clojure.test.generative
     [java] {:assert/pass 118974, :test/group 6, :test/test 120, :test/iter 998991}

2) The new (non-generative) part of the test lists many more namespaces being tested in the output, including gensym-ish ones like "Testing G__25228". However, both before and after the same number of tests and assertions are printed at the end. Not sure why these differ.

3) The all target could depend on test-all instead of both test and test-generative, but ok as is.

Comment by Alex Miller [ 07/Oct/14 12:51 PM ]

Patch needs some freshening...

Comment by Alex Miller [ 08/Oct/14 10:35 AM ]

Freshen patch and made some mods

Comment by Alex Miller [ 08/Oct/14 10:37 AM ]

New patch applies properly to master. I also:

  • added calls to set java.awt.headless per patches that have been added in the meanwhile
  • rejiggered the ant build targets so that "ant test" still does the same thing it did before but now there are test-example and test-generative. I think this is a smaller change and removes any need to change what we already have doc'ed for developers and screeners (just adds new sub-flavors)
  • added test.check to the dependencies




[CLJ-1330] Class name clash between top-level functions and defn'ed ones Created: 22/Jan/14  Updated: 14/Nov/14  Resolved: 14/Nov/14

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 8
Labels: aot, compiler

Attachments: Text File 0001-CLJ-1093-v3-no-locals-improv.patch     Text File 0001-CLJ-1093-v3.patch     Text File 0001-CLJ-1330-remove-local-binding-name-enhancement.patch     File demo1.clj    
Patch: Code
Approval: Ok

 Description   

Named anonymous fn's are not guaranteed to have unique class names when AOT-compiled.

For example:

(defn g [])
(def xx (fn g []))

When AOT-compiled both functions will emit user$g.class, the latter overwriting the former.

Impact: this affects apps like Cursive, which has been using a patched version of Clojure to get around this issue for quite a while.

Demonstration script: demo1.clj

Patch: 0001-CLJ-1093-v3.patch (already applied, see below)

Approach: Generate unique class names for named fn's the same way as for unnamed anonymous fn's.
The patch contains an additional enhancement to include the name of the local binding in the class name.

Comparison between pre and post patch naming scheme (N denotes unique number):

code before after note
(defn a []) user$a user$a same
(fn []) user$evalN$fn__N user$evalN$fn__N same
(fn a []) user$evalN$a__N user$evaN$a__N same
(let [a (fn [])] a) user$evalN$a__N user$evalN$a__N same
(let [a (fn x [])] a) user$eval1N$x__N user$evalN$a_x_N IMPROVED - contains local binding name
(def a (fn [])) user$a user$a same
(def a (fn x [])) user$x user$a_x_N FIXED conflict with (defn x [])
(def ^{:foo (fn [])} a) user$fn__N user$fn__N same
(def ^{:foo (fn a [])} a) user$a user$a__N FIXED conflict with (defn a [])
(def a (fn [] (fn []))) user$a$fn__N user$a$fn__N same
(def a (fn [] (fn x []))) user$a$x__N user$a$x__N same

See also: This patch also fixes the issue reported in CLJ-1227.

Screened by: Alex Miller - I am not sure whether the local binding name enhancement is worth doing. It improves debugging of which anonymous class you're talking about but has the downsides of increasing class name (and file name) length.

REOPENED Patch: 0001-CLJ-1093-v3-no-locals-improv.patch
REOPENED UPDATE: The local improvement version of this patch increases class names (and thus .class names) and we've had several reports of exceeding file system limits (~143 chars) - see comments for details. Thus, we should rollback the prior patch (0001-CLJ-1093-v3.patch) and apply the version without this enhancement. The replacement patch (0001-CLJ-1093-v3-no-locals-improv.patch) does not have the same effect on class name length.



 Comments   
Comment by Ambrose Bonnaire-Sergeant [ 22/Jan/14 11:12 AM ]

This seems like the reason why jvm.tools.analyzer cannot analyze clojure.core. On analyzing a definline, there is an "attempted duplicate class definition" error.

This doesn't really matter, but I thought it may or may not be useful information to someone.

Comment by Nicola Mometto [ 22/Jan/14 11:35 AM ]

Attached a fix.

This also fixes AOT compiling of code like:

(def x (fn foo []))
(fn foo [])
Comment by Nicola Mometto [ 22/Jan/14 11:39 AM ]

Cleaned up patch

Comment by Alex Miller [ 22/Jan/14 12:43 PM ]

It looks like the patch changes indentation of some of the code - can you fix that?

Comment by Nicola Mometto [ 22/Jan/14 3:57 PM ]

Updated patch without whitespace changes

Comment by Alex Miller [ 22/Jan/14 4:15 PM ]

Thanks, that's helpful.

Comment by Alex Miller [ 24/Jan/14 10:03 AM ]

There is consensus that this is a problem, however this is an area of the code with broad impacts as it deals with how classes are named. To that end, there is some work that needs to be done in understanding the impacts before we can consider it.

Some questions we would like to answer:

1) According to Rich, naming of (fn x []) function classes used to work in the manner of this patch - with generated names. Some code archaeology needs to be done on why that was changed and whether the change to the current behavior was addressing problems that we are likely to run into.

2) Are there issues with recursive functions? Are there impacts either in AOT or non-AOT use cases? Need some tests.

3) Are there issues with dynamic redefinition of functions? With the static naming scheme, redefinition causes a new class of the same name which can be picked up by reload of classes compiled to the old definition. With the dynamic naming scheme, redefinition will create a differently named class so old classes can never pick up a redefinition. Is this a problem? What are the impacts with and without AOT? Need some tests.

Comment by Nicola Mometto [ 24/Jan/14 11:39 AM ]

Looks like the current behaviour has been such since https://github.com/clojure/clojure/commit/4651e60808bb459355a3a5d0d649c4697c672e28

My guess is that Rich simply forgot to consider the (def y (fn x [] ..)) case.

Regarding 2 and 3, the dynamic naming scheme is no different than what happens for anonymous functions so I don't see how this could cause any issue.

Recursion on the fn arg is simply a call to .invoke on "this", it's classname unaware.

I can add some tests to test that

(def y (fn x [] 1))
and
(fn x [] 2)
compile to different classnames but other than that I don't see what should be tested.

Comment by Stuart Halloway [ 27/Jun/14 2:17 PM ]

incomplete pending the answers to Alex Miller's questions in the comments

Comment by Nicola Mometto [ 27/Jun/14 3:20 PM ]

I believe I already answered his questions, I'll try to be a bit more explicit:
I tracked the relevant commit from Rich which added the dynamic naming behaviour https://github.com/clojure/clojure/commit/4651e60808bb459355a3a5d0d649c4697c672e28#diff-f17f860d14163523f1e1308ece478ddbL3081 which clearly shows that this bug was present since then so.

Regarding redefinitions or recursive functions, both of those operations never take in account the generated fn name so they are unaffected.

Comment by Alex Miller [ 12/Sep/14 4:32 PM ]

Summarizing some cases here from before/after the patch:

1) top-level fn (always has name)
	1.6 - namespace$name
	patch - namespace$name
2) non-top-level fn with name
	1.6 - namespace$name (collides with prior case)
	patch - namespace$topname__x__name  	<-- CHANGED
3) anonymous fn (no name)
	1.6 - namespace$name$fn__x
	patch - namespace$name$fn__x
4) top-level anonymous fn (no name, not at all useful :)
	1.6 - namespace$fn__x
	patch - namespace$fn__x

The key problem is that the first 2 cases produce the identical class name on 1.6. The patch alters the non-top-level named fn so there is no conflict.

Prior to the referenced old commit, I believe cases 1 and 2 would both produce namespace$name__x (where x is unique) so they would not collide. The change was made to prevent the top-level name from changing ("don't append numbers on top-level fn class names"). While the similar change was made on non-top-level fn names, I do not think it needed to be.

I've thought through (and tried) a bunch of the implications of this with the help of Nicola's comments above and I do not see an issue with any of the things I've considered. From a binary compatibility point of view with existing AOT code, old code compiled together should be self-consistent and continue to work. New compiled code will also be consistent. I can't think of a way that new code would expect to know the old name of a non-top-level function such that there could be an issue.

One question - why change the code such that the new class name is namespace$name$topname__x__name instead of namespace$name$topname_name__x (or something else?). And relatedly, while the diff is small, could we refactor a couple more lines to make the intent and cases clearer?

I am 90% ok with this patch but want a little thought into that question before I mark screened.

Comment by Nicola Mometto [ 12/Sep/14 4:47 PM ]

Alex, the attached patch munges into ns$topname__name__x, not into ns$topname__x__name.

Comment by Nicola Mometto [ 12/Sep/14 5:22 PM ]

The attached patch 0001-Fix-CLJ-1330refactored.patch contains the same fix from 0001-FixCLJ-1330-make-top-level-named-functions-classnam.patch but also refactors the code that deals with fn name munging

Comment by Alex Miller [ 12/Sep/14 6:22 PM ]

Hmmm.. I will double-check. That's not why I recall seeing when I did AOT.

Comment by Nicola Mometto [ 12/Sep/14 7:26 PM ]

New patch 0001-CLJ-1093-v2.patch improves the fn naming scheme a lot.
I've threw together a number of test cases that show the improvement + bug fixes:

user=> (fn [])
;; pre:
#<user$eval1$fn__2 user$eval1$fn__2@4e13aa4e>
;; post: (no change)
#<user$eval1$fn__3 user$eval1$fn__3@3c92218c>
user=> (fn a [])
;; pre:
#<user$eval5$a__6 user$eval5$a__6@6946a317>
;; post: (no change)
#<user$eval6$a__8 user$eval6$a__8@6f85c59c>
user=> (let [a (fn [])] a)
;; pre:
#<user$eval9$a__10 user$eval9$a__10@15fdf894>
;; post: (no change)
#<user$eval11$a__13 user$eval11$a__13@4d051922>
user=> (let [a (fn x [])] a)
;; pre: (only contains the name of the fn)
#<user$eval17$x__18 user$eval17$x__18@7f0cd67f>
;; post: (contains the name of the local aswell as the name of the fn
#<user$eval21$a__x__23 user$eval21$a__x__23@528ef256>
user=> (def a (fn [])) a
#'user/a
;; pre:
#<user$a user$a@33e1ccbc>
;; post: (no change)
#<user$a user$a@6bef63f9>
user=> (def a (fn x [])) a
#'user/a
;; pre: (BUG!)
#<user$x user$x@59a04a1b> 
;; post: (bug fixed)
#<user$a__x__28 user$a__x__28@5f0bebef>
user=> (def ^{:foo (fn [])} a) (-> (meta #'a) :foo)
#'user/a
;; pre:
#<user$fn__23 user$fn__23@d9c21c6>
;; post: (no change)
#<user$fn__30 user$fn__30@4cf0f2eb>
user=> (def ^{:foo (fn a [])} a) (-> (meta #'a) :foo)
#'user/a
;; pre: (BUG!)
#<user$a user$a@420dd874>
;; post: (bug fixed)
#<user$a__35 user$a__35@37ff95a9>
user=> (def a (fn [] (fn []))) (a)
#'user/a
;; pre:
#<user$a$fn__30 user$a$fn__30@6f57be76>
;; post: (no change)
#<user$a$fn__41 user$a$fn__41@fd34eac>
user=> (def a (fn [] (fn x []))) (a)
#'user/a
;; pre:
#<user$a$x__35 user$a$x__35@79930089>
;; post: (no change)
#<user$a$x__48 user$a$x__48@6fc334de>
user=> (let [x (fn [])] (def a (fn [] x))) a (a)
#'user/a
;; pre:
#<user$eval40$a__43 user$eval40$a__43@6db1694e>
#<user$eval40$x__41 user$eval40$x__41@20bd16bb>
;; post (no change)
#<user$eval54$a__58 user$eval54$a__58@7c721de>
#<user$eval54$x__56 user$eval54$x__56@43f7b41b>
user=> (let [x (fn a [])] (def a (fn [] x))) (a)
#'user/a
;; pre: (the local binding name doesn't appear in the class name)
#<user$eval48$a__49 user$eval48$a__49@75d6d1d4>
;; post: (the local binding name is included in the class name)
#<user$eval64$x__a__66 user$eval64$x__a__66@460d4>

As you can see, this last patch not only fixes the two bugs, but also improves fn naming in let contexts by preserving the name of the local binding in the class name, this I believe will be a great improvement in the understandability of stacktraces.

Comment by Alex Miller [ 25/Sep/14 7:00 AM ]

The patch should be changed to not create suffix if it's not going to be used. Please update the patch to inline that into each branch name = nm.name + "__" + RT.nextID();.

I am unsure whether the "enhancement" part of this patch goes too far. I think it does provide some improvements in debugging but those seem small to me. I am somewhat concerned about greatly increasing the name of the class for nested locals thus making it harder to read stack traces. There is a large limit to class name size of 16 bits (what you can put in the constant table) but class names also map to file names and there have historically been issues on some older Windows architectures with file size limits - we are increasing the risk of running into problems with this. Small risks. I am ok with passing this on to Rich though and he can decide whether to kick that part back or not.

Comment by Nicola Mometto [ 25/Sep/14 7:08 AM ]

0001-CLJ-1093-v3.patch is identical to 0001-CLJ-1093-v2.patch except it doesn't call RT.nextID() when not necessary, as per Alex's request

Alex, if this is ok please change the "Patch:" field in the description, I won't do that myself since this ticket is now screened

Comment by Nicola Mometto [ 06/Oct/14 11:54 AM ]

Addressing the screening comment by Alex Miller, I've attached an alternative patch "0001-CLJ-1093v3-no-locals-improv.patch" which is identical to "0001CLJ-1093-v3.patch" except it doesn't include the local binding name enhancement, so that it can be picked in case Rich decides that that improvement is out of scope for this ticket.

Comment by Alex Miller [ 28/Oct/14 12:05 PM ]

I've reopened this issue based on early reports of breakage due to long file names.

Two reports:
https://groups.google.com/d/msg/clojure-dev/hnkJb9_il_M/4e5smM6mVlIJ
https://groups.google.com/d/msg/clojure/hnkJb9_il_M/QOaTdCo5wmkJ

Comment by Alex Miller [ 28/Oct/14 12:21 PM ]

Here's an example of a class name that is too long on Ubuntu 12.04 LTS 64bit / Java8 - reported max file size is 143 chars:

https://github.com/ska2342/nested-fn-breaks-clojure-17/blob/master/src/nested_fn_breaks_clojure_17/core.clj

With 1.6.0: (95 chars)
core$this_function_breaks_with_clojure_1_7$my_anonymous_function_18$iter1923$fn24$fn_25

With 1.7.0-alpha3: (144 chars)
core$this_function_breaks_with_clojure_1_7$my_anonymous_function_my_anonymous_function19$iter4951auto__iter2024$fn25$fn_26.class

With the alternate patch here, the name would be: (95 chars)
core$this_function_breaks_with_clojure_1_7$my_anonymous_function_19$iter2024$fn25$fn_26

Comment by Nicola Mometto [ 28/Oct/14 12:26 PM ]

patch "0001-CLJ-1330-remove-local-binding-name-enhancement.patch" has the same effect of reverting f149260c14a75367dc9eba91cbe9b78110113566 and applying "0001-CLJ-1093-v3-no-locals-improv.patch" in case this is preferable

Comment by Stefan Kamphausen [ 29/Oct/14 7:44 AM ]

The tiny and unusual max file size of 143 is standard in the Ubuntu 12.04 crypto container for the home directory. You can get it for any directory with 'getconf NAME_MAX /path/to/dir'.

My initial problem (other than the file to reproduce on github) was triggered by the fns in a for-expression. Don't know if that makes any difference for you.





[CLJ-1325] Report warnings if *unchecked-math* and boxing happens Created: 16/Jan/14  Updated: 11/Nov/14  Resolved: 29/Aug/14

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

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

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

 Description   

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

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

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

Patch: clj-1325-v3.patch

Screened by:



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

Moving to 1.7.

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

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

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

Ready for screening.

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

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

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

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

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

Screened. Comments:

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

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

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

Comment by Nicola Mometto [ 11/Nov/14 6:02 PM ]

With the new :warn-on-boxed, this code reports a warning:

user=> (defn f [x] (inc x))
Boxed math warning, NO_SOURCE_PATH:2:13 - call: public static java.lang.Number clojure.lang.Numbers.unchecked_in
#'user/f

but this does not:

user=> (defn f1 [^long x] (inc x))
#'user/f1

is this intentional?

Comment by Alex Miller [ 11/Nov/14 9:00 PM ]

The bytecode for those methods is:

f:
  public java.lang.Object invoke(java.lang.Object);
    Code:
       0: aload_1
       1: aconst_null
       2: astore_1
       3: invokestatic  #34                 // Method clojure/lang/Numbers.unchecked_inc:(Ljava/lang/Object;)Ljava/lang/Number;
       6: areturn

f1: 
  public final java.lang.Object invokePrim(long);
    Code:
       0: lload_1
       1: invokestatic  #36                 // Method clojure/lang/Numbers.unchecked_inc:(J)J
       4: invokestatic  #40                 // Method clojure/lang/Numbers.num:(J)Ljava/lang/Number;
       7: areturn

I assume your question is why the call to Numbers.num(long) at the end doesn't cause the warning due to the return type? I had those num() calls in my early list of questionables. This function is tricky because it's called from lots of other methods (many of which already trigger the warning), so it has the potential to cause multiple warnings on a single expression. But this does indeed seem like a common and important case to suggest a return type hint.

Any of these calls that take prims but return a Number or Object require a judgement call and an explicit annotation - there is certainly room for interpretation on some of them.

Adding the return type hint cleans things up pretty well:

public final long invokePrim(long);
    Code:
       0: lload_1
       1: lconst_1
       2: ladd
       3: lreturn
Comment by Alex Miller [ 11/Nov/14 9:06 PM ]

I created CLJ-1585 for this.





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

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

Type: Enhancement Priority: Critical
Reporter: Aaron Cohen Assignee: Unassigned
Resolution: Completed Votes: 9
Labels: aot, compiler, interop

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

 Description   

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

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

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

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

Screened by: Alex Miller - I have tested many open source Clojure projects with this change (particularly seeking out large, complicated, or known users of genclass/deftype/etc) and have found no projects adversely impacted. I know that Cursive has been running with this modification for a long time with no known issues. I am ok with unconditionally enabling this change (re the comment below). The impact is described in more detail in the suggested changelog diff in the comments below.

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

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



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

From original post:

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

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

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

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

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

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

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





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

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

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

Attachments: File better-error-messages-for-require.diff     Text File clj-1297-v3.patch     Text File clj-1297-v5.patch    
Patch: Code
Approval: Ok

 Description   

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

Exception in thread "main" java.io.FileNotFoundException: Could not locate a/b_c__init.class or a/b_c.clj on classpath:
...

Proposed:
Fix the bad ending colon in this sentence and add a second sentence only when the file name has an _ in it: "Please check that namespaces with dashes use underscores in the Clojure file name."

Patch: clj-1297-v5.patch

Screened by: Alex Miller



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

I would have saved a few hours as well.

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

This patch changes clojure.core/load such that:

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

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

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

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

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

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

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

Comment by Rich Hickey [ 29/Aug/14 9:48 AM ]

I see no reason to fish around in the file system at all. Why can't the message simply remind people that underscores are required and to check that they aren't using dashes?

Comment by Gary Fredericks [ 30/Sep/14 4:43 PM ]

The tradeoff is that fishing around in the file system means the error message is only shown if the user likely made the relevant mistake, whereas simply showing the error message would (if I understand correctly) mean this reminder gets shown for every require error, which I would estimate happens a whole lot more often than the mistake in question.

Comment by Andy Fingerhut [ 30/Sep/14 5:05 PM ]

Gary, there is an exception thrown in any case if the load fails. One approach that I am hacking up now is to add to the existing exception's message that maybe they need to replace - chars in file name with _, but only if the name attempted to be loaded has at least one '-' character in it. So no new output, except in an exception already being thrown, and then only if there is at least a possibility that this is the problem.

Comment by Andy Fingerhut [ 30/Sep/14 5:10 PM ]

clj-1297-v2.patch is similar to the previously proposed patch, but does not touch the file system in any way that wasn't already being done before this patch.

It adds an extra hint to the message of the exception already being thrown if the resource is not found, but only if there is a '-' character in the name that failed to load.

Comment by Andy Fingerhut [ 30/Sep/14 6:45 PM ]

clj-1297-v3.patch is nearly identical to clj-1297-v2.patch, described in the previous comment, except it eliminates an unnecessary let expression.

Comment by Alex Miller [ 01/Oct/14 3:05 PM ]

With latest:

user=> (require 'a-b)
FileNotFoundException Could not locate a_b__init.class or a_b.clj on classpath:   Perhaps a file you are attempting to load needs - chars in name replaced with _  clojure.core/load-one/fn--5135 (core.clj:5606)

That looks goofy due to the base message in RT.load(): throw new FileNotFoundException(String.format("Could not locate %s or %s on classpath: ", classfile, cljfile));

I would like to:
1) Fix that RT.load() message to not end with a colon: "Could not locate %s or %s on classpath."
2) Instead of changing load-one, just add the additional sentence in RT.load(). Second optional sentence could be: "Please check that namespaces with dashes use underscores in the Clojure file name."

Final message would then look like:

"Could not locate a_b__init.class or a_b.clj on classpath. Please check that namespaces with dashes use underscores in the Clojure file name."

Comment by Andy Fingerhut [ 02/Oct/14 12:06 AM ]

Patch clj-1297-v4.patch modifies only RT.load, including an extra message only if the file name in the argument contains a '_' character in it, with the message suggested by Alex Miller in his last comment before this.

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

The bad end colon is not fixed and the optional message is not included in the format string.

Comment by Andy Fingerhut [ 02/Oct/14 11:04 AM ]

clj-1297-v5.patch should be the one. I can only attempt to blame the previous bone-headed failure on lack of sleep, but even then ...





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

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

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

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

 Description   

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

Patch: CLJ-1274.patch
Screened by: Stu



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

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

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

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

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

Patch is straightforward, marking screened.

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





[CLJ-1261] Invalid defrecord results in exception attributed to namespace that imports namespace with defrecord Created: 12/Sep/13  Updated: 29/Aug/14  Resolved: 29/Aug/14

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

Type: Defect Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: defrecord, errormsgs

Attachments: File clj-1261-2.diff     File clj-1261-3.diff     File clj-1261-4.diff     File clj-1261-5.diff    
Patch: Code and Test
Approval: Ok

 Description   

I was introducing a namespace that included a defrecord.

My defrecord was wrong; it used a keyword to define a field, not a symbol. Minimal test case:

% cat src/useclj16/init.clj
(ns useclj16.init)

(defrecord Application [:shutdown-fn])
% cat src/useclj16/app.clj 
(ns useclj16.app
  (:require [useclj16.init :as init]))

However, the exception was perplexing:

% java -cp clojure-1.6.0-master-SNAPSHOT.jar:src clojure.main
user=> (require 'useclj16.app)
ClassCastException clojure.lang.Keyword cannot be cast to clojure.lang.IObj  clojure.core/with-meta (core.clj:214)

user=> (pst *e 100)
ClassCastException clojure.lang.Keyword cannot be cast to clojure.lang.IObj
        clojure.core/with-meta (core.clj:214)
        clojure.core/defrecord/fn--147 (core_deftype.clj:362)
        clojure.core/map/fn--4210 (core.clj:2494)
        clojure.lang.LazySeq.sval (LazySeq.java:42)
        clojure.lang.LazySeq.seq (LazySeq.java:60)
        clojure.lang.RT.seq (RT.java:484)
        clojure.lang.LazilyPersistentVector.create (LazilyPersistentVector.java:31)
        clojure.core/vec (core.clj:354)
        clojure.core/defrecord (core_deftype.clj:362)
        clojure.lang.Var.invoke (Var.java:427)
        clojure.lang.Var.applyTo (Var.java:532)
        clojure.lang.Compiler.macroexpand1 (Compiler.java:6483)
        clojure.lang.Compiler.macroexpand (Compiler.java:6544)
        clojure.lang.Compiler.eval (Compiler.java:6618)
        clojure.lang.Compiler.load (Compiler.java:7079)
        clojure.lang.RT.loadResourceScript (RT.java:370)
        clojure.lang.RT.loadResourceScript (RT.java:361)
        clojure.lang.RT.load (RT.java:440)
        clojure.lang.RT.load (RT.java:411)
        clojure.core/load/fn--5024 (core.clj:5546)
        clojure.core/load (core.clj:5545)
        clojure.core/load-one (core.clj:5352)
        clojure.core/load-lib/fn--4973 (core.clj:5391)
        clojure.core/load-lib (core.clj:5390)
        clojure.core/apply (core.clj:619)
        clojure.core/load-libs (core.clj:5429)
        clojure.core/apply (core.clj:619)
        clojure.core/require (core.clj:5512)
        useclj16.app/eval322/loading--4916--auto----323 (app.clj:1)
        useclj16.app/eval322 (app.clj:1)
        clojure.lang.Compiler.eval (Compiler.java:6634)
        clojure.lang.Compiler.eval (Compiler.java:6623)
        clojure.lang.Compiler.load (Compiler.java:7079)
        clojure.lang.RT.loadResourceScript (RT.java:370)
        clojure.lang.RT.loadResourceScript (RT.java:361)
        clojure.lang.RT.load (RT.java:440)
        clojure.lang.RT.load (RT.java:411)
        clojure.core/load/fn--5024 (core.clj:5546)
        clojure.core/load (core.clj:5545)
        clojure.core/load-one (core.clj:5352)
        clojure.core/load-lib/fn--4973 (core.clj:5391)
        clojure.core/load-lib (core.clj:5390)
        clojure.core/apply (core.clj:619)
        clojure.core/load-libs (core.clj:5429)
        clojure.core/apply (core.clj:619)
        clojure.core/require (core.clj:5512)
        user/eval318 (NO_SOURCE_FILE:1)
        clojure.lang.Compiler.eval (Compiler.java:6634)
        clojure.lang.Compiler.eval (Compiler.java:6597)
        clojure.core/eval (core.clj:2864)
        clojure.main/repl/read-eval-print--6594/fn--6597 (main.clj:260)
        clojure.main/repl/read-eval-print--6594 (main.clj:260)
        clojure.main/repl/fn--6603 (main.clj:278)
        clojure.main/repl (main.clj:278)
        clojure.main/repl-opt (main.clj:344)
        clojure.main/main (main.clj:442)
        clojure.lang.Var.invoke (Var.java:411)
        clojure.lang.Var.applyTo (Var.java:532)
        clojure.main.main (main.java:37)
nil

The error was attributed to app.clj (useclj16.app), a namespace which requires useclj16.init, the namespace containing the defrecord.

No indication that this concerned a defrecord, or even what namespace contained the error, was present in the exception.

Patch: clj-1261-5.diff

Approach: Check explicitly that the fields are all symbols, for both defrecord and deftype, and throw a CompilerException with file, line, and column number if not. Example of exception after patch is applied, in the case give above:

user=> (require 'useclj16.app)
CompilerException java.lang.AssertionError: defrecord and deftype fields must be symbols, useclj16.init.Application had: :shutdown-fn, compiling:(useclj16/init.clj:3:1)

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 12/Sep/13 8:58 PM ]

Can you include an example of the defrecord definition just so we're clear what it looks like?

Comment by Alex Miller [ 12/Sep/13 8:59 PM ]

Also, "feedback" is not a useful label. Please use "errormsgs" for stuff like this. See the list of many commonly used labels here: http://dev.clojure.org/display/community/Creating+Tickets

Comment by Howard Lewis Ship [ 13/Sep/13 10:42 AM ]

"Feedback" is my own personal crusade http://tapestryjava.blogspot.com/2013/05/once-more-feedback-please.html

In my case, my invalid code was:

(defrecord Application [:shutdown-fn])

And the mistake was that :shutdown-fn should be a symbol, not a keyword.

Here it is, more completely:

(ns novate.services.initialization
  "Infrastructure for system-as-transient state.")

(defrecord Application [:shutdown-fn])

and

(ns novate.services.activator
  "Responsible for bootstrapping the application by loading certain namespaces and invoking certain functions, guided by data in JAR manifests."
  (:gen-class)
  (:require [clojure.edn :as edn]
            [clojure.java.io :as io]
            [novate.util.logging :as l]
            [novate.services
             [initialization :as init]
             [ordering :as o]])
  (:import [java.io PushbackReader]))

The error was attributed to this file.

Comment by Andy Fingerhut [ 13/Sep/13 11:48 AM ]

Patch clj-1261-v1.txt throws an exception if any fields given to defrecord or deftype are not symbols. They are CompilerExceptions, so include an accurate file, line, and column number.

Comment by Andy Fingerhut [ 13/Sep/13 11:57 AM ]

Updated description to give minimal test case.

Comment by Andy Fingerhut [ 23/Nov/13 1:06 AM ]

Patch clj-1261-2.diff is identical to clj-1261-v1.txt except that it applies cleanly to latest master. The only change was to some lines of context due to recent commits to Clojure.

Comment by Alex Miller [ 18/Apr/14 1:16 PM ]

I think the patch is ok but I have two suggestions in the error message - first, include the record/type ns+name (I think the classname in the patched fn is what you want). Second, I think the wording could be adjusted a bit and the parens should go away - those look like but don't actually have meaning in the original context (since you are filtering out the symbols). Maybe something like:

"defrecord and deftype fields must be symbols, useclj16.init.Application had: :shutdown-fn, :foo-bar"

Comment by Andy Fingerhut [ 30/Apr/14 9:32 AM ]

Patch clj-1261-3.diff attempts to incorporate Alex's suggested error message changes.

There are other errors caught by function validate-fields that could have more details like the namespace and record/type name added to them, but I don't want to go out of scope for the ticket. I can create another patch that does that if there is interest.

Comment by Alex Miller [ 30/Apr/14 2:56 PM ]

Can you update the "after" example in the Approach section of the description to match new?

Comment by Andy Fingerhut [ 01/May/14 4:18 PM ]

Updated example output at end of description to be what is seen after patch clj-1261-3.diff is applied.

Comment by Alex Miller [ 05/May/14 1:56 PM ]

Description looks good, patch looks good. Test?

Comment by Andy Fingerhut [ 05/May/14 2:03 PM ]

I'd be happy to write one, if I had a "similar" one to pattern them on.

By similar, I mean: are there any existing tests that require a namespace that isn't already loaded & compiled when the tests begin running, and catch exceptions thrown during the require?

Comment by Alex Miller [ 05/May/14 3:56 PM ]

I think you should be able to test the right error message here by just invoking the defrecord form.

Otherwise, maybe https://github.com/clojure/clojure/blob/master/test/clojure/test_clojure/ns_libs.clj#L87 ?

Comment by Andy Fingerhut [ 10/May/14 6:43 PM ]

Patch clj-1261-4.diff is identical to clj-1261-3.diff except that it adds a couple of unit tests verifying that an exception of the desired type and with an appropriate message is thrown when keywords are used as defrecord or deftype fields.

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

same as -4 but changed final defrecord to deftype in test (seemed like a typo)

Comment by Andy Fingerhut [ 13/May/14 11:40 PM ]

Thanks for the catch on that typo in the tests. You changed it to what I had intended.

Comment by Alex Miller [ 13/May/14 11:54 PM ]

seemed pretty clear





[CLJ-1251] The update function: like update-in, for first level Created: 03/Sep/13  Updated: 29/Aug/14  Resolved: 29/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Michael O. Church Assignee: Ambrose Bonnaire-Sergeant
Resolution: Completed Votes: 3
Labels: None

Attachments: Text File CLJ-1251.patch     Text File update.patch    
Patch: Code and Test
Approval: Ok

 Description   

update-in is useful for updating nested structures. Very often we just want to update one level, so an update function optimised for this use case is useful.

It operates identically to update-in with a key path of length one so these are the same:

(update-in m [k] f args...)
(update m k f args...)

Patch: CLJ-1251.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 06/Sep/13 9:56 AM ]

I like this - kind of halfway between assoc and update-in.

Comment by Michael O. Church [ 07/Sep/13 12:41 PM ]

It's very useful. I assumed that its non-inclusion was for a reason (hence was hesitant to submit the patch) but it comes in handy a lot. One project I'd like to do with some free time is a library for turn-based strategy games, which use update frequently to express game-state changes.

The downside of this change is that 'update is probably a defined function in a good number of modules written by other people. IMO the strongest reason not to include it is that it's such a common name; but the benefits (in my view) outweigh the downsides.

Comment by Andy Fingerhut [ 14/Feb/14 11:50 AM ]

Patch update.patch dated Sep 3 2013 no longer applies cleanly to latest Clojure master as of Feb 14 2014. It did on Feb 7 2014. I haven't checked in detail, but this is probably simply due to some tests recently added to a test file that require updating some diff context lines.

Comment by Ambrose Bonnaire-Sergeant [ 06/May/14 2:36 PM ]

The vararg validation should be done in the same way as `assoc`.

Comment by Alan Malloy [ 06/May/14 2:41 PM ]

The most obvious reason, to me, that clojure.core/update doesn't exist already is that it's not clear what it should do when given more than 3 arguments. Consider, for example, (update m a b c d). What does this do? There are at least three reasonable interpretations: (update-in m [a] b c d), passing c and d as extra args to the function b; (-> m (update-in [a] b) (update-in [c] d)), treating the args as alternating key/function pairs; (reduce (fn [m k] (update-in m [k] a)) m [b c d]), treating a as a function to apply to each of b, c, and d.

Any of these are plausible meanings for the vague name "update", and there's no obvious behavior to choose, whereas there's only one reasonable way for assoc and assoc-in to behave. If one of them were chosen, it would be a little bit nontrivial to read code using it, at least until it became so well-known that everyone thinks it's obvious. I don't have anything against this function that Michael Church has written, or including it in core, but I don't like naming it update, as if it were the only possible dual to update-in.

Comment by Kyle Kingsbury [ 06/May/14 4:09 PM ]

I'd like to second Alan Malloy's concern; I've defined (update m k f arg1 arg2) in most of my Clojure work to be "change the value for this key to be (f current-value arg1 arg2 ...)"; this is consistent with swap!, update-in, etc., and is in my experience the most common need for update. It also composes well with swap! and other higher-order friends. I suggest we use that variant instead, and rely on assoc or -> threading when updating multiple fields.

Comment by Michael O. Church [ 07/May/14 10:32 AM ]

I agree with Kyle and Alan. There are several interpretations of how update should behave and while it's not clear which one is "correct", Kyle's is most consistent with the rest of the language and therefore probably more right than the one I started with.

The issue I see with including an "update" function is that it will break code for others who've defined it for themselves. Kyle's interpretation is more consistent with the rest of Clojure and will probably involve the least breakage. I'd be happy using his version, and renaming mine to something else.

Comment by Rich Hickey [ 13/May/14 6:09 AM ]

I am in favor, and it should work like everything else: (update m k f args...)

Comment by Ambrose Bonnaire-Sergeant [ 13/May/14 7:18 AM ]

I'm working on a new patch.

Comment by Ambrose Bonnaire-Sergeant [ 13/May/14 7:39 AM ]

update-like-update-in.patch is the new patch as Rich requests.

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

Ambrose, I think the example in the description no longer follows the (update m k f args...) form right? Can you update?

Comment by Ambrose Bonnaire-Sergeant [ 13/May/14 9:46 AM ]

Alex, I'm not sure what you're referencing?

Comment by Ambrose Bonnaire-Sergeant [ 13/May/14 9:47 AM ]

If you mean the docstring, I did try and update it for update by copying update-in and change and plural keys to singular.

Comment by Alex Miller [ 13/May/14 10:18 AM ]

I mean the description for this ticket needs to be updated to reflect what we are currently considering.

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

In the patch, the docstring has "If the key does not exist, a hash-map will be created." which is not applicable in update right? I think it would be more accurate to say that the fn will be invoked on nil.

This line occurs twice in the tests:

{:a [1 2]}   (update {:a [1]} :a conj 2)

There is no test for what happens when the key is absent. For example:

(update {:a 1} :b str)
=> {:b "", :a 1}
Comment by Ambrose Bonnaire-Sergeant [ 13/May/14 1:30 PM ]

I removed the mention of creating hash-maps, and replaced it with the explicit behaviour of passing `nil` for missing keys.

FWIW I proposed a similar wording in the patch for http://dev.clojure.org/jira/browse/CLJ-373

Added a test for missing key. Removed the duplicate test.

Comment by Gary Fredericks [ 16/May/14 8:45 PM ]

Is it worth unrolling several arities for the sake of premature optimization? e.g., https://github.com/Prismatic/plumbing/blob/master/src/plumbing/core.clj#L33-41

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

I think that's probably worth doing - who can update the patch with multiple arities?

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

Ambrose, can you (or anyone else really) update the patch to unroll small arities?

Comment by Ambrose Bonnaire-Sergeant [ 23/May/14 11:40 AM ]

Yes will do now.

Comment by Ambrose Bonnaire-Sergeant [ 23/May/14 12:16 PM ]

Add multiple arities + tests (CLJ-1251.patch)





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

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

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

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

 Description   

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

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

For example:

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

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

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

Patch: 0001-fix-CLJ-1241.patch

Screened by: Alex Miller



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

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

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

Verified on Clojure 1.5.1.

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

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

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

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

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

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

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

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

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

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

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

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

Something else?

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

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

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

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

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

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

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

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

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

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





[CLJ-1191] Improve apropos to show some indication of namespace of symbols found Created: 04/Apr/13  Updated: 29/Aug/14  Resolved: 29/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: repl

Attachments: Text File clj-1191-patch-v1.txt     Text File clj-1191-patch-v2.txt     Text File clj-1191-v3.patch    
Patch: Code and Test
Approval: Ok

 Description   

apropos does find all symbols in all namespaces that match the argument, but the return value gives no clue as to which namespace the found symbols are in. It can even return multiple occurrences of the same symbol, which only gives a clue that the symbol exists in more than one namespace, but not which ones. For example:

user=> (apropos "replace")
(postwalk-replace prewalk-replace replace re-quote-replacement replace replace-first)

user=> (apropos 'macro)
(macroexpand-all macroexpand macroexpand-1 defmacro)

It would be nice if the returned symbols could indicate the namespace.

With the screened patch clj-1191-v3.patch applied the output for the examples above becomes:

user=> (apropos "replace")
(clojure.core/replace clojure.string/re-quote-replacement clojure.string/replace clojure.string/replace-first clojure.walk/postwalk-replace clojure.walk/prewalk-replace)

user=> (apropos 'macro)
(clojure.core/defmacro clojure.core/macroexpand clojure.core/macroexpand-1 clojure.walk/macroexpand-all)

Patch: clj-1191-v3.patch

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 04/Apr/13 8:25 PM ]

Path clj-1191-patch-v1.txt enhances apropos to put a namespace/ qualifier before every symbol found that is not in the current namespace ns. It also finds the shortest namespace alias if there is more than one. Examples of output with patch:

user=> (apropos "replace")
(replace clojure.string/re-quote-replacement clojure.string/replace clojure.string/replace-first clojure.walk/postwalk-replace clojure.walk/prewalk-replace)

user=> (require '[clojure.string :as str])
nil
user=> (apropos "replace")
(replace clojure.walk/postwalk-replace clojure.walk/prewalk-replace str/re-quote-replacement str/replace str/replace-first)

user=> (in-ns 'clojure.string)
#<Namespace clojure.string>
clojure.string=> (clojure.repl/apropos "replace")
(re-quote-replacement replace replace-by replace-first replace-first-by replace-first-char replace-first-str clojure.core/replace clojure.walk/postwalk-replace clojure.walk/prewalk-replace)

Comment by Colin Jones [ 05/Apr/13 1:34 PM ]

+1

apropos as it already stands is quite helpful for already-referred vars, but not for vars that are only in other nses.

This update includes the information someone would need to further investigate the output.

Comment by Alex Miller [ 20/Aug/14 11:22 AM ]

If you have "use"d many namespaces (which is not uncommon at the repl), this updated apropos still doesn't help you understand where a particular function is coming from (as the ns will be omitted). It's cool that this patch is "unresolving" and finding the shortest-alias etc but I think it's actually doing too much. In my opinion, simply providing the full namespace for all matches would actually be more useful (and easier).

Comment by Andy Fingerhut [ 20/Aug/14 12:27 PM ]

Patch clj-1191-patch-v2.txt dated Aug 20 2014 modifies apropos so that every symbol returned has a full namespace qualifier, even if it is in clojure.core. Before this patch:

user=> (apropos "replace")
(prewalk-replace postwalk-replace replace replace-first re-quote-replacement replace)

user=> (apropos 'macro)
(macroexpand-all macroexpand macroexpand-1 defmacro)

After this patch:

user=> (apropos "replace")
(clojure.core/replace clojure.string/re-quote-replacement clojure.string/replace clojure.string/replace-first clojure.walk/postwalk-replace clojure.walk/prewalk-replace)

user=> (apropos 'macro)
(clojure.core/defmacro clojure.core/macroexpand clojure.core/macroexpand-1 clojure.walk/macroexpand-all)
Comment by Alex Miller [ 20/Aug/14 1:34 PM ]

Some comments on the code itself:

1) I don't think we should do anything special for ns - there are plenty of ways to search your current ns. I think it unnecessarily adds a lot of complexity without enough value.
2) Rather than finding vars and work back to syms, I think this should instead retain the ns context as it walks the ns-publics keys so that you can easily reassemble a fully-qualified symbol name.
3) Why do you need the set at the end? Seems like symbols should already be unique at this point?

Comment by Andy Fingerhut [ 20/Aug/14 5:02 PM ]

Patch clj-1191-patch-v3.txt attempts to address Alex Miller's comments on the v2 patch.

Perhaps the diff will get down to a 1-line change

Comment by Andy Fingerhut [ 20/Aug/14 5:05 PM ]

Patch clj-1191-v3.patch is identical to clj-1191-patch-v3.txt mentioned in the previous comment, but conforms to the requested .patch or .diff file name ending.





[CLJ-1185] `reductions should respect `reduced Created: 16/Mar/13  Updated: 29/Aug/14  Resolved: 29/Aug/14

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

Type: Defect Priority: Critical
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: None

Attachments: Text File CLJ-1181-v001.patch     Text File CLJ-1181-v002.patch    
Patch: Code and Test
Approval: Ok

 Description   

This returns 16:

(reduce (fn [acc x]
          (let [x' (* x x)]
            (if (> x' 10)
              (reduced x')
              x')))
        (range))

But replacing reduce with reductions will never terminate:

(reductions (fn [acc x]
              (let [x' (* x x)]
                (if (> x' 10)
                  (reduced x')
                  x')))
            (range))

Cause: reductions ignores clojure.lang.Reduced, it never tests for reduced?

Patch: CLJ-1181-v002.patch

Screened by: Alex Miller



 Comments   
Comment by Brandon Bloom [ 16/Mar/13 6:10 PM ]

Attaching patch

Comment by Satshabad Khalsa [ 13/Apr/14 1:53 AM ]

Would love some progress on this!

Comment by Andy Fingerhut [ 13/Apr/14 11:37 AM ]

It isn't guaranteed to help, but it can't hurt to vote on the ticket, and encourage anyone else you know who wants this fixed to vote on it.

Comment by Alex Miller [ 14/Jun/14 7:38 AM ]

Needs a test

Comment by Brandon Bloom [ 14/Jun/14 4:10 PM ]

New patch includes tests.





[CLJ-1169] Report line,column, and source in defmacro errors Created: 22/Feb/13  Updated: 29/Aug/14  Resolved: 29/Aug/14

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

Type: Enhancement Priority: Major
Reporter: Andrei Kleschinski Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: errormsgs
Environment:

Windows


Attachments: Text File 0001-CLJ-1169-proposed-patch.patch     Text File 0002-CLJ-1169-fix-unit-tests.patch     Text File CLJ-1169-code-and-test-1.patch     File defn_error_message.clj    
Patch: Code
Approval: Ok

 Description   

Summary This patch grew out of a desire to have defn report filename and line numbers for parameter declaration errors, but the approach chosen does something more broad, and likely very useful: Anytime defmacro is throwing a non-CompilerException, wrap it in a CompilerException that captures LINE, COLUMN, and SOURCE. Presumably this would improve reporting for many other macros as well. The patch also tweaks errors messages to add quotes, e.g. "problem" instead of problem, which seems useful.

Screened By Stu
Patch CLJ-1169-code-and-test-1.patch, which aggregates the work in other patches to a single patch that works on current master.

When mistyping parameter list in defn declaration, e.g.

(defn test
 (some-error))

error message shows name of parameter (without quotes), but not function name, filename or line number:

Exception in thread "main" java.lang.IllegalArgumentException: Parameter declaration some-error should be a vector
        at clojure.core$assert_valid_fdecl.invoke(core.clj:6567)
        at clojure.core$sigs.invoke(core.clj:220)
        at clojure.core$defn.doInvoke(core.clj:294)
        at clojure.lang.RestFn.invoke(RestFn.java:467)
        at clojure.lang.Var.invoke(Var.java:427)
        at clojure.lang.AFn.applyToHelper(AFn.java:172)
        at clojure.lang.Var.applyTo(Var.java:532)
        at clojure.lang.Compiler.macroexpand1(Compiler.java:6366)
        at clojure.lang.Compiler.macroexpand(Compiler.java:6427)
        at clojure.lang.Compiler.eval(Compiler.java:6495)
        at clojure.lang.Compiler.load(Compiler.java:6952)
        at clojure.lang.Compiler.loadFile(Compiler.java:6912)
        at clojure.main$load_script.invoke(main.clj:283)
        at clojure.main$init_opt.invoke(main.clj:288)
        at clojure.main$initialize.invoke(main.clj:316)
        at clojure.main$null_opt.invoke(main.clj:349)
        at clojure.main$main.doInvoke(main.clj:427)
        at clojure.lang.RestFn.invoke(RestFn.java:421)
        at clojure.lang.Var.invoke(Var.java:419)
        at clojure.lang.AFn.applyToHelper(AFn.java:163)
        at clojure.lang.Var.applyTo(Var.java:532)
        at clojure.main.main(main.java:37)


 Comments   
Comment by Andrei Kleschinski [ 22/Feb/13 5:39 AM ]

Proposed patch for issue
Process exceptions in macroexpand1 and wraps them in CompilerException with source,line,column information.

Also patch adds quotes around invalid symbol name in error message to make them more distinguishable from rest of message.

Comment by Andy Fingerhut [ 01/Mar/13 9:32 AM ]

Patch 0001-CLJ-1169-proposed-patch.patch dated Feb 22 2013 causes several tests to fail. Run "./antsetup.sh" then "ant" to see which ones (or "mvn package").

Comment by Andrei Kleschinski [ 01/Mar/13 10:25 AM ]

Fix for failed unit-tests

Comment by Stuart Halloway [ 27/Jun/14 2:40 PM ]

Andrei, can you please sign the CA (e-form at http://clojure.org/contributing) so that we can consider this patch?

Thanks!

Comment by Andrei Kleschinski [ 27/Jun/14 3:05 PM ]

Ok, I have signed the CA.

Comment by Alex Miller [ 27/Jun/14 4:06 PM ]

I can confirm that Andrei has signed the CA. Back in Vetted.





[CLJ-1039] Using 'def with metadata {:type :anything} throws ClassCastException during printing Created: 09/Aug/12  Updated: 29/Aug/14  Resolved: 29/Aug/14

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

Type: Defect Priority: Minor
Reporter: Gunnar Völkel Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: print
Environment:

Ubuntu, lein 1.7.1 - lein repl


Attachments: Text File CLJ-1039-tolerate-misleading-type-metadata-on-var-wh.patch    
Patch: Code and Test
Approval: Ok

 Description   

Specific to setting :type meta on a var:

user=> (def ^{:type :anything} mydef 1)
#<main$repl clojure.main$repl@6193b845>
CompilerException java.lang.ClassNotFoundException: main.clj:257, compiling:(NO_SOURCE_PATH:13:20)
ClassCastException clojure.lang.Var cannot be cast to clojure.lang.IObj  clojure.core/with-meta (core.clj:214)
user=> (pst *e)
ClassCastException clojure.lang.Var cannot be cast to clojure.lang.IObj
	clojure.core/with-meta (core.clj:214)
	clojure.core/vary-meta (core.clj:640)
	clojure.core/fn--5420 (core_print.clj:76)
	clojure.lang.MultiFn.invoke (MultiFn.java:232)
	clojure.core/pr-on (core.clj:3392)
	clojure.core/pr (core.clj:3404)
	clojure.core/apply (core.clj:624)
	clojure.core/prn (core.clj:3437)
	clojure.main/repl/read-eval-print--6627 (main.clj:241)
	clojure.main/repl/fn--6636 (main.clj:258)
	clojure.main/repl (main.clj:258)
	clojure.main/repl-opt (main.clj:324)

If it is intended to forbid setting the :type metadata, then there should be an appropriate error message instead of the ClassCastException.

Cause: This is caused by the printer dispatch function

(defmulti print-method (fn [x writer]
                         (let [t (get (meta x) :type)]
                           (if (keyword? t) t (class x)))))

which ends up calling the default dispatch, which tries to vary-meta.

Solution: Add a check in the default print-method for (instance? clojure.lang.IObj o) before calling vary-meta and fallback to print-simple.

Patch: CLJ-1039-tolerate-misleading-type-metadata-on-var-wh.patch

Screened by: Alex Miller



 Comments   
Comment by Stuart Halloway [ 10/Aug/12 1:40 PM ]

This is caused by the printer dispatch function

(defmulti print-method (fn [x writer]
                         (let [t (get (meta x) :type)]
                           (if (keyword? t) t (class x)))))

which ends up calling the default dispatch, which tries to vary-meta.

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

The :type metadata is used internally by Clojure. For the situation in this bug report, you have to take responsibility for providing a print-method if you put :type metatdata on your var.

This is not a good example, but it shows one way to work around the bug:

(defmethod print-method :anything [obj w] (print-method {:anything @obj} w))
Comment by Steve Miner [ 14/Apr/14 10:21 AM ]

On the other hand, the :default print-method probably should be more robust. I think a check for (instance? clojure.lang.IObj o) before calling vary-meta would be appropriate. Added a patch that calls print-simple if the o isn't an IObj. That works around the issue for Var, and seems reasonable for other exotic types. The only downside I can imagine is if someone had a custom print-method but accidentally had a typo in their :type metadata, they will no longer get an error. This was an edge case to begin with so that probably doesn't matter.





[CLJ-979] Clojure resolves to wrong deftype classes when AOT compiling or reloading Created: 03/May/12  Updated: 18/Feb/15  Resolved: 10/Jan/15

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

Type: Defect Priority: Critical
Reporter: Edmund Jackson Assignee: Unassigned
Resolution: Completed Votes: 14
Labels: aot, classloader, compiler

Attachments: Text File CLJ-979.patch     Text File clj-979-symptoms.patch     Text File CLJ-979-v2.patch     Text File CLJ-979-v3.patch     Text File CLJ-979-v4.patch     Text File CLJ-979-v5.patch     Text File CLJ-979-v6.patch     Text File CLJ-979-v7.patch    
Patch: Code and Test
Approval: Ok

 Description   

Compiling a class via `deftype` during AOT compilation gives different results for the different constructors. These hashes should be identical.

user=> (binding [*compile-files* true] (eval '(deftype Abc [])))
user.Abc
user=> (hash Abc)
16446700
user=> (hash (class (->Abc)))
31966239 ;; should be 16446700

This also means that whenever there's a stale AOT compiled deftype class in the classpath, that class will be used rather then the JIT compiled one, breaking repl interaction.

Another demonstration of this classloader issue (from CLJ-1495) when reloading deftypes (no AOT) :

user> (defrecord Foo [bar])
user.Foo
user> (= (->Foo 42) #user.Foo{:bar 42}) ;;expect this to evaluate to true
true
user> (defrecord Foo [bar])
user.Foo
user> (= (->Foo 42) #user.Foo{:bar 42}) ;;expect this to evaluate to true also -- but it doesn't!
false
user>

This bug also affects AOT compilation of multimethods that dispatch on a class, this affected core.match for years see http://dev.clojure.org/jira/browse/MATCH-86, http://dev.clojure.org/jira/browse/MATCH-98. David had to work-around this issue by using a bunch of protocols instead of multimethods.

Cause of the bug: currently clojure uses Class.forName to resolve a class from a class name, which ignores the class cache from DynamicClassLoader thus reloading deftypes or mixing AOT compilation at the repl with deftypes breaks, resolving to the wrong class.

Approach: the current patch (CLJ-979-v7.patch) addresses this issue in multiple ways:

  • it makes RT.classForName/classForNameNonLoading look in the class cache before delegating to Class/forName if the current classloader is not a DynamicClassLoader (this incidentally addresses also CLJ-1457)
  • it makes clojure use RT.classForName/classForNameNonLoading instead of Class/forName
  • it overrides Classloader/loadClass so that it's class cache aware – this method is used by the jvm to load classes
  • it changes gen-interface to always emit an in-memory interface along with the [optional] on-disk interface so that the in-memory class is always updated.

Patch: CLJ-979-v7.patch

Screened by: Alex Miller



 Comments   
Comment by Scott Lowe [ 12/May/12 9:05 PM ]

I can't reproduce this under Clojure 1.3 or 1.4, and Leiningen 1.7.1 on either Java 1.7.0-jdk7u4-b21 OpenJDK 64-Bit or Java 1.6.0_31 Java HotSpot 64-Bit. OS is Mac OS X 10.7.

Edmund, how are you running this AOT code? I wrapped your code in a main function and built an uberjar from it.

Comment by Edmund Jackson [ 13/May/12 2:20 AM ]

Hi Scott,

Interesting.

I have two use cases
1. AOT compile and call from repl.
My steps: git clone, lein compile, lein repl, (use 'aots.death), (in-ns 'aots.death), (= (class (Dontwork. nil)) (class (map->Dontwork {:a 1}))) => false

2. My original use case, which I've minimised here, is an AOT ns, producing a genclass that is called instantiated from other Java (no main). This produces the same error. I will produce an example of this and post it too.

Comment by Edmund Jackson [ 13/May/12 4:23 AM ]

Hi Scott,

Here is an example of it failing in the interop case: https://github.com/ejackson/aotquestion2
The steps I'm following to compile this all up are

git clone git@github.com:ejackson/aotquestion2.git
cd aotquestion2/cljside/
lein uberjar
lein install
cd ../javaside/
mvn package
java -jar ./target/aotquestion-1.0-SNAPSHOT.jar

and it dies with this:

Exception in thread "main" java.lang.ClassCastException: cljside.core.Dontwork cannot be cast to cljside.core.Dontwork
at cljside.MyClass.makeDontwork(Unknown Source)
at aotquestion.App.main(App.java:8)

The error message is really confusing (to me, anyway), but I think its the same root problem as for the REPL case.

What do you see when you run the above ?

Comment by Scott Lowe [ 13/May/12 8:41 AM ]

Ah, yes, looks like my initial attempt to reproduce was too simplistic. I used your second git repo, and can now confirm that it's failing for me with the same error.

Comment by Scott Lowe [ 13/May/12 10:35 PM ]

I looked into this a little further and the AOT generated code looks correct, in the sense that both code paths appear to be returning the same type.

However, I wonder if this is really a ClassLoader issue, whereby two definitions of the same class are being loaded at different times, because that would cause the x.y.Class cannot be cast to x.y.Class exception that we're seeing here.

Comment by Steve Miner [ 03/Sep/13 9:54 AM ]

This could be related to CLJ-1157 which deals with a ClassLoader issue with AOT compiled code.

Comment by Ambrose Bonnaire-Sergeant [ 29/Mar/14 1:11 PM ]

I've tried this patch attached to CLJ-1157 and it did not solve this issue.

Comment by Ambrose Bonnaire-Sergeant [ 29/Mar/14 2:27 PM ]

This bug seems to be rooted in different behaviour for do/let under compilation. Attached a patch showing these symptoms in the hope it helps people find the cause.

Comment by Peter Taoussanis [ 22/Sep/14 3:12 AM ]

Just a quick note to confirm that this still seems to be around as of Clojure 1.7.0-alpha2. Don't have any useful input on possible solutions, sorry.

Comment by Alex Miller [ 04/Dec/14 1:12 PM ]

Duplicates - CLJ-1495, CLJ-1132

Comment by Nicola Mometto [ 04/Dec/14 1:50 PM ]

The attached patch fixes the classloader issues by routing RT.classForName & variants through the DynamicClassLoader class cache before invoking Class.forName

Comment by Nicola Mometto [ 04/Dec/14 1:59 PM ]

Re-adding triaged status added by Alex Miller that got accidentaly nuked by a race-condition between my edits to the ticket description and Alex's ones

Comment by Nicola Mometto [ 04/Dec/14 2:30 PM ]

0001-CLJ-979-make-clojure-resolve-to-the-correct-Class-in-v2.patch is the same as 0001-CLJ-979-make-clojure-resolve-to-the-correct-Class-in.patch except it unconditionally looks for classes in the class cache of DynamicClassLoader, even if baseLoader() is not a DynamicClassLoader.
This fixes the bug of CLJ-1457 but might just be a workaround

Comment by Michael Blume [ 11/Dec/14 3:29 PM ]

Current patch blows up my Clojure build

https://gist.github.com/MichaelBlume/aa26fc715cbbdf711290

Comment by Nicola Mometto [ 11/Dec/14 3:45 PM ]

Michael: the current patch builds clojure fine for me, I'll try to reproduce. Which jvm version are you using?

Comment by Michael Blume [ 11/Dec/14 4:26 PM ]

[14:24][michael.blume@tcc-michael-4:~/workspace/clojure((0fc43db...))]$ java -version
java version "1.8.0_25"
Java(TM) SE Runtime Environment (build 1.8.0_25-b17)
Java HotSpot(TM) 64-Bit Server VM (build 25.25-b02, mixed mode)
[14:24][michael.blume@tcc-michael-4:~/workspace/clojure((0fc43db...))]$ mvn -version
Apache Maven 3.2.3 (33f8c3e1027c3ddde99d3cdebad2656a31e8fdf4; 2014-08-11T13:58:10-07:00)
Maven home: /usr/local/Cellar/maven/3.2.3/libexec
Java version: 1.8.0_25, vendor: Oracle Corporation
Java home: /Library/Java/JavaVirtualMachines/jdk1.8.0_25.jdk/Contents/Home/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "10.10.1", arch: "x86_64", family: "mac"

build was after I applied the patch to the current master branch of the clojure github repo

Comment by Andy Fingerhut [ 11/Dec/14 5:34 PM ]

I am seeing a similar compilation error as Michael Blume, with both JDK 1.7 and 1.8 on Mac OS X 10.9.5.

By accident I found that if I take latest Clojure master and do 'mvn package', then apply the patch CLJ-979.patch dated Dec 11 2014, then do 'mvn package' again without 'mvn clean', it compiles with no errors. If I do 'mvn clean' then 'mvn package' in a patched tree, I get the error every time I've tried.

Comment by Nicola Mometto [ 12/Dec/14 5:50 AM ]

The updated patch fixes the LinkageError Andy and Michael were getting.

Andy, Michael, can you confirm?

Comment by Nicola Mometto [ 12/Dec/14 9:38 AM ]

Added more testcases to new patch

Comment by Nicola Mometto [ 12/Dec/14 10:09 AM ]

Cleaned up the patch from whitespace changes

Comment by Andy Fingerhut [ 12/Dec/14 12:32 PM ]

I tried latest Clojure master plus patch CLJ-979-v4.patch, dated 12 Dec 2014, with Mac OS X 10.9.5 + JDK7, and Ubuntu Linux 14.04 with JDKs 6 through 9, and 'mvn clean' followed by 'mvn package' built and passed tests successfully with all of them.

I did notice that some files were created in the test directory that were not cleaned up by the end of the test, which you can use 'git status .' to see. Not sure if that is considered a bad thing for Clojure tests.

Comment by Nicola Mometto [ 12/Dec/14 1:07 PM ]

Thanks Andy, I've updated the patch and it now should remove all temporary classes created by the test.
It's probably not the best way to do it but I couldn't figure out how to do it another way.

Comment by Michael Blume [ 12/Dec/14 2:34 PM ]

Yep, looks good to me =)

Comment by Alex Miller [ 15/Dec/14 4:01 PM ]

Thanks first to Nicola for all his work so far on this!

Some feedback:
1) While the ticket itself isn't bad, I would really like to focus the title and description on a crisp statement of the real problem now that we understand it more. I'd like help on making sure we capture that correctly - how is this for a title: "Uses of Class.forName() miss classes in DynamicClassLoader cache?" ?

Similarly, the description should focus on the problem and show some examples. The defrecord one is good. The first example works for me before the patch and fails after?

2) The crux of this whole thing is the change in loading order in DCL.loadClass() - changing this is a big deal. We really need broader testing with things likely to be affected - off the top of my head: Immutant, Pomegranate, Leiningen, or anything else that monkeys with classloader stuff. Maybe something with Eclipse/OSGi if there is something testable out there.

3) DynamicClassLoader comments:
a) loadClass(String name) - I believe this is identical to the super impl, so can be removed.
b) findClass(String name) - now that we are hijacking loadClass(), I'm not sure it's even necessary to implement this or to call super.findClass() - if you get to the super.findClass(), I would expect that to always throw CNFE. Potentially, this method could even be removed (but this might do bad things if there are subclasses of DCL out there in the wild).
c) loadClass(String name, ...) - instead of calling findClass() and using the CNFE for control flow, you could just directly call findInMemoryClass(), then use a returned null as the decision factor. Again, this is possibly bad if there are DCL subclasses, so I'm on the fence about it.

4) Is the change in gen-interface something that should be a separate ticket? Seems like it could be separable.

5) I don't like the test changes with respect to set up and cleanup. The build already supports compiling a subset of test ns'es (like clojure.test-clojure.genclass.examples). I'd prefer to use that existing mechanism if at all possible. Check the build.xml for the hard-coded ns list.

6) What are the performance implications? I'm not expecting they are significant but we just made a bunch of changes for compilation performance and I'd hate to undo them all. Could findInMemoryClass be smarter about avoiding checks that can't succeed (avoiding "java.*" for example?).

Comment by Nicola Mometto [ 15/Dec/14 5:43 PM ]

1) It's not really about Class.forName() specifically, it's about DynamicClassLoader not being class cache aware in the loadClass method. The JVM uses the classloader loadClass method for resolving all kind of class usages,
including but not limited to Class.forName() (i.e. when loading some bytecode containing a "new" instruction, that class reference will be resolved via a call to loadClass)
I'll try to make the documentation a bit more clear, the first example is an exhibit of the bugged behaviour, the two calls should output the same hash.

2,4) So, there are 3 approaches to how DynamicClassLoader could go at it:

  • Prefer in-disk classes over in-memory classes, roughly the current approach (sometimes it will pick the in-memory class over the in-disk one causing weird bugs like Foo. and ->Foo constructing different classes), has the
    negative effect of breaking interaction between AOT compilation and JIT loading, which has created all sorts of troubles with redefinig deftypes/defprotocols in repls while having stale classfiles in disk.
  • Always pick the most-updated class, this has the advangate of being always correct but has several disadvantages that make it inpracticable in my opinion: we'd have to keep track of the timestamp in which a dynamic class
    is defined, and make the loadClass implementation such that if there a class is both in-memory and in-disk, it compares the timestamps and select the most updated one. This would complicate the implementation a lot and we'd
    likely have to pay a substantial performance hit.
  • Prefer in-memory classes over in-disk classes, the approach proposed in the current patches. It has the advantage of being almost always correct, make repl interaction & jit/aot mixing work fine and the implementation is
    mostly straightforward. The downside is that in cases like gen-class where an AOT class can actually be the most updated version, the in-memory version will be used. In clojure all the forms that do bytecode emission either
    only do AOT compilation or do AOT compilation on demand and always load the class in memory, except gen-interface that doesn't load the class in memory if it's being AOT compiled. Changing its semantics to behave like the
    other jit/aot compiling forms (deftype/defrecord/reify) is the only way to make this approach work so I don't think this should go in another ticket.

5) I don't like the previous testing strategy either but couldn't figure out a better way. Thanks for the pointer on the already in-place infrastructure, I'll check it out and update the patch

In the meantime I've uploaded a new patch addressing 3 and 6. Specifically:
3) I removed the unnecessary loadClass(String) arity, I've made loadClass(String, boolean) use findInMemoryClass(String) directly rather than relying on findClass(String) since nowhere in the documentation it guarantess that
findClass will be used by loadClass. However I've left the findClass(String) implementation in place in case there's code out there that relies on this.
6) I haven't done any serious testing but I haven't noticed any significant difference in compile times for any of my tools.* contrib libraries with the current patch. Filtering "java.*" class names before the inMemory check
didn't seem to produce any difference so it's not included in the updated patch. However I'll probably include an alternative patch with that filtering to do more performance testings and see if it can actually help a bit.

All this said, I'm afraid that I won't have time to personally do an in-depth benchmarking & cross project testing of this patch. I've been spending almost all the free time I had in the past weeks working through a bunch of tickets (mostly this one) but now because of school and other commitments I can't promise I will be able to do anything more than maintaining the current patch & answering to any questions about the bug. Any help in moving this ticket further would be appreciated, in particular to address points 2 and 6.

Comment by Alex Miller [ 16/Dec/14 8:33 AM ]

Thanks Nicola. I'll certainly take over sheparding the bug and appeal to the greater community for help in broad testing when I think we're ready for that.

Comment by Nicola Mometto [ 16/Dec/14 12:50 PM ]

Updated patch with better tests, addressing Alex Miller's comments.

Comment by Michael Blume [ 06/Jan/15 4:20 PM ]

New in-the-wild case: https://groups.google.com/forum/#!topic/clojure/WH6lJwH1vMg

Comment by Colin Fleming [ 17/Feb/15 10:30 PM ]

I believe I have a case which is now broken because of this patch. I'm not 100% clear on the details so I might be wrong, but I can't see any other explanation for what I'm seeing. The bug I'm looking at is Cursive #748. Cursive calls into Leiningen in-process, and before doing that I create a new DynamicClassLoader which uses an IntelliJ PluginClassLoader as its parent. I believe that what is happening is that PluginClassLoader.loadClass() delegates to various parent classloaders then throws CNFE if it can't find the class in question. Am I correct in thinking that after this patch DCL now delegates to the parent classloader before checking its URL list, whereas previously it did not?

Comment by Nicola Mometto [ 18/Feb/15 4:21 AM ]

Colin, I opened CLJ-1663 with a patch that should fix the issue. Can try it and comment on that ticket if it does?





[CLJ-887] Error when calling primitive functions with destructuring in the arg vector Created: 29/Nov/11  Updated: 29/Aug/14  Resolved: 29/Aug/14

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

Type: Defect Priority: Major
Reporter: Alexander Taggart Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: compiler

Attachments: Text File 0001-don-t-remove-meta-from-arg-vector-in-maybe-destructu.patch    
Patch: Code
Approval: Ok

 Description   

If one defines a primitive-taking function with destructuring, calling that function will result in a ClassCastException, IFF the primitive return-type hint is present.

Clojure 1.4.0-master-SNAPSHOT
user=> (defn foo [[a b] ^long x ^long y] 0)
#'user/foo
user=> (foo [1 2] 3 4)
0
user=> (defn foo ^long [[a b] ^long x ^long y] 0)
#'user/foo
user=> (foo [1 2] 3 4)
ClassCastException user$foo cannot be cast to clojure.lang.IFn$OLLL  user/eval9 (NO_SOURCE_FILE:4)
user=> (pst)
ClassCastException user$foo cannot be cast to clojure.lang.IFn$OLLL
	user/eval9 (NO_SOURCE_FILE:4)
	clojure.lang.Compiler.eval (Compiler.java:6493)
	clojure.lang.Compiler.eval (Compiler.java:6459)
	clojure.core/eval (core.clj:2796)
	clojure.main/repl/read-eval-print--5967 (main.clj:244)
	clojure.main/repl/fn--5972 (main.clj:265)
	clojure.main/repl (main.clj:265)
	clojure.main/repl-opt (main.clj:331)
	clojure.main/main (main.clj:427)
	clojure.lang.Var.invoke (Var.java:397)
	clojure.lang.Var.applyTo (Var.java:518)
	clojure.main.main (main.java:37)
nil

Cause: This was happening because maybe-destructured returned the arg vector without the type hint, so the function was getting compiled to a IFn$OLLO rather than a IFn$OLLL but the :arglists vector in the var meta was still tagged, so the compiler thought that foo was a IFn$OLLL.

Approach: This patch addresses this by preserving the original meta on the fn arglist.

Patch: 0001-don-t-remove-meta-from-arg-vector-in-maybe-destructu.patch

Screened by: Alex Miller



 Comments   
Comment by Nicola Mometto [ 03/Apr/14 1:35 PM ]

This was happening because maybe-destructured returned the arg vector without the type hint, so the function was getting compiled to a IFn$OLLO rather than a IFn$OLLL but the :arglists vector in the var meta was still tagged, so the compiler thought that foo was a IFn$OLLL.

This patch addresses this by preserving the original meta on the fn arglist

Comment by Alex Miller [ 03/Apr/14 1:40 PM ]

Weirdly, I saw this happen today in my own code.





[CLJ-823] Piping seque into seque can deadlock Created: 03/Aug/11  Updated: 29/Aug/14  Resolved: 29/Aug/14

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

Type: Defect Priority: Minor
Reporter: Greg Chapman Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None
Environment:

Windows 7; JVM 1.6; Clojure 1.3 beta 1


Attachments: Text File clj-823-v1.patch    
Patch: Code and Test
Approval: Ok

 Description   

I'm not sure if this is a supported scenario, but the following deadlocks in Clojure 1.3:

(let [xs (seque (range 150000))
      ys (seque (filter odd? xs))]
  (apply + ys))

Cause: As I understand it, the problem is that ys' fill takes place on an agent thread, so when it calls xs' drain, the (send-off agt fill) does not immediately trigger xs' fill, but is instead put on the nested list to be performed when ys' agent returns. Unfortunately, ys' fill will eventually block trying to take from xs, and so it never returns and the pending send-offs are never sent.

Approach: Use (release-pending-sends) in seque's drain function to avoid the deadlock when a seque is fed into another seque.

Patch: clj-823-v1.patch

Screened by: Alex Miller



 Comments   
Comment by Peter Monks [ 07/Jan/13 3:43 PM ]

Reproduced on 1.4.0 and 1.5.0-RC1 as well, albeit with this example:

(seque 3 (seque 3 (range 10)))

Comment by Stuart Halloway [ 30/Mar/13 9:16 AM ]

release-pending-sends?

Comment by Andy Fingerhut [ 21/Aug/14 6:13 PM ]

clj-823-v1.patch uses (release-pending-sends) in seque's drain function in an attempt to avoid the deadlock when a seque is fed into another seque, as suggested by Stuart Halloway. It adds Peter Monks's small quick test case demonstrating the deadlock, which fails (i.e. hangs until killed) without the change and passes with it.





[CLJ-803] IAtom interface Created: 27/May/11  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Enhancement Priority: Minor
Reporter: Pepijn de Vos Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: atom

Attachments: Text File 0001-atom-interface.patch     Text File iatom.patch    
Patch: Code
Approval: Ok

 Description   

Atom does not have an interface and is marked final. This patch extracts an IAtom interface and has Atom implement it.

Patch: 0001-atom-interface.patch

Screened by: Alex Miller

More info: Rich said "patch welcome for IAtom". See IRC logs: http://clojure-log.n01se.net/date/2010-12-29.html#10:04c



 Comments   
Comment by Stuart Halloway [ 27/May/11 2:33 PM ]

Please add a patch formatted by "git format-patch" so that attribution is included.

Comment by Pepijn de Vos [ 04/Jun/11 5:56 AM ]

I added the formatted patch a few days ago. Does 'no news is good news' apply here?

And, silly question, will it make it into 1.3? I can't figure out how to tell Jira to show me that.

Comment by Kevin Downey [ 04/Jul/11 9:06 PM ]

I fail to see the need for an IAtom, if you want something atom like for couchdb the interfaces are already there. Maybe I ICompareAndSwap. Atoms and couchdb are different so making them appear the same is a bad idea.

http://en.wikipedia.org/wiki/Fallacies_of_Distributed_Computing

http://clojure.org/state one of the distinctions between agents and actors raised in the section titled "Message Passing and Actors" is local vs. distributed and the same distinction can be made between couchdb (regardless of compare and swap) and atoms

Comment by Aaron Bedra [ 04/Jul/11 9:18 PM ]

This ticket has already been moved into approved backlog. It will be revisited again after the 1.3 release where we will take a closer look at things. For now, this will remain as is.

Comment by Aaron Craelius [ 10/Jul/14 12:15 PM ]

Any chance this patch could get implemented in an upcoming Clojure release. There is still continued interest, see this thread: https://groups.google.com/forum/#!topic/clojure-dev/y5QoMqd44Lc

One suggestion I would make is also removing the final marker from clojure.lang.Atom - I can see use cases where one would want to directly subclass Atom (to capture dependencies in reactive computations for instance).

Comment by Brandon Bloom [ 02/Aug/14 2:14 PM ]

I'd like to see an IAtom interface, but would prefer that `swap` not be part of it. Swapping can, and should, be defined in terms of `compareAndSet`. Seems like IAtom should only have `boolean compareAndSet(object oldval, object newval)` as well as `void reset(object newval)`.

Comment by Brandon Bloom [ 02/Aug/14 2:29 PM ]

Alternative patch that introduces IAtom and converts swap to be static.

Comment by Pepijn de Vos [ 03/Aug/14 2:59 AM ]

At the time I did the initial patch, I had the same idea to remove swap, but Rich said there where cases for having it, so it should stay in according to him.

Comment by Aaron Craelius [ 03/Aug/14 1:51 PM ]

One use case I can think of for overriding swap is if an IAtom is wrapping say a row of data stored in a database. Then comparing something like a version column (or transaction id in the case of datomic) is what should determine whether a swap is retried, not the actual value of the data. In this case then, compareAndSet would actually be a more complex operation than swap and it makes sense to define the two independently.

Comment by Aaron Craelius [ 03/Aug/14 1:56 PM ]

I should also mention my related issue: http://dev.clojure.org/jira/browse/CLJ-1470 which simply allows Atom (and also ARef) to be sub-classed. Both patches could ultimately work together to make the whole Atom/ARef infrastructure easier to extend.

Comment by Alex Miller [ 10/Sep/14 4:47 PM ]

This ticket needs some work before it can be screened:

  • description can lose the the Couch stuff
  • needs some mention of tradeoffs for the various swap alternatives
  • don't know what patch should be considered
Comment by Brandon Bloom [ 10/Sep/14 5:01 PM ]

I removed my patch because Pepijn/Rich is right: Swap should be part of the interface.





[CLJ-738] <= is incorrect when args include Double/NaN Created: 14/Feb/11  Updated: 29/Aug/14  Resolved: 29/Aug/14

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

Type: Defect Priority: Trivial
Reporter: Jason Wolfe Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: math
Environment:

Mac OS X, Java 6


Attachments: File 738.diff     File 738-tests.diff     File clj-738-v2.diff    
Patch: Code and Test
Approval: Ok

 Description   
user=> (<= Double/NaN 1)
false  
user=> (<= (double Double/NaN) 1)
true    ;; should match Double object result

Cause: The problem was that the logic for lte/gte depended on the fact that lte is equivalent to !gt.
However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

Solution: The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

Patch: clj-738-v2.diff

Screened by: Alex Miller



 Comments   
Comment by Jason Wolfe [ 14/Feb/11 7:18 PM ]

The source of the issue seems to be incorrect treatment of boxed NaN:

user> (<= 1000 (Double. Double/NaN))
true
user> (<= 1000 (double Double/NaN))
false

Comment by Alexander Taggart [ 28/Feb/11 11:14 PM ]

Primitive comparisons use java's primitive operators directly, which always return false for NaN, even when testing equality between two NaNs.

In clojure, Number comparisons are all logical variations around calls to Numbers.Ops.lt(Number, Number). So a call to (<= x y) is actually a call to (not (< y x)), which eventually uses the primitive < operator. Alas that logical premise doesn't hold when dealing with NaN:

user=> (<= 1 Double/NaN)
false
user=> (not (< Double/NaN 1))
true

So the bug is not that boxed NaN is treated incorrectly, but rather:

user> (<= 1000 (Double. Double/NaN)) ; becomes !(NaN < 1000) 
true
user> (<= 1000 (double Double/NaN))  ; becomes (1000 <= NaN)
false

In the original example, since there are more than two args, the primitive looking args were boxed:

user=> (<= 10 Double/NaN 1) ; equivalent to logical-and of the following
true
user=> (<= 10 (Double. Double/NaN))  ; becomes !(NaN < 10)
true
user=> (<= (Double. Double/NaN) 1)   ; becomes !(1 < NaN)
true

Note however that java object comparisons for NaNs behave differently: NaN is the largest Double, and NaNs equal each other (see the javadoc).

If we make object NaN comparisons always return false, we would need to add the rest of the comparison methods to Numbers.Ops. Yet doing so could also make collection sorting algorithms behave oddly, deviating from sorting written in java. Besides, (= NaN NaN) => false is annoying.

Clojure already throws out the notion of error-free dividing by zero (which for doubles would otherwise result in NaN or Infinity, depending on the dividend). Perhaps we could similarly error on NaNs passed to clojure numeric ops. They seem to be more trouble than they're worth. That said, people smarter than me thought they were useful.

Then there's that -0.0 nonsense...

Comment by Jouni K. Seppänen [ 19/Mar/11 3:02 PM ]

On current master, (<= x y) seems to be special-cased by the compiler, but when <= is called dynamically, the bug is still there:

user=> (<= 1 Float/NaN)
false
user=> (let [op <=] (op 1 Float/NaN))
true

Since CLJ-354 got marked "Completed", perhaps there was an attempt to fix this.

Comment by Alexander Taggart [ 19/Mar/11 6:45 PM ]

Using let forces calling <= as a function rather than inlining Numbers/lte, which means the args are treated as objects not primitives, thus the different behaviour as I discussed earlier.

Comment by Aaron Bedra [ 28/Jun/11 6:28 PM ]

Rich, what should the behavior be?

Comment by Jouni K. Seppänen [ 29/Jun/11 1:22 AM ]

My suggestion for the behavior is to follow Java (Java Language Specification §15.20.1) and IEEE 754. The java.sun.com site seems to be down right now, but here's a Google cache link:

http://webcache.googleusercontent.com/search?q=cache:http://java.sun.com/docs/books/jls/third_edition/html/expressions.html#15.20.1

Comment by Rich Hickey [ 29/Jul/11 7:48 AM ]

It should work the same as primitive double in all cases

Comment by Luke VanderHart [ 26/Aug/11 11:33 AM ]

Added patches. The problem was that our logic for lte/gte depended on the fact that lte is equivalent to !gt.

However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

Comment by Alex Miller [ 04/Mar/14 3:18 PM ]

David Welte noted: "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."

I concur that this patch was not applied. It looks likely that Luke marked this as Resolved when the patch was ready instead of whatever state change would have been appropriate at the time of the ticket (the process has varied over the years). AFAICT, this ticket should be open and Vetted (accepted as a problem) but probably needs release targeting and an updated patch based on current code.

Comment by Andy Fingerhut [ 05/Mar/14 12:32 PM ]

Might want to make the "Fix Version" on this ticket empty so it is back on the JIRA state chart as Vetted.

Comment by Andy Fingerhut [ 18/Apr/14 11:41 AM ]

Patch clj-738-v2.diff is identical in content to Luke's 2 patches 738.diff and 738-tests.diff, and includes them both, retaining his authorship. The only change is to a few context lines so that the new patch applies cleanly to latest master, whereas the older patches did not.

Comment by Alex Miller [ 24/Apr/14 3:22 PM ]

While the patch looks good and tests all pass, the example listed in the ticket description did not actually change behavior with the patch?

Comment by Jason Wolfe [ 24/Apr/14 5:19 PM ]

The ticket description has a typo (long, not double) – sorry. The first comment has a real test case.

user> (<= 1000 (Double. Double/NaN))
true
user> (<= 1000 (double Double/NaN))
false

Comment by Alex Miller [ 24/Apr/14 8:22 PM ]

Doh! Thank you. I'm the one that introduced it too.





Generated at Tue Mar 03 03:10:29 CST 2015 using JIRA 4.4#649-r158309.