<< Back to previous view

[CLJ-1529] Significantly improve compile time by reducing calls to Class.forName Created: 21/Sep/14  Updated: 24/Oct/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: Zach Tellman Assignee: Unassigned
Resolution: Unresolved Votes: 27
Labels: compiler, performance

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

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

This patch improves compilation time from 18 seconds to 7 seconds. The gain is exaggerated by the number of macros I use, but I would expect at least 50% improvements across a wide variety of codebases.

This patch does introduce a slight semantic change by privileging lexical scope over classnames. Consider this code:

(let [String "foo"]
(. String substring 0 1))

Previously, this would be treated as a static call to 'java.lang.String', but with the patch would be treated as a call to the lexical variable 'String'. Since the new semantic is what I (and I think everyone else) would have expected in the first place, it's probably very likely that no one is shadowing classes with their variable names, since someone would have complained about this. If anyone feels this is at all risky, however, I'm happy to discuss it further.

[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





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

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

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

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





[CLJ-1572] into does not work with IReduceInit Created: 24/Oct/14  Updated: 24/Oct/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: None

Attachments: Text File clj-1572.patch    
Patch: Code
Approval: Incomplete

 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)


 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.





[CLJ-1546] Widen vec to take Iterable/IReduce Created: 02/Oct/14  Updated: 24/Oct/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: Text File clj-1546-2.patch     Text File clj-1546-3.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. Add a number of cases to vec to support different cases.

Patch: clj-1546-3.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.





[CLJ-1192] vec function is substantially slower than into function Created: 06/Apr/13  Updated: 24/Oct/14  Resolved: 24/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: performance

Approval: Incomplete

 Description   

(vec coll) and (into [] coll) do exactly the same thing. However, due to into using transients, it is substantially faster. On my machine:

(time (dotimes [_ 100] (vec (range 100000))))
"Elapsed time: 732.56 msecs"

(time (dotimes [_ 100] (into [] (range 100000))))
"Elapsed time: 491.411 msecs"

This is consistently repeatable.

Since vec's sole purpose is to transform collections into vectors, it should do so at the maximum speed available.



 Comments   
Comment by Andy Fingerhut [ 07/Apr/13 5:50 PM ]

I am pretty sure that Clojure 1.5.1 also uses transient vectors for (vec (range n)) (probably also some earlier versions of Clojure, too).

Look at vec in core.clj. It checks whether its arg is a java.util.Collection, which lazy seqs are, so calls (clojure.lang.LazilyPersistentVector/create coll).

LazilyPersistentVector's create method checks whether its argument is an ISeq, which lazy seqs are, so it calls PersistentVector.create(RT.seq(coll)).

All 3 of PersistentVector's create() methods use transient vectors to build up the result.

I suspect the difference in run times are not because of transients or not, but because of the way into uses reduce, and perhaps may also have something to do with the perhaps-unnecessary call to RT.seq in LazilyPersistentVector's create method (in this case, at least – it is likely needed for other types of arguments).

Comment by Alan Malloy [ 14/Jun/13 2:17 PM ]

I'm pretty sure the difference is that into uses reduce: since reducers were added in 1.5, chunked sequences know how to reduce themselves without creating unnecessary cons cells. PersistentVector/create doesn't use reduce, so it has to allocate a cons cell for each item in the sequence.

Comment by Gary Fredericks [ 08/Sep/13 1:55 PM ]

Is there any downside to (defn vec [coll] (into [] coll)) (or the inlined equivalent)?

Comment by Ghadi Shayban [ 11/Apr/14 5:13 PM ]

While I agree that there are improvements and possibly low-hanging fruit, FWIW https://github.com/clojure/tools.analyzer/commit/cf7dda81a22f4c9c1fe64c699ca17e7deed61db4#commitcomment-5989545

showed a 5% slowdown from a few callsites in tools.analyzer.

This ticket's benchmark is incomplete in that it covers a single type of argument (chunked range), and flawed as it timing the expense of realizing the range. (That could be a legit benchmark case, but it shouldn't be the only one).

Sorry to rain on a parade. I promise like speed too!

Comment by Greg Chapman [ 25/Apr/14 5:23 PM ]

One thing to note is that vec has a subtle difference from into when the collection is an Object array of length <= 32. In that case, vec aliases the supplied array, rather than copying it (this is noted in the warning here: http://clojuredocs.org/clojure_core/clojure.core/vec). I believe I read some place that this behavior is intentional, but I can't find the citation.

Comment by Andy Fingerhut [ 25/Apr/14 10:18 PM ]

Greg, CLJ-893 might be what you remember. That is the ticket that was closed by a patch updating the documentation of vec.

Comment by Mike Anderson [ 18/May/14 7:41 AM ]

I think there are quite a few performance improvements that can be made to vec in general. For example, if given a List it should use PersistentVector.create(List) rather than producing an unnecessary seq, which appears to be the case at the moment. Also it should probably return the same object if passed an existing IPersistentVector.

Basically there are a number of cases that we could be handling more efficiently....

I'm taking a look at this now.... will propose a quick patch if it seems there is a good solution.

Comment by Mike Anderson [ 24/Jul/14 4:01 AM ]

I've looked at this issue and it is quite complex. There are multiple types that need to potentially be converted into vectors, and doing so efficiently will often require making use of reduce-style operations on the source collections.

Doing this efficiently will probably in turn require making use of the IReduce interface, which doesn't yet seem to be fully utilised across the Clojure code based. If we do this, lots of operations (not just vec!) can be made faster but it will be quite a major change.

I have a branch that implements some of this but would appreciate feedback if this is the right direction before I take it any further:
https://github.com/mikera/clojure/tree/clj-1192-vec-performance

Comment by Alex Miller [ 24/Jul/14 9:45 AM ]

Thanks Mike! It may take a few days before I can get back to you about this.

Comment by Mike Anderson [ 25/Jul/14 3:44 AM ]

Basically the approach I am proposing is:

  • Make various collections implement IReduce efficiently (if they don't already). Especially applied to chunked seqs etc.
  • Have RT.reduce(...) methods that implement reduce on the Java side
  • Make the Clojure side use IReduce where relevant (should be as simple as extending the existing protocols)
  • Implement vec (and other similar operations) in terms of IReduce - which will solve this specific issue

If we really care about pushing vector performance even further, we can also consider:

  • Create specialised small vector types where appropriate - e.g. a specialised SmallPersistentVector class for <32 elements. This should outperform the more generic PersistentVector which is better suited for large vectors.
  • Some dedicated construction functions that know how to efficiently exploit knowledge about the data source (e.g. creating a vec from a segment of a big Object array can be done with a bunch of arraycopys into 32-element chunks and then constructing a PersistentVector around these)

This should give us a decent speedup overall (of course it would need benchmarking... but I'd hope to see some sort of measurable improvement on a macro benchmark like building and testing Clojure).

Comment by Alex Miller [ 24/Oct/14 8:36 AM ]

I believe this will be covered by a combination of CLJ-1546 (which makes vec work in more cases and faster on everything but chunked seqs) and CLJ-1515 (which will make the primary chunked seq use case of range faster) so marking as a dupe. After those are done, would consider more targeted ticket for whatever case is left.





[CLJ-1515] Reify the result of range Created: 29/Aug/14  Updated: 24/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: File patch.diff     File range-patch3.diff    
Patch: Code and Test
Approval: Incomplete

 Description   

Currently range simply returns a lazy seq. If the return value of range were reified into a type (as it is in ClojureScript) we could optimize many functions on that resulting type. Some operations such as count and nth become O(1) in this case, while others such as reduce could receive a performance boost do to the reduced number of allocations.

Approach: this patch revives the unused (but previously existing) clojure.lang.Range class. This class acts as a lazy seq and implements several other appropriate interfaces such as Counted and Indexed. This type is implemented in Java since range is needed fairly on in core.clj before deftype is defined. The attached patch uses Numbers.* methods for all math due to the input types to range being unknown. The class also supplies a .iterator() method which allows for allocation free reducing over range.

Note: this code keeps backwards compatibility with the existing range code. This means some parts of the class (mostly relating to a step size of 0) are a bit more complex than desired, but these bits were needed to get all the tests to pass.

Note: this code does not preserve the chunked-seq nature of the original range. The fact that range used to return chunked seqs was not published in the doc strings and so it was removed to allow for simpler code in Range.java.

Performance:
(timings done at the repl run via java -jar)

(dotimes [x 100] (time (dotimes [x 1] (count (range (* 1024 1024))))))
master => 80-110ms
patch => 0.014ms


(dotimes [x 100] (time (dotimes [x 1] (reduce + (map inc (range (* 1024 1024)))))))
master => 76-87ms
patch => 340-360ms


(dotimes [x 100] (time (dotimes [x 1] (reduce + (map inc (map inc (range (* 1024 1024))))))))
master => 97-123ms
patch=> 490-577ms



(dotimes [x 100] (time (dotimes [x 1] (count (filter odd? (range (* 1024 1024)))))))
master => 87-104ms
patch => 370-330ms


(dotimes [x 100] (time (dotimes [x 1] (transduce (comp (map inc) (map inc)) + (range (* 1024 1024))))))
master=>76-116ms
patch => 44ms-59ms

Patch: range-patch3.diff



 Comments   
Comment by Alex Miller [ 29/Aug/14 3:19 PM ]

1) Not sure about losing chunked seqs - that would make older usage slower, which seems undesirable.
2) RangeIterator.next() needs to throw NoSuchElementException when walking off the end
3) I think Range should implement IReduce instead of relying on support for CollReduce via Iterable.
4) Should let _hash and _hasheq auto-initialize to 0 not set to -1. As is, I think _hasheq always would be -1?
5) _hash and _hasheq should be transient.
6) count could be cached (like hash and hasheq). Not sure if it's worth doing that but seems like a win any time it's called more than once.
7) Why the change in test/clojure/test_clojure/serialization.clj ?
8) Can you squash into a single commit?

Comment by Timothy Baldridge [ 29/Aug/14 3:40 PM ]

1) I agree, adding chunked seqs to this will dramatically increase complexity, are we sure we want this?
2) exception added
3) I can add IReduce, but it'll pretty much just duplicate the code in protocols.clj. If we're sure we want that I'll add it too.
4) fixed hash init values, defaults to -1 like ASeq
5) hash fields are now transient
6) at the cost of about 4 bytes we can cache the cost of a multiplication and an addition, doesn't seem worth it?
7) the tests in serialization.clj assert that the type of the collection roundtrips. This is no longer the case for range which starts as Range and ends as a list. The change I made converts range into a list so that it properly roundtrips. My assumption is that we shouldn't rely on all implementations of ISeq to properly roundtrip through EDN.
8) squashed.

Comment by Alex Miller [ 29/Aug/14 3:49 PM ]

6) might be useful if you're walking through it with nth, which hits count everytime, but doubt that's common
7) yep, reasonable

Comment by Andy Fingerhut [ 18/Sep/14 6:52 AM ]

I have already pointed out to Edipo in personal email the guidelines on what labels to use for Clojure JIRA tickets here: http://dev.clojure.org/display/community/Creating+Tickets

Comment by Timothy Baldridge [ 19/Sep/14 10:02 AM ]

New patch with IReduce directly on Range instead of relying on iterators

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

The new patch looks good. Could you do a test to determine the perf difference from walking the old chunked seq vs the new version? If the perf diff is negligible, I think we can leave as is.

Another idea: would it make sense to have a specialized RangeLong for the (very common) case where start, end, and step could all be primitive longs? Seems like this could help noticeably.

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

Looks like chunked seqs do make lazy seq code about 5x faster in these tests.

Comment by Ghadi Shayban [ 03/Oct/14 10:22 AM ]

I think penalizing existing code possibly 5x is a hard cost to stomach. Is there another approach where a protocolized range can live outside of core? CLJ-993 has a patch that makes it a reducible source in clojure.core.reducers, but it's coll-reduce not IReduce, and doesn't contain an Iterator. Otherwise we might have to take the chunked seq challenge.

Alex: Re long/float. Old reified Ranged.java in clojure.lang blindly assumes ints, it would be nice to have a long vs. float version, though I believe the contract of reduce boxes numbers. (Unboxed math can be implemented very nicely as in Prismatic's Hiphip array manipulation library, which takes the long vs float specialization to the extreme with different namespaces)

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

I don't think anyone is suggesting we push unboxed math all the way down through transducers. Instead, this patch contains a lot of calls to Numbers.*, if we were to assume that the start end and step params of range are all Longs, then we could remove all of these calls and only box when returning an Object (in .first) or when calling IFn.invoke (inside .reduce)

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

I agree that 5x slowdown is too much - I don't think we can give up chunked seqs if that's the penalty.

On the long case, I was suggesting what Tim is talking about, in the case of all longs, create a Range that stores long prims and does prim math, but still return boxed objects as necessary. I think the only case worth optimizing is all longs - the permutation of other options gets out of hand quickly.

Comment by Ghadi Shayban [ 03/Oct/14 11:00 AM ]

Tim, I'm not suggesting unboxed math, but the singular fast-path of all-Longs that you and Alex describe. I mistakenly lower-cased Long/Float.





[CLJ-1568] Incorrect error locations reported in the stacktrace Created: 19/Oct/14  Updated: 22/Oct/14

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

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

Attachments: Text File 0001-CLJ-1568-fix-incorrect-error-locations.patch    
Patch: Code
Approval: Triaged

 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: 0001-CLJ-1568-fix-incorrect-error-locations.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.





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

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

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

Attachments: Text File kwinvoke.patch    
Patch: Code

 Description   

Summary: Don't emit KeywordLookup thunks and machinery for namespaced keyword access

Description: When the compiler sees a keyword at the beginning of a sexpr, (:foo x), it emits some machinery that takes into account that 'x' could be a defrecord with a defined 'foo' field. This exists to fast-path it into a field lookup. Here is the supporting code from the target defrecord: https://github.com/clojure/clojure/blob/master/src/clj/clojure/core_deftype.clj#L185-L198
The compiler currently emits the same machinery for (:foo/bar x), a namespaced keyword access, but defrecords don't have any fast path field access for that. This trivial patch turns that scenario into a normal invocation.

Here is the disassembly for (fn [x] (:foo/bar x))
https://gist.github.com/anonymous/d94fc56fba4a1665f73f

There are two static fields on the IFn also for every kw access.

With the trivial patch, it turns into a normal invoke. (emit the fn aka the namespaced keyword, then the args Aka the target, and call IFn invoke: kw.invoke(target))






[CLJ-1561] Incorrect line numbers are emitted Created: 10/Oct/14  Updated: 22/Oct/14

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

Type: Defect Priority: Major
Reporter: Paul Stadig Assignee: Unassigned
Resolution: Unresolved Votes: 22
Labels: errormsgs

Attachments: Text File 0001-Mark-line-number-after-emitting-children.patch     Text File 0002-Mark-line-number-after-emitting-children.patch    
Patch: Code
Approval: Triaged

 Description   

The Clojure JVM compiler marks the line number for a form before emitting the children for that form. Marking the line number before emitting children leads to incorrect line numbers when a runtime error occurs. For example, when

 (foo bar
      baz)

is emitted the compiler will visit the line number for the expression, then emit the children expressions ('bar' and 'baz') which will mark their own line numbers, then come back and emit the invoke bytecode for 'foo', but since the last line number to be marked was that of 'baz', if 'foo' throws an exception the line number of 'baz' will be reported instead of the line number for the expression as a whole.

This same issue was being manifested with special forms and inlined functions, and was especially bad in the case of the threading macro '->', because it is usually spread across several lines, and the line number reported could end up being very different than the line actually causing an exception.

A demonstration of the incorrect line numbers (and how the fix affects line numbers) can be seen here https://github.com/pjstadig/clojure-line-numbers



 Comments   
Comment by Jozef Wagner [ 10/Oct/14 1:57 PM ]

additions in your patch mixes tabs and spaces. Could you please update the patch so that your added lines indent only with tab characters? Not everyone has tab set at 4 spaces...

Comment by Paul Stadig [ 10/Oct/14 2:42 PM ]

There's already a mixture of just tabs, just spaces, and tabs & spaces in Compiler.java. I'm not sure what the "standard" is, but I've changed the patch to match the surrounding lines.

Comment by Paul Stadig [ 10/Oct/14 2:42 PM ]

Patch with whitespace changes.

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

These changes will affect the line number tables for a variety of Clojure constructs when compiled. It would be very helpful to me to have a set of examples that covered each case touched in the patch so that I could compile them and look at the bytecode vs the source. This would greatly accelerate the screening process.

Comment by Paul Stadig [ 20/Oct/14 2:29 PM ]

Alex,
I have created a repo on github that has a sample file demonstrating the line number changes.

https://github.com/pjstadig/clojure-line-numbers

Hope that helps!

BTW, I'd be glad to do a skype call or hangout, if you have questions.

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

This is very helpful, thanks!!

Comment by Alex Miller [ 22/Oct/14 11:35 AM ]

In the hunk at 3191 in KeywordInvokeExpr, a call to visitLineNumber was added, but the prior call 4 lines earlier was not removed. Should it be?

Comment by Paul Stadig [ 22/Oct/14 12:05 PM ]

I left that in thinking that if something goes wrong with the getstatic instruction (null pointer exception? class cast exception?) it should report the line number of the KeywordInvokeExpr. It may be that there isn't a realistic possibility that anything could actually happen with that getstatic instruction, but that was the thought process.

My general rule of thumb was if an emit method emits any instructions before it calls the emit method on another expr, then it should mark its line number before and after the recursive emit call (assuming that the recursive emit call would mark its own line number). In cases where an emit method immediately calls another emit method, then I don't bother to mark a line number until afterwards.





[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-1569] transduce does not respect the init arity of transducers Created: 19/Oct/14  Updated: 20/Oct/14  Resolved: 20/Oct/14

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

Type: Defect Priority: Minor
Reporter: Daniel James Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: transducers


 Description   

Note: I initially raised this issue for discussion on the mailing list
https://groups.google.com/d/msg/clojure/uVKP4_0KMwQ/-oUJahvUarIJ

transduce and other transducible processes currently ignore the 'init' arity of transducers. The currently implementation of transduce takes the 'init' from the reducing function before being transformed by the transducer, rather the reducing function after being transformed.

The current implementation of transduce is equivalent to the following (simplified for exposition purposes):

Current implementation of transduce
(defn transduce
  ([xform f coll]
     (transduce xform f (f) coll))
  ([xform f init coll]
     (let [rf (xform f)]
       (rf (reduce rf init coll)))))

The arity 3 case uses (f) to construct the seed value of the reduction. The arity 4 case uses the explicitly provided seed, init.

I would like to propose an alternate implementation of transduce, one which makes use of the transducer when seeding the reduction.

Proposed implementation of transduce
(defn alt-transduce
  ([xform f coll]
     (let [rf (xform f)]
       (rf (reduce rf (rf) coll))))
  ([xform f init coll]
     (let [rf (xform
               (fn
                 ([] init)
                 ([result] (f result))
                 ([result input] (f result input))))]
       (rf (reduce rf (rf) coll)))))

Now, the arity 3 case uses (xform f) to construct the seed value of the reduction. The arity 4 case combines both f and init into a new reducing function that is given to xform. Both of these ensure that the init arity of the transducer is used.

As into is implemented in terms of transduce, it is also taken care of. However, sequence is separate, and would also have to be tweaked to respect the init arity.



 Comments   
Comment by Daniel James [ 19/Oct/14 1:24 PM ]

As a small addition, I just wanted to point out an example of where the current implementation raised curiosity:
https://groups.google.com/d/msg/clojure/M-13lRPfguc/IspgdpKDaGsJ

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

In transduce, the transducer is applied to the elements of the input and should not be entangled with the accumulation at all (either in initializing it or the act of accumulation). f is the final reducing function that deals with accumulation and initialization.

Comment by Daniel James [ 20/Oct/14 10:00 AM ]

Hi Alex,

I feel that you've misunderstood my proposal.

Could you explain how you consider

(defn init-with [x]
  (fn [rf]
    (fn
      ([] (rf (rf) x))
      ([result] (rf result))
      ([result input] (rf result input)))))

to be “entangled with the accumulation at all (either in initializing it or the act of accumulation).”

This seems like a completely legitimate transducer to me. It makes use of the init arity, while remaining oblivious to the accumulation.

Your explanation also seems to be at odds with

http://clojure.org/transducers

The inner function is defined with 3 arities used for different purposes:

  • Init (arity 0) - in most cases, this will just call the init arity on the nested transform xf, which will eventually call out to the transducing process to supply an initial value. It is also a place to establish the initial reducing state for the transducer.
Comment by Alex Miller [ 20/Oct/14 11:57 AM ]

By "entangling" I mean that in your alternate transduce you invoke the xform to obtain the initial value: ((xform f)) instead of (f). Transducers should not know about or be involved in the accumulating process.

The transducers page is in error and I will correct it (I wrote it; the error is mine).

Comment by Daniel James [ 20/Oct/14 3:25 PM ]

Ok, at the risk of belaboring the point (I have enough self-awareness to realized that I am probably about to do exactly that…) I feel that you are still missing something here.

Permit me to try one more time to explain my position.

Consider map

the map transducer
(defn map [f]
  (fn [rf]
    (fn
      ([] (rf))
      ([result] (rf result))
      ([result input] (rf result (f input))))))

It defines all three arities, init, step, and completion. It doesn’t have anything to do in init arity, and so the only thing it can do is “call the init arity on the nested transform rf, which will eventually call out to the transducing process.” (taken from your update to http://clojure.org/transducers)

Saying that transducers should not be involved in the accumulating process has the right spirit, but you are missing something. It is involved, but in a strictly constrained way. The transducer’s responsibility is to carefully thread the accumulator value around. Sure, it should not know what the value is, or what type it has, but it is still there. Every arity of map has access to it! In the init arity, map delegates to rf to construct it. In the completion arity, map has the result, but the only valid thing it can do with it is to pass it on to rf. Again, in the step arity, map has the result, and again the only legitimate thing it can do with it is to thread to through to rf.

Now consider the identity transducer:

the identity transducer
(def identity
  (fn [rf]
    ([] (rf))
    ([result] (rf result))
    ([result input] (rf result input))))

This is a transducer in its purest form. All it has to do is correctly thread the accumulation value around. It doesn’t and shouldn’t know any details of what that value is, nonetheless, it still has the responsibility of threading that value correctly.

In each arity the identity transducer does the ‘trivial’ thing. In my post to the mailing list, I illustrated three example of transducers that do something beyond the trivial thing in each of the three arities. (I’ll copy them here for completeness.)

non trivial threading of the accumulator in the init arity
(defn init-with
  [x]
  (fn [rf]
    (fn
      ([] (rf (rf) x))
      ([result] (rf result))
      ([result input]
         (rf result input)))))
non trivial threading of the accumulator in the completion arity
(defn complete-with
  [x]
  (fn [rf]
    (fn
      ([] (rf))
      ([result]
         (rf (rf result x)))
      ([result input]
         (rf result input)))))
non trivial threading of the accumulator in the step arity
(defn dupl
  []
  (fn [rf]
    (fn
      ([] (rf))
      ([result] (rf result))
      ([result input]
         (rf (rf result input)
             input)))))

I would consider all of these to be perfectly valid transducers. However, unless I’ve misunderstood, you appear to be taking issue with init-with. If so, I’m very curious as to why!

a closer look at the init arity of init-with
(defn init-with
  [x]
  (fn [rf]
    (fn
      ([] (rf (rf) x))
      ...

Rather than just delegating to (rf), it threads that value immediately into rf with (rf (rf) x). So I don’t agree at all that any of these, init-with, complete-with, or dupl, are “entangled” with the accumulation value or the accumulation process. They are completely oblivious to both its value and its type!

So, returning to transduce,

the first case of an alternate transduce
(defn alt-transduce
  ([xform f coll]
     (let [rf (xform f)]
       (rf (reduce rf (rf) coll))))
  ...

A valid transducer is one that threads the accumlation value correctly. Therefore, ((xform f)) is (f) threaded through xform. All the transducers in clojure.core have the trivial ([] (rf)), so ((xform f)) built from these core transducers degenerates into (identity (f)).
However, as transduce, into, and sequence never even invoke the init arity, it begs the question, why even require that transducers have that arity in the first place? Personally, I think that init arity is great as it enables a transducer such as init-with (while remaining stateless), but that requires transducible processes to actually make use of the init arity! Hence why I raised this issue.
It seems troubling to me that complete-with works perfectly fine in the current framework, yet init-with, its dual, does not.

I recognize that the various discussions around ‘typing transducers’ have made various approximations at elucidating the properties of transducers, but I feel strongly that the discussions around rank-2 polymorphism have some bearing on exactly this issue. In fact, it says rather a lot about correctly threading the accumulation value throught transducers without ever “entangling” it in the precise accumulation process of where a transducer is being used.

And on this, it appears that Rich Hickey agrees: “The rank-2 type in particular captures an important property.” (http://conscientiousprogrammer.com/blog/2014/08/07/understanding-cloure-transducers-through-types/#comment-1533318972) Maybe I’ve got him all wrong, but as of right now I’m pretty convinced I don’t. Still, I’m willing to be convinced otherwise

Comment by Alex Miller [ 20/Oct/14 10:03 PM ]

Rich asked me to decline the ticket because the init arity of the xform should not be involved in the reducing function accumulation.

Comment by Daniel James [ 20/Oct/14 10:34 PM ]

Ok, as you can guess I’m a little perplexed by that design choice, but I’ll accept it.

I’d appreciate any further insight you can offer on why this design choice has been taken.
Is the init arity simply a case of compatibility, despite it not being used? Is this a case of attempting to prevent the transducer writer from erroneously corrupting a transducible process? Is init-with actually actually considered to be an invalid transducer, and thus the only way to implement something equivalent would be as a stateful transducer?





[CLJ-1570] Core clojure code mixes tabs with spaces Created: 20/Oct/14  Updated: 20/Oct/14

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

Type: Defect Priority: Trivial
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

A handful of functions in clojure.core, clojure.core-proxy, clojure.inspector, clojure.xml, clojure.pprint, clojure.stacktrace, clojure.set, and clojure.test switch partway through from indenting with spaces to indenting with tabs. This may cause them to display incorrectly depending on how the developer's editor is configured.

(not sure if this should be marked defect or task)



 Comments   
Comment by Andy Fingerhut [ 20/Oct/14 1:41 PM ]

Some similarities to CLJ-1026, although this problem does not cause the same issues with warnings on git patches as CLJ-1026 does, as far as I know.

One similarity is that if it is of interest (I don't know if it is), Alex or other Clojure screeners may want a procedure to clean them all up, and perhaps repeat that process periodically, e.g. before each major release.





[CLJ-1567] Unused local in clojure.core/condp definition Created: 17/Oct/14  Updated: 20/Oct/14

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

Type: Enhancement Priority: Trivial
Reporter: Jan Krajicek Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: ft, newbie

Attachments: Text File 0001-Remove-unused-local-in-clojure.core-condp.patch    
Patch: Code
Approval: Triaged

 Description   

The 'gres' local in clojure.core/condp definition is not used:

https://github.com/clojure/clojure/blob/eccff113e7d68411d60f7204711ab71027dc5356/src/clj/clojure/core.clj#L6071

Screened by: Alex Miller



 Comments   
Comment by Bozhidar Batsov [ 19/Oct/14 12:07 AM ]

Patch added.





[CLJ-1566] Documentation for clojure.core/require does not document :rename Created: 16/Oct/14  Updated: 19/Oct/14

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

Type: Defect Priority: Minor
Reporter: James Laver Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File refer.patch    
Patch: Code

 Description   

By contrast, clojure.core/use does mention :rename.

I attach a patch



 Comments   
Comment by Andy Fingerhut [ 16/Oct/14 1:33 PM ]

James, your patch removes any mention of the :all keyword, and that keyword is not mentioned in the doc string for clojure.core/refer.

I haven't checked whether refer can take :all as an argument, but clojure.core/require definitely can.

Comment by James Laver [ 16/Oct/14 1:39 PM ]

Ah, you're quite right. Fixed now. See updated patch in a sec.

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

For sake of reduced confusion, it would be better if you could either name your patches differently, or delete obsolete ones with identical names as later ones. JIRA allows multiple patches to have the same names, without replacing the earlier ones.

Comment by James Laver [ 17/Oct/14 12:44 AM ]

Okay, that's done. The JIRA interface is a bit tedious in places.

Comment by Bozhidar Batsov [ 19/Oct/14 1:34 AM ]

Seems to me the sentence should end with a dot.

Comment by James Laver [ 19/Oct/14 4:36 AM ]

Added a dot.





[CLJ-899] Accept and ignore colon between key and value in map literals Created: 18/Dec/11  Updated: 19/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: reader


 Description   

Original title was 'treat colons as whitespace' which isn't a problem description but a (flawed) implementation approach

For JSON compatibility
known problems when no spaces - x:true and y:false



 Comments   
Comment by Tassilo Horn [ 23/Dec/11 3:22 AM ]

Discussed here: https://groups.google.com/d/msg/clojure/XvJUzaY1jec/l8xEwlFl8EUJ

Comment by Kevin Downey [ 11/Jan/12 2:23 PM ]

please no

Comment by Tavis Rudd [ 16/Jan/12 12:17 PM ]

Alan Malloy raises a good point in the google group discussion (https://groups.google.com/d/msg/clojure/XvJUzaY1jec/aVpWBicwGhsJ) about accidental confusion between trailing (or floating) and leading colons:
"It isn't even as simple as "letting them
be whitespace", because presumably you want (read-string "{a: b}") to
result in (hash-map 'a 'b), but (read-string "{a :b}") to result in
(hash-map 'a :b)."

This issue could be avoided by only treating a colon as whitespace when followed by a comma. As easy cut-paste of json seems the be the key motivation here, the commas are going to be there anyway: valid {"v":, 1234} vs syntax error {a-key: should-be-a-keyword}.

Comment by Alex Baranosky [ 16/Jan/12 5:23 PM ]

This would be visually confusing imo.

Comment by Laurent Petit [ 17/Jan/12 5:01 PM ]

Please, oh please, no.

Comment by Tavis Rudd [ 18/Jan/12 2:40 PM ]

Er, brain fart. I was typing faster than I was thinking and put the comma in the wrong place. In my head I meant the form following the colon would have to have a comma after it. Thus, {"a-json-key": 1234, ...} would be valid while {"a-json-key": was-supposed-to-be-a-keyword "another-json-key" foo} would complain about the colon being an Invalid Token. I don't see the need for it, however.

Comment by Joseph Smith [ 27/Feb/12 10:55 AM ]

Clojure already has reader syntax for a map. If we support JSON, do we also support ruby map literals? Seems like this addition would only add confusion, imo, given colons are used in keywords and keywords are frequently used in maps - e.g., when de-serializing from XML, or even JSON.

Comment by David Nolen [ 27/Feb/12 11:19 AM ]

Clojure is no longer a language hosted only on the JVM. Clojure is also hosted on the CLR, and JavaScript. In particular ClojureScript can't currently easily deal with JSON literals - an extremely common (though problematic) data format. By allowing colon whitespace in map literals - Clojure data structures can effectively become an extensible JSON superset - giving the succinctness of JSON and the expressiveness of XML.

+1 from me.

Comment by Tim McCormack [ 13/Nov/12 7:27 PM ]

Clojure is only hosted on the JVM; ClojureScript is hosted on JS VMs. If this is useful for CLJS, it should just be a CLJS feature.

Comment by Mike Anderson [ 10/Dec/12 11:51 PM ]

-1 for this whole idea: that way madness lies....

If we keep adding syntactical oddities like this then the language will become unmaintainably complex. It's the exact opposite of simple to have lots of special cases and ambiguities that you have to remember.

If people want to use JSON that is fine, but then the best approach use a specific JSON parser/writer, not just paste it into Clojure source and expect it to work.

Comment by Laszlo Török [ 11/Dec/12 4:54 AM ]

-1 for reasons mentioned by Allan Malloy and Mike Anderson

Comment by Bozhidar Batsov [ 19/Oct/14 3:06 AM ]

-1 Don't repeat the mistake made in Ruby...





[CLJ-1078] Add queue and queue? to clojure.core Created: 26/Sep/12  Updated: 19/Oct/14

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

Type: Enhancement Priority: Minor
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: data-structures, queue

Attachments: File clj-1048-add-queue-functions.diff     Text File queue.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Add queue function to create queues from collections and queue? predicate to check queueness.

Patch: clj-1048-add-queue-functions.diff



 Comments   
Comment by Andy Fingerhut [ 28/Sep/12 8:43 AM ]

Timothy, I tried applying both of these Sep 26, 2012 patches to latest Clojure master as of that date. I had to apply 0001-make-PersistentQueue-ctor-public.patch by hand since it failed to apply using git or patch. It built fine, but failed to pass several of the Clojure tests. Have you looked into those test failures to see if you can find the cause and fix them? I tested on Ubuntu 11.10 with Oracle JDK 1.6 and 1.7, and saw similar failures with both.

Comment by Timothy Baldridge [ 26/Oct/12 5:23 PM ]

Fixed the patch. Tests pass, created the patch, applied it to a different copy of the source and the tests still pass. So this new patch should be good to go.

Comment by Andy Fingerhut [ 26/Oct/12 5:43 PM ]

Timothy, I'm not sure how you are getting successful results when applying this patch. Can you try the steps below and see what happens for you? I get errors trying to apply the patch with latest Clojure master as of Oct 26, 2012. Also please use the steps on the JIRA workflow page to create a git format patch (http://dev.clojure.org/display/design/JIRA+workflow under "Development" heading).

% git clone git://github.com/clojure/clojure.git
% cd clojure
% patch -p1 < queues.patch
patching file src/clj/clojure/core.clj
patching file src/jvm/clojure/lang/PersistentQueue.java
Hunk #1 FAILED at 32.
1 out of 1 hunk FAILED – saving rejects to file src/jvm/clojure/lang/PersistentQueue.java.rej
patching file test/clojure/test_clojure/data_structures.clj
Hunk #1 succeeded at 123 with fuzz 2.
Hunk #2 succeeded at 861 with fuzz 2.
Hunk #3 FAILED at 872.
1 out of 3 hunks FAILED – saving rejects to file test/clojure/test_clojure/data_structures.clj.rej
patching file test/clojure/test_clojure/java_interop.clj

Comment by Timothy Baldridge [ 26/Oct/12 6:08 PM ]

I was using git apply. I tried the method you show above, and now I'm seeing the same issues you show above.

Comment by Andy Fingerhut [ 26/Oct/12 6:26 PM ]

Just so you know, the preferred way to create and apply patches are the "git format-patch master --stdout > patch.txt" to create a patch (after doing the branching commands described on the JIRA workflow page to create a branch for your changes), and the "git am --keep-cr -s < patch.txt" to apply a patch. If a patch was created that way and applies cleanly with that command, then you are definitely good to go.

The "patch -p1 < patch.txt" command is just a secondary method sometimes used to try to apply patches that aren't in the format produced above, or have errors when applying using that method.

Comment by Timothy Baldridge [ 26/Oct/12 9:15 PM ]

Just so you know, the preferred way to create and apply patches are the "git format-patch master --stdout > patch.txt" to create a patch (after doing the branching commands described on the JIRA workflow page to create a branch for your changes), and the "git am --keep-cr -s < patch.txt" to apply a patch. If a patch was created that way and applies cleanly with that command, then you are definitely good to go.

The "patch -p1 < patch.txt" command is just a secondary method sometimes used to try to apply patches that aren't in the format produced above, or have errors when applying using that method.

Comment by Timothy Baldridge [ 26/Oct/12 9:16 PM ]

added patch

Comment by Andy Fingerhut [ 26/Oct/12 9:37 PM ]

That one applies cleanly and passes all tests. It should show up on the next list of prescreened patches. Thanks.

Comment by Rich Hickey [ 29/Nov/12 9:54 AM ]

we don't use the queue* convention elsewhere, e.g. vec and vector. I think queue should take a collection like vec and set. (queue [1 2 3]) could be made to 'adopt' the collection as front.

Comment by Andy Fingerhut [ 11/Dec/12 1:00 PM ]

Patch queue.patch dated Oct 26 2012 no longer applies cleanly after recent CLJ-1000 commits, but only because of one line of changed patch context. It still applies cleanly with "patch -p1 < queue.patch". Not bothering to update the stale patch given Rich's comments suggesting more substantive changes.

Comment by Steve Miner [ 06/Apr/13 8:06 AM ]

See also CLJ-976 (tagged literal support for PersistentQueue)

Comment by John Jacobsen [ 23/May/13 8:54 PM ]

Don't want to step on Timothy B's toes here, but it looks straightforward to adopt his patch to implement Rich's suggestion. I'd offer to give it a whack if nobody else wants the ticket now.

Comment by John Jacobsen [ 26/May/13 9:04 AM ]

Discussion initiated on clojure-dev: https://groups.google.com/forum/?fromgroups#!topic/clojure-dev/2BOqHm24Vc4

Comment by John Jacobsen [ 31/May/13 9:58 AM ]

This patch (if accepted) supersedes Timothy Baldridge's patch; it implements "queue" and "queue?" (but not "queue*"); "queue" accepts a collection rather than being a variadic function, as per Rich's suggestion.

Comment by Andy Fingerhut [ 30/Jan/14 5:00 PM ]

The patch clj-1048-queue-takes-collections.diff applied cleanly to latest Clojure master as of Jan 23 2014, but not on Jan 30 2014. There were several commits made to Clojure during that week involving updating the hash functions that conflict in some way with this patch. I have not checked to see how easy or difficult it might be to update the patch.

Comment by John Jacobsen [ 05/Feb/14 1:45 PM ]

Hi Andy, I updated the patch and removed my previous version. The new one should apply cleanly and pass all tests.

Comment by John Jacobsen [ 05/Feb/14 2:24 PM ]

Updated ticket title.

Comment by Alex Miller [ 05/Feb/14 5:33 PM ]

Hi John... Can you condense these changes into a single commit? Please also remove the comments above queue* in java_interop.clj. Thanks...

Comment by John Jacobsen [ 05/Feb/14 6:55 PM ]

Hi Alex, the updated patch removes that comment and rebases all three commits into c9f77dd. Let me know if you need anything else. Thanks!

Comment by Bozhidar Batsov [ 19/Oct/14 3:00 AM ]

A tiny remark - I think the docstrings should end with a dot.





[CLJ-1527] Harmonize accepted / documented symbol and keyword syntax over various readers Created: 18/Sep/14  Updated: 19/Oct/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: 6
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



 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.





Generated at Sat Oct 25 04:12:58 CDT 2014 using JIRA 4.4#649-r158309.