<< Back to previous view

[CLJ-1278] Provide a useful implementation of toString() for Clojure functions Created: 10/Oct/13  Updated: 25/Nov/14

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: errormsgs, interop

Attachments: Text File CLJ-1278-2.patch     Text File CLJ-1528--function-tostring.patch    
Patch: Code and Test

 Description   

Clojure functions do not provide a useful override of the default Java Object.toString() method.

Because of this, any time a Clojure function is printed out, it places a load on the develop to mentally reverse the necessary name mangling to get back to the namespace and function name.

I would vastly prefer better information here: the non-mangled name of the function, including namespace and, ideally, a little bit of the available meta-data: the file name and line number.

In other words, instead of novate.core.processing.async$locate_destination@2690d691 something more like novate.core.processing.async/locate-destination(async.clj:231).

Ideally, anything that is code generated as a class should implement a useful toString(). It would be nice if reified types could identify the containing namespace and function (and file/line number) as part of their default toString().

I suspect this would need to be controlled by a compiler option as it would slightly increase the generated bytecode size, and (imperceptably?) affect compilation speed.



 Comments   
Comment by Howard Lewis Ship [ 10/Oct/13 8:39 PM ]

Contains changes and updated tests. I don't have any details on if this affects compiler performance or generated code size in any significant or even measurable way.

Comment by Andy Fingerhut [ 11/Oct/13 4:06 PM ]

Howard, sorry I do not have more useful comments on the changes you make in your patch. Right now I only have a couple of minor comments on its form. The preferred format for patches is that created using the instructions shown on this wiki page: http://dev.clojure.org/display/community/Developing+Patches

Also, there are several parts of your patch that appear to only make changes in the whitespace of lines. It would be best to leave such changes out of a proposed patch.

Comment by Howard Lewis Ship [ 11/Oct/13 5:00 PM ]

Yes, I didn't notice the whitespace changes until after; I must have hit reformat at some point, despite my best efforts. I'll put together a new patch shortly.

Comment by Howard Lewis Ship [ 11/Oct/13 6:26 PM ]

Clean patch

Comment by Howard Lewis Ship [ 25/Nov/14 6:00 PM ]

FYI, it's been a year. The correct file is CLJ-1278-2.patch.

Comment by Howard Lewis Ship [ 25/Nov/14 6:07 PM ]

... hm, something's changed in recent times.

     [java] FAIL in (fn-toString) (fn.clj:83)
     [java] nested functions
     [java] expected: (= (simple-name (.toString (factory-function))) (str "clojure.test-clojure.fn/" "factory-function/fn"))
     [java]   actual: (not (= "clojure.test-clojure.fn/factory-function/fn__7565" "clojure.test-clojure.fn/factory-function/fn"))
     [java]
     [java] FAIL in (fn-toString) (fn.clj:83)
     [java] nested functions
     [java] expected: (= (simple-name (.toString (named-factory-function))) (str "clojure.test-clojure.fn/" "named-factory-function/a-function-name"))
     [java]   actual: (not (= "clojure.test-clojure.fn/named-factory-function/a-function-name__7568" "clojure.test-clojure.fn/named-factory-function/a-function-name"))

I'd be willing to update my patch if there's any indication that it will ever be picked up. It's been over a year since last update.





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

Status: Open
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: Unresolved Votes: 0
Labels: transducers

Attachments: Text File clj-1601-2.patch     Text File clj-1601.patch    
Patch: Code and Test
Approval: Vetted

 Description   
  • with generative tests
  • with examples demonstrating performance

Patch: clj-1601-2.patch

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.





[CLJ-1604] AOT bug prevents overriding of clojure.core functions Created: 25/Nov/14  Updated: 25/Nov/14

Status: Open
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: Unresolved Votes: 0
Labels: aot, compiler

Attachments: Text File CLJ-1604-only-core.patch    
Patch: Code
Approval: Vetted

 Description   

Currently the AOT compiler doesn't do any interning at the runtime point corresponding to a def expression, relying on the var creation in the static initializer to (implicitely) do the interning.

This means that `(def a "foo")` will be compiled to something like (not actual bytecode):

__load0 {
getstatic #1
getstatic #2
invokevirtual "bindRoot"
}

<clinit> {
ldc "user"
ldc "a"
invokestatic "RT.var(String,String)" // the interning happens (implicitely) here
putstatic #1
ldc "foo"
putstatic #2
}

This is problematic if a runtime call interns a clojure.core var with the same name of the var at some point before the def expression:

(refer-clojure)
(def update "foo")

In this case the correct var will be mapped by RT.var call in the static initializer, but at runtime the refer-cloure call will replace that mapping with the clojure.core Var.

To fix this issue the proposed patch explicitely interns the Var during loading rather than implicitely during the static initialization if a conflict with a clojure.core var is possible thus avoiding the emission of the additional 7 instructions per def expression on most cases.






[CLJ-1591] Symbol not being bound in namespace when name clashes with clojure.core Created: 14/Nov/14  Updated: 25/Nov/14

Status: Reopened
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: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Approval: Incomplete

 Description   

The following code fails (both in 1.6 and latest 1.7-alpha4):

user=> (ns foo)
nil
foo=>  (def inc inc)
WARNING: inc already refers to: #'clojure.core/inc in namespace: foo, being replaced by: #'foo/inc
#'foo/inc

;; Note inc is unbound at this point, which causes the exception below
foo=> inc
#<Unbound Unbound: #'foo/inc>
foo=> (ns bar)
nil
bar=> (require ['foo :refer ['inc]])
WARNING: inc already refers to: #'clojure.core/inc in namespace: bar, being replaced by: #'foo/inc
nil
bar=> (inc 8)

IllegalStateException Attempting to call unbound fn: #'foo/inc  clojure.lang.Var$Unbound.throwArity (Var.java:43)

Further investigation shows that foo/inc is unbound:

foo/inc
=> #<Unbound Unbound: #'foo/inc>

Further investigation also shows that replacing the (def inc inc) with almost anything else, e.g. (def inc dec), (def inc clojure.core/inc), or (def inc (fn [n] (+ n 1))), causes no exception (but the warnings remain).

I would expect:
a) foo/inc should be bound and have the same value as clojure.core/inc
b) No error when requiring foo/inc
c) bar/inc should be bound to foo/inc



 Comments   
Comment by Nicola Mometto [ 14/Nov/14 10:04 PM ]

The second error should be expected, the right syntax should be (require ['foo :refer ['inc]]) (note the leading quote before inc)

Comment by Mike Anderson [ 14/Nov/14 10:20 PM ]

Thanks for the catch Nicola - I've edited the description. Still get the same error however (just with a slightly different message)

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

See comment...

Comment by Mike Anderson [ 14/Nov/14 10:24 PM ]

@Alex what comment? Note that the error still occurs even with the right syntax....

Comment by Mike Anderson [ 14/Nov/14 10:26 PM ]

Appears to have been closed prematurely

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

I can't reproduce with the correct syntax:

Clojure 1.7.0-master-SNAPSHOT
user=> (ns foo)
nil
foo=> (def inc inc)
WARNING: inc already refers to: #'clojure.core/inc in namespace: foo, being replaced by: #'foo/inc
#'foo/inc
foo=> (ns bar)
nil
bar=> (require ['foo :refer ['inc]])
WARNING: inc already refers to: #'clojure.core/inc in namespace: bar, being replaced by: #'foo/inc
nil
Comment by Mike Anderson [ 14/Nov/14 10:55 PM ]

The problem is that the var is still unbound and causes e.g. the following error:

=> (foo/inc 8)
IllegalStateException Attempting to call unbound fn: #'foo/inc clojure.lang.Var$Unbound.throwArity (Var.java:43)

I don't think that should be expected - or am I missing something?

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

Ah, will take a look. But not right now.

Comment by Andy Fingerhut [ 15/Nov/14 1:09 PM ]

Updated the description with a few more details. The exception goes away if you do (def inc (fn [n] (+ n 1))) instead of (def inc inc), for example. The warnings remain.

Comment by Tom Crayford [ 20/Nov/14 11:07 AM ]

Unsure if this is the same issue (I think it might be?), but I reproduced the exact same error message with AOT compilation involved:

reproduced in this git repository: https://github.com/yeller/compiler_update_not_referenced_bug

clone it, run `lein do clean, uberjar, test`, and that error message will show up every time for me

Comment by Andy Fingerhut [ 20/Nov/14 5:43 PM ]

Mike, I think replacing (def inc inc) in your example with (def inc clojure.core/inc) should be considered as a reasonable workaround for this issue, unless you have some use case where you need to def inc to something that is not in clojure.core (and if so, why?)

The reason (def inc inc) behaves this way is, if not absolutely necessary, at least commonly used in Clojure programs to define recursive functions, e.g. (defn fib [n] (if (<= n 1) 1 (+ (fib (dec n)) (fib (- n 2))))), so that the occurrences of fib in the body are resolved to the fib being defined.

Comment by Alex Miller [ 22/Nov/14 9:05 AM ]

Moving to 1.7 until I can look at this more deeply.

Comment by Mike Anderson [ 23/Nov/14 6:08 PM ]

Andy - yes the workaround is fine for me right now.

I don't think this is an urgent issue but it may be exposing a subtle complexity regarding assumptions about the state of the namespace at different times. Perhaps the semantics should be something like:

  • The def statement itself should be run before the var is interned. e.g. (def inc (inc 5)) should result in (def inc 6)
  • Anything complied / deferred to run after completion of the def statement should use the new var (i.e. the new var should be referenced by fns, lazy sequences etc.)
Comment by Andy Fingerhut [ 23/Nov/14 6:36 PM ]

I'm not sure what your proposal means in a case like this:

(def inc (fn [x] (inc x)))

Is the second inc to be interpreted/resolved before or after the new inc is created? Because it is (fn ...) it should be the after-behavior? What else besides fn should cause the after-behavior, rather than the before-behavior?

Even more fun (not saying that people often write code like this, but the compiler can handle it today):

(def inc (if (> (inc y) 5)
           (fn [x] (inc x))
           (fn [x] (dec x))))

I think the current compiler behavior of 'in the body of a def, the def'd symbol always refers to the new var, not any earlier def'd vars' is fairly straightforward to explain.

Comment by Tom Crayford [ 23/Nov/14 9:15 PM ]

Should I file the AOT issue reproduced in that thing as a new issue?

Comment by Andy Fingerhut [ 24/Nov/14 5:16 PM ]

Tom: Alex Miller or another screener would be best to say whether the AOT issue should be a separate ticket, but my best guess would be "go for it". I tried to look at the link you gave but it seems not to point to anything. Could you double-check that link?

Comment by Tom Crayford [ 24/Nov/14 6:48 PM ]

Andy,

Great. I'll write one up tomorrow sometime. I accidentally left that repo as private, should be visible now.

Comment by Andy Fingerhut [ 24/Nov/14 8:11 PM ]

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 [ 25/Nov/14 3:44 PM ]

Tom, I've opened a ticket with a patch fixing the AOT issue: http://dev.clojure.org/jira/browse/CLJ-1604





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

Status: Open
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: Unresolved 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: Screened

 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-1603] cycle and iterate return vals should IReduceInit Created: 25/Nov/14  Updated: 25/Nov/14

Status: Open
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: Unresolved Votes: 0
Labels: None

Approval: Vetted

 Description   
  • with generative tests
  • with perf examples


 Comments   
Comment by Ghadi Shayban [ 25/Nov/14 11:01 AM ]

Stu, do you intend these to be in Java or Clojure? It could be trickier to implement in Clojure directly, as loading would have to be deferred until core_deftype loads. It's certainly tractable without breaking any backwards compatibility, and I've explored this while experimenting with Range as a deftype https://github.com/ghadishayban/clojure/commit/906cd6ed4d7c624dc4e553373dabfd57550eeff2

A macro to help with Seq&List participation could be certainly useful, as efficiently being both a Seq/List and IReduceInit isn't a party.

May be useful to list requirements for protocol/iface participation.

It seems like 'repeatedly' is another missing link in the IReduceInit story.

Rich mentioned the future integration of reduce-kv at the conj, it would also be useful to know how that could fit in.

---- Other concerns and ops that may belong better on the mailing list ----

In experimenting with more reducible sources, I put out a tiny repo (github.com/ghadishayban/reducers) a couple weeks ago that includes some sources and operations. The sources were CollReduce and not ISeq.

Relatedly, caching the hashcode as a Java `transient` field is not supported when implementing a collection using deftype (patch w/ test in CLJ-1573).

Sources:
Iterate was one of them https://github.com/ghadishayban/reducers/blob/master/src/ghadi/reducers.clj#L43-L51
Repeatedly https://github.com/ghadishayban/reducers/blob/master/src/ghadi/reducers.clj#L43-L51

Reduce/transduce-based Operations that accept transducers:
some, any, yield-first https://github.com/ghadishayban/reducers/blob/master/src/ghadi/reducers.clj#L52-L80
(any could use a better name, equiv to (first (filter...)))
some and any have a symmetry like filter/remove.

Novelty maybe for 1.8:
A transducible context for Iterables similar to LazyTransformer:
https://github.com/ghadishayban/reducers/blob/master/src/ghadi/reducers.clj#L157-L161

The unless-reduced macro was very useful in implementing the collections:
https://github.com/ghadishayban/reducers/blob/master/src/ghadi/reducers.clj#L7-L15
It is different than the ensure-reduced and unreduced functions in core.

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

When we discussed this in the past, it was in the vein of reusing some of the range work (in Java) to implement cycle and iterate (per CLJ-1515).





[CLJ-1602] vals and keys return values should implement IReduceInit Created: 25/Nov/14  Updated: 25/Nov/14

Status: Open
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: Unresolved Votes: 0
Labels: None

Approval: Vetted

 Description   
  • with generative tests
  • with perf demos


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

Could leverage CLJ-1499 for the bulk of this, may pull that back from 1.8 into 1.7. Waiting on further work till that's answered.





[CLJ-1546] Widen vec to take Iterable/IReduce Created: 02/Oct/14  Updated: 25/Nov/14

Status: Open
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: Unresolved Votes: 0
Labels: None

Attachments: PNG File benchmark.png     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.patch    
Patch: Code and Test
Approval: Vetted

 Description   

These examples should work but does 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.

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. If maps, sets, and range were IReduce later they would also use this path and see noticeable boosts. This is also the branch that will handle the Eduction and IReduceInit cases added in the patch.

3) (LPV) instanceof ISeq: If the coll is a sequence already, best to 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 most 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 would be fixed by CLJ-1499 and/or making PHM/PHS IReduceInit.

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-5.patch

Screened by:



 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.





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

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

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

Android ART runtime


Attachments: Text File 0001-CLJ-1472-Locking-macro-fails-bytecode-verification.patch    
Patch: Code
Approval: Triaged

 Description   

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

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

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



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

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

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

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

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

Comment by Adam Clements [ 04/Aug/14 4:24 PM ]

Uploaded a new patch (and signed the contributor agreement). This passes both the JVM and ART bytecode verification, The extra try/catch around the monitor exit is optional (verification passes with or without it) but where the java version retries monitor-exit indefinitely and shows the deadlock at the right time, without catching errors in the monitor-exit an undetermined monitor-enter in the future might fail, not showing up the actual bug.

It's not very pretty, but without finer grained control of the generated bytecode, this is the best I could do.

Comment by Adam Clements [ 25/Nov/14 8:31 AM ]

Have just tested with Lollipop, and this patch might no longer be sufficient.

Getting in touch with the ART guys to see if they can shed a little more light and verify whether it will work on the current master branch of AOSP

Comment by Adam Clements [ 25/Nov/14 9:49 AM ]

Bug filed with AOSP project, hopefully they can shed some light on whether it is our problem and if so how we can fix it.

https://code.google.com/p/android/issues/detail?id=80823





[CLJ-1580] Transient collections should guarantee thread visibility Created: 05/Nov/14  Updated: 25/Nov/14

Status: Open
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: Unresolved Votes: 0
Labels: transient

Approval: Incomplete

 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.






[CLJ-1599] Add get-and-set! to expose AtomicReference.getAndSet() in atoms Created: 24/Nov/14  Updated: 24/Nov/14

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

Type: Enhancement Priority: Minor
Reporter: Steven Yi Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: enhancement

Attachments: File get-and-set.diff    
Patch: Code

 Description   

DESCRIPTION

This patch adds get-and-set! to core to allow getting the last value from an atom and setting it to a new value. This is useful for atomically draining an atom of its value and setting to a new value. The implementation delegates to Java's AtomicReference.getAndSet().

The equivalent operation in Clojure code would be:

(defn get-and-set! [atm newv]
(loop [oldv @atm]
(if (compare-and-set! atm oldv newv)
oldv
(recur @atm))))

This is close to a 1:1 translation of the Java code in sun.misc.Unsafe's getAndSetObject, used by AtomicReference (as of current JDK9 source code).

APPLICATIONS

  • User may want to check if an operation has occurred before by using an atom as a flag. I.e.,

(def has-run-once (atom false))
...
(when-not (get-and-set! has-run-once true)
(do something))

  • User may want to use an atom similarly to a java.util.concurrent.LinkedTransferQueue, for the case of pairing up adds by writers and drainTo by readers:

Thread 1: (swap! atm conj item1)
Thread 2: (swap! atm conj item2)
Thread 3: (let [new-vals (get-and-set! atm [])]
(do-something new-vals))

ALTERNATIVES

  • For case of atom as flag, user can use existing compare-and-set!:

(def has-run-once (atom false))
...
(when-not (compare-and-set! has-run-once false true)
(do something))

Argument: get-and-set! is a little clearer in intent as it is using the value of the atom, rather than the success of the cas operation. Also, this would not be applicable to situations where the value stored is not a boolean.

  • User can just go ahead and use LinkedTransferQueue.

Argument: User not fluent in Java may not be readily able to use this.

==

Argument for: This seems like a sufficiently primitive operation to include in core for atoms. I am unsure of the rationale, but assume it was vetted to include into Java's AtomicReference for a reason. Also, if users are using atoms and have this available, they are less likely to try to do this incorrectly, such as:

(let [vals @some-atom]
(reset! some-atom [])
(do-something-with vals))

Argument against: This may not be sufficiently primitive enough to include in core. Users have a workaround to implement the get-and-set! operation in user-code as given above.

Note: This request is similar to CLJ-1454 (http://dev.clojure.org/jira/browse/CLJ-1454), but differs in that this is not a swap! operation that accepts an IFn argument. Also, I looked to add a test in test/clojure/test_clojure/atoms.clj, but saw that the other operations weren't tested. (I assume this is due to the other operations delegating to AtomicReference and hence not deemed test-worthy.)






[CLJ-1598] Make if forms compile directly to the appropriate branch expression if the test is a literal Created: 24/Nov/14  Updated: 24/Nov/14

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler, performance, primitives

Attachments: Text File 0001-if-test-expr-of-an-if-statement-is-a-literal-don-t-e.patch    
Approval: Triaged

 Description   

This allows expressions like `(cond (some-expr) 1 :else 2)` to be eligible for unboxed use, which is not currently possible since the cond macro always ends up with a nil else branch that the compiler currently takes into account.

With the attached patch, the trailing (if :else 2 nil) in the macroexpansion will be treated as 2 by the Compiler, thus allowing the unboxed usage of the cond expression.






[CLJ-1597] Allow ISeq args to map conj Created: 22/Nov/14  Updated: 22/Nov/14

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, maps

Attachments: Text File 0001-allow-ISeq-args-to-map-conj.patch    
Patch: Code and Test

 Description   

Currently conj on maps can take only maps or vectors, this patch allows it to take any ISeq:

user=> (conj {} '(1 2))
{1 2}





[CLJ-1594] Colons followed by spaces are not ignored within (comment ...) Created: 19/Nov/14  Updated: 22/Nov/14  Resolved: 22/Nov/14

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

Type: Defect Priority: Major
Reporter: Ed Ward Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug, comment, function
Environment:

Ubuntu Linux 14.10
Clojure 1.6.0
OpenJDK 64-Bit Server VM 1.7.0_65-b32



 Description   

Running

(comment abc:def)
works, while running
(comment abc: def)
and
(comment abc : def)
fail with the exception

RuntimeException Invalid token: :  clojure.lang.Util.runtimeException (Util.java:221)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: def in this context, compiling:(/tmp/form-init1585368677683647130.clj:1:1010) 
RuntimeException Unmatched delimiter: )  clojure.lang.Util.runtimeException (Util.java:221)


 Comments   
Comment by Nicola Mometto [ 19/Nov/14 11:19 AM ]

`comment` is a macro, it doesn't bypess the reader.
":" is not a regular clojure token so it's expected that this throws.
The only way to include non clojure tokens in a source code is after a ";" comment

Comment by Andy Fingerhut [ 19/Nov/14 3:37 PM ]

I've just added some notes about this to ClojureDocs.org here: http://clojuredocs.org/clojure.core/comment

Comment by Alex Miller [ 22/Nov/14 9:12 AM ]

agreed with Nicola's comment above





[CLJ-1286] Fix reader spec and regex to match code for keywords starting with digits Created: 31/Oct/13  Updated: 21/Nov/14

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

Type: Defect Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: reader


 Description   

The reader page at http://clojure.org/reader states that symbols (and keywords) cannot start with a number and the regex used in LispReader (and EdnReader) also has this intention. CLJ-1252 addressed this by fixing the broken reader regex to match the spec. However, that broke some existing code so we rolled back the change. There is still a disconnect here and this ticket serves to decide what to do instead.

I presume that we are effectively deciding that keywords like :5 are ok to read. If so, we should alter the regex to more accurately capture that intent - right now it allows these purely by accident due to backtracking. A secondary question is whether the Clojure and EDN reader spec should also explicitly allow these as valid. My preference would be to have the reader and the spec match, so I would lobby to loosen the reader spec.

Related ticket: CLJ-1527



 Comments   
Comment by Nicola Mometto [ 12/Nov/13 4:50 PM ]

what about keywords like :1/1 or :1/a? Clojure currently accepts the latter but not the former.

Comment by Francis Avila [ 02/Jul/14 3:13 PM ]

There's more discussion of this problem (and symbol/keyword parsing in general) in the context of cljs.reader at CLJS-677.

Comment by Andy Fingerhut [ 02/Jul/14 3:27 PM ]

Francis, can you double-check that ticket number? The one you mention (CLJS-667) doesn't seem to have any discussion of this problem.

Comment by Francis Avila [ 02/Jul/14 3:39 PM ]

Sincere apologies, it's CLJS-677. (Original post corrected too.)





[CLJ-1527] Harmonize accepted / documented symbol and keyword syntax over various readers Created: 18/Sep/14  Updated: 21/Nov/14

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

Type: Defect Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: reader

Approval: Triaged

 Description   

Documentation Issues

http://clojure.org/reader#The%20Reader--Reader%20forms is ambigous on whether foo/bar/baz is allowed. Also, it doesn't mention the tick ' as a valid constituent character.
The EDN spec also currently omits ', ticket here: https://github.com/edn-format/edn/issues/67

Implementation Issues

clojure.core/read, as well as clojure.edn/read accept symbols like foo/bar/baz, even though they should be rejected.

References

https://groups.google.com/d/topic/clojure-dev/b09WvRR90Zc/discussion
Related ticket: CLJ-1286



 Comments   
Comment by Andy Fingerhut [ 17/Oct/14 2:13 AM ]

The Clojure reader documentation also does not mention the following symbols as valid constituent characters. They are all mentioned as valid symbol constituent characters in the EDN readme here: https://github.com/edn-format/edn#symbols

dollar sign - used in Clojure/JVM to separate Java subclass names from class names, e.g. java.util.Map$Entry
percent sign - not sure why this is part of edn spec. In Clojure it seems only to be used inside #() for args like % %1 %&
ampersand - like in &form and &env in macro definitions
equals - clojure.core/= and many others
less-than - clojure.core/< clojure.core/<=
greater-than - clojure.core/> clojure.core/>=

I don't know whether Clojure and edn specs should be the same in this regard, but it seemed worth mentioning for this ticket.





[CLJ-1579] source-fn can fail due to reading namespace-aliased keywords while in another namespace context Created: 05/Nov/14  Updated: 21/Nov/14

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

Type: Defect Priority: Minor
Reporter: Reid McKenzie Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Read-src-in-appropriate-ns-context.patch    
Patch: Code
Approval: Triaged

 Description   

clojure.repl/source-fn functions by using a custom reader to read a source form at the location specified by line & file metadata on a given symbol. While this works well for most things, I encountered an issue when applying source-fn to code containing keyword namespace aliases ala ::T/foo. ::T/foo is a legitimate namespace keyword in the context where it occurs, because a namespace alias to T is created in the ns header. When the keyword ::T/foo is read then, it resolves to :my-other.ns/foo as one would expect because ns has the appropriate alias. However when attempting to read source via clojure.repl/source-fn, ns may no longer be the original read context of the indicated form thus leading to the erroneous exception java.lang.RuntimeException: Invalid token: ::T/foo.

The solution is that the reading operation of clojure.repl/source-fn must be wrapped in (binding [*ns* (.ns v)] ...) so that source reading will take place in the original load reading context thus preventing this error.

A patched equivalent function exists here, https://github.com/clojure-grimoire/lein-grim/blob/master/src/grimoire/doc.clj#L50-L74, and I will submit a patch against 1.6.0 in the morning.






[CLJ-1596] Using keywords in place of symbols for defrecord fields causes a compiler exception with incorrect line number Created: 20/Nov/14  Updated: 20/Nov/14

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

Type: Defect Priority: Minor
Reporter: Kyle Kingsbury Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, defrecord

Approval: Triaged

 Description   

Possibly related to http://dev.clojure.org/jira/browse/CLJ-1261: a defrecord like

(defn foo [x])

(defrecord Bar [:b])

Throws an exception, like you'd expect:

java.lang.ClassCastException: clojure.lang.Keyword cannot be cast to clojure.lang.IObj, compiling:(tesser/quantiles_test.clj:45:15)

However, this exception's line and character indicates the error is in the previous form: the defn, not the defrecord. This can be really tricky to figure out when the expressions are more complicated.



 Comments   
Comment by Alex Miller [ 20/Nov/14 4:17 PM ]

Related: CLJ-1261

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

Possibly fixed by CLJ-1561, not sure.





[CLJ-1595] Nested doseqs leak with sequence of huge lazy-seqs Created: 20/Nov/14  Updated: 20/Nov/14

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

Type: Defect Priority: Minor
Reporter: Andrew Rudenko Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File doseq_leaks.diff    
Patch: Code

 Description   

Hello!

This little snippet demonstrates the problem:

(doseq [outer-seq (list (range)) inner-seq outer-seq])

That's it. It is not just eats my processor, but also eats all available memory. Practically it can affect (and it is) at consuming of complex lazy structures like huge XML-documents.

I think this is at least non trivial behaviour.

It can be fixed by this small patch. We can get next element before current iteration, not after, so outer loop will not hold reference to the head of inner-seq.

This patch doesn't solve all problems, for example this code:

(doseq [outer-seq [(range)] inner-seq outer-seq])

leaks. Because chunked-seqs (vector in this case) holds current element by design.






[CLJ-1074] Read/print round-trip for +/-Infinity and NaN Created: 21/Sep/12  Updated: 19/Nov/14

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

Type: Defect Priority: Minor
Reporter: Colin Jones Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: print, reader

Attachments: Text File 0001-Read-Infinity-and-NaN.patch     Text File clj-1074-read-infinity-and-nan-patch-v2-plus-edn-reader.patch    
Patch: Code and Test
Approval: Triaged

 Description   

A few float-related forms (namely, Double/POSITIVE_INFINITY, Double/NEGATIVE_INFINITY, Double/NaN) are not eval-able after a round-trip via

(read-string (binding [*print-dup* true] (pr-str f))

The two options I see are to provide print-method implementations for these and their Float cousins, or to make Infinity, -Infinity, +Infinity, and NaN readable values. Since it sounds like edn may want to provide a spec for these values (see https://groups.google.com/d/topic/clojure-dev/LeJpOhHxESs/discussion and https://github.com/edn-format/edn/issues/2), I think making these values directly readable as already printed is preferable. Something like Double/POSITIVE_INFINITY seems too low-level from edn's perspective, as it would refer to a Java class and constant.

I'm attaching a patch implementing reader support for Infinity, -Infinity, +Infinity, and NaN.



 Comments   
Comment by Timothy Baldridge [ 03/Dec/12 11:34 AM ]

Please bring this up on clojure-dev. We'll be able to vet this ticket after that.

Comment by Colin Jones [ 03/Dec/12 1:18 PM ]

Should I respond to my original clojure-dev post about this (linked in the issue description above), or start a new one?

Comment by Andy Fingerhut [ 24/May/13 1:11 PM ]

Patch clj-1074-read-infinity-and-nan-patch-v2.txt dated May 24 2013 is identical to 0001-Read-Infinity-and-NaN.patch dated Sep 21 2012, except it applies cleanly to latest master. The older patch conflicts with a recent commit made for CLJ-873.

Comment by Nicola Mometto [ 25/May/13 11:55 AM ]

clj-1074-read-infinity-and-nan-patch-v2-plus-edn-reader.patch is the same as clj-1074-read-infinity-and-nan-patch-v2.txt except it patches EdnReader too, but it must be applied after #CLJ-873 0001-Fix-CLJ-873-for-EdnReader-too.patch get merged





Generated at Tue Nov 25 18:32:19 CST 2014 using JIRA 4.4#649-r158309.