<< Back to previous view

[CLJ-1544] AOT bug involving protocols Created: 01/Oct/14  Updated: 01/Oct/14

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

Type: Defect Priority: Major
Reporter: Allen Rohner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: aot, protocols

Approval: Triaged

 Description   

See the example in this repo:

https://github.com/arohner/clj-aot-repro

Note that the test, `lein compile aot-repro.main aot-repro.main` seems like a pathological case, I've seen the same behavior in 'real' repos by just running `lein uberjar`. I haven't found the minimal testcase for 'normal' usage, but the errors I see are the same.

The bug seems to involve:

  • AOT
  • defining a protocol
  • getting the protocol-defining namespace loaded before compilation starts.





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

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

Type: Defect Priority: Major
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: compiler, typehints

Attachments: Text File 0001-auto-qualify-arglists-class-names.patch     Text File 0001-throw-on-non-qualified-class-names-that-are-not-auto.patch    
Patch: Code
Approval: Triaged

 Description   

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

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

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

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

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

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



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

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

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

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

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

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

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

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

Comment by Andy Fingerhut [ 10/Jun/14 3:13 PM ]

I would suggest something like the following for a documentation change, after this part of the text on the page Alex links in the previous comment:

For function return values, the type hint can be placed before the arguments vector:

(defn hinted
(^String [])
(^Integer [a])
(^java.util.List [a & args]))

-> #user/hinted

If the return value type hint is for a class that is outside of java.lang, which is the part auto-imported by Clojure, then it must be a fully qualified class name, e.g. java.util.List, not List.

Comment by Nicola Mometto [ 10/Jun/14 4:02 PM ]

I don't understand why we should enforce this complexity to the user.
Why can't we just make the Compiler (or even defn itself) update all the arglists tags with properly resolved ones? (that's what I'm doing in tools.analyzer.jvm)

Comment by Alexander Kiel [ 19/Jul/14 10:02 AM ]

I'm with Nicola here. I also think that defn should resolve the type hint according the imports of the namespace defn is used in.

Comment by Max Penet [ 22/Jul/14 7:06 AM ]

Same here, I was bit by this in the past. The current behavior is clearly counterintuitive.

Comment by Nicola Mometto [ 28/Aug/14 12:58 PM ]

Attached two patches implementing two different solutions:

  • 0001-auto-qualify-arglists-class-names.patch makes the compiler automatically qualify all the tags in the :arglists
  • 0001-throw-on-non-qualified-class-names-that-are-not-auto.patch makes the compiler throw an exception for all public defs whose return tag is a symbol representing a non-qualified class that is not in the auto-import list (approach proposed in IRC by Alex Miller)
Comment by Tassilo Horn [ 29/Aug/14 1:49 AM ]

For what it's worth, I'd prefer the first patch because the second doesn't help in situations where the caller lives in a namespace where the called function's return type hinted class is `ns-unmap`-ed. And there a good reasons for doing that. For example, Process is a java.lang class and Process is a pretty generic name. So in some namespace, I want to define my own Process deftype or defrecord. Without unmapping 'Process first to get rid of the java.lang.Process auto-import, I'd get an exception:

user> (deftype Process [])
IllegalStateException Process already refers to: class java.lang.Process in namespace: user  clojure.lang.Namespace.referenceClass (Namespace.java:140)

Now when I call some function from some library that has a `^Process` return type hint (meaning java.lang.Process there), I get the same exception as in my original report.

I can even get into troubles when only using standard Clojure functions because those have `^String` and `^Class` type hints. IMO, Class is also a pretty generic name I should be able to name my custom deftype/defrecord. And I might also want to have a custom String type/record in my astrophysics system.

Comment by Andy Fingerhut [ 30/Sep/14 4:39 PM ]

Not sure whether the root cause of this behavior is the same as the example in the description or not, but seems a little weird that even for fully qualified Java class names hinting the arg vector, it makes a difference whether it is done with defn or def:

Clojure 1.6.0
user=> (set! *warn-on-reflection* true)
true
user=> (defn f1 ^java.util.LinkedList [coll] (java.util.LinkedList. coll))
#'user/f1
user=> (def f2 (fn ^java.util.LinkedList [coll] (java.util.LinkedList. coll)))
#'user/f2
user=> (.size (f1 [2 3 4]))
3
user=> (.size (f2 [2 3 4]))
Reflection warning, NO_SOURCE_PATH:5:1 - reference to field size can't be resolved.
3
Comment by Alex Miller [ 30/Sep/14 6:21 PM ]

Andy, can you file that as a separate ticket?

Comment by Andy Fingerhut [ 30/Sep/14 9:08 PM ]

Created ticket CLJ-1543 for the issue raised in my comment earlier on 30 Sep 2014.

Comment by Andy Fingerhut [ 01/Oct/14 12:38 PM ]

Tassilo (or anyone), is there a reason to prefer putting the tag on the argument vector in your example? It seems that putting it on the Var name instead avoids this issue:

user=> (clojure-version)
"1.6.0"
user=> (set! *warn-on-reflection* true)
true
user=> (import '(java.util List))
java.util.List
user=> (defn ^List linkedlist [] (java.util.LinkedList.))
#'user/linkedlist
user=> (.size (linkedlist))
0
user=> (ns user2)
nil
user2=> (refer 'user)
nil
user2=> (.size (linkedlist))
0

I suppose that only allows a single type tag, rather than an independent one for each arity.





[CLJ-1499] Replace seq-based iterators with direct iterators for all non-seq collections that use SeqIterator Created: 08/Aug/14  Updated: 01/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: Rich Hickey Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File clj-1499-all.diff     File clj-1499-v2.diff     File clj-1499-v3.diff     Text File defrecord-iterator.patch     File defrecord-iterator-v2.diff    
Patch: Code and Test
Approval: Vetted

 Description   

Add support for direct iterators instead of seq-based iterators for non-seq collections that use SeqIterator.

Patch adds support for direct iterators on the following (removing use of SeqIterator):

  • PersistentHashMap - new internal iterator (~20% faster)
  • APersistentSet - use internal map impl iterator (~5% faster)
  • PersistentQueue
  • PersistentStructMap
  • records (in core_deftype.clj)

Patch does not change use of SeqIterator in:

  • LazyTransformer$MultiStepper (not sure if this could be changed)
  • ASeq
  • LazySeq

Patch: clj-1499-all.diff



 Comments   
Comment by Alex Miller [ 13/Aug/14 1:57 PM ]

The list of non-seqs that uses SeqIterator are:

  • records (in core_deftype.clj)
  • APersistentSet - fallback, maybe is ok?
  • PersistentHashMap
  • PersistentQueue
  • PersistentStructMap

Seqs (that do not need to be changed) are:

  • ASeq
  • LazySeq.java

LazyTransformer$MultiStepper - not sure

Comment by Ghadi Shayban [ 27/Sep/14 2:16 PM ]

attached iterator impl for defrecords. ready to leverage iteration for extmap when PHM iteration lands.

Comment by Alex Miller [ 29/Sep/14 12:52 PM ]

PHM patch

Comment by Alex Miller [ 29/Sep/14 10:26 PM ]

New patch that fixes bugs with PHMs with null keys (and added tests to expose those issues), added support for PHS.

Comment by Ghadi Shayban [ 29/Sep/14 10:45 PM ]

Alex, the defrecord patch already uses the iterator for extmap. It's just made better by the PHM patch.

Comment by Alex Miller [ 29/Sep/14 10:47 PM ]

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

Heh. Skate to where the puck is going to be – Gretzky

Re: defrecord iterator: As is, it propagates exceptions from reaching the end of the ExtMap's iterator. As noted in CLJ-1453, PersistentArrayMap's iterator improperly returns an ArrayIndexOutOfBoundsException, rather than NoSuchElementException.

Comment by Alex Miller [ 30/Sep/14 6:41 PM ]

Hey Ghadi, rather than rebuilding the case map to pass to the RecordIterator, why don't you just pass the fields in iteration order to it and leverage the case map via .valAt like everything else?

Comment by Ghadi Shayban [ 30/Sep/14 7:30 PM ]

defrecord-iterator-v2.diff reuses valAt and minimizes macrology.





[CLJ-1543] Type tags on argument vector appear to help avoid reflection when used with defn, but not with def foo (fn ...) Created: 30/Sep/14  Updated: 01/Oct/14

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop, typehints


 Description   

I would have expected that both of the Java interop calls below would avoid reflection, but only the first involving f1 does.

Clojure 1.6.0
user=> (set! *warn-on-reflection* true)
true
user=> (defn f1 ^java.util.LinkedList [coll] (java.util.LinkedList. coll))
#'user/f1
user=> (def f2 (fn ^java.util.LinkedList [coll] (java.util.LinkedList. coll)))
#'user/f2
user=> (.size (f1 [2 3 4]))
3
user=> (.size (f2 [2 3 4]))
Reflection warning, NO_SOURCE_PATH:5:1 - reference to field size can't be resolved.
3

Not sure if this has anything to do with CLJ-1232, but was discovered when testing variants of that issue.



 Comments   
Comment by Andy Fingerhut [ 30/Sep/14 9:08 PM ]

What a nice number for a ticket, 1543. The year Copernicus's most celebrated book was published: http://en.wikipedia.org/wiki/Nicolaus_Copernicus

Comment by Jozef Wagner [ 01/Oct/14 4:05 AM ]

Isn't type hinting of arg vector meant only for primitive type hints? AFAIK non-primitive type hints should be on a function name, everything else is non idiomatic.

Comment by Nicola Mometto [ 01/Oct/14 7:05 AM ]

This isn't an issue of arg vector hinting vs function name hinting.
The issue here is that return type hinting cannot be put on anonymous functions but only on defns as the :arglists will be added by defn on the Var's metadata.

This is one of the reasons why I'd like to have that information as a field on the fn rather than as metadata on the Var

Comment by Andy Fingerhut [ 01/Oct/14 10:55 AM ]

Jozef, you may be correct that non-primitive type hints on the argument vector are non idiomatic. Do you have any source for that I could read?





[CLJ-1537] Audit IReduce usages for proper Reduced handling Created: 26/Sep/14  Updated: 30/Sep/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: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File audit-ireduce.diff     File clj-1537-v2.diff    
Patch: Code
Approval: Incomplete

 Description   

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

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

Patch: clj-1537-v2.diff



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

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

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

Should these changes be deref'ing ret?

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





[CLJ-130] Namespace metadata lost in AOT compile Created: 19/Jun/09  Updated: 30/Sep/14

Status: In Progress
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Stuart Sierra Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: aot, metadata

Attachments: Text File 0001-CLJ-130-preserve-metadata-for-AOT-compiled-namespace.patch     File aot-drops-metadata-demo.sh    
Patch: Code
Approval: Triaged

 Description   

AOT-compilation drops namespace metadata.

This also affects all of the namespaces packaged with Clojure, except clojure.core, for which metadata is explicitly added in core.clj.

This behavior was originally reported against Clojure 1.1.0-alpha. Using the attached test script aot-drops-metadata-demo.sh, I can confirm it still exists for Clojure 1.3.0, 1.4.0, and 1.5.1.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 6:45 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/130

Comment by Assembla Importer [ 24/Aug/10 6:45 AM ]

richhickey said: Updating tickets (#127, #128, #129, #130)

Comment by Assembla Importer [ 24/Aug/10 6:45 AM ]

juergenhoetzel said: This is still a issue on

Clojure 1.2.0-master-SNAPSHOT

Any progress, hints? I prefer interactive documentiation via slime/repl

Comment by Howard Lewis Ship [ 09/Sep/14 9:44 AM ]

This is of great concern to me, as the Rook web services framework we're building depends on availability of namespace metadata at runtime.

Comment by Howard Lewis Ship [ 09/Sep/14 9:53 AM ]

BTW, I verified that this still exists in 1.6.0.

Comment by Howard Lewis Ship [ 09/Sep/14 10:11 AM ]

For me personally, I would raise the priority of this issue. And I think in general, anything that works differently with AOT vs. non-AOT should be major, if not blocker, priority.

Comment by Howard Lewis Ship [ 09/Sep/14 10:25 AM ]

Alex Miller:

@hlship I think the question is where it would go. note no one has suggested a solution in last 5 yrs.

Alas, I have not delved into the AOT compilation code (since, you know, I value my sanity). But it seems to me like the __init class for the namespace could construct the map and update the Namespace object.

Comment by Howard Lewis Ship [ 09/Sep/14 4:27 PM ]

Just playing with javap, I can see that the meta data is being assembled in some way, so it's a question of why it is not accessible ...

  public static void __init0();
    Code:
       0: ldc           #108                // String clojure.core
       2: ldc           #110                // String in-ns
       4: invokestatic  #116                // Method clojure/lang/RT.var:(Ljava/lang/String;Ljava/lang/String;)Lclojure/lang/Var;
       7: checkcast     #12                 // class clojure/lang/Var
      10: putstatic     #10                 // Field const__0:Lclojure/lang/Var;
      13: aconst_null
      14: ldc           #118                // String fan.auth
      16: invokestatic  #122                // Method clojure/lang/Symbol.intern:(Ljava/lang/String;Ljava/lang/String;)Lclojure/lang/Symbol;
      19: checkcast     #124                // class clojure/lang/IObj
      22: iconst_4
      23: anewarray     #4                  // class java/lang/Object
      26: dup
      27: iconst_0
      28: aconst_null
      29: ldc           #126                // String meta-foo
      31: invokestatic  #130                // Method clojure/lang/RT.keyword:(Ljava/lang/String;Ljava/lang/String;)Lclojure/lang/Keyword;
      34: aastore
      35: dup
      36: iconst_1
      37: aconst_null
      38: ldc           #132                // String meta-bar
      40: invokestatic  #130                // Method clojure/lang/RT.keyword:(Ljava/lang/String;Ljava/lang/String;)Lclojure/lang/Keyword;
      43: aastore
      44: dup
      45: iconst_2
      46: aconst_null
      47: ldc           #134                // String doc
      49: invokestatic  #130                // Method clojure/lang/RT.keyword:(Ljava/lang/String;Ljava/lang/String;)Lclojure/lang/Keyword;
      52: aastore
      53: dup
      54: iconst_3
      55: ldc           #136                // String Defines the resources for the authentication service.
      57: aastore
      58: invokestatic  #140                // Method clojure/lang/RT.map:([Ljava/lang/Object;)Lclojure/lang/IPersistentMap;
      61: checkcast     #64                 // class clojure/lang/IPersistentMap
      64: invokeinterface #144,  2          // InterfaceMethod clojure/lang/IObj.withMeta:(Lclojure/lang/IPersistentMap;)Lclojure/lang/IObj;

If I'm reading the code correctly, a Symbol named after the namespace is interned, and the meta-data for the namespace is applied to the symbol, so it's just a question of commuting that meta data to the Namespace object. I must be missing something.

Comment by Nicola Mometto [ 30/Sep/14 6:45 PM ]

Attached patch fixes this issue by explicitely attaching the metadata to the namespace after its creation using a .resetMeta call.

Comment by Nicola Mometto [ 30/Sep/14 7:46 PM ]

Here's an explaination of why this bug happens:

  • a namespace inherits the metadata of the symbol used to create that namespace the first time
  • the namespace is created in the load() method, that is invoked after the __init() method
  • the __init0() method creates all the Vars of the namespace
  • interning a Var in a namespace that doesn't exist forces that namespace to be created

This means that the namespace will have been already created (with nil metadata) by the time the load() method gets invoked and thus the call to in-ns will be a no-op and the metadata will be lost.





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

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

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

Attachments: File better-error-messages-for-require.diff     Text File clj-1297-v3.patch    
Patch: Code and Test
Approval: Vetted

 Description   

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

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

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

Proposed:

  • When loading the resource-root of lib throws a FileNotFoundException, the name of the lib is analyzed.
  • If it contains no dash character, throw the original exception (same as current behavior).
  • If it contains a dash character, throw an exception with the same message as the original, except an additional hint is appended to it that perhaps the file should have dash characters replaced with underscores

Patch: clj-1297-v3.patch



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

I would have saved a few hours as well.

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

This patch changes clojure.core/load such that:

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

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

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

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

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

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

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

Comment by Rich Hickey [ 29/Aug/14 9:48 AM ]

I see no reason to fish around in the file system at all. Why can't the message simply remind people that underscores are required and to check that they aren't using dashes?

Comment by Gary Fredericks [ 30/Sep/14 4:43 PM ]

The tradeoff is that fishing around in the file system means the error message is only shown if the user likely made the relevant mistake, whereas simply showing the error message would (if I understand correctly) mean this reminder gets shown for every require error, which I would estimate happens a whole lot more often than the mistake in question.

Comment by Andy Fingerhut [ 30/Sep/14 5:05 PM ]

Gary, there is an exception thrown in any case if the load fails. One approach that I am hacking up now is to add to the existing exception's message that maybe they need to replace - chars in file name with _, but only if the name attempted to be loaded has at least one '-' character in it. So no new output, except in an exception already being thrown, and then only if there is at least a possibility that this is the problem.

Comment by Andy Fingerhut [ 30/Sep/14 5:10 PM ]

clj-1297-v2.patch is similar to the previously proposed patch, but does not touch the file system in any way that wasn't already being done before this patch.

It adds an extra hint to the message of the exception already being thrown if the resource is not found, but only if there is a '-' character in the name that failed to load.

Comment by Andy Fingerhut [ 30/Sep/14 6:45 PM ]

clj-1297-v3.patch is nearly identical to clj-1297-v2.patch, described in the previous comment, except it eliminates an unnecessary let expression.





[CLJ-1515] Reify the result of range Created: 29/Aug/14  Updated: 30/Sep/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: 0
Labels: None

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

 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.

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





[CLJ-1542] Docstring for deliver should describe its return value Created: 30/Sep/14  Updated: 30/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Approval: Triaged

 Description   

It is presumably useful when delivering a promise to know if the delivery was successful or not (where it might be unsuccessful if it was already delivered, perhaps on another thread).

The deliver function seems to currently communicate this by returning a truthy value (the promise itself) on success and a falsy value (nil) on failure. If this is intentional, the docstring should say so so that users can comfortably rely on it.

In CLJ-1038 Rich elected for the docstring to not describe the return value; I'm not sure if that was a reluctance to fully specify the return value (promise vs nil) even if partially describing it (truthy vs falsy) would be okay.






[CLJ-1250] Reducer (and folder) instances hold onto the head of seqs Created: 03/Sep/13  Updated: 30/Sep/14

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

Type: Defect Priority: Major
Reporter: Christophe Grand Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: memory, reducers

Attachments: Text File after-change.txt     Text File before-change.txt     Text File CLJ-1250-08-29.patch     Text File CLJ-1250-08-29-ws.patch     Text File CLJ-1250-20131211.patch     Text File CLJ-1250-AllInvokeSites-20140204.patch     Text File CLJ-1250-AllInvokeSites-20140320.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Problem Statement
A shared function #'clojure.core.reducers/reducer holds on to the head of a reducible collection, causing it to blow up when the collection is a lazy sequence.

Reproduction steps:
Compare the following calls:

(time (reduce + 0 (map identity (range 1e8))))
(time (reduce + 0 (r/map identity (range 1e8))))

The second call should fail on a normal or small heap.

(If reducers are faster than seqs, increase the range.)

Cause: #'reducer closes over a collection when in order to reify CollReduce, and the closed-over is never cleared. When code attempts to reduce over this anonymous transformed collection, it will realize the tail while the head is stored in the closed-over.

Patch
CLJ-1250-08-29.patch

Approach:

Clear the reference to 'this' on the stack just before a tail call occurs

Removes calls to emitClearLocals(), which is a no-op.

When the context is RETURN (indicating a tail call), and the operation
is an InvokeExpr, StaticMethodExpr, or InstanceMethodExpr, clear the
reference to 'this' which is in slot 0 of the locals.

Edge-case: Inside the body of a try block, we cannot clear 'this' at the tail
position as we might need to keep refs around for use in the catch or finally
clauses. Introduces another truthy dynamic binding var to track position being
inside a try block.

Adds two helpers to emitClearThis and inTailCall.

Advantages: Fixes this case with no user code changes. Enables GC to do reclaim closed-overs references earlier.
Disadvantages: A compiler change.

Alternate Approaches:

1) Reimplement the #'reducer (and #'folder) transformation fns similar to the manner that Christophe proposes here:

(defrecord Reducer [coll xf])

(extend-protocol 
  clojure.core.protocols/CollReduce
  Reducer
      (coll-reduce [r f1]
                   (clojure.core.protocols/coll-reduce r f1 (f1)))
      (coll-reduce [r f1 init]
                   (clojure.core.protocols/coll-reduce (:coll r) ((:xf r) f1) init)))

(def rreducer ->Reducer) 

(defn rmap [f coll]
  (rreducer coll (fn [g] 
                   (fn [acc x]
                     (g acc (f x))))))

Advantages: Relatively non-invasive change.
Disadvantages: Not evident code. Additional protocol dispatch, though only incurred once

2) Alternate approach

from Christophe Grand:
Another way would be to enhance the local clearing mechanism to also clear "this" but it's complex since it may be needed to keep a reference to "this" for caches long after obvious references to "this" are needed.

Advantages: Fine-grained
Disadvantages: Complex, invasive, and the compiler is hard to hack on.

Mitigations
Avoid reducing on lazy-seqs and instead operate on vectors / maps, or custom reifiers of CollReduce or CollFold. This could be easier with some implementations of common collection functions being available (like iterate and partition).

See https://groups.google.com/d/msg/clojure-dev/t6NhGnYNH1A/2lXghJS5HywJ for previous discussion.



 Comments   
Comment by Gary Fredericks [ 03/Sep/13 8:53 AM ]

Fixed indentation in description.

Comment by Ghadi Shayban [ 11/Dec/13 11:08 PM ]

Adding a patch that clears "this" before tail calls. Verified that Christophe's repro case is fixed.

Will upload a diff of the bytecode soon.

Any reason this juicy bug was taken off 1.6?

Comment by Ghadi Shayban [ 11/Dec/13 11:17 PM ]

Here's the bytecode for the clojure.core.reducers/reducer reify before and after the change... Of course a straight diff isn't useful because all the line numbers changed. Kudos to Gary Trakhman for the no.disassemble lein plugin.

Comment by Christophe Grand [ 12/Dec/13 6:58 AM ]

Ghadi, I'm a bit surprised by this part of the patch: was the local clearing always a no-op here?

-		if(context == C.RETURN)
+		if(shouldClear)
 			{
-			ObjMethod method = (ObjMethod) METHOD.deref();
-			method.emitClearLocals(gen);
+                            gen.visitInsn(Opcodes.ACONST_NULL);
+                            gen.visitVarInsn(Opcodes.ASTORE, 0);
 			}

The problem with this approach (clear this on tail call) is that it adds yet another special case. To me the complexity stem from having to keep this around even if the user code doesn't refer to it.

Comment by Ghadi Shayban [ 12/Dec/13 7:19 AM ]

Thank you - I failed to mention this in the commit message: it appears that emitClearLocals() belonging to both ObjMethod and FnMethod (its child) are empty no-ops. I believe the actual local clearing is on line 4855.

I agree re: another special case in the compiler.

Comment by Alex Miller [ 12/Dec/13 8:56 AM ]

Ghadi re 1.6 - this ticket was never in the 1.6 list, it has not yet been vetted by Rich but is ready to do so when we open up again after 1.6.

Comment by Ghadi Shayban [ 12/Dec/13 8:59 AM ]

Sorry I confused the critical list with the Rel1.6 list.

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

New patch 20131214 that handles all tail invoke sites (InvokeExpr + StaticMethodExpr + InstanceMethodExpr). 'StaticInvokeExpr' seems like an old remnant that had no active code path, so that was left as-is.

The approach taken is still the same as the original small patch that addressed only InvokeExpr, except that it is now using a couple small helpers. The commit message has more details.

Also a 'try' block with no catch or finally clause now becomes a BodyExpr. Arguably a user error, historically accepted, and still accepted, but now they are a regular BodyExpr, instead of being wrapped by a the no-op try/catch mechanism. This second commit can be optionally discarded.

With this patch on my machine (4/8 core/thread Ivy Bridge) running on bare clojure.main:
Christophe's test cases both run i 3060ms on a artificially constrained 100M max heap, indicating a dominant GC overhead. (But they now both work!)

When max heap is at a comfortable 2G the reducers version outpaces the lazyseq at 2100ms vs 2600ms!

Comment by Ghadi Shayban [ 13/Jan/14 10:48 AM ]

Updating stale patch after latest changes to master. Latest is CLJ-1250-AllInvokeSites-20140113

Comment by Ghadi Shayban [ 04/Feb/14 3:50 PM ]

Updating patch after murmur changes

Comment by Tassilo Horn [ 13/Feb/14 4:52 AM ]

Ghadi, I suffer from the problem of this issue. Therefore, I've applied your patch CLJ-1250-AllInvokeSites-20140204.patch to the current git master. However, then I get lots of "java.lang.NoSuchFieldError: array" errors when the clojure tests are run:

     [java] clojure.test-clojure.clojure-set
     [java] 
     [java] java.lang.NoSuchFieldError: array
     [java] 	at clojure.core.protocols$fn__6026.invoke(protocols.clj:123)
     [java] 	at clojure.core.protocols$fn__5994$G__5989__6003.invoke(protocols.clj:19)
     [java] 	at clojure.core.protocols$fn__6023.invoke(protocols.clj:147)
     [java] 	at clojure.core.protocols$fn__5994$G__5989__6003.invoke(protocols.clj:19)
     [java] 	at clojure.core.protocols$seq_reduce.invoke(protocols.clj:31)
     [java] 	at clojure.core.protocols$fn__6017.invoke(protocols.clj:48)
     [java] 	at clojure.core.protocols$fn__5968$G__5963__5981.invoke(protocols.clj:13)
     [java] 	at clojure.core$reduce.invoke(core.clj:6213)
     [java] 	at clojure.set$difference.doInvoke(set.clj:61)
     [java] 	at clojure.lang.RestFn.invoke(RestFn.java:442)
     [java] 	at clojure.test_clojure.clojure_set$fn__1050$fn__1083.invoke(clojure_set.clj:109)
     [java] 	at clojure.test_clojure.clojure_set$fn__1050.invoke(clojure_set.clj:109)
     [java] 	at clojure.test$test_var$fn__7123.invoke(test.clj:704)
     [java] 	at clojure.test$test_var.invoke(test.clj:704)
     [java] 	at clojure.test$test_vars$fn__7145$fn__7150.invoke(test.clj:721)
     [java] 	at clojure.test$default_fixture.invoke(test.clj:674)
     [java] 	at clojure.test$test_vars$fn__7145.invoke(test.clj:721)
     [java] 	at clojure.test$default_fixture.invoke(test.clj:674)
     [java] 	at clojure.test$test_vars.invoke(test.clj:718)
     [java] 	at clojure.test$test_all_vars.invoke(test.clj:727)
     [java] 	at clojure.test$test_ns.invoke(test.clj:746)
     [java] 	at clojure.core$map$fn__2665.invoke(core.clj:2515)
     [java] 	at clojure.lang.LazySeq.sval(LazySeq.java:40)
     [java] 	at clojure.lang.LazySeq.seq(LazySeq.java:49)
     [java] 	at clojure.lang.Cons.next(Cons.java:39)
     [java] 	at clojure.lang.RT.boundedLength(RT.java:1655)
     [java] 	at clojure.lang.RestFn.applyTo(RestFn.java:130)
     [java] 	at clojure.core$apply.invoke(core.clj:619)
     [java] 	at clojure.test$run_tests.doInvoke(test.clj:761)
     [java] 	at clojure.lang.RestFn.applyTo(RestFn.java:137)
     [java] 	at clojure.core$apply.invoke(core.clj:617)
     [java] 	at clojure.test.generative.runner$run_all_tests$fn__527.invoke(runner.clj:255)
     [java] 	at clojure.test.generative.runner$run_all_tests$run_with_counts__519$fn__523.invoke(runner.clj:251)
     [java] 	at clojure.test.generative.runner$run_all_tests$run_with_counts__519.invoke(runner.clj:251)
     [java] 	at clojure.test.generative.runner$run_all_tests.invoke(runner.clj:253)
     [java] 	at clojure.test.generative.runner$test_dirs.doInvoke(runner.clj:304)
     [java] 	at clojure.lang.RestFn.applyTo(RestFn.java:137)
     [java] 	at clojure.core$apply.invoke(core.clj:617)
     [java] 	at clojure.test.generative.runner$_main.doInvoke(runner.clj:312)
     [java] 	at clojure.lang.RestFn.invoke(RestFn.java:408)
     [java] 	at user$eval564.invoke(run_tests.clj:3)
     [java] 	at clojure.lang.Compiler.eval(Compiler.java:6657)
     [java] 	at clojure.lang.Compiler.load(Compiler.java:7084)
     [java] 	at clojure.lang.Compiler.loadFile(Compiler.java:7040)
     [java] 	at clojure.main$load_script.invoke(main.clj:274)
     [java] 	at clojure.main$script_opt.invoke(main.clj:336)
     [java] 	at clojure.main$main.doInvoke(main.clj:420)
     [java] 	at clojure.lang.RestFn.invoke(RestFn.java:408)
     [java] 	at clojure.lang.Var.invoke(Var.java:379)
     [java] 	at clojure.lang.AFn.applyToHelper(AFn.java:154)
     [java] 	at clojure.lang.Var.applyTo(Var.java:700)
     [java] 	at clojure.main.main(main.java:37)
Comment by Ghadi Shayban [ 13/Feb/14 8:23 AM ]

Can you give some details about your JVM/environment that can help reproduce? I'm not encountering this error.

Comment by Tassilo Horn [ 13/Feb/14 9:41 AM ]

Sure. It's a 64bit ThinkPad running GNU/Linux.

% java -version
java version "1.7.0_51"
OpenJDK Runtime Environment (IcedTea 2.4.5) (ArchLinux build 7.u51_2.4.5-1-x86_64)
OpenJDK 64-Bit Server VM (build 24.51-b03, mixed mode)
Comment by Ghadi Shayban [ 13/Feb/14 10:19 AM ]

Strange, that is exactly my mail env, OpenJDK7 on Arch, 64-bit. I have also tested on JDK 6/7/8 on OSX mavericks. Are you certain that the git tree is clean besides the patch? (Arch users unite!)

Comment by Tassilo Horn [ 14/Feb/14 1:13 AM ]

Yes, the tree is clean. But now I see that I get the same error also after resetting to origin/master, so it's not caused by your patch at all. Oh, now the error vanished after doing a `mvn clean`! So problem solved.

Comment by Nicola Mometto [ 19/Feb/14 12:32 PM ]

Ghandi, FnExpr.parse should bind IN_TRY_BLOCK to false before analyzing the fn body, consider the case

(try (do something (fn a [] (heap-consuming-op a))) (catch Exception e ..))

Here in the a function the this local will never be cleared even though it's perfectly safe to.
Admittedly this is an edge case but we should cover this possibility too.

Comment by Ghadi Shayban [ 19/Feb/14 2:06 PM ]

You may have auto-corrected my name to Ghandi instead of Ghadi. I wish I were that wise =)

I will update the patch for FnExpr (that seems reasonable), but maybe after 1.6 winds down and the next batch of tickets get scrutiny. It would be nice to get input on a preferred approach from Rich or core after it gets vetted – or quite possibly not vetted.

Comment by Nicola Mometto [ 19/Feb/14 6:11 PM ]

hah, sorry for the typo on the name

Seems reasonable to me, in the meantime I just pushed to tools.analyzer/tools.emitter complete support for "this" clearing, I'll test this a bit in the next few days to make sure it doesn't cause unexpected problems.

Comment by Andy Fingerhut [ 24/Feb/14 12:13 PM ]

Patch CLJ-1250-AllInvokeSites-20140204.patch no longer applies cleanly to latest master as of Feb 23, 2014. It did on Feb 14, 2014. Most likely some of its context lines are changed by the commit to Clojure master on Feb 23, 2014 – I haven't checked in detail.

Comment by Ghadi Shayban [ 20/Mar/14 4:39 PM ]

Added a patch that 1) applies cleanly, 2) binds the IN_TRY_EXPR to false initially when analyzing FnExpr and 3) uses RT.booleanCast

Comment by Alex Miller [ 22/Aug/14 9:31 AM ]

Can you squash the patch and add tests to cover all this stuff?

Comment by Ghadi Shayban [ 22/Aug/14 9:47 AM ]

Sure. Have any ideas for how to test proper behavior of reference clearing? Know of some prior art in the test suite?

Comment by Alex Miller [ 22/Aug/14 10:25 AM ]

Something like the test in the summary would be a place to start. I don't know of any test that actually inspects bytecode or anything but that's probably not wise anyways. Need to make that kind of a test but get coverage on the different kinds of scenarios you're covering - try/catch, etc.

Comment by Ghadi Shayban [ 22/Aug/14 12:13 PM ]

Attached new squashed patch with a couple of tests.

Removed (innocuous but out-of-scope) second commit that analyzed try blocks missing a catch or finally clause as BodyExprs

Comment by Ghadi Shayban [ 29/Aug/14 11:43 AM ]

Rebased to latest master. Current patch CLJ-1250-08-29

Comment by Jozef Wagner [ 29/Aug/14 2:40 PM ]

CLJ-1250-08-29.patch is fishy, 87k size and it includes many unrelated commits

Comment by Alex Miller [ 29/Aug/14 2:44 PM ]

Agreed, Ghadi that last rebase looks wrong.

Comment by Ghadi Shayban [ 29/Aug/14 3:06 PM ]

Oops. Used format-patch against the wrong base. Updated.

Apologies that ticket is longer than War & Peace

Comment by Alex Miller [ 08/Sep/14 7:02 PM ]

I have not had enough time to examine all the bytecode diffs that I want to on this yet but preliminary feedback:

Compiler.java

  • need to use tabs instead of spaces to blend into the existing code better
  • why do StaticFieldExpr and InstanceFieldExpr not need this same logic?

compilation.clj

  • has some whitespace diffs that you could get rid of
  • there seem to be more cases in the code than are covered in the tests here?
Comment by Ghadi Shayban [ 08/Sep/14 11:19 PM ]

The germ of the issue is to clear the reference to 'this' (arg 0) when transferring control to another activation frame. StaticFieldExpr and InstanceFieldExpr do not transfer control to another frame. (StaticMethod and InstanceMethod do transfer control, and are covered by the patch)

Comment by Alex Miller [ 25/Sep/14 9:03 AM ]

Makes sense - can you address the tabs and whitespace issues?

Comment by Ghadi Shayban [ 26/Sep/14 12:51 PM ]

Latest patch CLJ-1250-08-29-ws.patch with whitespace issues fixed.





[CLJ-703] Improve writeClassFile performance Created: 04/Jan/11  Updated: 30/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Jürgen Hötzel Assignee: Unassigned
Resolution: Unresolved Votes: 17
Labels: Compiler, performance

Attachments: Text File 0001-use-File.mkdirs-instead-of-mkdir-every-single-direct.patch     Text File 0002-Ensure-atomic-creation-of-class-files-by-renaming-a-.patch     Text File improve-writeclassfile-perf.patch    
Patch: Code
Approval: Triaged

 Description   

This Discussion about timing issues when writing class files led to the the current implementation of synchronous writes to disk. This leads to bad performance/system-load when compiling Clojure code.

This Discussion questioned the current implentation.

Synchronous writes are not necessary and also do not ensure (crash while calling write) valid classfiles.

These Patches (0001 is just a code cleanup for creating the directory structure) ensures atomic creation of classfiles by using File.renameTo()



 Comments   
Comment by David Powell [ 17/Jan/11 2:16 PM ]

Removing sync makes clojure build much faster. I wonder why it was added in the first place? I guess only Rich knows? I assume that it is not necessary.

If we are removing sync though, I wouldn't bother with the atomic rename stuff. Doing that sort of thing can cause problems on some platforms, eg with search indexers and virus checkers momentarily locking files after they are created.

The patch seems to be assuming that sync is there for some reason, but my initial assumption would be that sync isn't necessary - perhaps it was working around some issue that no longer exists?

Comment by Jürgen Hötzel [ 19/Jan/11 2:05 PM ]

Although its unlikely: there is a possible race condition "loading a paritally written classfile"?:

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/RT.java#L393

Comment by John Szakmeister [ 25/May/12 4:22 AM ]

The new improve-writeclassfile-perf version of the patch combines the two previous patches into a single patch file and brings them up-to-date with master. I can split the two changes back out into separate patch files if desired, but I figured out current tooling is more geared towards a single patch being applied.

Comment by John Szakmeister [ 25/May/12 4:36 AM ]

FWIW, both fixes look sane. The first one is a nice cleanup. The second one is a little more interesting in that uses a rename operation to put the final file into place. It removes the sync call, which does make things faster. In general, if we're concerned about on-disk consistency, we should really have a combination of the two: write the full contents to a tmp file, sync it, and atomically rename to the destination file name.

Neither the current master, nor the current patch will guarantee on-disk consistency across a machine wide crash. The current master could crash before the sync() occurs, leaving the file in an inconsistent state. With the patch, the OS may not get the file from file cache to disk before an OS level crash occurs, and leave the file in an inconsistent state as well. The benefit of the patch version is that the whole file does atomically come into view at once. It does have a nasty side effect of leaving around a temp file if the compiler crashes just before the rename though.

Perhaps a little more work to catch an exception and clean up is in order? In general, I like the patched version better.

Comment by Ivan Kozik [ 05/Oct/12 7:07 PM ]

File.renameTo returns false on (most?) errors, but the patch doesn't check for failure. Docs say "The return value should always be checked to make sure that the rename operation was successful." Failure might be especially likely on Windows, where files are opened by others without FILE_SHARE_DELETE.

Comment by Dave Della Costa [ 23/Jun/14 11:37 PM ]

We've been wondering why our compilation times on linux were so slow. It became the last straw when we walked away from one project and came back after 15 minutes and it was not done yet.

After some fruitless investigation into our linux configuration and lein java args, we stumbled upon this issue via the associated Clojure group thread. Upon commenting out the flush() and sync() lines (https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L7171-L7172) and compiling Clojure 1.6 ourselves, our projects all started compiling in under a minute.

Point being, can we at least provide some flag to allow for "unsafe compilation" or something? As it is, this is bad enough that we've manually modified all our local versions of Clojure to work around the issue.

Comment by Tim McCormack [ 30/Sep/14 4:10 PM ]

Additional motivation: This becomes really unpleasant on an encrypted filesystem, since write and read latency become higher.

As a partial workaround, I've been using this script to mount a ramdisk over top of target, which speeds up compilation 2-4x: https://gist.github.com/timmc/6c397644668bcf41041f (but removing flush() and sync() entirely would probably speed things up even more, if safe)





[CLJ-1541] System/getProperty "user.dir" gives wrong output Created: 30/Sep/14  Updated: 30/Sep/14  Resolved: 30/Sep/14

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

Type: Defect Priority: Major
Reporter: Khuram U. Khalid Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: System/getProperty, bug, user.dir
Environment:

Windows 8.1 - Java version 1.8.0 - Clojure 1.6.0 - IntelliJ IDEA - Maven 3.0.5



 Description   

;; For example if current project is in C:\Projects\My Project
;; Following gives...

(ns my.project.com.core)
(defn -main [] (println (System/getProperty "user.dir"))

;;=> C:\Projects\My Project\src\main\my\project\com

;; While when same Clojure code is run from a Java project gives...
public static void main(String[] args) { my.project.com.core.main(); }
;;=> C:\Projects\My Project

Expected same behavior and hence correct output in Clojure.



 Comments   
Comment by Alex Miller [ 30/Sep/14 9:18 AM ]

I tried this on a simple project at the command line and saw no difference in behavior between Java and Clojure. Clojure does not modify the user.dir system property and you are calling directly into the System class, just like Java does, so the only difference is in how your environment is configured when running in these two contexts.

It's possible that your environment (IDEA) is configuring Java and Clojure projects differently, but you should ask on the mailing list or issue tracker for the tool.





[CLJ-1540] Make main function to run when using on the fly compilation, not just ahead-of-time compilation Created: 29/Sep/14  Updated: 29/Sep/14  Resolved: 29/Sep/14

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

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


 Description   

From this doc
http://clojure.org/compilation

Clojure compiles all code you load on-the-fly into JVM bytecode, but sometimes it is advantageous to compile ahead-of-time (AOT). Some reasons to use AOT compilation are:
To generate named classes for use by Java among the reason

and only named classes can run off main function.

So if not using AOT, the main method will not be executed.

Hence the following main can only run in AOT using named classes.
(defn -main
(println "runme")))

Will that be possible to run the main function using on the fly compilation ?

Basically, it should work similarly to Java. If the clojure file has a main function then it should run the file if user select it to run (eg in IDE) regardless of mode of compilation.

For example, in IntelliJ ide, a clojure file (eg hello.clj) has the following code

(defn testme[]
(println "hello"))

(defn -main
(println "runme")
(testme))

if user choose "Run hello.clj" from Intellij, then it should execute the main function.

Will be great if this feature is consider in next release of clojure

thank



 Comments   
Comment by Alex Miller [ 29/Sep/14 12:49 PM ]

There are (already) a variety of ways to start a Clojure script or program and I believe what you request (and more) is already possible.

See: http://clojure.org/repl_and_main

An example command-line for your hello.clj example would be:

java -cp clojure.jar clojure.main -i hello.clj -e "(-main)"

but if you are only running this as a script you could embed the code to run your app at the end of the hello.clj script file and do:

java -cp clojure.jar clojure.main hello.clj
Comment by Kevin Downey [ 29/Sep/14 3:44 PM ]

checkout the `-m` option





[CLJ-1539] Allow Records to be imported "Normally" Created: 28/Sep/14  Updated: 28/Sep/14  Resolved: 28/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: David Williams Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

I know about records, and how they are compiled to Java classes, etc. The thing is, the import of a record type has an undocumented quirk, the need to turn dashes into underscore

(:require [my-fancy.namespace])
(:import [my_fancy.namespace MyRecord])

Granted this is trivial, but I just spent an hour or two tracking this down after some initial unsuccessful attempts to import a record between namespaces. IMHO this is not user friendly and could be smoothed out.



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

There are no plans to change this. Typically you don't need to import the record class at all, just require the ns and use the > and map> constructor functions. When you do import the class, you are doing so as a Java class, so it follows java class import rules.





[CLJ-1538] Duplicate check occurs too early. Created: 27/Sep/14  Updated: 27/Sep/14

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

Type: Defect Priority: Minor
Reporter: Chhi'mèd Künzang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reader


 Description   

I cannot use literal syntax to create a set/map with unique members/keys if the elements are generated with an identical form. Examples of such legal forms: (rand), (read), (clojure.core.async/<!!), etc. I will use (rand) in these examples.

user=> #{(rand) (rand)}
IllegalArgumentException Duplicate key: (rand)  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:68)

user=> {(rand) 1, (rand) 2}

IllegalArgumentException Duplicate key: (rand)  clojure.lang.PersistentArrayMap.createWithCheck (PersistentArrayMap.java:70)

It appears that the input is being checked for duplicates before the arguments to the collection constructors are evaluated. However, this doesn't prevent the need to run the check again later.

Note that duplicates are still (correctly) detected, after evaluation, even if duplicates do not appear as literals in the source:

user=> #{(+ 1 1) 2}

IllegalArgumentException Duplicate key: 2  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:56)
user=> {(+ 1 1) :a, 2 :b}

IllegalArgumentException Duplicate key: 2  clojure.lang.PersistentArrayMap.createWithCheck (PersistentArrayMap.java:70)

The first duplicate check therefore seems to be both redundant and incorrect.

Note that this eager duplicate-checking seems to have higher precedence even than the syntax-quote reader macro.

user=> `#{~(rand) ~(rand)}

IllegalArgumentException Duplicate key: (clojure.core/unquote (rand))  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:68)

user=> `{~(rand) 1, ~(rand) 2}

IllegalArgumentException Duplicate key: (clojure.core/unquote (rand))  clojure.lang.PersistentArrayMap.createWithCheck (PersistentArrayMap.java:70)

This is odd – since syntax-quote should not realize a collection at all at read time:

For Lists/Vectors/Sets/Maps, syntax-quote establishes a template of the corresponding data structure. Within the template, unqualified forms behave as if recursively syntax-quoted, but forms can be exempted from such recursive quoting by qualifying them with unquote or unquote-splicing, in which case they will be treated as expressions and be replaced in the template by their value, or sequence of values, respectively. (http://clojure.org/reader)

Definitions aside, based on the apparent expansion of syntax-quote, I would expect the previous to have worked correctly.

If I fake the expected macroexpansion by manually substituting the desired inputs, I get the expected results:

user=> '`#{~:a ~:b}
(clojure.core/apply clojure.core/hash-set (clojure.core/seq (clojure.core/concat (clojure.core/list :b) (clojure.core/list :a))))
user=> (clojure.core/apply clojure.core/hash-set (clojure.core/seq (clojure.core/concat (clojure.core/list (rand)) (clojure.core/list (rand)))))
#{0.27341896385866227 0.3051522362827035}
user=> '`{~:a 1, ~:b 2}
(clojure.core/apply clojure.core/hash-map (clojure.core/seq (clojure.core/concat (clojure.core/list :a) (clojure.core/list 1) (clojure.core/list :b) (clojure.core/list 2))))
user=> (clojure.core/apply clojure.core/hash-map (clojure.core/seq (clojure.core/concat (clojure.core/list (rand)) (clojure.core/list 1) (clojure.core/list (rand)) (clojure.core/list 2))))
{0.12476921225204185 2, 0.5807961046096718 1}

It seems to me that there is a superfluous duplicate check being run before the set/map reader macros evaluate their arguments. This check should seemingly be removed. Even if the check did not catch some false-positive duplicates (as it does), it would be unnecessary since the apparent second post-evaluation check would catch all true duplicates.

All that said, it's unclear that this check should happen at all. If I try to create sets/map with duplicate members/keys, I don't get an error. The duplicates are silently removed or superseded.

user=> (set (list 1 1))
#{1}
user=> (hash-map 1 2 1 3)
{1 3}

It seems it would be most consistent for literals constructed by the reader syntax to do the same.

I can see the argument that a literal representation is not a 'request to construct' but rather an attempt to simulate the printed representation of a literal data object. From that perspective, disallowing 'illegal' printed representations seems reasonable. Unfortunately, the possibility of evaluated forms inside literal vectors, sets, and maps (since lists are evaluated at read time) already breaks this theory. That is, the printed representation of such collections is not an accurately readable form, so read-time duplicate checking still cannot prevent seeming inconsistencies in print/read representations:

user=> '#{(+ 1 1) 2}
#{(+ 1 1) 2}
user=> #{(+ 1 1) 2}

IllegalArgumentException Duplicate key: 2  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:56)

Given that the problem cannot be completely avoided at all, it seems simplest and most consistent to treat reader literal constructors like their run-time counterparts, as syntax quote would in the absence of the spurious duplicate check.






[CLJ-1005] Use transient map in zipmap Created: 30/May/12  Updated: 26/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: performance

Attachments: Text File 0001-Use-transient-map-in-zipmap.2.patch     Text File 0001-Use-transient-map-in-zipmap.patch     Text File 0002-CLJ-1005-use-transient-map-in-zipmap.patch     Text File CLJ-1005-zipmap-iterators.patch    
Patch: Code
Approval: Vetted

 Description   

The attached patch changes zipmap to use a transient map internally. The definition is also moved so that it resides below that of #'transient. The original definition is commented out (like that of #'into).



 Comments   
Comment by Aaron Bedra [ 14/Aug/12 9:24 PM ]

Why is the old implementation left and commented out? If we are going to move to a new implementation, the old one should be removed.

Comment by Michał Marczyk [ 15/Aug/12 4:17 AM ]

As mentioned in the ticket description, the previously attached patch follows the pattern of into whose non-transient-enabled definition is left in core.clj with a #_ in front – I wasn't sure if that's something desirable in all cases.

Here's a new patch with the old impl removed.

Comment by Andy Fingerhut [ 15/Aug/12 10:37 AM ]

Thanks for the updated patch, Michal. Sorry to raise such a minor issue, but would you mind using a different name for the updated patch? I know JIRA can handle multiple attached files with the same name, but my prescreening code isn't quite that talented yet, and it can lead to confusion when discussing patches.

Comment by Michał Marczyk [ 15/Aug/12 10:42 AM ]

Thanks for the heads-up, Andy! I've reattached the new patch under a new name.

Comment by Andy Fingerhut [ 16/Aug/12 8:24 PM ]

Presumptuously changing Approval from Incomplete back to None after the Michal's updated patch was added, addressing the reason the ticket was marked incomplete.

Comment by Aaron Bedra [ 11/Apr/13 5:32 PM ]

The patch looks good and applies cleanly. Are there additional tests that we should run to verify that this is providing the improvement we think it is. Also, is there a discussion somewhere that started this ticket? There isn't a lot of context here.

Comment by Michał Marczyk [ 11/Apr/13 6:19 PM ]

Hi Aaron,

Thanks for looking into this!

From what I've been able to observe, this change hugely improves zipmap times for large maps. For small maps, there is a small improvement. Here are two basic Criterium benchmarks (transient-zipmap defined at the REPL as in the patch):

;;; large map
user=> (def xs (range 16384))
#'user/xs
user=> (last xs)
16383
user=> (c/bench (zipmap xs xs))
Evaluation count : 13920 in 60 samples of 232 calls.
             Execution time mean : 4.329635 ms
    Execution time std-deviation : 77.791989 us
   Execution time lower quantile : 4.215050 ms ( 2.5%)
   Execution time upper quantile : 4.494120 ms (97.5%)
nil
user=> (c/bench (transient-zipmap xs xs))
Evaluation count : 21180 in 60 samples of 353 calls.
             Execution time mean : 2.818339 ms
    Execution time std-deviation : 110.751493 us
   Execution time lower quantile : 2.618971 ms ( 2.5%)
   Execution time upper quantile : 3.025812 ms (97.5%)

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

;;; small map
user=> (def ys (range 16))
#'user/ys
user=> (last ys)
15
user=> (c/bench (zipmap ys ys))
Evaluation count : 16639020 in 60 samples of 277317 calls.
             Execution time mean : 3.803683 us
    Execution time std-deviation : 88.431220 ns
   Execution time lower quantile : 3.638146 us ( 2.5%)
   Execution time upper quantile : 3.935160 us (97.5%)
nil
user=> (c/bench (transient-zipmap ys ys))
Evaluation count : 18536880 in 60 samples of 308948 calls.
             Execution time mean : 3.412992 us
    Execution time std-deviation : 81.338284 ns
   Execution time lower quantile : 3.303888 us ( 2.5%)
   Execution time upper quantile : 3.545549 us (97.5%)
nil

Clearly the semantics are preserved provided transients satisfy their contract.

I think I might not have started a ggroup thread for this, sorry.

Comment by Andy Fingerhut [ 03/Sep/14 8:10 PM ]

Patch 0001-Use-transient-map-in-zipmap.2.patch dated Aug 15 2012 does not apply cleanly to latest master after some commits were made to Clojure on Sep 3 2014.

I have not checked whether this patch is straightforward to update. See the section "Updating stale patches" at http://dev.clojure.org/display/community/Developing+Patches for suggestions on how to update patches.

Comment by Michał Marczyk [ 14/Sep/14 12:48 PM ]

Thanks, Andy. It was straightforward to update – an automatic rebase. Here's the updated patch.

Comment by Ghadi Shayban [ 22/Sep/14 9:58 AM ]

New patch using clojure.lang.RT/iter, criterium shows >30% more perf in the best case. Less alloc probably but I didn't measure. CLJ-1499 (better iterators) is related





[CLJ-1536] Remove usage of sun.misc.Signal (which may not be available in Java 9) Created: 26/Sep/14  Updated: 26/Sep/14

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

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


 Description   

It looks like Java 9 will not continue to provide access to "internal" classes like sun.misc.Signal. Clojure currently uses this in the REPL to trap ctrl-c (SIGINT) and cancel current evaluation instead of process shutdown.

There is a page of alternatives here:
https://wiki.openjdk.java.net/display/JDK8/Java+Dependency+Analysis+Tool

But there is no suggested alternative for sun.misc.Signal and I'm not aware of a portable solution to it.






[CLJ-803] IAtom interface Created: 27/May/11  Updated: 26/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Pepijn de Vos Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: atom

Attachments: Text File 0001-atom-interface.patch     Text File iatom.patch    
Patch: Code
Approval: Screened

 Description   

Atom does not have an interface and is marked final. This patch extracts an IAtom interface and has Atom implement it.

Patch: 0001-atom-interface.patch

Screened by: Alex Miller

More info: Rich said "patch welcome for IAtom". See IRC logs: http://clojure-log.n01se.net/date/2010-12-29.html#10:04c



 Comments   
Comment by Stuart Halloway [ 27/May/11 2:33 PM ]

Please add a patch formatted by "git format-patch" so that attribution is included.

Comment by Pepijn de Vos [ 04/Jun/11 5:56 AM ]

I added the formatted patch a few days ago. Does 'no news is good news' apply here?

And, silly question, will it make it into 1.3? I can't figure out how to tell Jira to show me that.

Comment by Kevin Downey [ 04/Jul/11 9:06 PM ]

I fail to see the need for an IAtom, if you want something atom like for couchdb the interfaces are already there. Maybe I ICompareAndSwap. Atoms and couchdb are different so making them appear the same is a bad idea.

http://en.wikipedia.org/wiki/Fallacies_of_Distributed_Computing

http://clojure.org/state one of the distinctions between agents and actors raised in the section titled "Message Passing and Actors" is local vs. distributed and the same distinction can be made between couchdb (regardless of compare and swap) and atoms

Comment by Aaron Bedra [ 04/Jul/11 9:18 PM ]

This ticket has already been moved into approved backlog. It will be revisited again after the 1.3 release where we will take a closer look at things. For now, this will remain as is.

Comment by Aaron Craelius [ 10/Jul/14 12:15 PM ]

Any chance this patch could get implemented in an upcoming Clojure release. There is still continued interest, see this thread: https://groups.google.com/forum/#!topic/clojure-dev/y5QoMqd44Lc

One suggestion I would make is also removing the final marker from clojure.lang.Atom - I can see use cases where one would want to directly subclass Atom (to capture dependencies in reactive computations for instance).

Comment by Brandon Bloom [ 02/Aug/14 2:14 PM ]

I'd like to see an IAtom interface, but would prefer that `swap` not be part of it. Swapping can, and should, be defined in terms of `compareAndSet`. Seems like IAtom should only have `boolean compareAndSet(object oldval, object newval)` as well as `void reset(object newval)`.

Comment by Brandon Bloom [ 02/Aug/14 2:29 PM ]

Alternative patch that introduces IAtom and converts swap to be static.

Comment by Pepijn de Vos [ 03/Aug/14 2:59 AM ]

At the time I did the initial patch, I had the same idea to remove swap, but Rich said there where cases for having it, so it should stay in according to him.

Comment by Aaron Craelius [ 03/Aug/14 1:51 PM ]

One use case I can think of for overriding swap is if an IAtom is wrapping say a row of data stored in a database. Then comparing something like a version column (or transaction id in the case of datomic) is what should determine whether a swap is retried, not the actual value of the data. In this case then, compareAndSet would actually be a more complex operation than swap and it makes sense to define the two independently.

Comment by Aaron Craelius [ 03/Aug/14 1:56 PM ]

I should also mention my related issue: http://dev.clojure.org/jira/browse/CLJ-1470 which simply allows Atom (and also ARef) to be sub-classed. Both patches could ultimately work together to make the whole Atom/ARef infrastructure easier to extend.

Comment by Alex Miller [ 10/Sep/14 4:47 PM ]

This ticket needs some work before it can be screened:

  • description can lose the the Couch stuff
  • needs some mention of tradeoffs for the various swap alternatives
  • don't know what patch should be considered
Comment by Brandon Bloom [ 10/Sep/14 5:01 PM ]

I removed my patch because Pepijn/Rich is right: Swap should be part of the interface.





[CLJ-1535] Make boxed math warning suppressible Created: 26/Sep/14  Updated: 26/Sep/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: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: math

Approval: Vetted

 Description   

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






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

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

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

Attachments: File class-for-name.diff    
Patch: Code
Approval: Triaged

 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.





[CLJ-1533] Oddity in type tag usage for primInvoke Created: 24/Sep/14  Updated: 25/Sep/14

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: ft, typehints

Attachments: Text File 0001-CLJ-1533-inject-original-var-form-meta-in-constructe.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Some odd behavior demonstrated in Clojure 1.6.0 REPL below. Why does the (Math/abs (f2 -3)) call issue a reflection warning? It seems like perhaps it should not, given the other examples.

user=> (clojure-version)
"1.6.0"
user=> (set! *warn-on-reflection* true)
true
user=> (defn ^{:tag 'long} f1 [x] (inc x))
#'user/f1
user=> (Math/abs (f1 -3))
2
user=> (defn ^{:tag 'long} f2 [^long x] (inc x))
#'user/f2
user=> (Math/abs (f2 -3))
Reflection warning, NO_SOURCE_PATH:6:1 - call to static method abs on java.lang.Math can't be resolved (argument types: java.lang.Object).
2
user=> (defn ^{:tag 'long} f3 ^long [^long x] (inc x))
#'user/f3
user=> (Math/abs (f3 -3))
2

Cause: invokePrim path does not take into account var or form meta

Approach: apply var and form meta to invokePrim expression

Patch: 0001-CLJ-1533-inject-original-var-form-meta-in-constructe.patch



 Comments   
Comment by Nicola Mometto [ 25/Sep/14 9:47 AM ]

The issue is similar to http://dev.clojure.org/jira/browse/CLJ-1491

Comment by Nicola Mometto [ 25/Sep/14 9:58 AM ]

The root cause was also almost the same, the proposed patch is a superset of the one proposed for CLJ-1491

Comment by Alex Miller [ 25/Sep/14 10:09 AM ]

Can we include 1491 cases in this ticket and mark 1491 a duplicate?

Comment by Alex Miller [ 25/Sep/14 10:09 AM ]

Also needs tests in the patch.

Comment by Nicola Mometto [ 25/Sep/14 10:23 AM ]

Updated the patch with testcases for both issues, I agree that CLJ-1491 should be closed as duplicate





[CLJ-1491] External type hint inconsistency between regular functions and primitive functions Created: 05/Aug/14  Updated: 25/Sep/14  Resolved: 25/Sep/14

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

Type: Defect Priority: Minor
Reporter: Gunnar Völkel Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: compiler, typehints

Attachments: Text File 0001-preserve-fn-meta-on-invokePrim.patch    
Patch: Code
Approval: Triaged

 Description   

Consider the following example.

(set! *warn-on-reflection* true)

(defn f [n] (java.util.ArrayList. (int n)))

(let [al ^java.util.ArrayList (f 10)]
  (.add al 23))

As expected this does not warn about reflection. The following example shows the same scenario for a primitive function.

(set! *warn-on-reflection* true)

(defn g [^long n] (java.util.ArrayList. n))

(let [al ^java.util.ArrayList (g 10)]
  (.add al 23))
; Reflection warning, NO_SOURCE_PATH:2:3 - call to method add on java.lang.Object can't be resolved (no such method).

So the behavior of external type hints is inconsistent for regular functions and primitive functions.
Most likely, the external type hint information is somehow ignored for primitive functions since the case where they return no primitive value is not treated separately.



 Comments   
Comment by Nicola Mometto [ 05/Aug/14 4:32 AM ]

The following patch preserves the original metadata of the invoke form on the transformed .invokePrim expression

Comment by Alex Miller [ 05/Aug/14 7:40 AM ]

Not challenging the premise at all but workaround:

(let [^java.util.ArrayList al (g 10)]
  (.add al 23))
Comment by Gunnar Völkel [ 05/Aug/14 8:09 AM ]

Well, the example above was already changed such that you can also place the type hint on the binding to check whether that works.
The actual problem arose when using the return value of the function exactly once without an additional binding.

Comment by Jozef Wagner [ 05/Aug/14 10:48 AM ]

Responding to Alex's comment, is there a consensus on which variant is (more) idiomatic? IMHO latter variant seems to be more reliable (as this issue shows, and for primitive hits too), and is consistent with 'place hint on a symbol' idiom which is applied when type hinting vars or fn args.

(let [symbol ^typehint expr] body)
(let [^typehint symbol expr] body)
Comment by Alex Miller [ 05/Aug/14 4:59 PM ]

They have different meanings. Generally the latter covers some cases that the former does not so it's probably the better one. I believe one of the cases is that if expr is a macro, the typehint is lost in the former.

Comment by Nicola Mometto [ 25/Sep/14 9:59 AM ]

The patch for http://dev.clojure.org/jira/browse/CLJ-1533?jwupdated=61127&focusedCommentId=35814 fixes this issue and more and should be preferred over this

Comment by Alex Miller [ 25/Sep/14 10:31 AM ]

Dupe of CLJ-1533





[CLJ-1157] Classes generated by gen-class aren't loadable from remote codebase for mis-implementation of static-initializer Created: 04/Feb/13  Updated: 25/Sep/14

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

Type: Defect Priority: Minor
Reporter: Tsutomu Yano Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: gen-class
Environment:

Tested on Mac OS X 10.9.1 and Oracle JVM 1.7.0_51 with Clojure 1.6 master SNAPSHOT


Attachments: File 20140121_fix_classloader.diff     File clj-1157-v2.diff    
Patch: Code
Approval: Vetted

 Description   

When a genclass'ed object is serialized and sent to a remote system, the remote system throws an exception loading the object:

Exception in thread "main" java.lang.ExceptionInInitializerError
        at java.io.ObjectStreamClass.hasStaticInitializer(Native Method)
        at java.io.ObjectStreamClass.computeDefaultSUID(ObjectStreamClass.java:1723)
        at java.io.ObjectStreamClass.access$100(ObjectStreamClass.java:69)
        at java.io.ObjectStreamClass$1.run(ObjectStreamClass.java:247)
        at java.io.ObjectStreamClass$1.run(ObjectStreamClass.java:245)
        at java.security.AccessController.doPrivileged(Native Method)
        at java.io.ObjectStreamClass.getSerialVersionUID(ObjectStreamClass.java:244)
        at java.io.ObjectStreamClass.initNonProxy(ObjectStreamClass.java:600)
        at java.io.ObjectInputStream.readNonProxyDesc(ObjectInputStream.java:1601)
        at java.io.ObjectInputStream.readClassDesc(ObjectInputStream.java:1514)
        at java.io.ObjectInputStream.readOrdinaryObject(ObjectInputStream.java:1750)
        at java.io.ObjectInputStream.readObject0(ObjectInputStream.java:1347)
        at java.io.ObjectInputStream.readObject(ObjectInputStream.java:369)
        at sun.rmi.server.UnicastRef.unmarshalValue(UnicastRef.java:324)
        at sun.rmi.server.UnicastRef.invoke(UnicastRef.java:173)
        at java.rmi.server.RemoteObjectInvocationHandler.invokeRemoteMethod(RemoteObjectInvocationHandler.java:194)
        at java.rmi.server.RemoteObjectInvocationHandler.invoke(RemoteObjectInvocationHandler.java:148)
        at $Proxy0.getResult(Unknown Source)
        at client.SampleClient$_main.doInvoke(SampleClient.clj:12)
        at clojure.lang.RestFn.invoke(RestFn.java:397)
        at clojure.lang.AFn.applyToHelper(AFn.java:159)
        at clojure.lang.RestFn.applyTo(RestFn.java:132)
        at client.SampleClient.main(Unknown Source)
 Caused by: java.io.FileNotFoundException: Could not locate remoteserver/SampleInterfaceImpl__init.class or remoteserver/SampleInterfaceImpl.clj on classpath: 
        at clojure.lang.RT.load(RT.java:434)
        at clojure.lang.RT.load(RT.java:402)
        at clojure.core$load$fn__5039.invoke(core.clj:5520)
        at clojure.core$load.doInvoke(core.clj:5519)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at clojure.lang.Var.invoke(Var.java:415)
        at remoteserver.SampleInterfaceImpl.<clinit>(Unknown Source)
        ... 23 more

Reproduce:

// build
git clone git://github.com/tyano/clojure_genclass_fix.git
cd clojure_genclass_fix
sh build.sh
// start rmiregistry
rmiregistry -J-Djava.rmi.server.useCodebaseOnly=false &
// start server
cd remoteserver
sh start.sh
// Start client
cd ../client
sh start.sh

Cause:

A gen-classed class (in this case, SampleInterfaceImpl.class) uses a static-initializer for loading SampleInterfaceImpl__init.class like:

static
  {
    RT.var("clojure.core", "load").invoke("/remoteserver/SampleInterfaceImpl");
  }

RT.load in default uses a context-classloader for loading __init.class but all classes depending on a gen-classed class must be loaded from the same classloader. In this case, RT.load must use a remote URLClassLoader which loads the main class.

Proposed:

Instead produce the equivalent of this in the static initializer:

static
  {
    Var.pushThreadBindings(RT.map(new Object[] { Compiler.LOADER, SampleInterfaceImpl.class.getClassLoader() }));
    try {
        RT.var("clojure.core", "load").invoke("/remoteserver/SampleInterfaceImpl");
    }
    finally 
    {
        Var.popThreadBindings();
    }
  }

With this code, RT.load will uses a same classloader which load SampleInterfaceImpl.class.

Patch: clj-1157-v2.diff

Screened by:



 Comments   
Comment by Stuart Halloway [ 01/Mar/13 10:20 AM ]

This sounds reasonable, but anything touching classloaders must be considered very carefully.

Comment by Stuart Halloway [ 01/Mar/13 12:12 PM ]

It seems overly complex to have the patch do so much code generation. Why not implement a method that does this job, and have the generated code call that?

Comment by Andy Fingerhut [ 11/Jan/14 2:47 PM ]

Patch 20130204_fix_classloader.diff dated Feb 3, 2013 no longer applies cleanly as of the latest commits to Clojure master on Jan 11, 2014. The only conflict in applying the patch appears to be in the file src/jvm/clojure/asm/commons/GeneratorAdapter.java. This is probably due to the commit for ticket CLJ-713 that was committed today, updating the ASM library.

Comment by Tsutomu Yano [ 21/Jan/14 3:01 AM ]

I put a new patch applicable on the latest master branch.
This new patch is simpler and robust because the code-generation becomes very simple. Now It just call a method implemented with Java.

And I fixed my sample program and the 'HOW TO REPRODUCT THIS ISSUE' section of this ticket, because old description is not runnable on newest JVM. It is because the specification of remote method call of the newest JVM was changed from the old one. In the newest JVM, we need a 'java.rmi.server.useCodebaseOnly=false' option for making the behavior of remote call same as old one.
pull the newest sample.

Comment by Alex Miller [ 25/Sep/14 10:16 AM ]

Added almost the same patch that does not have whitespace error.





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

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

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

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

 Description   

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

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

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

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

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

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

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

The compiler can easily avoid these by counting forms.

Patch: v3_0001-CLJ-1456-counting-forms-to-catch-malformed-throw-for.patch



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

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

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

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

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

Newest patch also contains a few tests.

Comment by Andy Fingerhut [ 29/Aug/14 4:54 PM ]

All patches dated Jun 30 2014 and earlier no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. They did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch. See section "Updating Stale Patches" on this wiki page for some tips on updating patches: http://dev.clojure.org/display/community/Developing+Patches

Alf, it can help avoid confusion if different patches have different file names. JIRA lets you create multiple attachments with the same name, but I wouldn't recommend it.

Comment by Alf Kristian Støyle [ 30/Aug/14 2:18 AM ]

It was easy to fix the patch. Uploaded the new patch v3_0001-CLJ-1456-counting-forms-to-catch-malformed-throw-for.patch, which applies cleanly to the current master.





[CLJ-1313] Correct a few unit tests Created: 23/Dec/13  Updated: 25/Sep/14

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: ft

Attachments: File clj-1313-v1.diff     File clj-1313-v2.diff    
Patch: Code and Test
Approval: Triaged

 Description   

Several unit tests do not test what they appear to have been intended to test, because of missing is statements around (= expr1 expr2) expressions, or because of use of (is (thrown? ...)) instead of (is (thrown-with-msg? ...))

Patch: clj-1313-v2.diff



 Comments   
Comment by Andy Fingerhut [ 23/Dec/13 3:11 AM ]

Patch clj-1313-v1.diff wraps (is ...) around several = expressions in unit tests that appeared to have been missing them, and changes several thrown? to thrown-with-msg? when there were regexes that were unused.

Comment by Stuart Halloway [ 31/Jan/14 12:36 PM ]

please update to apply cleanly on master

Comment by Andy Fingerhut [ 31/Jan/14 3:29 PM ]

clj-1313-v2.diff is identical to clj-1313-v1.diff except that it removes the portion that conflicts with the latest Clojure master. That portion needs updating for a different reason anyway (ticket CLJ-1328), and is probably best put into a patch for that ticket.





[CLJ-1466] clojure.core/bean should implement Iterable Created: 16/Jul/14  Updated: 25/Sep/14

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

Type: Defect Priority: Minor
Reporter: Ambrose Bonnaire-Sergeant Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: ft, interop

Attachments: File iterable-bean-v2.diff    
Patch: Code and Test
Approval: Triaged

 Description   

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

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

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

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



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

One workaround:

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

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

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

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

(iterator [] (.iterator pmap)

to the bean proxy definition?

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

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

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

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

Added new patch that just adds iterator to bean.





[CLJ-1501] LazySeq switches to equiv when using equals Created: 11/Aug/14  Updated: 25/Sep/14

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

Type: Defect Priority: Major
Reporter: Jozef Wagner Assignee: Jozef Wagner
Resolution: Unresolved Votes: 0
Labels: collections, ft, interop, seq

Attachments: File clj-1501.diff    
Patch: Code and Test
Approval: Triaged

 Description   

When comparing lazy seqs with java equality operator .equals, the implementation switches to the Clojures .equiv comparison. This switch is not present in any other Seq or ordered collection type.

user> (.equals '(3) '(3N))
false
user> (.equals [3] [3N])
false
user> (.equals (seq [3]) (seq [3N]))
false
user> (.equals (lazy-seq [3]) (lazy-seq [3N]))
true


 Comments   
Comment by Jozef Wagner [ 11/Aug/14 9:32 AM ]

Patch clj-1501.diff with tests added





[CLJ-1330] Class name clash between top-level functions and defn'ed ones Created: 22/Jan/14  Updated: 25/Sep/14

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Alex Miller
Resolution: Unresolved Votes: 8
Labels: aot, compiler

Attachments: Text File 0001-CLJ-1093-v2.patch     Text File 0001-CLJ-1093-v3.patch     Text File 0001-Fix-CLJ-1330-make-top-level-named-functions-classnam.patch     Text File 0001-Fix-CLJ-1330-refactored.patch     File demo1.clj    
Patch: Code
Approval: Screened

 Description   

Named anonymous fn's are not guaranteed to have unique class names when AOT-compiled.

For example:

(defn g [])
(def xx (fn g []))

When AOT-compiled both functions will emit user$g.class, the latter overwriting the former.

Demonstration script: demo1.clj

Patch: 0001-CLJ-1093-v3.patch

Approach: Generate unique class names for named fn's the same way as for unnamed anonymous fn's.
The patch contains an additional enhancement to include the name of the local binding in the class name.

Comparison between pre and post patch naming scheme (N denotes unique number):

code before after note
(defn a []) user$a user$a same
(fn []) user$evalN$fn__N user$evalN$fn__N same
(fn a []) user$evalN$a__N user$evaN$a__N same
(let [a (fn [])] a) user$evalN$a__N user$evalN$a__N same
(let [a (fn x [])] a) user$eval1N$x__N user$evalN$a_x_N IMPROVED - contains local binding name
(def a (fn [])) user$a user$a same
(def a (fn x [])) user$x user$a_x_N FIXED conflict with (defn x [])
(def ^{:foo (fn [])} a) user$fn__N user$fn__N same
(def ^{:foo (fn a [])} a) user$a user$a__N FIXED conflict with (defn a [])
(def a (fn [] (fn []))) user$a$fn__N user$a$fn__N same
(def a (fn [] (fn x []))) user$a$x__N user$a$x__N same

See also: This patch also fixes the issue reported in CLJ-1227.

Screened by: Alex Miller - I am not sure whether the local binding name enhancement is worth doing. It improves debugging of which anonymous class you're talking about but has the downsides of increasing class name (and file name) length.



 Comments   
Comment by Ambrose Bonnaire-Sergeant [ 22/Jan/14 11:12 AM ]

This seems like the reason why jvm.tools.analyzer cannot analyze clojure.core. On analyzing a definline, there is an "attempted duplicate class definition" error.

This doesn't really matter, but I thought it may or may not be useful information to someone.

Comment by Nicola Mometto [ 22/Jan/14 11:35 AM ]

Attached a fix.

This also fixes AOT compiling of code like:

(def x (fn foo []))
(fn foo [])
Comment by Nicola Mometto [ 22/Jan/14 11:39 AM ]

Cleaned up patch

Comment by Alex Miller [ 22/Jan/14 12:43 PM ]

It looks like the patch changes indentation of some of the code - can you fix that?

Comment by Nicola Mometto [ 22/Jan/14 3:57 PM ]

Updated patch without whitespace changes

Comment by Alex Miller [ 22/Jan/14 4:15 PM ]

Thanks, that's helpful.

Comment by Alex Miller [ 24/Jan/14 10:03 AM ]

There is consensus that this is a problem, however this is an area of the code with broad impacts as it deals with how classes are named. To that end, there is some work that needs to be done in understanding the impacts before we can consider it.

Some questions we would like to answer:

1) According to Rich, naming of (fn x []) function classes used to work in the manner of this patch - with generated names. Some code archaeology needs to be done on why that was changed and whether the change to the current behavior was addressing problems that we are likely to run into.

2) Are there issues with recursive functions? Are there impacts either in AOT or non-AOT use cases? Need some tests.

3) Are there issues with dynamic redefinition of functions? With the static naming scheme, redefinition causes a new class of the same name which can be picked up by reload of classes compiled to the old definition. With the dynamic naming scheme, redefinition will create a differently named class so old classes can never pick up a redefinition. Is this a problem? What are the impacts with and without AOT? Need some tests.

Comment by Nicola Mometto [ 24/Jan/14 11:39 AM ]

Looks like the current behaviour has been such since https://github.com/clojure/clojure/commit/4651e60808bb459355a3a5d0d649c4697c672e28

My guess is that Rich simply forgot to consider the (def y (fn x [] ..)) case.

Regarding 2 and 3, the dynamic naming scheme is no different than what happens for anonymous functions so I don't see how this could cause any issue.

Recursion on the fn arg is simply a call to .invoke on "this", it's classname unaware.

I can add some tests to test that

(def y (fn x [] 1))
and
(fn x [] 2)
compile to different classnames but other than that I don't see what should be tested.

Comment by Stuart Halloway [ 27/Jun/14 2:17 PM ]

incomplete pending the answers to Alex Miller's questions in the comments

Comment by Nicola Mometto [ 27/Jun/14 3:20 PM ]

I believe I already answered his questions, I'll try to be a bit more explicit:
I tracked the relevant commit from Rich which added the dynamic naming behaviour https://github.com/clojure/clojure/commit/4651e60808bb459355a3a5d0d649c4697c672e28#diff-f17f860d14163523f1e1308ece478ddbL3081 which clearly shows that this bug was present since then so.

Regarding redefinitions or recursive functions, both of those operations never take in account the generated fn name so they are unaffected.

Comment by Alex Miller [ 12/Sep/14 4:32 PM ]

Summarizing some cases here from before/after the patch:

1) top-level fn (always has name)
	1.6 - namespace$name
	patch - namespace$name
2) non-top-level fn with name
	1.6 - namespace$name (collides with prior case)
	patch - namespace$topname__x__name  	<-- CHANGED
3) anonymous fn (no name)
	1.6 - namespace$name$fn__x
	patch - namespace$name$fn__x
4) top-level anonymous fn (no name, not at all useful :)
	1.6 - namespace$fn__x
	patch - namespace$fn__x

The key problem is that the first 2 cases produce the identical class name on 1.6. The patch alters the non-top-level named fn so there is no conflict.

Prior to the referenced old commit, I believe cases 1 and 2 would both produce namespace$name__x (where x is unique) so they would not collide. The change was made to prevent the top-level name from changing ("don't append numbers on top-level fn class names"). While the similar change was made on non-top-level fn names, I do not think it needed to be.

I've thought through (and tried) a bunch of the implications of this with the help of Nicola's comments above and I do not see an issue with any of the things I've considered. From a binary compatibility point of view with existing AOT code, old code compiled together should be self-consistent and continue to work. New compiled code will also be consistent. I can't think of a way that new code would expect to know the old name of a non-top-level function such that there could be an issue.

One question - why change the code such that the new class name is namespace$name$topname__x__name instead of namespace$name$topname_name__x (or something else?). And relatedly, while the diff is small, could we refactor a couple more lines to make the intent and cases clearer?

I am 90% ok with this patch but want a little thought into that question before I mark screened.

Comment by Nicola Mometto [ 12/Sep/14 4:47 PM ]

Alex, the attached patch munges into ns$topname__name__x, not into ns$topname__x__name.

Comment by Nicola Mometto [ 12/Sep/14 5:22 PM ]

The attached patch 0001-Fix-CLJ-1330refactored.patch contains the same fix from 0001-FixCLJ-1330-make-top-level-named-functions-classnam.patch but also refactors the code that deals with fn name munging

Comment by Alex Miller [ 12/Sep/14 6:22 PM ]

Hmmm.. I will double-check. That's not why I recall seeing when I did AOT.

Comment by Nicola Mometto [ 12/Sep/14 7:26 PM ]

New patch 0001-CLJ-1093-v2.patch improves the fn naming scheme a lot.
I've threw together a number of test cases that show the improvement + bug fixes:

user=> (fn [])
;; pre:
#<user$eval1$fn__2 user$eval1$fn__2@4e13aa4e>
;; post: (no change)
#<user$eval1$fn__3 user$eval1$fn__3@3c92218c>
user=> (fn a [])
;; pre:
#<user$eval5$a__6 user$eval5$a__6@6946a317>
;; post: (no change)
#<user$eval6$a__8 user$eval6$a__8@6f85c59c>
user=> (let [a (fn [])] a)
;; pre:
#<user$eval9$a__10 user$eval9$a__10@15fdf894>
;; post: (no change)
#<user$eval11$a__13 user$eval11$a__13@4d051922>
user=> (let [a (fn x [])] a)
;; pre: (only contains the name of the fn)
#<user$eval17$x__18 user$eval17$x__18@7f0cd67f>
;; post: (contains the name of the local aswell as the name of the fn
#<user$eval21$a__x__23 user$eval21$a__x__23@528ef256>
user=> (def a (fn [])) a
#'user/a
;; pre:
#<user$a user$a@33e1ccbc>
;; post: (no change)
#<user$a user$a@6bef63f9>
user=> (def a (fn x [])) a
#'user/a
;; pre: (BUG!)
#<user$x user$x@59a04a1b> 
;; post: (bug fixed)
#<user$a__x__28 user$a__x__28@5f0bebef>
user=> (def ^{:foo (fn [])} a) (-> (meta #'a) :foo)
#'user/a
;; pre:
#<user$fn__23 user$fn__23@d9c21c6>
;; post: (no change)
#<user$fn__30 user$fn__30@4cf0f2eb>
user=> (def ^{:foo (fn a [])} a) (-> (meta #'a) :foo)
#'user/a
;; pre: (BUG!)
#<user$a user$a@420dd874>
;; post: (bug fixed)
#<user$a__35 user$a__35@37ff95a9>
user=> (def a (fn [] (fn []))) (a)
#'user/a
;; pre:
#<user$a$fn__30 user$a$fn__30@6f57be76>
;; post: (no change)
#<user$a$fn__41 user$a$fn__41@fd34eac>
user=> (def a (fn [] (fn x []))) (a)
#'user/a
;; pre:
#<user$a$x__35 user$a$x__35@79930089>
;; post: (no change)
#<user$a$x__48 user$a$x__48@6fc334de>
user=> (let [x (fn [])] (def a (fn [] x))) a (a)
#'user/a
;; pre:
#<user$eval40$a__43 user$eval40$a__43@6db1694e>
#<user$eval40$x__41 user$eval40$x__41@20bd16bb>
;; post (no change)
#<user$eval54$a__58 user$eval54$a__58@7c721de>
#<user$eval54$x__56 user$eval54$x__56@43f7b41b>
user=> (let [x (fn a [])] (def a (fn [] x))) (a)
#'user/a
;; pre: (the local binding name doesn't appear in the class name)
#<user$eval48$a__49 user$eval48$a__49@75d6d1d4>
;; post: (the local binding name is included in the class name)
#<user$eval64$x__a__66 user$eval64$x__a__66@460d4>

As you can see, this last patch not only fixes the two bugs, but also improves fn naming in let contexts by preserving the name of the local binding in the class name, this I believe will be a great improvement in the understandability of stacktraces.

Comment by Alex Miller [ 25/Sep/14 7:00 AM ]

The patch should be changed to not create suffix if it's not going to be used. Please update the patch to inline that into each branch name = nm.name + "__" + RT.nextID();.

I am unsure whether the "enhancement" part of this patch goes too far. I think it does provide some improvements in debugging but those seem small to me. I am somewhat concerned about greatly increasing the name of the class for nested locals thus making it harder to read stack traces. There is a large limit to class name size of 16 bits (what you can put in the constant table) but class names also map to file names and there have historically been issues on some older Windows architectures with file size limits - we are increasing the risk of running into problems with this. Small risks. I am ok with passing this on to Rich though and he can decide whether to kick that part back or not.

Comment by Nicola Mometto [ 25/Sep/14 7:08 AM ]

0001-CLJ-1093-v3.patch is identical to 0001-CLJ-1093-v2.patch except it doesn't call RT.nextID() when not necessary, as per Alex's request

Alex, if this is ok please change the "Patch:" field in the description, I won't do that myself since this ticket is now screened





[CLJ-1534] Adding condp-> and condp->> macros to core library Created: 24/Sep/14  Updated: 24/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Kuldeep Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: enhancement, macro

Attachments: File condp-threading-macros-25sept2014.diff    
Patch: Code

 Description   

After introduction of cond-> and cond->> macros in 1.5. It makes sense to have condp-> and condp->> macros in the core library.

(condp-> {}
(complement :a) (assoc :a 1)
:a (assoc :b 2)) ;=> {:b 2, :a 1}

In the above example the result of each expr which was evaluated is being passed to the next predicate.






[CLJ-1290] clojure.xml parse docstring omits InputSource Created: 01/Nov/13  Updated: 24/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, newbie, xml

Approval: Triaged

 Description   

The clojure.xml parse docstring mentions that parameter s "can be a File, InputStream or String naming a URI." But those choices do not cover a common case, parsing the value of a String. Actually, parse also allows InputSource, which solves the problem. The docstring should mention InputSource (or clarify its omission, if not inadvertent).

user> (use '[clojure.xml :as xml])
nil
user> (import '[java.io StringReader])
java.io.StringReader
user> (import '[org.xml.sax InputSource])
org.xml.sax.InputSource
user> (xml/parse (InputSource. (StringReader. "<egg>green</egg>")))
{:tag :egg, :attrs nil, :content ["green"]}


 Comments   
Comment by Édipo L Féderle [ 15/Sep/14 3:57 PM ]

You and mean that de (doc xml/parse) should include also "can be a xml String" ?
I don't know if I understand you right.
Thanks.

Comment by Phill Wolf [ 24/Sep/14 6:06 AM ]

InputSource is the use of xml/parse that is not encompassed by the docstring:

(xml/parse (InputSource. (StringReader. "<egg>green</egg>")))

Perhaps xml/parse aimed to hide InputSource by making specific provision for some of InputSource's capabilities. But reading a String is important, and xml/parse does not accept a StringReader, so InputSource remains important.





[CLJ-1503] allow for `{~@foo} and `#{~(gensym) ~(gensym)} Created: 14/Aug/14  Updated: 24/Sep/14  Resolved: 24/Sep/14

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

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

Attachments: Text File 0001-allow-for-foo-and-gensym-gensym.patch    
Patch: Code

 Description   

Currently both `{@foo} and `#{(gensym) ~(gensym)} throw an exception at read time even though they could actually return valid run-time code.
This patch introduces the SyntaxQuotedMap and SyntaxQuotedSet classes that are used internally in the reader to represent syntax quoted maps and sets, that may skip the duplicate key and length checks.

The SyntaxQuotedMap extends PersistentArrayMap as a workaround for the lack of defrecords in java, since a SyntaxQuotedMap needs to be an IPersistentMap in case it's used as metadata.



 Comments   
Comment by Nicola Mometto [ 24/Sep/14 5:04 AM ]

Duplicated of http://dev.clojure.org/jira/browse/CLJ-1425
Also I no longer think this is a good idea





[CLJ-1532] pr-str captures stdout from printing side-effects of lazily evaluated expressions. Created: 23/Sep/14  Updated: 23/Sep/14

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

Type: Defect Priority: Minor
Reporter: Silas Davis Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print
Environment:

Linux



 Description   

Because clojure.core/pr-str uses with-out-str to capture the output of pr (and pr cannot be parsed a writable thing - just uses out).

If you pr-str the result of something lazy you can get side-effects written to stdout with println interspersed with the output. For example in my case I was extracting benchmarks from the library criterium and trying to print the data structure to the file. The solution would be to provide an overload of pr/pr-str that takes a writer. I note that pr-on provides some of the functionality but it is private.

This is an ugly bug when you're trying to persist program output in EDN, because the randomly interspersed stdout messages make it invalid for read-string. We shouldn't need our functions to be pure for pr-str to work as expected.

I've omitted a patch because although I think a fix is straight-forward I'm not sure quite where it should go (e.g. make pr-on public, change pr, change pr-str)






[CLJ-1531] inc always warns when *unchecked-math* is set Created: 23/Sep/14  Updated: 23/Sep/14  Resolved: 23/Sep/14

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

Type: Defect Priority: Major
Reporter: Pierre-Yves Ritschard Assignee: Pierre-Yves Ritschard
Resolution: Not Reproducible Votes: 0
Labels: bug, errormsgs


 Description   

While testing 1.7-alpha2 I stumbled this (affects clojure.data.codec amongst others). inc inlines a call to clojure.lang.Numbers's inc method which according to the rules of CLJ-1325 will warn.

I can't find a way around it for now, except maybe having a primitive-inc and primitive-dec java method which would be inlined in that case.

Happy to work on a patch but would prefer discussing it first.



 Comments   
Comment by Nicola Mometto [ 23/Sep/14 12:42 PM ]

I cannot reproduce this:

Clojure 1.7.0-master-SNAPSHOT
user=> (set! *unchecked-math* true)
true
user=> (inc 1)
2

Looking at Numbers.java I see both unchecked_inc and inc have long/double taking methods.

Comment by Pierre-Yves Ritschard [ 23/Sep/14 1:49 PM ]

you're right, i must have been confused.

Comment by Pierre-Yves Ritschard [ 23/Sep/14 1:50 PM ]

not a bug





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

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

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

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


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

 Description   

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

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

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



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

reproduces with jdk 1.8.0 and clojure 1.6

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Hmm, I cannot reproduce your results.

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

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

I added a medium and small (range) too.

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

I pasted the results side by side for easier viewing.

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

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

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

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

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

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

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

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

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

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

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

nor for intrisics like (inc a)

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

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

Comment by Ghadi Shayban [ 04/Aug/14 2:55 PM ]

A form like the following will not work with this patch:

(go (doseq [c chs] (>! c :foo)))

as the go macro doesn't traverse fn boundaries. The only such code I know is core.async/mapcat*, a private fn supporting a fn that is marked deprecated.

Comment by Ghadi Shayban [ 07/Aug/14 2:09 PM ]

I see #'clojure.core/run! was just added, which has a similar limitation

Comment by Rich Hickey [ 29/Aug/14 8:19 AM ]

Please consider Ghadi's feedback, esp re: closures.

Comment by Ghadi Shayban [ 22/Sep/14 4:36 PM ]

The current expansion of a doseq [1] under a go form is less than ideal due to the amount of control flow. 14 states in the state machine vs. 7 with loop/recur

[1] Comparison of macroexpansion of (go ... doseq) vs (go ... loop/recur)
https://gist.github.com/ghadishayban/639009900ce1933256a1





[CLJ-1530] Make foo/bar/baz unreadable Created: 22/Sep/14  Updated: 22/Sep/14

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

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

Attachments: Text File 0001-fix-LispReader-and-EdnReader-so-that-foo-bar-baz-is-.patch    
Patch: Code and Test

 Description   

Currently keywords and symbols containing more than one slash are disallowed by the spec, but allowed by the readers.
This trivial patch makes them unreadable by the readers too.

Pre:

user=> :foo/bar/baz
:foo/bar/baz

Post:

user=> :foo/bar/baz
RuntimeException Invalid token: :foo/bar/baz  clojure.lang.Util.runtimeException (Util.java:221)


 Comments   
Comment by Andy Fingerhut [ 22/Sep/14 12:14 PM ]

Perhaps overlap with CLJ-1527 ?





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






[CLJ-1400] Error "Can't refer to qualified var that doesn't exist" should name the bad symbol Created: 09/Apr/14  Updated: 22/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: Compiler, errormsgs
Environment:

OS X


Attachments: File clj-1400-2.diff     File clj-1400-3.diff     File clj-1400-4.diff    
Patch: Code and Test
Approval: Screened

 Description   

Def of var with a ns that doesn't exist will yield this error:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Can't refer to qualified var that doesn't exist, compiling:(NO_SOURCE_PATH:1:1)

Cause: Compiler.lookupVar() returns null if the ns in a qualified var does not exist yet.

Proposed: The error message would be improved by naming the symbol and throwing a CompilerException with file/line/col info. It's not obvious, but this may be the only case where this error occurs. If so, the error message could be more specific that the ns is the part that doesn't exist.

Patch: clj-1400-4.diff

Screened by: Alex Miller



 Comments   
Comment by Scott Bale [ 25/Jun/14 9:58 AM ]

This looks to me like relatively low hanging fruit unless I'm missing something; assigning to myself.

Comment by Scott Bale [ 26/Jun/14 11:23 PM ]

Patch clj-1400-1.diff to Compiler.java.

With this patch the example would now look like:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

I'm not sure the if(namesStaticMember(sym)) [see below], and the 2nd branch, is even necessary. Just by inspection I suspect it is not.

[footnote]

public static boolean namesStaticMember(Symbol sym){
	return sym.ns != null && namespaceFor(sym) == null;
}
Comment by Scott Bale [ 26/Jun/14 11:24 PM ]

patch: code and test

Comment by Scott Bale [ 26/Jun/14 11:27 PM ]

I tested on an actual source file, and the exception message included the file/line/col info as desired:

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)
Comment by Andy Fingerhut [ 29/Aug/14 4:46 PM ]

Patch clj-1400-1.diff dated Jun 26 2014 no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. It did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch. See section "Updating Stale Patches" on this wiki page for some tips on updating patches: http://dev.clojure.org/display/community/Developing+Patches

Comment by Scott Bale [ 31/Aug/14 3:53 PM ]

Attached is an updated patch: "clj-1400-2.diff". I removed the stale patch.

Comment by Alex Miller [ 09/Sep/14 9:29 AM ]

Few comments to address:

  • Compiler diff was using spaces, not tabs, which makes it harder to diff. I attached a -3.diff that fixes this.
  • the call to namesStaticMember seems weird. The name of that method is confusing for this use. Beyond that, I think it's doing more than you need. That method is going to attempt resolve the qualified name in terms of the current ns, but I think you don't even want to do that. Rather you just want to know if the sym has a ns (sym.ns != null) - isn't that enough?
  • In what case will the other error "Var doesn't exist" occur? In other words, in what case will lookupVar not succeed in creating a new var here? If there is no such case, then remove this case. If there is such a case, then add a test.
Comment by Scott Bale [ 11/Sep/14 11:19 PM ]

Agree with all three of your bullets. Attached is an updated patch, clj-1400-4.diff.

  • I used tabs in Compiler.java
  • After close inspection of call to lookupVar(...), I believe null is returned only in the case of exactly this ticket (the symbol having a non-null namespace which has not been loaded yet). So I've taken out the conditional and the 2nd branch.
  • (Test is unchanged)
Comment by Scott Bale [ 11/Sep/14 11:22 PM ]

(properly named patch)

Comment by Alex Miller [ 11/Sep/14 11:37 PM ]

You could throw a CompilerException with the location of the problem instead (as the ticket description suggests).

Comment by Scott Bale [ 19/Sep/14 2:37 PM ]

Sorry, I should've mentioned because this wasn't obvious to me either (and in fact I forgot until just now): the RuntimeException is already caught and wrapped in a CompilerException.

I'm not sure which try-catch block within Compiler.java this is happening in, there are multiple. But you can see in the output that the exception is a CompilerException and the file|line|col info is there:

In the Repl...

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

...or in a source file

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)

Also, at the point at which the RuntimeException of this patch is being thrown, the source line and col params to CompilerException are not available, or at least not afaict.





[CLJ-1528] clojure.test/inc-report-counter is not thread safe Created: 19/Sep/14  Updated: 22/Sep/14

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

Type: Defect Priority: Minor
Reporter: Alexander Redington Assignee: Alexander Redington
Resolution: Unresolved Votes: 0
Labels: test
Environment:

OS X, Clojure 1.7, Macbook pro


Attachments: File fix-CLJ-1528.diff    
Patch: Code
Approval: Triaged

 Description   

clojute.test/inc-report-counter, as implemented at https://github.com/clojure/clojure/blob/919a7100ddf327d73bc2d50d9ee1411d4a0e8921/src/clj/clojure/test.clj#L313, is not thread safe.

The commute operation described combines dereferencing the report-counters ref and operating on the previous state of the ref, leading to race conditions during concurrent access.

Specifically, the report-counters ref is dereferenced on 320, instead of the commute function operating entirely as a function of its inputs.



 Comments   
Comment by Alexander Redington [ 19/Sep/14 10:58 AM ]

Fixes 1528





[CLJ-1526] clojure.core/> inconsistent behavior wrt to documentation. Created: 17/Sep/14  Updated: 22/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Phillip Lord Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: math


 Description   

The > function is inconsistent wrt to their behaviour for 0 arity.

user> (doc >)
-------------------------
clojure.core/>
([x] [x y] [x y & more])
  Returns non-nil if nums are in monotonically decreasing order,
  otherwise false.
nil
user> (> 3 2)
true
user> (> 3)
true
user> (>)
ArityException Wrong number of args (0) passed to: core/>  clojure.lang.AFn.throwArity (AFn.java:429)

This is mostly likely to become problematic when using > via apply where

(or (= 0 (count l))
    (apply > l))

It seems that the documentation should be updated, 0-arg case should return true, or the 1-arg case should also throw an exception.

This affects the other comparators also.



 Comments   
Comment by Robert Tweed [ 17/Sep/14 9:48 AM ]

As per my original post on this (here: https://groups.google.com/d/msg/clojure/8zkpO9FBN64/u2LAQsR93IgJ), while the question of whether an empty set has monotonic order perhaps has more than one answer in theory, from a purely pragmatic engineering perspective, it makes the most sense to evaluate to true here.

This /should/ not be a breaking change. Therefore it is fairly safe to introduce into a minor revision. It's a also a trivial fix. But it is possible (though highly unlikely) that someone could have code that depends on the exception being raised at runtime (as it does now) to handle empty lists in some special way. Such code is horrible and ought to be rewritten, so should not be seen as justification for retaining the current behaviour, which limits the general usefulness of these functions and may be responsible for subtle bugs in existing production code.

However such a change should probably not be backported to existing 1.6.x branches, just to be 100% safe, since it is not a security issue. My suggestion therefore would be to add a note to the docs in existing maintenance branches (any future 1.6.x) and evaluate to true in future versions (1.7+).





[CLJ-1525] bean function returns mutable maps Created: 16/Sep/14  Updated: 22/Sep/14  Resolved: 22/Sep/14

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

Type: Defect Priority: Major
Reporter: Simone Mosciatti Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

Linux



 Description   

Please take a look at this snippet.

user> (import 'java.util.Date)
java.util.Date
user> (def now (Date.))
#'user/now
user> now
#inst "2014-09-17T03:14:13.821-00:00"
user> (def bean-map (bean now))
#'user/bean-map
user> bean-map
{:day 3, :date 17, :time 1410923653821, :month 8, :seconds 13, :year 114, :class java.util.Date, :timezoneOffset -120, :hours 5, :minutes 14}
user> (.setMonth now 1)
nil
user> bean-map
{:day 1, :date 17, :time 1392610453821, :month 1, :seconds 13, :year 114, :class java.util.Date, :timezoneOffset -60, :hours 5, :minutes 14}

The same snippet here. https://gist.github.com/siscia/032bff669bbc6fb0fe57



 Comments   
Comment by Jozef Wagner [ 17/Sep/14 1:32 AM ]

It works as expected. bean fn returns a clojuresque abstraction on top of live bean. map-like abstraction returned from bean is intended to be 'mutable' in sense that it always return the latest value. Otherwise it is read only.

Comment by Simone Mosciatti [ 17/Sep/14 1:42 AM ]

Hi,

sorry, the documentation didn't mention the "mutable" part so I was expecting an immutable map as always.

Sorry, about that.

Greets

Comment by Alex Miller [ 22/Sep/14 9:38 AM ]

this is the expected behavior





[CLJ-979] map->R returns different class when invoked from AOT code Created: 03/May/12  Updated: 22/Sep/14

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

Type: Defect Priority: Major
Reporter: Edmund Jackson Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: aot
Environment:

Mac OS X 10.5, lein 1.7 and lein 2.0


Attachments: Text File clj-979-symptoms.patch    

 Description   

Compiling a class via `deftype` during AOT compilation gives different results for the different constructors. These hashes should be identical.

user=> (binding [*compile-files* true] (eval '(deftype Abc [])))
user.Abc
user=> (hash Abc)
16446700
user=> (hash (class (->Abc)))
31966239


 Comments   
Comment by Scott Lowe [ 12/May/12 9:05 PM ]

I can't reproduce this under Clojure 1.3 or 1.4, and Leiningen 1.7.1 on either Java 1.7.0-jdk7u4-b21 OpenJDK 64-Bit or Java 1.6.0_31 Java HotSpot 64-Bit. OS is Mac OS X 10.7.

Edmund, how are you running this AOT code? I wrapped your code in a main function and built an uberjar from it.

Comment by Edmund Jackson [ 13/May/12 2:20 AM ]

Hi Scott,

Interesting.

I have two use cases
1. AOT compile and call from repl.
My steps: git clone, lein compile, lein repl, (use 'aots.death), (in-ns 'aots.death), (= (class (Dontwork. nil)) (class (map->Dontwork {:a 1}))) => false

2. My original use case, which I've minimised here, is an AOT ns, producing a genclass that is called instantiated from other Java (no main). This produces the same error. I will produce an example of this and post it too.

Comment by Edmund Jackson [ 13/May/12 4:23 AM ]

Hi Scott,

Here is an example of it failing in the interop case: https://github.com/ejackson/aotquestion2
The steps I'm following to compile this all up are

git clone git@github.com:ejackson/aotquestion2.git
cd aotquestion2/cljside/
lein uberjar
lein install
cd ../javaside/
mvn package
java -jar ./target/aotquestion-1.0-SNAPSHOT.jar

and it dies with this:

Exception in thread "main" java.lang.ClassCastException: cljside.core.Dontwork cannot be cast to cljside.core.Dontwork
at cljside.MyClass.makeDontwork(Unknown Source)
at aotquestion.App.main(App.java:8)

The error message is really confusing (to me, anyway), but I think its the same root problem as for the REPL case.

What do you see when you run the above ?

Comment by Scott Lowe [ 13/May/12 8:41 AM ]

Ah, yes, looks like my initial attempt to reproduce was too simplistic. I used your second git repo, and can now confirm that it's failing for me with the same error.

Comment by Scott Lowe [ 13/May/12 10:35 PM ]

I looked into this a little further and the AOT generated code looks correct, in the sense that both code paths appear to be returning the same type.

However, I wonder if this is really a ClassLoader issue, whereby two definitions of the same class are being loaded at different times, because that would cause the x.y.Class cannot be cast to x.y.Class exception that we're seeing here.

Comment by Steve Miner [ 03/Sep/13 9:54 AM ]

This could be related to CLJ-1157 which deals with a ClassLoader issue with AOT compiled code.

Comment by Ambrose Bonnaire-Sergeant [ 29/Mar/14 1:11 PM ]

I've tried this patch attached to CLJ-1157 and it did not solve this issue.

Comment by Ambrose Bonnaire-Sergeant [ 29/Mar/14 2:27 PM ]

This bug seems to be rooted in different behaviour for do/let under compilation. Attached a patch showing these symptoms in the hope it helps people find the cause.

Comment by Peter Taoussanis [ 22/Sep/14 3:12 AM ]

Just a quick note to confirm that this still seems to be around as of Clojure 1.7.0-alpha2. Don't have any useful input on possible solutions, sorry.





[CLJ-304] clojure.repl/source does not work with deftype Created: 20/Apr/10  Updated: 19/Sep/14

Status: In Progress
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: repl


 Description   

clojure.repl/source does not work on a deftype

user> (deftype Foo [a b])
user.Foo
user> (source Foo)
Source not found

Cause: deftype creates a class but not a var so no file/line info is attached anywhere.

Approach:

Patch:

Screened by:



 Comments   
Comment by Assembla Importer [ 24/Aug/10 4:38 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/304

Comment by Assembla Importer [ 24/Aug/10 4:38 PM ]

chouser@n01se.net said: That's a great question. get-source just needs a file name and line number.

If IMeta were a protocol, it could be extended to Class. That implementation could look for a "well-known" static field, perhaps? __clojure_meta or something? Then deftype would just have to populate that field, and get-source would be all set.

Does that plan have any merit? Is there a better place to store a file name and line number?

Comment by Assembla Importer [ 24/Aug/10 4:38 PM ]

stu said: Seems like a reasonable idea, but this is going to get back-burnered for now, unless there is a dire use case we have missed.

Comment by Gary Trakhman [ 19/Feb/14 10:31 AM ]

I could use this for cider's file/line jump-around mechanism as well.

With records, I can work around it by deriving and finding the corresponding constructor var, but it's a bit nasty.

Comment by Bozhidar Batsov [ 03/Mar/14 6:37 AM ]

I'd also love to see this fixed.

Comment by Andy Fingerhut [ 03/Mar/14 8:33 AM ]

Bozhidar, voting on a ticket (clicking the Vote link in the right of the page when viewing the ticket) can help push it upwards on listings of tickets by # of votes.

Comment by Bozhidar Batsov [ 19/Sep/14 1:17 PM ]

Andy, thanks for the pointer. They should have made this button much bigger, I hadn't noticed it all until now.





[CLJ-668] Improve slurp performance by using native Java StringWriter and jio/copy Created: 01/Nov/10  Updated: 19/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Jürgen Hötzel Assignee: Timothy Baldridge
Resolution: Unresolved Votes: 0
Labels: ft, io, performance

Attachments: File slurp-perf-patch.diff    
Approval: Triaged

 Description   

Instead of copying each character from InputReader to StringBuffer.

Performance improvement:

Generate a 10meg file:
user> (spit "foo.txt" (apply str (repeat (* 1024 1024 10) "X")))

Test code:
user> (dotimes [x 100] (time (do (slurp "foo.txt") 0)))

From:
...
Elapsed time: 136.387 msecs"
"Elapsed time: 143.782 msecs"
"Elapsed time: 153.174 msecs"
"Elapsed time: 211.51 msecs"
"Elapsed time: 155.429 msecs"
"Elapsed time: 145.619 msecs"
"Elapsed time: 142.641 msecs"
...


To:
...
"Elapsed time: 23.408 msecs"
"Elapsed time: 25.876 msecs"
"Elapsed time: 41.449 msecs"
"Elapsed time: 28.292 msecs"
"Elapsed time: 25.765 msecs"
"Elapsed time: 24.339 msecs"
"Elapsed time: 32.047 msecs"
"Elapsed time: 23.372 msecs"
"Elapsed time: 24.365 msecs"
"Elapsed time: 26.265 msecs"
...

Approach: Use StringWriter and jio/copy vs character by character copy. Results from the current patch see a 4-5x perf boost after the jit warms up, with purely in-memory streams (ByteArrayInputStream over a 6MB string).

Patch: slurp-perf-patch.diff



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

This is double-better with the changes in Clojure 1.6 to improve jio/copy performance by using the NIO impl. Rough timing difference on a 25M file: old= 2316.021 msecs, new= 93.319 msecs.

Filer did not supply a patch and is not a contributor. If someone wants to make a patch (and better timing info demonstrating performance improvements), that would be great.

Comment by Timothy Baldridge [ 10/Sep/14 10:29 PM ]

Fixed the ticket formatting a bit, and added a patch I coded up tonight. Should be pretty close to the old patch, as we both use StringWriter, but I didn't really look at the old patch beyond noticing that it was using StringWriter.

Comment by Alex Miller [ 11/Sep/14 7:01 AM ]

Can you update the perf comparison on latest code and do both a small and big file?





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

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

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


 Description   

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

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

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

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

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

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



 Comments   
Comment by Nahuel Greco [ 18/Sep/14 6:08 PM ]

It also breaks when extending only one array type:

(extend-protocol P
  String               (p [_] "string")
  (Class/forName "[B") (p [_] "ints") 
  )

;=> CompilerException java.lang.UnsupportedOperationException: nth not supported on this type ...

But changing the declaration order fixes it:

(extend-protocol P
  (Class/forName "[B") (p [_] "ints") 
  String               (p [_] "string")
  )

;=> OK




[CLJ-1486] Make fnil var-arg Created: 31/Jul/14  Updated: 18/Sep/14

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

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

Attachments: Text File 0001-make-fnil-vararg.patch    
Patch: Code and Test

 Description   

Currently fnil is defined only for 1 to 3 args, this patch makes it var-arg






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

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

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

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

 Description   

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

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


 Comments   
Comment by Andy Fingerhut [ 06/Aug/14 2:19 PM ]

Patch 0001-Add-2-arity-version-of-cycle-that-takes-the-number-o.patch dated Apr 28 2014 no longer applies cleanly to latest Clojure master due to some changes committed earlier today. This appears trivial to update, as it is likely only a couple of lines of diff context that have changed.

Comment by Nicola Mometto [ 06/Aug/14 2:36 PM ]

Updated patch to apply to HEAD





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

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

Type: Enhancement Priority: Minor
Reporter: Phillip Lord Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs, newbie

Attachments: Text File deref.patch     File tests-patch.diff    
Approval: Triaged

 Description   

Consider the following code:

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

(+ @x y)

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



 Comments   
Comment by Tobias Kortkamp [ 15/Sep/14 2:08 PM ]

Attached a patch with better error messages for deref. The above example now throws:

IllegalArgumentException class java.lang.Long is not derefable  clojure.core/deref (core.clj:2211)

and e.g.

(deref (delay 1) 500 :foo)

throws

IllegalArgumentException class clojure.lang.Delay is not derefable with a timeout  clojure.core/deref (core.clj:2222)
Comment by Mark Nutter [ 16/Sep/14 6:57 PM ]

Patch file clj-1436-patch-2014-09-16.diff updates the deref function so that it checks whether its arg is a future before sending it to deref-future. It also updates the deref function to provide clearer error messages. If the arg is not a future, and does not implement IDeref, the patched version of deref throws an IllegalArgumentException with a message that the arg cannot be dereferenced because it is not a ref/future/etc.

Comment by Mark Nutter [ 16/Sep/14 7:00 PM ]

Oops, I had this page open from yesterday and didn't see the patch submitted by Tobias. His has everything mine does, so I'll withdraw mine.

Comment by Mark Nutter [ 17/Sep/14 5:13 AM ]

One suggestion: the error message might sound better as "IllegalArgumentException cannot dereference clojure.lang.Delay; not a future or reference type".

Comment by Mark Nutter [ 17/Sep/14 5:44 AM ]

Tobias' patch does not contain the tests I had in mine, so I'm re-submitting just the tests as tests-patch.diff. If you install the tests patch without installing the deref patch, the tests will fail with the error message "Wrong exception type when passing non-IDeref/non-future to deref/@". Applying the deref patch as well will allow the tests to pass.





[CLJ-740] Unnecessary boxing of primitives in case form Created: 17/Feb/11  Updated: 15/Sep/14  Resolved: 15/Sep/14

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

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


 Description   

Found this while profiling some performance-critical code.

Consider the following Clojure function:

(defn test-case ^double [^long i ^double d1 ^double d2]
  (case (int i)
    0 d1
    d2))

Current Clojure 1.3 snapshot compiles it to:

public final double invokePrim(long, double, double)   throws java.lang.Exception;
  Code:
   0:	lload_1
   1:	invokestatic	#67; //Method clojure/lang/RT.intCast:(J)I
   4:	istore	7
   6:	iload	7
   8:	i2l
   9:	invokestatic	#73; //Method clojure/lang/Numbers.num:(J)Ljava/lang/Number;
   12:	invokestatic	#79; //Method clojure/lang/Util.hash:(Ljava/lang/Object;)I
   15:	iconst_0
   16:	ishr
   17:	iconst_1
   18:	iand
   19:	tableswitch{ //0 to 0
		0: 36;
		default: 58 }
   36:	iload	7
   38:	i2l
   39:	invokestatic	#73; //Method clojure/lang/Numbers.num:(J)Ljava/lang/Number;
   42:	getstatic	#45; //Field const__3:Ljava/lang/Object;
   45:	invokestatic	#83; //Method clojure/lang/Util.equals:(Ljava/lang/Object;Ljava/lang/Object;)Z
   48:	ifeq	58
   51:	dload_3
   52:	invokestatic	#88; //Method java/lang/Double.valueOf:(D)Ljava/lang/Double;
   55:	goto	63
   58:	dload	5
   60:	invokestatic	#88; //Method java/lang/Double.valueOf:(D)Ljava/lang/Double;
   63:	checkcast	#92; //class java/lang/Number
   66:	invokevirtual	#96; //Method java/lang/Number.doubleValue:()D
   69:	dreturn
}

This bytecode contains boxing of primitives (calls to clojure/lang/Numbers.num and java/lang/Double.valueOf) and calls to clojure/lang/Util.hash and clojure/lang/Util.equals that does not seem necessary.

At 60-66 primitive double is boxed into Double only to be converted back into primitive.

The equivalent Java code compiles to much simpler and faster bytecode:

public double testCase(long, double, double);
  Code:
   0:	lload_1
   1:	l2i
   2:	lookupswitch{ //1
		0: 20;
		default: 22 }
   20:	dload_3
   21:	dreturn
   22:	dload	5
   24:	dreturn
}


 Comments   
Comment by Alexander Taggart [ 28/Feb/11 2:16 PM ]

Improved via patch on CLJ-426.

(defn test-case ^double [^long i ^double d1 ^double d2]
  (case (int i)
    0 d1
    d2))

now emits as

 0  lload_1 [i]
 1  invokestatic clojure.lang.RT.intCast(long) : int [67]
 4  istore 7 [G__7903] // let-bound expression
 6  iload 7 [G__7903]
 8  tableswitch default: 32
      case 0: 28
28  dload_3 [d2]
29  goto 34
32  dload 5 [arg2]
34  dreturn

or if the int cast of the expression is omitted:

 0  lload_1 [i]
 1  lstore 7 [G__7903] // let-bound expression
 3  lload 7 [G__7903]
 5  l2i
 6  tableswitch default: 35
      case 0: 24
24  lconst_0           // match, verify long expr wasn't truncated
25  lload 7 [G__7903]
27  lcmp
28  ifne 35
31  dload_3 [d2]
32  goto 37
35  dload 5 [arg2]
37  dreturn
Comment by Ghadi Shayban [ 14/Sep/14 10:00 PM ]

I can't reproduce this on -master. No boxing here:

  public final double invokePrim(long i, double d2, double arg2);
     0  lload_1 [i]
     1  invokestatic clojure.lang.RT.intCast(long) : int [44]
     4  istore 7 [G__3439]
     6  iload 7 [G__3439]
     8  tableswitch default: 32
          case 0: 28
    28  dload_3 [d2]
    29  goto 34
    32  dload 5 [arg2]
    34  dreturn
      Line numbers:
        [pc: 0, line: 1]
        [pc: 0, line: 2]
        [pc: 6, line: 2]
      Local variable table:
        [pc: 6, pc: 34] local: G__3439 index: 7 type: int
        [pc: 0, pc: 34] local: this index: 0 type: java.lang.Object
        [pc: 0, pc: 34] local: i index: 1 type: long
        [pc: 0, pc: 34] local: d1 index: 2 type: double
        [pc: 0, pc: 34] local: d2 index: 3 type: double
Comment by Alex Miller [ 15/Sep/14 10:54 AM ]

The case stuff has been worked on several times (including in 1.6) since it's release, so it's likely this was fixed as a side effect of prior changes.





[CLJ-1152] PermGen leak in multimethods and protocol fns when evaled Created: 30/Jan/13  Updated: 15/Sep/14

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

Type: Defect Priority: Critical
Reporter: Chouser Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: memory, protocols

Attachments: File naive-lru-for-multimethods-and-protocols.diff     File protocol_multifn_weak_ref_cache.diff    
Patch: Code
Approval: Vetted

 Description   

There is a PermGen memory leak that we have tracked down to protocol methods and multimethods called inside an eval, because of the caches these methods use. The problem only arises when the value being cached is an instance of a class (such as a function or reify) that was defined inside the eval. Thus extending IFn or dispatching a multimethod on an IFn are likely triggers.

Reproducing: The easiest way that I have found to test this is to set "-XX:MaxPermSize" to a reasonable value so you don't have to wait too long for the PermGen spaaaaace to fill up, and to use "-XX:+TraceClassLoading" and "-XX:+TraceClassUnloading" to see the classes being loaded and unloaded.

leiningen project.clj
(defproject permgen-scratch "0.1.0-SNAPSHOT"
  :dependencies [[org.clojure/clojure "1.5.0-RC1"]]
  :jvm-opts ["-XX:MaxPermSize=32M"
             "-XX:+TraceClassLoading"
             "-XX:+TraceClassUnloading"])

You can use lein swank 45678 and connect with slime in emacs via M-x slime-connect.

To monitor the PermGen usage, you can find the Java process to watch with "jps -lmvV" and then run "jstat -gcold <PROCESS_ID> 1s". According to the jstat docs, the first column (PC) is the "Current permanent space capacity (KB)" and the second column (PU) is the "Permanent space utilization (KB)". VisualVM is also a nice tool for monitoring this.

Multimethod leak

Evaluating the following code will run a loop that eval's (take* (fn foo [])).

multimethod leak
(defmulti take* (fn [a] (type a)))

(defmethod take* clojure.lang.Fn
  [a]
  '())

(def stop (atom false))
(def sleep-duration (atom 1000))

(defn run-loop []
  (when-not @stop
    (eval '(take* (fn foo [])))
    (Thread/sleep @sleep-duration)
    (recur)))

(future (run-loop))

(reset! sleep-duration 0)

In the lein swank session, you will see many lines like below listing the classes being created and loaded.

[Loaded user$eval15802$foo__15803 from __JVM_DefineClass__]
[Loaded user$eval15802 from __JVM_DefineClass__]

These lines will stop once the PermGen space fills up.

In the jstat monitoring, you'll see the amount of used PermGen space (PU) increase to the max and stay there.

-    PC       PU        OC          OU       YGC    FGC    FGCT     GCT
 31616.0  31552.7    365952.0         0.0      4     0    0.000    0.129
 32000.0  31914.0    365952.0         0.0      4     0    0.000    0.129
 32768.0  32635.5    365952.0         0.0      4     0    0.000    0.129
 32768.0  32767.6    365952.0      1872.0      5     1    0.000    0.177
 32768.0  32108.2    291008.0     23681.8      6     2    0.827    1.006
 32768.0  32470.4    291008.0     23681.8      6     2    0.827    1.006
 32768.0  32767.2    698880.0     24013.8      8     4    1.073    1.258
 32768.0  32767.2    698880.0     24013.8      8     4    1.073    1.258
 32768.0  32767.2    698880.0     24013.8      8     4    1.073    1.258

A workaround is to run prefer-method before the PermGen space is all used up, e.g.

(prefer-method take* clojure.lang.Fn java.lang.Object)

Then, when the used PermGen space is close to the max, in the lein swank session, you will see the classes created by the eval'ing being unloaded.

[Unloading class user$eval5950$foo__5951]
[Unloading class user$eval3814]
[Unloading class user$eval2902$foo__2903]
[Unloading class user$eval13414]

In the jstat monitoring, there will be a long pause when used PermGen space stays close to the max, and then it will drop down, and start increasing again when more eval'ing occurs.

-    PC       PU        OC          OU       YGC    FGC    FGCT     GCT
 32768.0  32767.9    159680.0     24573.4      6     2    0.167    0.391
 32768.0  32767.9    159680.0     24573.4      6     2    0.167    0.391
 32768.0  17891.3    283776.0     17243.9      6     2   50.589   50.813
 32768.0  18254.2    283776.0     17243.9      6     2   50.589   50.813

The defmulti defines a cache that uses the dispatch values as keys. Each eval call in the loop defines a new foo class which is then added to the cache when take* is called, preventing the class from ever being GCed.

The prefer-method workaround works because it calls clojure.lang.MultiFn.preferMethod, which calls the private MultiFn.resetCache method, which completely empties the cache.

Protocol leak

The leak with protocol methods similarly involves a cache. You see essentially the same behavior as the multimethod leak if you run the following code using protocols.

protocol leak
(defprotocol ITake (take* [a]))

(extend-type clojure.lang.Fn
  ITake
  (take* [this] '()))

(def stop (atom false))
(def sleep-duration (atom 1000))

(defn run-loop []
  (when-not @stop
    (eval '(take* (fn foo [])))
    (Thread/sleep @sleep-duration)
    (recur)))

(future (run-loop))

(reset! sleep-duration 0)

Again, the cache is in the take* method itself, using each new foo class as a key.

Workaround: A workaround is to run -reset-methods on the protocol before the PermGen space is all used up, e.g.

(-reset-methods ITake)

This works because -reset-methods replaces the cache with an empty MethodImplCache.

Patch: multifn_weak_method_cache.diff

Screened by:



 Comments   
Comment by Chouser [ 30/Jan/13 9:10 AM ]

I think the most obvious solution would be to constrain the size of the cache. Adding an item to the cache is already not the fastest path, so a bit more work could be done to prevent the cache from growing indefinitely large.

That does raise the question of what criteria to use. Keep the first n entries? Keep the n most recently used (which would require bookkeeping in the fast cache-hit path)? Keep the n most recently added?

Comment by Jamie Stephens [ 18/Oct/13 9:35 AM ]

At a minimum, perhaps a switch to disable the caches – with obvious performance impact caveats.

Seems like expensive LRU logic is probably the way to go, but maybe don't have it kick in fully until some threshold is crossed.

Comment by Alex Miller [ 18/Oct/13 4:28 PM ]

A report seeing this in production from mailing list:
https://groups.google.com/forum/#!topic/clojure/_n3HipchjCc

Comment by Adrian Medina [ 10/Dec/13 11:43 AM ]

So this is why we've been running into PermGen space exceptions! This is a fairly critical bug for us - I'm making extensive use of multimethods in our codebase and this exception will creep in at runtime randomly.

Comment by Kevin Downey [ 17/Apr/14 9:52 PM ]

it might be better to split this in to two issues, because at a very abstract level the two issues are the "same", but concretely they are distinct (protocols don't really share code paths with multimethods), keeping them together in one issue seems like a recipe for a large hard to read patch

Comment by Kevin Downey [ 26/Jul/14 5:49 PM ]

naive-lru-method-cache-for-multimethods.diff replaces the methodCache in multimethods with a very naive lru cache built on PersistentHashMap and PersistentQueue

Comment by Kevin Downey [ 28/Jul/14 7:09 PM ]

naive-lru-for-multimethods-and-protocols.diff creates a new class clojure.lang.LRUCache that provides an lru cache built using PHashMap and PQueue behind an IPMap interface.

changes MultiFn to use an LRUCache for its method cache.

changes expand-method-impl-cache to use an LRUCache for MethodImplCache's map case

Comment by Kevin Downey [ 30/Jul/14 3:10 PM ]

I suspect my patch naive-lru-for-multimethods-and-protocols.diff is just wrong, unless MethodImplCache really is being used as a cache we can't just toss out entries when it gets full.

looking at the deftype code again, it does look like MethidImplCache is being used as a cache, so maybe the patch is fine

if I am sure of anything it is that I am unsure so hopefully someone who is sure can chime in

Comment by Nicola Mometto [ 31/Jul/14 11:02 AM ]

I haven't looked at your patch, but I can confirm that the MethodImplCache in the protocol function is just being used as a cache

Comment by dennis zhuang [ 08/Aug/14 6:21 AM ]

I developed a new patch that convert the methodCache in MultiFn to use WeakReference for dispatch value,and clear the cache if necessary.

I've test it with the code in ticket,and it looks fine.The classes will be unloaded when perm gen is almost all used up.

Comment by Alex Miller [ 22/Aug/14 4:55 PM ]

I don't know which to evaluate here. Does multifn_weak_method_cache.diff supersede naive-lru-for-multimethods-and-protocols.diff or are these alternate approaches both under consideration?

Comment by Kevin Downey [ 22/Aug/14 8:26 PM ]

the most straight forward thing, I think, is to consider them as alternatives, I am not a huge fan of weakrefs, but of course not using weakrefs we have to pick some bounding size for the cache, and the cache has a strong reference that could prevent a gc, so there are trade offs. My reasons to stay away from weak refs in general are using them ties the behavior of whatever you are building to the behavior of the gc pretty strongly. that may be considered a matter of personal taste

Comment by Andy Fingerhut [ 29/Aug/14 4:31 PM ]

All patches dated Aug 8 2014 and earlier no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. They did apply cleanly before that day.

I have not checked how easy or difficult it might be to update the patches.

Comment by Kevin Downey [ 29/Aug/14 7:00 PM ]

I've updated naive-lru-for-multimethods-and-protocols.diff to apply to the current master

Comment by Andy Fingerhut [ 29/Aug/14 7:34 PM ]

Thanks, Kevin. While JIRA allows multiple attachments to a ticket with the same filename but different contents, that can be confusing for people looking for a particular patch, and for a program I have that evaluates patches for things like whether they apply and build cleanly. Would you mind removing the older one, or in some other way making all the names unique?

Comment by Kevin Downey [ 29/Aug/14 8:43 PM ]

I deleted all of my attachments accept for my latest and greatest

Comment by dennis zhuang [ 30/Aug/14 9:51 AM ]

I updated multifn_weak_method_cache2.diff patch too.

I think using weak reference cache is better,because we have to keep one cache per multifn.When you have many multi-functions, there will be many LRU caches in memory,and they will consume too much memory and CPU for evictions. You can't choose a proper threshold for LRU cache in every environment.
But i don't have any benchmark data to support my opinion.

Comment by Alex Miller [ 10/Sep/14 2:38 PM ]

I'm going to set the LRU cache patch aside. I don't think it's possible to find a "correct" size for it and it seems weird to me to extend APersistentMap to build such a thing anyways.

I think it makes more sense to follow the same strategy used for other caches (such as the Keyword cache) - a combination ConcurrentHashMap with WeakReferences and a ReferenceQueue for clean-up. I don't see any compelling reason not to take the same path as other internal caches.

Comment by Alex Miller [ 10/Sep/14 3:44 PM ]

Stepping back a little to think about the problem.... our requirements are:
1) cache map of dispatch value (could be any Object) to multimethod function (IFn)
2) do we want keys to be compared based on equality or identity? identity-based opens up more reference-based caching options and is fine for most common dispatch types (Class, Keyword), but reduces (often eliminates?) cache hits for all other types where values are likely to be equiv but not identical (vector of strings for example)
3) concurrent access to cache
4) cache cannot grow without bound
5) cache cannot retain strong references to dispatch values (the cache keys) because the keys might be instances of classes that were loaded in another classloader which will prevent GC in permgen

multifn_weak_method_cache.diff uses a ConcurrentHashMap (#3) that maps RefWrapper around keys to IFn (#1). The patch uses Util.equals() (#2) for (Java) equality-based comparisons. The RefWrapper wraps them in WeakReferences to avoid #5. Cache clearing based on the ReferenceQueue is used to prevent #4.

A few things definitely need to be fixed:

  • Util.equals() should be Util.equiv()
  • methodCache and rq should be final
  • Why does RefWrapper have obj and expect rq to possibly be null?
  • RefWrapper fields should all be final
  • Whitespace errors in patch

Another idea entirely - instead of caching dispatch value, cache based on hasheq of dispatch value then equality check on value. Could then use WeakHashMap and no RefWrapper.

This patch does not cover the protocol cache. Is that just waiting for the multimethod case to look good?

Comment by dennis zhuang [ 10/Sep/14 7:18 PM ]

Hi, alex, thanks for your review.But the latest patch is multifn_weak_method_cache2.diff. I will update the patch soon by your review, but i have a few questions to be explained.

1) I will use Util.equiv() instead of Util.equals().But what's the difference of them?
2) When the RefWrapper is retained as key in ConcurrentHashMap, it wraps the obj in WeakReference.But when trying to find it in ConcurrentHashMap, it uses obj directly as strong reference, and create it with passing null ReferenceQueue.Please look at the multifn_weak_method_cache2.diff line number 112. It short, the patch stores the dispatch value as weak reference in cache,but uses strong reference for cache getting.

3) If caching dispatch value based on hasheq , can we avoid hasheq value conflicts? If two different dispatch value have a same hasheq( or why it doesn't happen?), we would be in trouble.

Sorry, the patch doesn't cover the protocol cache, i will add it ASAP.

Comment by dennis zhuang [ 11/Sep/14 2:02 AM ]

The new patch 'protocol_multifn_weak_ref_cache.diff' is uploaded.

1) Using Util.equiv() instead of Util.equals()
2) Moved the RefWrapper and it's associated methods to Util.java, and refactor the code based on alex's review.
3) Fixed whitespace errors.
4) Fixed PermGen leak in protocol fns.





[CLJ-1458] Use transients in merge and merge-with Created: 04/Jul/14  Updated: 14/Sep/14

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

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

Attachments: File transient-merge.diff    
Patch: Code
Approval: Triaged

 Description   

It would be nice if merge used transients.



 Comments   
Comment by Jason Wolfe [ 13/Sep/14 5:09 PM ]

I will take a crack at a patch today.

Comment by Jason Wolfe [ 13/Sep/14 5:42 PM ]

This patch (transient-merge.diff) makes merge, merge-with, and zipmap (since it was right there and could obviously benefit from transients as well) use transients.

Three potential issues:

  • I had to move the functions, since they depend on transient and friends. I assume this is preferable to a forward declaration. This was the best place I could find, but happy to move them elsewhere.
  • I added multiple arities, to avoid potential performance cost of transient-ing a single argument. Happy to undo this if desired.
  • I had to slightly alter the logic in merge-with, since transient maps don't support contains? (or find).
Comment by Michał Marczyk [ 14/Sep/14 12:43 PM ]

I posted a separate ticket for zipmap, with patch, on 30/May/12: CLJ-1005.

Comment by Jason Wolfe [ 14/Sep/14 5:28 PM ]

Ah, sorry if I overstepped then. Happy to remove that change from this patch then if that will simplify things – just let me know.





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

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

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

all environments


Patch: Code

 Description   

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

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

because it is expanded to:

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

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

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

now the previous example will expand to:

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

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

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

Compare with the original defmulti:

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


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

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

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

Comment by Nahuel Greco [ 13/Sep/14 6:15 AM ]

Related: you cannot use recur as the last expression of as->, because the macroexpansion will not place it at tail position. The fix proposed above also fixes that, so you can use something like:

(loop []
  (as-> [] x
        ;;  manipulate x
        (when (empty? x) (recur)))))




[CLJ-1093] Empty PersistentCollections get incorrectly evaluated as their generic clojure counterpart Created: 24/Oct/12  Updated: 12/Sep/14

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: collections, compiler

Attachments: Text File 0001-CLJ-1093-fix-compilation-of-empty-PersistentCollecti.patch     Text File 0001-CLJ-1093-v2.patch     Text File clj-1093-fix-empty-record-literal-patch-v2.txt    
Patch: Code and Test
Approval: Vetted

 Description   
user> (defrecord x [])
user.x
user> #user.x[]   ;; expect: #user.x{}
{}
user> #user.x{}   ;; expect: #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1)  ;; expect: clojure.lang.PersistentTreeMap
clojure.lang.PersistentArrayMap

Cause: Compiler's ConstantExpr parser returns an EmptyExpr for all empty persistent collections, even if they are of types other than the core collections (for example: records, sorted collections, custom collections). EmptyExpr reports its java class as one the classes - IPersistentList/IPersistentVector/IPersistentMap/IPersistentSet rather than the original type.

Proposed: If one of the Persistent* classes, then create EmptyExpr as before, otherwise retain the ConstantExpression of the original collection.
Since EmptyExpr is a compiler optimization that applies only to some concrete clojure collections, making EmptyExpr dispatch on concrete types rather than on generic interfaces makes the compiler behave as expected

Patch: 0001-CLJ-1093-v2.patch

Screened by:



 Comments   
Comment by Timothy Baldridge [ 27/Nov/12 11:41 AM ]

Unable to reproduce this bug on latest version of master. Most likely fixed by some of the recent changes to data literal readers.

Marking Not-Approved.

Comment by Timothy Baldridge [ 27/Nov/12 11:41 AM ]

Could not reproduce in master.

Comment by Nicola Mometto [ 01/Mar/13 1:23 PM ]

I just checked, and the problem still exists for records with no arguments:

Clojure 1.6.0-master-SNAPSHOT
user=> (defrecord a [])
user.a
user=> #user.a[]
{}

Admittedly it's an edge case and I see little usage for no-arguments records, but I think it should be addressed aswell since the current behaviour is not what one would expect

Comment by Herwig Hochleitner [ 02/Mar/13 8:14 AM ]

Got the following REPL interaction:

% java -jar ~/.m2/repository/org/clojure/clojure/1.5.0/clojure-1.5.0.jar
user=> (defrecord a [])
user.a
user=> (a.)
#user.a{}
user=> #user.a{}
{}
#user.a[]
{}

This should be reopened or declined for another reason than reproducability.

Comment by Nicola Mometto [ 10/Mar/13 2:18 PM ]

I'm reopening this since the bug is still there.

Comment by Andy Fingerhut [ 13/Mar/13 2:04 PM ]

Patch clj-1093-fix-empty-record-literal-patch-v2.txt dated Mar 13, 2013 is identical to Bronsa's patch 001-fix-empty-record-literal.patch dated Oct 24, 2012, except that it applies cleanly to latest master. I'm not sure why the older patch doesn't but git doesn't like something about it.

Comment by Nicola Mometto [ 26/Jun/13 8:06 PM ]

Patch 0001-CLJ-1093-fix-empty-records-literal-v2.patch solves more issues than the previous patch that was not evident to me at the time.

Only collections that are either PersistentList or PersistentVector or PersistentHash[Map|Set] or PersistentArrayMap can now be EmptyExpr.
This is because we don't want every IPersistentCollection to be emitted as a generic one eg.

user=> (class #clojure.lang.PersistentTreeMap[])
clojure.lang.PersistentArrayMap

Incidentally, this patch also solves CLJ-1187
This patch should be preferred over the one on CLJ-1187 since it's more general

Comment by Jozef Wagner [ 09/Aug/13 2:08 AM ]

Maybe this is related:

user=> (def x `(quote ~(list 1 (clojure.lang.PersistentTreeMap/create (seq [1 2 3 4])))))
#'user/x
user=> x
(quote (1 {1 2, 3 4}))
user=> (class (second (second x)))
clojure.lang.PersistentTreeMap
user=> (eval x)
(1 {1 2, 3 4})
user=> (class (second (eval x)))
clojure.lang.PersistentArrayMap

Even if the collection is not evaluated, it is still converted to the generic clojure counterpart.

Comment by Alex Miller [ 24/Apr/14 4:44 PM ]

In the change for ObjectExpr.emitValue() where you've added PersistentArrayMap to the PersistentHashMap case, should the IPersistentVector case below that be PersistentVector instead, otherwise it would snare a custom IPersistentVector that's not a PersistentVector, right?

This line: "else if(form instanceof ISeq)" at the end of the Compiler diff has different leading tabs which makes the diff slightly more confusing than it could be.

Would be nice to add a test for the sorted map case in the description.

Marking incomplete to address some of these.

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

bump

Comment by Nicola Mometto [ 14/May/14 4:24 AM ]

Attached patch 0001-CLJ-1093-fix-empty-collection-literal-evaluation.patch which implements your suggestions.

replacing IPersistentVector with PersistentVector in ObjectExpr.emitValue() exposes a print-dup limitation: it expects every IPersistentCollection to have a static "create" method.

This required special casing for MapEntry and APersistentVector$SubVector

Comment by Nicola Mometto [ 16/May/14 3:57 PM ]

I updated the patch adding print-dups for APersistentVector$SubVec and other IPersistentVectors rather than special casing them in the compiler

Comment by Alex Miller [ 23/May/14 4:21 PM ]

All of the checks on concrete classes in the Compiler parts of this patch don't sit well with me. I understand how you got to this point and I don't have an alternate recommendation (yet) but all of that just feels like the wrong direction.

We want to be built on abstractions such that internal collections are not special; they should conform to the same expectations as an external collection and both should follow the same paths in the compiler - needing to check for those types is a flag for me that something is amiss.

I am marking Incomplete for now based on these thoughts.

Comment by Nicola Mometto [ 06/Jul/14 10:01 AM ]

I've been thinking for a while about this issue and I've come to the conclusion that in my later patches I've been trying to incorporate fixes for 3 different albeit related issues:

1- Clojure transforms all empty IPersistentCollections in their generic Clojure counterpart

user> (defrecord x [])
user.x
user> #user.x[]   ;; expected: #user.x{}
{}
user> #user.x{}   ;; expected: #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1)  ;; expected: clojure.lang.PersistentTreeMap
clojure.lang.PersistentArrayMap

2- Clojure transforms all the literals of collections implementing the Clojure interfaces (IPersistentList, IPersistentVector ..) that are NOT defined with deftype or defrecord, to their
generic Clojure counterpart

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

3- print-dup is broken for some Clojure persistent collections

user=> (print-dup (subvec [1] 0) *out*)
#=(clojure.lang.APersistentVector$SubVector/create [1])
user=> #=(clojure.lang.APersistentVector$SubVector/create [1])
IllegalArgumentException No matching method found: create  clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:53)

I'll keep this ticket regarding issue #1 and open two other tickets for issue #2 and #3

Comment by Nicola Mometto [ 06/Jul/14 10:15 AM ]

I've attached a new patch fixing only this issue, the approach is explained in the description

Comment by Nicola Mometto [ 12/Sep/14 5:45 PM ]

0001-CLJ-1093-v2.patch is an updated patch that correctly handles metadata evaluation semantics on empty literals and adds some tests for it





[CLJ-1100] Reader literals cannot contain periods Created: 02/Nov/12  Updated: 12/Sep/14  Resolved: 12/Sep/14

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

Type: Defect Priority: Minor
Reporter: Kevin Lynagh Assignee: Unassigned
Resolution: Declined Votes: 1
Labels: reader

Attachments: Text File CLJ-1100-reader-tags-with-periods.patch     Text File clj-1100-v2.patch    
Patch: Code and Test
Approval: Incomplete

 Description   

The reader tries to read a record instead of a literal if the tag contains periods.

user> (binding [*data-readers* {'foo/bar #'identity}] (read-string "#foo/bar 1"))
1
user> (binding [*data-readers* {'foo/bar.x #'identity}] (read-string "#foo/bar.x 1"))
ClassNotFoundException foo/bar.x  java.lang.Class.forName0 (Class.java:-2)

Summary of reader forms:

Kind Example Constraint Status
Record #user.Foo[1] record class name OK
Class #java.lang.String["abc"] class name OK
Clojure reader tag #uuid "c48d7d6e-f3bb-425a-abc5-44bd014a511d" not a class name, no "/" OK
Library reader tag #my/card "5H" not a class name, has "/" OK
  #my.ns/card "5H" not a class name, has "/" OK
  #my/playing.card "5H" not a class name, has "/" BROKEN - read as record

Note: reader tags should not be allowed to override the record reader.

Cause: In LispReader, CtorReader.invoke() decides between record and tagged literal based on whether the tag has a ".".

Proposed: Change the discriminator in CtorReader by doing more string inspection:

  • If name has a "/" -> readTagged (not a legal class name)
  • If name has no "/" or "." -> readTagged (records must have qualified names)
  • Else -> readRecord (also covers Java classes)

Tradeoffs: Clojure-defined data reader tags must not contain periods. Not possible to read a Java class with no package. Avoids unnecessary class loading/construction for all tags.

Patch: CLJ-1100-v2.patch

Screened by: Alex Miller

Alternatives considered:

Using class checks:

  • Attempt readRecord (also covers Java classes)
  • If failed, attempt readTagged

Tradeoffs: Clojure tags could not override Java/record constructors - not sure that's something we'd ever want to do, but this would cut that off. This alternative may attempt classloading when it would not have before.



 Comments   
Comment by Steve Miner [ 06/Nov/12 9:41 AM ]

The suggested patch (clj-1100-reader-literal-periods.patch) will break reading records when *default-data-reader-fn* is set. Try adding a test like this:

(deftest tags-containing-periods-with-default
      ;; we need a predefined record for this test so we (mis)use clojure.reflect.Field for convenience
      (let [v "#clojure.reflect.Field{:name \"fake\" :type :fake :declaring-class \"Fake\" :flags nil}"]
        (binding [*default-data-reader-fn* nil]
          (is (= (read-string v) #clojure.reflect.Field{:name "fake" :type :fake :declaring-class "Fake" :flags nil})))
        (binding [*default-data-reader-fn* (fn [tag val] (assoc val :meaning 42))]
          (is (= (read-string v) #clojure.reflect.Field{:name "fake" :type :fake :declaring-class "Fake" :flags nil})))))
Comment by Rich Hickey [ 29/Nov/12 9:36 AM ]

The problem assessment is ok, but the resolution approach may not be. What happens should be based not upon what is in data-readers but whether or not the name names a class.

Is the intent here to allow readers to circumvent records? I'm not in favor of that.

Comment by Steve Miner [ 29/Nov/12 4:01 PM ]

New patch following Rich's comments. The decision to read a record is now based on the symbol containing periods and not having a namespace. Otherwise, it is considered a data reader tag. User
defined tags are required to be qualified but they may now have periods in the name. Tests added to show that
data readers cannot override record classes. Note: Clojure-defined data reader tags may be unqualified, but they should not contain periods in order to avoid confusion with record classes.

Comment by Steve Miner [ 29/Nov/12 4:17 PM ]

I deleted my old patch and some comments referring to it to avoid confusion.

In Clojure 1.5 beta 1, # followed by a qualified symbol with a period in the name is considered a record and causes an exception for the missing record class. With the patch, only non-qualified symbols containing periods are considered records. That allows user-defined qualified symbols with periods in their names to be used as data reader tags.

Comment by Andy Fingerhut [ 07/Feb/13 9:05 AM ]

clj-1100-periods-in-data-reader-tags-patch-v2.txt dated Feb 7 2013 is identical to CLJ-1100-periods-in-data-reader-tags.patch dated Nov 29 2012, except it applies cleanly to latest master. The only change appears to be in some white space in the context lines.

Comment by Andy Fingerhut [ 07/Feb/13 12:53 PM ]

I've removed clj-1100-periods-in-data-reader-tags-patch-v2.txt mentioned in the previous comment, because I learned that CLJ-1100-periods-in-data-reader-tags.patch dated Nov 29 2012 applies cleanly to latest master and passes all tests if you use this command to apply it.

% git am --keep-cr -s --ignore-whitespace < CLJ-1100-periods-in-data-reader-tags.patch

I've already updated the JIRA workflow and screening patches wiki pages to mention this --ignore-whitespace option.

Comment by Andy Fingerhut [ 13/Feb/13 11:31 AM ]

Both of the current patches, CLJ-1100-periods-in-data-reader-tags.patch dated Nov 29 2012, and clj-1100-reader-literal-periods.patch dated Nov 6 2012, fail to apply cleanly to latest master (1.5.0-RC15) as of today, although they did last week. Given all of the changes around read / read-string and edn recently, they should probably be evaluated by their authors to see how they should be updated.

Comment by Steve Miner [ 14/Feb/13 12:23 PM ]

I deleted my patch: CLJ-1100-periods-in-data-reader-tags.patch. clj-1100-reader-literal-periods.patch is clearly wrong, but the original author or an administrator has to delete that.

Comment by Kevin Lynagh [ 14/Feb/13 1:28 PM ]

I cannot figure out how to remove my attachment (clj-1100-reader-literal-periods.patch) in JIRA.

Comment by Steve Miner [ 14/Feb/13 1:43 PM ]

Downarrow (popup) menu to the right of the "Attachments" section. Choose "manager attachments".

Comment by Kevin Lynagh [ 14/Feb/13 2:02 PM ]

Great, thanks Steve. Are you going to take another pass at this issue, or should I give it a go?

Comment by Steve Miner [ 14/Feb/13 3:04 PM ]

Kevin, I'm not planning to work on this right now as 1.5 is pretty much done. It might be worthwhile discussing the issue a bit on the dev mailing list before working on a patch, but that's up to you. I think my approach was correct, although now changes would have to be applied to both LispReader and EdnReader.

Comment by Alex Miller [ 09/Apr/14 10:29 AM ]

Updated description based on my understanding.

Comment by Steve Miner [ 22/Apr/14 3:30 PM ]

I will resurrect my old patch and update it for the changes since 1.5.

Comment by Steve Miner [ 28/Apr/14 8:21 AM ]

Added patch to allow reader tags to have periods, but only with a namespace. Added tests to confirm that it works, but does not allow overriding a record name with a data-reader.

Comment by Steve Miner [ 28/Apr/14 8:51 AM ]

The patch implements Alex's alternative 1. It's purely lexical. A tag symbol without a namespace and containing periods is handled as a record (Java class). Otherwise, it's a data-reader tag. Of course, unqualified symbols without periods are still data-reader tags.

IMHO, a Java class without a package is a pathological case which Clojure doesn't need to worry about. This patch follows the convention that Java classes are named by unqualified symbols containing dots.

I did try alternative 2, testing for an actual class, but the implementation was more complicated. Also, it would open the possibility of breaking working code by adding a record or Java class that accidentally collided with an unqualified dotted tag that had previously worked fine. It's better to follow a simple rule that unqualified dotted symbols always refer to classes. Maybe the class doesn't actually exist, but that doesn't mean the symbol might be a data-literal tag.

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

Added clj-1100-v2.patch - identical, just removes whitespace to simplify change.

Comment by Rich Hickey [ 29/Aug/14 9:16 AM ]

I think we should disallow this rather than enable it. We don't generally support foo/bar.x

Comment by Nicola Mometto [ 29/Aug/14 9:27 AM ]

I created http://dev.clojure.org/jira/browse/CLJ-1516 with a patch that throws an exception on `(def foo.bar)`

Comment by Alex Miller [ 12/Sep/14 4:45 PM ]

Declined per Rich's comment and replaced with alternative on CLJ-1516





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

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

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

Attachments: Text File CLJ-1408-2.patch     Text File CLJ-1408-3.patch     Text File CLJ-1408.patch    
Patch: Code and Test
Approval: Screened

 Description   

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

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

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

;; keyword example

(def k1 (serialize :k))
(println :k)
(def k2 (serialize :k))

(= k1 k2) ;;=> false 

;; symbol example

(def sym 'a)

(def s1 (serialize sym))
(println sym)
(def s2 (serialize sym))

(= s1 s2) ;;=> false

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

Approach: Add transient keyword to _str field in Keyword and Symbol classes

Patch: CLJ-1408-3.patch

Screened by: Brenton Ashworth



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

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

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

Thanks, I'll sign and send CLA ASAP.

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

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

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

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

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

Removed `private` keyword

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

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

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

Tomasz, in addition to Alex's previous comment, it appears that a commit made to Clojure master earlier today causes your patches to no longer apply cleanly. I haven't looked to see whether updating the patches would be easy, but likely it is.

Comment by Alex Miller [ 22/Aug/14 11:00 PM ]

Updated the patch for latest master and added the obvious test.





[CLJ-1259] Speed up pprint Created: 09/Sep/13  Updated: 12/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: ft, performance, print

Attachments: Text File clj-1259-1.txt    
Patch: Code
Approval: Triaged

 Description   

There are many occurrences of reflection in the pprint implementation.

By eliminating all of them, I ran one benchmark of pprint'ing a Clojure map that resulted in a 300 Kbyte output. After eliminating reflection, the elapsed time to pprint was reduced by 18% (about 14.0 sec down to about 11.5 sec) on a recent model MacBook Pro.



 Comments   
Comment by Andy Fingerhut [ 09/Sep/13 11:36 PM ]

Patch clj-1259-1.txt eliminates all occurrences of reflection in pprint, and all files loaded from pprint.clj. It also sets warn-on-reflection to true for those files, in hopes of making it more obvious if a new use of reflection is added there.





[CLJ-1277] Speed up printing of time instants by adding type hints Created: 10/Oct/13  Updated: 12/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: ft, performance

Attachments: Text File clj-1277-1.txt    
Patch: Code
Approval: Triaged

 Description   

There are several occurrences of reflection in instant.clj that slow down the printing of time instants.

Clojure Google group conversation link: https://mail.google.com/mail/u/0/?shva=1#label/clojure/1419e1e6f6cc5b3d

The addition of a few type hints is enough to speed the printing of time instants by a factor of about 3 to 4.5, in a few small benchmarks.



 Comments   
Comment by Andy Fingerhut [ 10/Oct/13 12:07 AM ]

Patch clj-1277-1.txt adds 4 type hints that eliminate all reflection occurrences in source file instant.clj. Benchmarks show that it speeds up printing of java.util.Date and java.sql.Timestamp objects by a factor of about 3 to 4.5.

Latest Clojure master as of Oct 9 2013:

user=> (time (let [d (java.util.Date.)] (dotimes [i 3000000] (pr-str d))))
"Elapsed time: 24094.282 msecs"
user=> (import 'java.sql.Timestamp)
user=> (time (let [d (java.sql.Timestamp. 1300000000000)] (dotimes [i 2000000] (pr-str d))))
"Elapsed time: 20856.957 msecs"

That version of Clojure plus the patch clj-1277-1.txt:

user=> (time (let [d (java.util.Date.)] (dotimes [i 3000000] (pr-str d))))
"Elapsed time: 5085.847 msecs"
user=> (time (let [d (java.sql.Timestamp. 1300000000000)] (dotimes [i 2000000] (pr-str d))))
"Elapsed time: 7582.233 msecs"

Comment by Alexander Kiel [ 10/Oct/13 4:54 AM ]

Thanks for the patch Andy. But I don't like the (set! warn-on-reflection true). I think its better to use it only in the dev profile in leiningen. Not in real production code.

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

Alexander, Leiningen is not used for building Clojure itself. The two supported choices are Maven and ant. Several Clojure source files, e.g. core/protocols.clj and core/reducers.clj, set warn-on-reflection to true, I believe so that if code is changed in such a way as to introduce a warning, it will be caught more quickly.

If the screeners or Rich think it is inappropriate, it is easy enough to remove.

Comment by Alex Miller [ 10/Oct/13 4:48 PM ]

Setting that is not uncommon in core clojure code and seems fine to me here.





[CLJ-1329] Unused local variable in PersistentVector.cons() Created: 22/Jan/14  Updated: 12/Sep/14

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

Type: Enhancement Priority: Trivial
Reporter: Smit Shah Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, ft

Attachments: Text File fix.patch    
Patch: Code
Approval: Triaged

 Description   

in src/jvm/clojure/lang/PersistentVector.java:168, there is an integer i being defined which is not being used anywhere in the method.

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/PersistentVector.java#L168



 Comments   
Comment by Stuart Halloway [ 31/Jan/14 6:14 PM ]

Smit, can you please submit a CA? http://clojure.org/contributing

Comment by Smit Shah [ 02/Feb/14 1:16 PM ]

Stuart, I will send the CA via post ASAP.
It might take a couple of days to reach Rich though.

Comment by Smit Shah [ 01/Mar/14 11:51 AM ]

Stuart, I have successfully submitted the CA (http://clojure.org/contributing).
I guess now merging this patch shouldn't be a problem

Comment by Alex Miller [ 02/Mar/14 11:37 AM ]

Thanks Smit!





[CLJ-1357] It's a small typo in the gen-class doc-string Created: 17/Feb/14  Updated: 12/Sep/14

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

Type: Enhancement Priority: Trivial
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, ft

Attachments: Text File CLJ-1357-its-typo.patch    
Patch: Code
Approval: Triaged

 Description   

"It's" should be "its" (possessive) in "It's return value is ignored."






[CLJ-1477] Fixed a typo Created: 27/Jul/14  Updated: 12/Sep/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: documentation, ft

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Triaged

 Description   

Just a simple typo fix - "directy" -> "directly".






[CLJ-1478] Doc typo Created: 27/Jul/14  Updated: 12/Sep/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, ft

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Triaged

 Description   

Another small typo fix.






[CLJ-1479] Typo in filterv example Created: 27/Jul/14  Updated: 12/Sep/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: documentation, ft

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Triaged

 Description   

filter -> filterv






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

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

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

Attachments: Text File clj-1414-v1.patch    
Approval: Triaged

 Description   

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

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

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

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

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

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



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

Sounds reasonable. Needs patch from contributor.

Comment by Andy Fingerhut [ 30/Aug/14 3:07 PM ]

Patch clj-1414-v1.patch dated Aug 30 2014 adds the sentence "Guaranteed to be stable: equal elements will not be reordered." to the doc strings of both sort and sort-by.





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

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

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

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

 Description   

clojure/java/io.clj line 125

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

Should read

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






[CLJ-1480] Incorrect param name reference in defmulti's docstring Created: 27/Jul/14  Updated: 12/Sep/14

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

Type: Defect Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, ft

Attachments: Text File 0001-Fix-param-name-reference-in-defmulti-s-docstring.patch    
Patch: Code
Approval: Triaged

 Description   

attribute-map should actually be attr-map






[CLJ-1481] Typo in type-reflect's docstring Created: 27/Jul/14  Updated: 12/Sep/14

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

Type: Defect Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, ft

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Triaged

 Description   

membrer -> member






[CLJ-1493] Fast keyword intern Created: 06/Aug/14  Updated: 11/Sep/14

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

Type: Enhancement Priority: Major
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: keywords, performance
Environment:

Mac OS X 10.9.4 / 2.6 GHz Intel Core i5 / 8 GB 1600 MHz DDR3
java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)


Attachments: File fast_keyword_intern.diff    
Patch: Code
Approval: Triaged

 Description   

Keyword's intern(Symbol) method uses recursive invocation to get a valid keyword instance.I think it can be rewrite into a 'for loop'
to reduce method invocation cost.
So i developed this patch, and make some simple benchmark.Run the following command line three times after 'ant jar':

java -Xms64m -Xmx64m -cp test:clojure.jar clojure.main -e "(time (dotimes [n 10000000] (keyword (str n))))"

Before patched:

"Elapsed time: 27343.827 msecs"
"Elapsed time: 26172.653 msecs"
"Elapsed time: 25673.764 msecs"

After patched:

"Elapsed time: 24884.142 msecs"
"Elapsed time: 23933.423 msecs"
"Elapsed time: 25382.783 msecs"

It looks the patch make keyword's intern a little more fast.

The patch is attached and test.

Thanks.

P.S. I've signed the contributor agreement, and my email is killme2008@gmail.com .



 Comments   
Comment by Alex Miller [ 07/Aug/14 9:01 AM ]

Looks intriguing (and would be a nice change imo). I ran this on a json parsing benchmark I used for the keyword changes and saw ~3% improvement.

Comment by dennis zhuang [ 07/Aug/14 9:54 PM ]

Updated the patch, remove the 'k == null' clause in for loop,it's not necessary.

Comment by Andy Fingerhut [ 11/Aug/14 1:29 AM ]

Dennis, while JIRA can handle multiple patches with the same name, it can be confusing for people discussing the patches, and for some scripts I have to evaluate them. Please consider giving the patches different names (e.g. with version numbers in them), or removing older ones if they are obsolete.

Comment by dennis zhuang [ 11/Aug/14 9:19 AM ]

Hi,andy

Thank you for reminding me.I deleted the old patch.

Comment by dennis zhuang [ 11/Sep/14 10:34 AM ]

I am glad to see it is helpful.I benchmark the patch with current master branch,it's fine too.





[CLJ-1473] Bad pre/post conditions silently passed Created: 24/Jul/14  Updated: 11/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs

Attachments: Text File 0001-Validate-that-pre-and-post-conditions-are-vectors.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Before:

user=> ((fn [x] {:pre (pos? x)} x) -5) ; ouch!
-5
user=> ((fn [x] {:pre [(pos? x)]} x) -5) ; meant this
AssertionError Assert failed: (pos? x)  user/eval4075/fn--4076 (form-init5464179453862723045.clj:1)

After:

user=> ((fn [x] {:pre (pos? x)} x) -5)
CompilerException java.lang.IllegalArgumentException: Pre and post conditions should be vectors, compiling:(NO_SOURCE_PATH:1:2) 
user=> ((fn [x] {:pre [(pos? x)]} x) -5)                                  
AssertionError Assert failed: (pos? x)  user/eval2/fn--3 (NO_SOURCE_FILE:2)
user=> ((fn [x] {:post (pos? x)} x) -5)
CompilerException java.lang.IllegalArgumentException: Pre and post conditions should be vectors, compiling:(NO_SOURCE_PATH:3:2) 
user=> ((fn [x] {:post [(pos? x)]} x) -5)              
AssertionError Assert failed: (pos? x)  user/eval7/fn--8 (NO_SOURCE_FILE:4)





[CLJ-1282] The quote special form should throw an exception if passed more than one form to quote Created: 23/Oct/13  Updated: 10/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, reader

Attachments: Text File CLJ-1282-p1.patch     Text File CLJ-1282-p2.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Quote currently ignores all but the first argument. In the case of being called accidentally with multiple values, it should throw an exception specifying the error.

user> (quote 1 2 3)
1

------- Original: --------

Every once in a while, you can just go down the rabbit hole.

I had an errant expression in my code:

(-> message get-message-values 'DESTINATION_MERCHANT_ID)

One would think this would work; it certainly would if the key was a keyword and not a symbol.

One would expect this to expand to:

('DESTINATION_MERCHANT_ID (get-message-values message))

however, the reader is involved, so it is as if the source were:

(-> message get-message-values (quote DESTINATION_MERCHANT_ID))

which expands to:

(quote (-> message get-message-values) DESTINATION_MERCHANT_ID))

... hilarity ensues! Because quote currently ignores extra parameters, my code gets the quoted value '(clojure.core/-> message get-message-values) rather than the expected string from the map; this shifts us from the "there's a bug in my code" to "the nature of reality is broken".

The correct expression is:

(-> message get-message-values (get 'DESTINATION_MERCHANT_ID))

This took quite a while to track down; if the

special form checked that it was passed exactly one form to quote and threw an exception otherwise, I think I would have caught this much earlier. It could even identify the expression it is quoting, which would provide a lot better understanding of where I went wrong.



 Comments   
Comment by Howard Lewis Ship [ 23/Oct/13 2:02 PM ]

Sorry, can't edit the description now to correct the formatting errors.

Comment by Howard Lewis Ship [ 24/Oct/13 6:09 PM ]

I just wanted to point out that your description is shorter, but makes it appear that such a use is unlikely and therefore unimportant; the detail of my description is to point out a reasonable situation where something explicable, but completely counterintuitive and confusing, does occur.

Comment by Alex Miller [ 24/Oct/13 8:28 PM ]

That's why I left the original in there too.

Comment by Gary Fredericks [ 09/Dec/13 7:07 AM ]

(quote) currently returns nil. Do we have an opinion about that?

Comment by Gary Fredericks [ 09/Dec/13 9:32 AM ]

Attached p1, which throws an IllegalArgumentException (wrapped in a CompilerException of course) for anything but 1 arg, and includes the number of args that were passed.

I can't think of any reason why (quote) would be useful, so I decided to throw on that too. Very easy to change of course.

Also added a test that (eval '(quote 1 2 3)) throws.

Comment by Stuart Halloway [ 31/Jan/14 12:46 PM ]

I recommend the following changes:

  • throw an ex-info that includes the offending form in its map {:form ...}
  • check only for the map data, not exception type or message, in the tests
Comment by Andy Fingerhut [ 31/Jan/14 6:31 PM ]

Patch CLJ-1282-p1.patch no longer applies cleanly after commits made to Clojure master on Jan 31 2014, probably due to the patch committed for CLJ-1318, and probably only because some lines of context changed in the test file. That would be trivial to update, but Stu's comments above suggest that more significant changes need to be made.

Comment by Gary Fredericks [ 01/Feb/14 9:19 AM ]

Throwing an ex-info is easy enough. I don't know how to avoid at least incidentally checking for the exception type, since the ExceptionInfo is wrapped in a CompilerException. I'll make a patch that keeps the class name in the test but doesn't do any checks on the cause aside from the ex-data. Let me know if I should do anything different.

Comment by Gary Fredericks [ 01/Feb/14 9:58 AM ]

Attached CLJ-1282-p2.patch which is off of the current master and addresses Stu's points.

Comment by Alex Miller [ 04/Feb/14 11:23 PM ]

Moving back to Triaged for more looks.

Comment by Nicola Mometto [ 16/Feb/14 12:12 PM ]

Currently (quote) returns nil, is it intended that this patch makes that an error or was this by accident?

Comment by Gary Fredericks [ 16/Feb/14 12:23 PM ]

I consciously chose to make (quote) an error – I made a comment about that earlier and didn't get any feedback, so I unilaterally decided to make it an error due to the fact that I couldn't think of any possible use for (quote).

It's an easy switch if somebody thinks differently.

Comment by Nicola Mometto [ 16/Feb/14 1:13 PM ]

I'm sorry I did not notice your previois comment.
I'm asking because I need to know whether I should throw on (quote) for tools.analyzer, currently it is allowed but I too think that (quote) should be an error.





[CLJ-1295] Speed up dissoc on array-maps Created: 15/Nov/13  Updated: 10/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: performance

Attachments: File clj-1295-1.diff    
Patch: Code
Approval: Triaged

 Description   

In latest Clojure master as of Nov 15 2013, the method without() in PersistentArrayMap.java first searches for a matching key using indexOf(key) and saves the result in i.

If a matching key was found, the code then copies the old array to the new smaller one, but unnecessarily repeats the comparison of every key in the map to the key being removed, even though its location is already stored in i.



 Comments   
Comment by Andy Fingerhut [ 15/Nov/13 7:05 PM ]

The patch clj-1295-1.diff changes PersistentArrayMap's without() to use System.arraycopy to copy only the necessary parts from the current array to newArray, similar to PersistentHashMap's method removePair().

Benchmark 1 has strings for keys, which are relatively slow to compare to each other.

(def m1 (array-map "abcdef" 1 "abcdeg" 2 "abcdeh" 3 "abcdei" 4))
(time (dotimes [i 100000000] (dissoc m1 "abcdei")))

1.6.0-alpha2 with no changes:
"Elapsed time: 29663.443 msecs"
"Elapsed time: 29490.225 msecs"
"Elapsed time: 29600.138 msecs"
"Elapsed time: 29627.948 msecs"

1.6.0-alpha2 with patch clj-1295-1.diff:
"Elapsed time: 6362.006 msecs"
"Elapsed time: 6121.006 msecs"
"Elapsed time: 6163.377 msecs"
"Elapsed time: 6155.299 msecs"
"Elapsed time: 6395.224 msecs"

Averages about 21% of the run time before the change.

Benchmark 2 has keywords for keys, which are compared via Java ==, so as fast as comparison can get.

(def m2 (array-map :abcdef 1 :abcdeg 2 :abcdeh 3 :abcdei 4))
(time (dotimes [i 100000000] (dissoc m2 :abcdei)))

1.6.0-alpha2 with no changes:
"Elapsed time: 5033.863 msecs"
"Elapsed time: 5028.327 msecs"
"Elapsed time: 5045.019 msecs"
"Elapsed time: 5004.751 msecs"
"Elapsed time: 5039.143 msecs"

1.6.0-alpha2 with patch clj-1295-1.diff:
"Elapsed time: 2874.748 msecs"
"Elapsed time: 2862.878 msecs"
"Elapsed time: 2887.778 msecs"
"Elapsed time: 2874.196 msecs"
"Elapsed time: 2861.807 msecs"

Averages about 57% of the run time before the change.





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

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

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

Approval: Triaged

 Description   

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

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

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



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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





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

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

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

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

 Description   

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



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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




[CLJ-1519] Added extra arity to clojure.core/ns-* fns Created: 04/Sep/14  Updated: 10/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Alex Baranosky Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: enhancement, patch

Attachments: Text File new-ns-arity.patch    
Patch: Code and Test

 Description   

Hello,

Adds another arity where the "ns" parameter is set to a default value of *ns* in these fns:

ns-unmap, ns-resolve, ns-name, ns-map, ns-publics, ns-imports, ns-interns, ns-refers, ns-aliases, ns-unalias

I find I very often use ns-unalias and ns-unmap from the repl, and passing the *ns* arg gets a little tedious.






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

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

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

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

 Description   

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

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

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

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

Patch: clj-1524.diff

Screened by:

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



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

Patch not applied, but similar change applied directly here:

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





[CLJ-1523] Add 'doseq' like macro for transducers Created: 08/Sep/14  Updated: 09/Sep/14

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

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

Attachments: File doreduced2.diff     File doreduced.diff    
Patch: Code and Test

 Description   

Doseq is currently a good way to execute a lazy sequence and perform side-effects. It would be nice to have a matching macro for transducers.

Approach: The included patch simply calls transduce with the provided xform, collection, and a reducing function that throws away the accumulated value at each step. The value from each reducing step is bound to the provided symbol. A shorter arity is provided for those cases when no xform is desired, but fast doseq-like semantics are still wanted.

Patch: doreduced2.diff



 Comments   
Comment by Jozef Wagner [ 09/Sep/14 4:19 AM ]

How about making xform parameter optional? And you have a typo in docstring example, doseq -> doreduced.

Comment by Timothy Baldridge [ 09/Sep/14 7:52 AM ]

Good point, fixed typeo, added other arity.





[CLJ-1148] adds docstring support to defonce Created: 17/Jan/13  Updated: 09/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Joe Gallo Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: docstring

Attachments: Text File 0001-new-defonce-hotness.patch     Text File clj-1148-defonce-2.patch     Text File clj-1148-defonce-3.patch     Text File clj-1148-defonce-4.patch     Text File clj-1148-defonce-4.patch     Text File clj-1148-defonce-5.patch     Text File clj-1148-defonce-6.patch     Text File defonce_fixes.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Pass all args from defonce on to def so it supports docstrings (or potentially other future features) just like def.

Docstrings and other Var metadata will be lost when the defonce is reëvaluated.

Patch: clj-1148-defonce-3.patch

Screened by: Stuart Sierra



 Comments   
Comment by Alex Miller [ 29/Aug/13 9:53 AM ]

Changed to defect for stomping metadata.

Comment by Stuart Halloway [ 18/Oct/13 8:00 AM ]

Please add tests. The clojure.test-helper namespace has useful temporary namespace support.

Comment by Joe Gallo [ 24/Oct/13 12:44 PM ]

This new patch includes the changes to defonce and also tests.

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

Changing to Vetted so this is screenable again.

Comment by Rich Hickey [ 22/Nov/13 11:31 AM ]

I disagree about the stomp metadata - different metadata was provided. The purpose of defonce is to avoid the re-evaluation of the init. Is this the simplest change that accomplishes the doc string? In any case split in two.

Comment by Alex Miller [ 29/Dec/13 10:24 PM ]

Reduced scope of ticket to just passing defonce args on to def to add support for docstring. Added new patch that does this.

Comment by Stuart Sierra [ 10/Jan/14 4:09 PM ]

Screened clj-1148-defonce-2.patch but returning to 'incomplete' status.

The :arglists metadata in this patch (a list of symbols) is inconsistent with all other uses of :arglists (a list of vectors).

Other than that the patch is good.

Comment by Alex Miller [ 10/Jan/14 5:04 PM ]

Updated patch to address inconsistency in arglist format and attached clj-1148-defonce-3.patch.

Comment by Stuart Sierra [ 17/Jan/14 9:36 AM ]

The patch clj-1148-defonce-3.patch is OK but it doesn't really address the docstring issue because defonce still destroys metadata. For example:

user=> (defonce foo "docstring for foo" (do (prn 42) 42))
42
#'user/foo
user=> (doc foo)
-------------------------
user/foo
  docstring for foo
nil
user=> (defonce foo "docstring for foo" (do (prn 42) 42))
nil
user=> (doc foo)
-------------------------
user/foo
  nil
Comment by Stuart Sierra [ 17/Jan/14 10:03 AM ]

Screened with reservations noted.

Comment by Rich Hickey [ 24/Jan/14 10:15 AM ]

Stuart is right, second defonce should retain the doc string (since it again provides it, should be no-op)

Comment by Alex Miller [ 20/Feb/14 10:41 AM ]

pull out of 1.6

Comment by Linus Ericsson [ 28/Aug/14 12:30 PM ]

This version looks for previously defined var with resolve. A repeated defonce won't affect the namespace at all if the variable is already defined and bounded.

Please confirm using (resolve '~name) is not a problem w.r.t ns-bindings or similar.

This patch also contains the tests from clj-1148-defonce-3.patch as well as the :arglists property.

(patch 4 missed one def-row, sorry for mailbox noise).

Comment by Linus Ericsson [ 09/Sep/14 4:27 AM ]

Yet another, simpler version of defonce. No test-cases included.

This version just makes an (or (nil? v#) (not (.hasRoot v#)) test on the resolved variable. If this is true, really define by (def ~name ~@args) else do nothing.





[CLJ-1522] Enhance multimethods metadata Created: 08/Sep/14  Updated: 09/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: metadata


 Description   

I think that multimethod metadata can be extended a bit with some property indicating the var in question is referring to a multimethod (we have something similar for macros) and some default arglists property.

I'm raising this issue because as a tool writer (CIDER) I'm having hard time determining if something is a multimethod (I have to resort to code like (instance? clojure.lang.MultiFn obj) which is acceptable, but not ideal I think (compared to macros and special forms)). There's also the problem that I cannot provide the users with eldoc (function signature) as it's not available in the metadata (this issue was raised on the mailing list as well https://groups.google.com/forum/#!topic/clojure/crje_RLTWdk).

I feel that we really have a problem with the missing arglist and we should solve it somehow. I'm not sure I'm suggesting the best solution and I'll certainly take any solution.



 Comments   
Comment by Bozhidar Batsov [ 09/Sep/14 4:24 AM ]

Btw, I failed to mention this as I thought it was obvious, but I think we should use the dispatch function's arglist in the multimethod metadata.





[CLJ-1010] A left-to-right-variant of `comp` Created: 11/Jun/12  Updated: 08/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0001-CLJ-1010-Add-a-left-to-right-version-of-comp-comp.patch    
Patch: Code

 Description   

The function composition function `comp` is quite inefficient in cases like `(apply comp large-seq-of-fns)`, because its arity-greater-than-3 version reverses the seq.

I would be great if there was an alternative `comp*` (or whatever) function which is just like `comp` but composes left-to-right.



 Comments   
Comment by Tassilo Horn [ 11/Jun/12 5:16 AM ]

Here's an implementation.

Comment by Tassilo Horn [ 11/Jun/12 12:41 PM ]

There's something strange with my patch. The creation of a composition of a huge seq of functions is much faster with `comp*` than with `comp`, which is expected, because the seq doesn't need to be reversed.

The strange thing however is that the compositions created with `comp` evaluate about 10-20% faster than those created with `comp*` although the arbitrary-arity version of `comp` is defined in terms of `comp*`: `(apply comp* (reverse (list* f1 f2 f3 fs)))`.

For some benchmarking details, see: https://groups.google.com/d/msg/clojure/MizwTxHwLE4/hGLrMfetlP8J

Comment by Tassilo Horn [ 14/Jun/12 2:32 AM ]

Here's some benchmark:

user> (use 'criterium.core)
nil
user> (let [coll (doall (take 1000000 (repeat inc)))
	   f1 (apply comp* coll) 
	   f2 (apply comp coll)] 
	   (bench (f1 0) :verbose) 
	   (println "---------------------------------------")
	   (bench (f2 0) :verbose))
amd64 Linux 3.4.2-gentoo 2 cpu(s)
OpenJDK 64-Bit Server VM 22.0-b10
Runtime arguments: -agentlib:jdwp=transport=dt_socket,server=y,suspend=n -XX:+TieredCompilation -Xmx1G -Dclojure.compile.path=/home/horn/Repos/clj/testi/target/classes -Dtesti.version=0.1.0-SNAPSHOT -Dclojure.debug=false
Evaluation count             : 600
             Execution time mean : 112.324465 ms  95.0% CI: (112.247218 ms, 112.380682 ms)
    Execution time std-deviation : 6.513809 ms  95.0% CI: (6.477450 ms, 6.553029 ms)
         Execution time lower ci : 105.609401 ms  95.0% CI: (105.609401 ms, 105.622918 ms)
         Execution time upper ci : 122.353763 ms  95.0% CI: (122.353763 ms, 122.405315 ms)
---------------------------------------
amd64 Linux 3.4.2-gentoo 2 cpu(s)
OpenJDK 64-Bit Server VM 22.0-b10
Runtime arguments: -agentlib:jdwp=transport=dt_socket,server=y,suspend=n -XX:+TieredCompilation -Xmx1G -Dclojure.compile.path=/home/horn/Repos/clj/testi/target/classes -Dtesti.version=0.1.0-SNAPSHOT -Dclojure.debug=false
Evaluation count             : 1440
             Execution time mean : 43.519663 ms  95.0% CI: (43.516732 ms, 43.524062 ms)
    Execution time std-deviation : 492.299089 us  95.0% CI: (490.829889 us, 494.198137 us)
         Execution time lower ci : 42.781398 ms  95.0% CI: (42.781398 ms, 42.781398 ms)
         Execution time upper ci : 44.157311 ms  95.0% CI: (44.157311 ms, 44.158513 ms)
nil
Comment by Tassilo Horn [ 14/Jun/12 3:40 AM ]

Ok, I've tracked down the performance difference. This comes from the fact that `reverse` creates a clojure.lang.PersistentList which can be iterated much faster than a (fully realized) LazySeq. If you provide your fncoll in `(apply comp* fncoll)` as vector or list, the application of the created composition is as fast as for `comp`.

So for me, the patch is good and makes sense.

Comment by Andy Fingerhut [ 05/Sep/14 11:45 AM ]

Patch 0001-CLJ-1010-Add-a-left-to-right-version-of-comp-comp.patch dated Jun 11 2012 no longer applies cleanly to latest master after some commits were made to Clojure on or near Sep 5 2014.

I have not checked whether this patch is straightforward to update. See the section "Updating stale patches" at http://dev.clojure.org/display/community/Developing+Patches for suggestions on how to update patches.

Comment by Tassilo Horn [ 08/Sep/14 4:39 AM ]

Here is an updated patch which applies to the git master as of 2014-09-08.





[CLJ-1521] A little improvement for parsing let expr Created: 07/Sep/14  Updated: 08/Sep/14

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

Type: Enhancement Priority: Trivial
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: let, parser
Environment:

Mac OSX 10.9.4
java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)


Attachments: File improve_parse_let_expr.diff    
Patch: Code

 Description   

The recurMismatches vector in LetExpr parser as see in

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L6062-6065

There is not necessary to add initialize value 'false' into it when it is not a loop expression.

We can rewrite it into:

if(isLoop)
			    {
				for (int i = 0; i < bindings.count()/2; i++)
				    {
				    recurMismatches = recurMismatches.cons(RT.F);
				    }				
			    }

It's a little improvement for parsing let expression.



 Comments   
Comment by Andy Fingerhut [ 07/Sep/14 11:16 AM ]

Dennis, you might want to clarify the description a little bit, if I understand this ticket correctly. The proposed change would be no change to the behavior of the compiler, except a small speed improvement during compilation?

Comment by dennis zhuang [ 08/Sep/14 2:36 AM ]

Yep,the patch doesn't change the behavior of the compiler.All test is fine.

The recurMismatches vector in LetExpr parser as see in

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L6062-6065

is only used when detecting type mismatch for loop special form,it's not necessary to be initialized for let special form.So i just added a if(isLoop) clause before initializing it.





[CLJ-1312] clojure.string/split on empty string includes empty string in results Created: 21/Dec/13  Updated: 07/Sep/14  Resolved: 21/Dec/13

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

Type: Defect Priority: Minor
Reporter: Russell Dunphy Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: string


 Description   

Splitting a string using clojure.string/split with an empty regex includes the empty string in the results - is this expected behaviour?

Example:

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
user=> (clojure.string/split "abc" #"")
["" "a" "b" "c"]


 Comments   
Comment by Alex Miller [ 21/Dec/13 8:05 AM ]

Yes, I think so. This is a case where Clojure defers to the host (Java) for behavior. I think the way to interpret this is that the empty pattern matches all strings. Split checks left to right whether there is a next chunk of string that matches the pattern. The empty pattern matches at the beginning to a string of length 0. Something like that.

Comment by Mark Engelberg [ 07/Sep/14 12:27 PM ]

This bug is a real problem, because it works differently on Windows than on Linux. On Windows, clojure.string/split behaves exactly as you'd expect:

user=> (clojure.string/split "abc" #"")
["a" "b" "c"]

Only on Linux do you get the strange behavior where the empty string shows up at the beginning of the list.

I recently had a student that got burned by this in some webserver code that relied on splitting using the empty regex. It performed flawlessly on her local Windows machine, but mysteriously broke when she uploaded the uberwar to the cloud. The bug was very difficult to track down.

If this were a bug on both Windows and Linux, at least you could plan around it. But right now, it's an obstacle to Clojure's capability of running consistently across platforms.

Comment by Mark Engelberg [ 07/Sep/14 12:40 PM ]

Upon further research, I've found that this is not a Windows/Linux issue, rather it's a difference between Java 7 and Java 8. On Java 8, splitting with the empty string no longer produces a sequence that begins with an empty string.

As you said before, this is just a gotcha relating to Java, not a Clojure issue.





[CLJ-1430] Improve performance of partial Created: 23/May/14  Updated: 05/Sep/14  Resolved: 29/Aug/14

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

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

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

 Description   

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

Performance benchmark:

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

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

Results for 1.6.0:

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

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

Results for 1.7.0 with this patch:

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

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

Benchmarks performed via lein uberjar + running via the commandline.

Patch: partial-perf.diff

Screened by: Alex Miller



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

Screened, looks as expected.

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

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

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

fixed email

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

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

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

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

Comment by Alex Baranosky [ 05/Sep/14 9:11 PM ]

Very nice patch. I've gotten into the habit of not using partial anymore for performance sensitive code. Perhaps this change means I need to rethink that.





[CLJ-1209] clojure.test does not print ex-info in error reports Created: 11/May/13  Updated: 05/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File 0002-CLJ-1209-show-ex-data-in-clojure-test.patch     File clj-test-print-ex-data.diff     Text File output-with-0002-patch.txt    
Patch: Code
Approval: Triaged

 Description   

clojure.test does not print the data attached to ExceptionInfo in error reports.

Approach: In clojure.stacktrace, which clojure.test uses for printing exceptions, add a check for ex-data and pr it.

Patch: 0002-CLJ-1209-show-ex-data-in-clojure-test.patch



 Comments   
Comment by Alex Miller [ 20/Dec/13 9:53 AM ]

Great idea, thx for the patch!

Comment by Alex Miller [ 20/Dec/13 9:54 AM ]

Would be great to see a before and after example of the output.

Comment by Ivan Kozik [ 12/Jul/14 10:35 PM ]

Attaching sample output

Comment by Stuart Sierra [ 05/Sep/14 3:24 PM ]

As pointed out on IRC, there's a possible risk of trying to print an infinite lazy sequence that happened to be included in ex-data.

To mitigate, consider binding *print-length* and *print-level* to small numbers around the call to pr.





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

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

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

Attachments: Text File clj-1449-basic-v1.patch     Text File clj-1449-more-v1.patch    
Approval: Triaged

 Description   

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



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

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

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

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

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

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

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

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

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

Comment by Pierre Masci [ 01/Aug/14 5:27 AM ]

Hi, I was thinking the same about starts-with and .ends-with, as well as (.indexOf s "c") and (.lastIndexOf "c").

I read the whole Java String API recently, and these 4 functions seem to be the only ones that don't have an equivalent in Clojure.
It would be nice to have them.

Andy Fingerhut who maintains the Clojure Cheatsheet told me: "I maintain the cheatsheet, and I put .indexOf and .lastIndexOf on there since they are probably the most common thing I saw asked about that is in the Java API but not the Clojure API, for strings."
Which shows that there is a demand.

Because Clojure is being hosted on several platforms, and might be hosted on more in the future, I think these functions should be part of the de-facto ecosystem.

Comment by Andy Fingerhut [ 30/Aug/14 3:39 PM ]

Updating summary line and description to add contains? as well. I can back this off if it changes your mind about triaging it, Alex.

Comment by Andy Fingerhut [ 30/Aug/14 3:40 PM ]

Patch clj-1449-basic-v1.patch dated Aug 30 2014 adds starts-with? ends-with? contains? functions to clojure.string.

Patch clj-1449-more-v1.patch is the same, except it also replaces several Java method calls with calls to these Clojure functions.

Comment by Andy Fingerhut [ 05/Sep/14 1:02 PM ]

Patch clj-1449-basic-v1.patch dated Sep 5 2014 is identical to the patch I added recently called clj-1149-basic-v1.patch. It is simply renamed without the typo'd ticket number in the file name.





[CLJ-1520] assoc-in with empty key path assoc-es to nil Created: 05/Sep/14  Updated: 05/Sep/14

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

Type: Defect Priority: Major
Reporter: Francis Avila Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   
(assoc-in {} [] 1) ;=> {nil 1}

This should probably throw an exception.

CLJ-373 has a patch (CLJ-373-nested-ops.patch) which fixes this (by throwing an exception on empty key paths), the related broken behavior of update-in, and documents empty key path behavior in get-in et al. I can pull just the assoc-in stuff out of that into a separate patch, but I am really hoping that all the issues in the patch addresses are resolved at once, I.e.:

(get-in {} [] :notfound) ;=> {} ; ok
(get-in {nil 1} [] :notfound) ;=> {nil 1} ; ok
(assoc-in {} [] 1) ;=> {nil 1} ; wat?
(assoc-in {nil 0} [] 1) ;=> {nil 1} ; wat?
(update-in {} [] identity) ;=> {nil nil} ; wat?
(update-in {nil 0} [] inc) ;=> {nil 1} ; wat?





[CLJ-15] Incremental hashcode calculation for collections Created: 17/Jun/09  Updated: 05/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Rich Hickey Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: performance

Attachments: File lazy-incremental-hashes.diff    
Patch: Code

 Description   
Reported by richhickey, Dec 17, 2008
So hachCode can be final, more efficient to calc as you go.
Formerly Google Code Issue 11


 Comments   
Comment by Assembla Importer [ 24/Aug/10 3:44 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/15

Comment by Christophe Grand [ 08/Mar/13 6:20 AM ]

Wouldn't the naive approach incur realizing lazy sequences when adding them to a list or a vector or as values in a map?

Comment by Christophe Grand [ 26/Aug/13 3:40 AM ]

The lazy-incremental-hashes.diff introduces lazy incremental hashes based on structural sharing.

Comment by Christophe Grand [ 26/Aug/13 3:42 AM ]

Why is this identified as Blocker?

Comment by Christophe Grand [ 26/Aug/13 3:43 AM ]

setting priority to minor

Comment by Andy Fingerhut [ 26/Aug/13 7:46 AM ]

I've seen this "edit a ticket, it changes to Priority=Blocker" behavior before. I believe some of the older tickets have no Priority field at all, and when you edit any of their properties, it creates the priority field with a default value of Blocker.

Comment by Alex Miller [ 26/Aug/13 8:06 AM ]

Yes, concur with Andy's explanation on priority change. I just bulk-edited all open CLJ tickets with null priority and set their priority.

Comment by Andy Fingerhut [ 30/Jan/14 8:01 PM ]

Patch lazy-incremental-hashes.diff still applies cleanly as of Jan 30 2014 latest Clojure master, but now fails tests due to recent commits involving hash changes. I have not checked how difficult or easy it might be to update the patch to pass tests again.

Comment by Andy Fingerhut [ 29/Aug/14 4:25 PM ]

Patch lazy-incremental-hashes.diff dated Aug 26 2013 no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. It did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch.

Comment by Andy Fingerhut [ 05/Sep/14 11:41 AM ]

Patch lazy-incremental-hashes.diff dated Aug 26 2013 applies cleanly again to latest Clojure master as of Sep 5 2014, even though the patch has not been updated. I haven't checked, but I would guess this is because one of the changes made to Clojure master recently was reverted with this commit: https://github.com/clojure/clojure/commit/46be47c9f51ef10d0082f1bd39ffff1008682861





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

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

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

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

 Description   

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



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

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





[CLJ-1113] `reductions` reducer Created: 23/Nov/12  Updated: 03/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Marshall T. Vandegrift Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reducers

Attachments: File reductions-reducer.diff    
Patch: Code and Test

 Description   

It would be nice to have a reducers implementation of the core `reductions` function.

Initial implementation attempt included. This version requires an initial reduction value parameter (vs using (f)) and includes that initial value in the final reduction. I'm not certain either of these decisions is optimal, but results in a function which most closely mimics `clojure.core/reductions`.



 Comments   
Comment by Andy Fingerhut [ 03/Sep/14 8:12 PM ]

Patch reductions-reducer.diff dated Nov 23 2012 no longer applies cleanly to latest master after some commits were made to Clojure on Sep 3, 2014.

I have not checked whether this patch is straightforward to update, nor do I know if there is interest in an updated patch, or whether transducers are the preferred way to go over reducers, and thus all reducers-related tickets will be closed. See the section "Updating stale patches" at http://dev.clojure.org/display/community/Developing+Patches for suggestions on how to update patches.





[CLJ-1046] Drop-while as a reducer Created: 18/Aug/12  Updated: 03/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reducers

Attachments: Text File drop-while-reducer.patch    
Patch: Code and Test

 Description   

Implement drop-while as a reducer. Follows the same atom-based strategy as drop and take.

Does not depend on any of my other reducer patches, but there will probably be some minor merge conflicts unless it is merged after CLJ-1045, and before CLJ-992 and CLJ-993.



 Comments   
Comment by Andy Fingerhut [ 03/Sep/14 8:11 PM ]

Patch drop-while-reducer.patch dated Aug 18 2012 no longer applies cleanly to latest master after some commits were made to Clojure on Sep 3, 2014.

I have not checked whether this patch is straightforward to update, nor do I know if there is interest in an updated patch, or whether transducers are the preferred way to go over reducers, and thus all reducers-related tickets will be closed. See the section "Updating stale patches" at http://dev.clojure.org/display/community/Developing+Patches for suggestions on how to update patches.





[CLJ-991] partition-by reducer Created: 10/May/12  Updated: 03/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reducers

Attachments: File reducer-partition-by2.diff     File reducer-partition-by3.diff     File reducer-partition-by4.diff     File reducer-partition-by.diff    
Patch: Code and Test

 Comments   
Comment by Rich Hickey [ 14/Aug/12 1:52 PM ]

I'd like to see something much faster than this.

Comment by Kevin Downey [ 15/Aug/12 1:58 AM ]

For reference here is a benchmark of a non-reducers (seq based) process that uses partition-by

user=> (def x (vec (range 1e6)))
#'user/x
user=> (bench (reduce + (map count (partition-by #(or (zero? (mod % 3)) (zero? (mod % 5))) x))))
Evaluation count             : 60
             Execution time mean : 1.072157 sec  95.0% CI: (1.070606 sec, 1.073381 sec)
    Execution time std-deviation : 165.818282 ms  95.0% CI: (163.873585 ms, 168.271261 ms)
         Execution time lower ci : 972.562000 ms  95.0% CI: (972.562000 ms, 973.301850 ms)
         Execution time upper ci : 1.419148 sec  95.0% CI: (1.419148 sec, 1.419148 sec)

Found 7 outliers in 60 samples (11.6667 %)
	low-severe	 2 (3.3333 %)
	low-mild	 5 (8.3333 %)
 Variance from outliers : 85.8489 % Variance is severely inflated by outliers
nil
user=>

Same again using r/partition-by from reducer-partition-by.diff

user=> (bench (r/reduce + (r/map count (r/partition-by #(or (zero? (mod % 3)) (zero? (mod % 5))) x))))
Evaluation count             : 60
             Execution time mean : 1.418350 sec  95.0% CI: (1.417738 sec, 1.418948 sec)
    Execution time std-deviation : 66.736477 ms  95.0% CI: (66.186568 ms, 67.610777 ms)
         Execution time lower ci : 1.370419 sec  95.0% CI: (1.370419 sec, 1.370419 sec)
         Execution time upper ci : 1.544151 sec  95.0% CI: (1.544151 sec, 1.544156 sec)

Found 10 outliers in 60 samples (16.6667 %)
	low-severe	 2 (3.3333 %)
	low-mild	 8 (13.3333 %)
 Variance from outliers : 33.5591 % Variance is moderately inflated by outliers
nil
user=> 

(using https://github.com/hugoduncan/criterium for benchmarking)

Comment by Kevin Downey [ 15/Aug/12 2:17 AM ]

same again for r/partition-by from reducers-partition-by2.diff

user=> (bench (r/reduce + (r/map count (r/partition-by #(or (zero? (mod % 3)) (zero? (mod % 5))) x))))
Evaluation count             : 180
             Execution time mean : 307.596806 ms  95.0% CI: (307.271339 ms, 307.961550 ms)
    Execution time std-deviation : 34.060809 ms  95.0% CI: (33.613169 ms, 34.416837 ms)
         Execution time lower ci : 285.339333 ms  95.0% CI: (285.339333 ms, 285.339333 ms)
         Execution time upper ci : 385.087950 ms  95.0% CI: (385.087950 ms, 385.087950 ms)

Found 10 outliers in 60 samples (16.6667 %)
	low-severe	 4 (6.6667 %)
	low-mild	 6 (10.0000 %)
 Variance from outliers : 73.8053 % Variance is severely inflated by outliers
nil
user=> 

same again driven using r/fold (for grins) instead of r/reduce

user=> (bench (r/fold + (r/map count (r/partition-by #(or (zero? (mod % 3)) (zero? (mod % 5))) x))))
Evaluation count             : 360
             Execution time mean : 215.214486 ms  95.0% CI: (214.915417 ms, 215.664236 ms)
    Execution time std-deviation : 36.588464 ms  95.0% CI: (36.305548 ms, 36.847846 ms)
         Execution time lower ci : 185.575000 ms  95.0% CI: (185.575000 ms, 185.575000 ms)
         Execution time upper ci : 287.605175 ms  95.0% CI: (286.547833 ms, 287.605175 ms)

Found 6 outliers in 60 samples (10.0000 %)
	low-severe	 3 (5.0000 %)
	low-mild	 3 (5.0000 %)
 Variance from outliers : 87.6303 % Variance is severely inflated by outliers
nil
user=> 

reducers-partition-by2.diff is faster, but I am not wild about introducing a type and a protocol. also not sure about the CollFold impl.

Comment by Rich Hickey [ 15/Aug/12 6:58 AM ]

Let's leave fold out for this first patch, please.

Comment by Kevin Downey [ 15/Aug/12 2:34 PM ]

reducer-partition-by3.diff is a cleaned up version of reducer-partition-by2.diff without fold

Comment by Devin Walters [ 20/Oct/12 7:07 PM ]

Per Andy Fingerhut's email reducer-partition-by3.diff was failing to apply. This patch should apply cleanly to current master.

Comment by Andy Fingerhut [ 01/Nov/12 6:59 PM ]

Presumptuously changing Approval from Incomplete to None, since the reason for its being marked Incomplete seems to have been addressed with the latest patch.

Comment by Kevin Downey [ 04/Mar/13 2:49 PM ]

should this be assigned to someone?

Comment by Andy Fingerhut [ 03/Sep/14 8:09 PM ]

Patch reducer-partition-by4.diff dated Oct 20 2012 no longer applies cleanly to latest master after some commits were made to Clojure on Sep 3, 2014.

I have not checked whether this patch is straightforward to update, nor do I know if there is interest in an updated patch, or whether transducers are the preferred way to go over reducers, and thus all reducers-related tickets will be closed. See the section "Updating stale patches" at http://dev.clojure.org/display/community/Developing+Patches for suggestions on how to update patches.





[CLJ-1517] unrolled small collections Created: 01/Sep/14  Updated: 03/Sep/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: Zach Tellman Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: collections, performance

Attachments: File unrolled-collections-2.diff     File unrolled-collections.diff    
Patch: Code
Approval: Vetted

 Description   

As discussed on the mailing list [1], this patch has two unrolled variants of vectors and maps, with special inner classes for each cardinality. Currently both grow to six elements before spilling over into the general versions of the data structures, which is based on rough testing but can be easily changed. At Rich's request, I haven't included any integration into the rest of the code, and there are top-level static create() methods for each.

The sole reason for this patch is performance, both in terms of creating data structures and performing operations on them. This can be seen as a more verbose version of the trick currently played with PersistentArrayMap spilling over into PersistentHashMap. Based on the benchmarks, which can be run by cloning cambrian-collections [2] and running 'lein test :benchmark', this should supplant PersistentArrayMap. Performance is at least on par with PAM, and often much faster. Especially noteworthy is the creation time, which is 5x faster for maps of all sizes (lein test :only cambrian-collections.map-test/benchmark-construction), and on par for 3-vectors, but 20x faster for 5-vectors. There are similar benefits for hash and equality calculations, as well as calls to reduce().

This is a big patch (over 5k lines), and will be kind of a pain to review. My assumption of correctness is based on the use of collection-check, and the fact that the underlying approach is very simple. I'm happy to provide a high-level description of the approach taken, though, if that will help the review process.

I'm hoping to get this into 1.7, so please let me know if there's anything I can do to help accomplish that.

[1] https://groups.google.com/forum/#!topic/clojure-dev/pDhYoELjrcs
[2] https://github.com/ztellman/cambrian-collections



 Comments   
Comment by Zach Tellman [ 01/Sep/14 10:13 PM ]

Oh, I forgot to mention that I didn't make a PersistentUnrolledSet, since the existing wrappers can use the unrolled map implementation. However, it would be moderately faster and more memory efficient to have one, so let me know if it seems worthwhile.

Comment by Nicola Mometto [ 02/Sep/14 5:23 AM ]

Zach, the patch you added isn't in the correct format, they need to be created using `git format-patch`

Comment by Nicola Mometto [ 02/Sep/14 5:31 AM ]

Also, I'm not sure if this is on-scope with the ticket but those patches break with *print-dup*, as it expects a static create(x) method for each inner class.

I'd suggest adding a create(Map x) static method for the inner PersistentUnrolledMap classes and a create(ISeq x) one for the inner PersistentUnrolledVector classes

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

Re making patches, see: http://dev.clojure.org/display/community/Developing+Patches

Comment by Jozef Wagner [ 02/Sep/14 9:16 AM ]

I wonder what is the overhead of having meta and 2 hash fields in the class. Have you considered a version where the hash is computed on the fly and where you have two sets of collections, one with meta field and one without, using former when the actual metadata is attached to the collection?

Comment by Zach Tellman [ 02/Sep/14 12:13 PM ]

I've attached a patch using the proper method. Somehow I missed the detailed explanation for how to do this, sorry. I know the guidelines say not to delete previous patches, but since the first one isn't useful I've deleted it to minimize confusion.

I did the print-dup friendly create methods, and then realized that once these are properly integrated, 'pr' will just emit these as vectors. I'm fairly sure the create methods aren't necessary, so I've commented them out, but I'm happy to add them back in if they're useful for some reason I can't see.

I haven't given a lot of thought to memory efficiency, but I think caching the hashes are worthwhile. I can see an argument for creating a "with-meta" version of each collection, but since that would double the size of an already enormous patch, I think that should probably wait.

Comment by Zach Tellman [ 03/Sep/14 4:31 PM ]

I found a bug! Like PersistentArrayMap, I have a special code path for comparing keywords, but my generators for collection-check were previously using only integer keys. There was an off-by-one error in the transient map implementation [1], which was not present for non-keyword lookups.

I've taken a close look for other gaps in my test coverage, and can't find any. I don't think this substantively changes the risk of this patch (an updated version of which has been uploaded as 'unrolled-collections-2.diff'), but obviously where there's one bug, there may be others.

[1] https://github.com/ztellman/cambrian-collections/commit/eb7dfe6d12e6774512dbab22a148202052442c6d#diff-4bf78dbf5b453f84ed59795a3bffe5fcR559





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

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

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

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

 Description   

Motivation:

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

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

API:

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

Patch: volatile3.diff

Screened by: fogus



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

Dumb benchmark before/after...

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

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

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

From Stu H elsewhere:

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

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

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

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

Why is vswap! a macro?

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

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

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

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

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

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

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

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

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

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

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

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

Made changes as requested.

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

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

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

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





[CLJ-1494] remove flatmap in favor of mapcat Created: 07/Aug/14  Updated: 03/Sep/14  Resolved: 03/Sep/14

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

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

Attachments: Text File 0001-remove-flatmap-use-1-arity-mapcat-instead.patch    
Patch: Code

 Description   

While all the transducers functions are implemented as an arity in the matching clojure core sequence, for mapcat a new function has been added: flatmap.
The reason for this is, as Rich said in a HN comment, "because mapcat's signature was not amenable to the additional arity".
This patch changes the mapcat signature to take at least one collection so that it's possible to add the 1-arity for the transducer function, eliminating the need for a different function, flatmap.

There has been no loss by removing the 1-arity version of mapcat as a sequence function since trying to use (mapcat f) as currently defined (not as redefined with this patch) would fail before transducers, and after transducers:
Before transducers (mapcat f) would result in a call to (map f) which would fail with an ArityException
After transducers that (map f) call would return a function, which then would be used as an argument to (apply concat the-f), resulting in a IllegalArgumentException since apply expects a sequence but it's been given a fn.



 Comments   
Comment by Alex Miller [ 03/Sep/14 11:02 AM ]

Done as a result of https://github.com/clojure/clojure/commit/7d84a9f6f35a503cddf98487b6544d18937c669e





[CLJ-700] contains? broken for transient collections Created: 01/Jan/11  Updated: 01/Sep/14

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

Type: Defect Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: None

Attachments: Java Source File 0001-Refactor-of-some-of-the-clojure-.java-code-to-fix-CL.patch     File clj-700-7.diff     File clj-700-8.diff     File clj-700.diff     Text File clj-700-patch4.txt     Text File clj-700-patch6.txt    
Patch: Code and Test
Approval: Vetted

 Description   

Behavior with Clojure 1.6.0:

user=> (contains? (transient {:x "fine"}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentArrayMap$TransientArrayMap  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient (hash-map :x "fine")) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashMap$TransientHashMap  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient [1 2 3]) 0)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentVector$TransientVector  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient #{:x}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashSet$TransientHashSet  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (:x (transient #{:x}))
nil
;; expected: :x

user=> (get (transient #{:x}) :x)
nil
;; expected: :x

Behavior with latest Clojure master as of Jun 27 2014 (same as Clojure 1.6.0) plus patch clj-700-7.diff. In all cases it matches the expected results shown in comments above:

user=> (contains? (transient {:x "fine"}) :x)
true
user=> (contains? (transient (hash-map :x "fine")) :x)
true
user=> (contains? (transient [1 2 3]) 0)
true
user=> (contains? (transient #{:x}) :x)
true
user=> (:x (transient #{:x}))
:x
user=> (get (transient #{:x}) :x) 
:x

Analysis by Alexander Redington: This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Questions on this approach from Stuart Halloway to Rich Hickey:

1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?

2. what are good names for the interfaces introduced here?

Alex Miller: Should also keep an eye on CLJ-787 as it may have some collisions with this one.

Patch: clj-700-8.diff

One 'trailing whitespace' warning is perfectly normal when applying this patch to latest Clojure master as of Sep 1 2014, as shown below. This is simply because of carriage returns at the end of lines in file Associative.java. I know of no way to avoid such a warning without removing CRs from all Clojure source files (e.g. CLJ-1026):

% git am -s --keep-cr --ignore-whitespace < ~/clj/patches/clj-700-8.diff
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/andy/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq


 Comments   
Comment by Herwig Hochleitner [ 01/Jan/11 8:01 PM ]

the same is also true for TransientVectors

{{(contains? (transient [1 2 3]) 0)}}

false

Comment by Herwig Hochleitner [ 01/Jan/11 8:25 PM ]

As expected, TransientSets have the same issue; plus an additional, probably related one.

(:x (transient #{:x}))

nil

(get (transient #{:x}) :x)

nil

Comment by