<< Back to previous view

[CLJ-2027] bean printing regression from namespace map printing Created: 24/Sep/16  Updated: 26/Sep/16

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

Type: Defect Priority: Major
Reporter: Trey Sullivan Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print, regression
Environment:

Clojure 1.9.0-alpha12


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

 Description   

The new namespace map printing is causing a failure in printing bean maps (which are proxies that don't support empty):

user=> (bean (java.util.Date.))
UnsupportedOperationException empty  clojure.core.proxy$clojure.lang.APersistentMap$ff19274a.empty (:-1)

user=> (pst *e)
UnsupportedOperationException empty
	clojure.core.proxy$clojure.lang.APersistentMap$ff19274a.empty (:-1)
	clojure.core/empty (core.clj:5151)
	clojure.core/lift-ns (core_print.clj:237)

Cause: The internal lift-ns function calls empty on the map too early (here it doesn't need to call it at all).

Proposed: Defer calling (empty m) until we know map has namespace keywords and namespace maps will be used for printing.

Patch: clj-2027.patch (note that into is not used in the change b/c it has not yet been defined at this point)






[CLJ-2024] Check should specize function specs before checking Created: 19/Sep/16  Updated: 22/Sep/16

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

Type: Defect Priority: Major
Reporter: James Reeves Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Attachments: Text File clj-2024.patch    
Patch: Code
Approval: Vetted

 Description   

This code works fine in 1.9.0-alpha12:

(defn f [x] (+ x 1))
(s/def f (s/fspec :args (s/cat :x number?) :ret number?))
(stest/check `f)

But if we factor the fspec out into its own keyword:

(defn f [x] (+ x 1))
(s/def ::inc (s/fspec :args (s/cat :x number?) :ret number?))
(s/def f ::inc)
(stest/check `f)

The check fails with the exception:

({:failure #error {
 :cause "No :args spec"
 :data #:clojure.spec{:failure :no-args-spec}
 :via
 [{:type clojure.lang.ExceptionInfo
   :message "No :args spec"
   :data #:clojure.spec{:failure :no-args-spec}
   :at [...]}]
 :trace
 [...]}, :sym user/f, :spec :user/inc})

The check function doesn't seem to be resolving ::inc, when presumably it should.






[CLJ-322] Enhance AOT compilation process to emit classfiles only for explicitly-specified namespaces Created: 29/Apr/10  Updated: 07/Sep/16

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

Type: Enhancement Priority: Major
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Unresolved Votes: 27
Labels: aot

Attachments: Text File 0322-limit-aot-resolved.patch     File CLJ-322.diff     Text File compile-interop-1.patch     GZip Archive write-classes-1.diff.gz    
Patch: Code and Test
Approval: Vetted
Waiting On: Chas Emerick

 Description   

Summary: still needs decision on implementation approach.

This was originally/erroneously reported by Howard Lewis Ship in the clojure-contrib assembla:

My build file specifies the namespaces to AOT compile but if I include another namespace
(even from a JAR dependency) that is not AOT compiled, the other namespace will be compiled as well.

In my case, I was using clojure-contrib's clojure.contrib.str-utils2 namespace, and I got a bunch of
clojure/contrib/str_utils2 classes in my output directory.

I think that the AOT compiler should NOT precompile any namespaces that are transitively reached,
only namespaces in the set specified by the command line are appropriate.

As currently coded, you will frequently find unwanted third-party dependencies in your output JARs;
further, if multiple parties depend on the same JARs, this could cause bloating and duplication in the
eventual runtime classpath.

Having the option of shipping either all AOT-compiled classfiles or mixed source/AOT depending upon one's distribution requirements would make that phase of work with a clojure codebase significantly easier and less error-prone. The only question in my mind is what the default should be. We're all used to the current behaviour, but I'd guess that any nontrivial project where the form of the distributable matters (i.e. the source/AOT mix), providing as much control as possible by default makes the most sense. Given the tooling that most people are using, it's trivial (and common practice, IIUC) to provide a comprehensive list of namespaces one wishes to compile, so making that the default shouldn't be a hurdle to anyone. If an escape hatch is desired, a --transitive switch to clojure.lang.Compile could be added.



 Comments   
Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/322
Attachments:
aot-transitivity-option-compat-322.diff - https://www.assembla.com/spaces/clojure/documents/aI7Eu-HeGr35ImeJe5cbLA/download/aI7Eu-HeGr35ImeJe5cbLA
aot-transitivity-option-322.diff - https://www.assembla.com/spaces/clojure/documents/aIWFiWHeGr35ImeJe5cbLA/download/aIWFiWHeGr35ImeJe5cbLA

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

hlship said: I'd like to reinforce this. I've been doing research on Clojure build tools for an upcoming talk and all of them (Maven, Leiningen, Gradle) have the same problem: the AOT compile extends from the desired namespaces (such as one containing a :gen-class) to every reached namespace. This is going to cause a real ugliness when application A uses libraries B and C that both depend on library D (such as clojure-contrib) and B and C are thus both bloated with duplicate, unwanted AOT compiled classes from the library D.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: This behaviour is an implementation detail of Clojure's AOT compilation process, and is orthogonal to any particular build tooling.

I am working on a patch that would provide a mechanism for such tooling to disable this default behaviour.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: A first cut of a change to address this issue is here (caution, work in progress!):

http://github.com/cemerick/clojure/commit/6f14e0790c0d283a7e44056adf1bb3f36bb16e0e

This makes available a new recognized system property, clojure.compiler.transitive, which defaults to true. When set/bound to false (i.e. -Dclojure.compiler.transitive=false when using clojure.lang.Compile), only the first loaded file (either the ns named in the call to compile or each of the namespaces named as arguments to clojure.lang.Compile) will have classfiles written to disk.

This means that this compilation invocation:

java -cp <your classpath> -Dclojure.compiler.transitive=false clojure.lang.Compile com.bar com.baz

will generate classfiles only for com.bar and com.baz, but not for any of the namespaces or other files they load, require, or use.


The only shortcoming of this WIP patch is that classfiles are still generated for proxy and gen-class classes defined outside of the explicitly-named namespaces. What I thought was a solution for this ended up breaking the loading of generated interfaces (as produced by defprotocol, etc).

I'll take a second look at this before the end of the week, but wanted to get this out there so as to get any comments people might have.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

technomancy said: Looks good, but I'm having trouble getting it to work. I tried compiling from master of Chas's fork on github, but I still got the all the .class files generated with -Dclojure.compiler.transitive=false. It could be a quirk of the way I'm using ant to fork off processes though. Is it possible to set it using System/setProperty, or must it be given as a property on the command-line?

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: Bah, that's just bad documentation. :-/

The system property is only provided by clojure.lang.Compile; the value of it drives the binding of clojure.core/transitive-compile, which has a root binding of true.

You should be able to configure the transitivity the same way you configure compile-path (system prop to clojure.lang.Compile or a direct binding when at the REPL, etc).

If not, ping me in irc or elsewhere.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

meikelbrandmeyer said: I think, excluding parts 'load'ed is a little strong. I have some namespaces which load several parts from different files, but which belong to the same namespace. The most prominent example of such a case is clojure.core itself. I'm find with stopping require and use, but load is a bit too much, I'd say.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

technomancy said: Chas: Thanks; will give that a go.

Meikel: Do people actually use load outside of clojure.core? I thought it was only used there because clojure.core is a "special" namespace where you want more vars to be available than can reasonably fit in a single file. Splitting up a namespace into several files is quite unadvisable otherwise.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

technomancy said: I can confirm that this works for me modulo the proxy/gen-class issue that Chas mentioned. I would love to see this in Clojure 1.2; it would really clean up a lot of build-related issues.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

meikelbrandmeyer said: I used it several times and this is the first time, I hear that it is unadvisable to do so. Even with a lower number of Vars in the namespace (c.c is here certainly exceptional) and might be of use to split several "sections" of code which belong to the same namespace but have different functionality. Whether to use a big comment in the source to indicate the section or split things into subfiles is a matter of taste. But it's a perfectly reasonable thing todo.

Another use case, where I use this (and c.c.lazy-xml, IIRC) is to conditionally load code depending on whether a dependency is available or not. Eg. vimclojure uses c.c.pprint and c.c.stacktrace/clj-stacktrace transparently depending on their availability.

There are perfectly legal uses of load. I don't see any "unadvisable" here.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: Thanks, Meikel; I had forgotten about that use case, as I don't use load directly myself at all. I probably wouldn't say it's inadvisable, just mostly unnecessary. In any case, that's a good catch. It complicates things a bit, but we'll see what happens. I'm going to take another whack at resolving the proxy/gen-class case and narrowing the impact of nontransitivity to use and require later tonight.

I agree wholeheartedly that this should be in 1.2, assuming the technical bits work out. This has been an irritant for quite a long time. I actually believe that nontransitivity should be the default – no one wants or expects to have classfiles show up for dependencies show up when a project is AOT-compiled. I think the only negative impact would be whoever still fiddles with compilation at the REPL, and doesn't use maven or lein – and even then, it's just a matter of binding another var.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

meikelbrandmeyer said: Then the var should be added to the default bindings in the clojure.main repl. Then it's set!-able like the other vars ��� warn-on-reflection and friends.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: This is looking pretty good (still WIP):

http://github.com/cemerick/clojure/commit/fedfb022ecef420a932b3d69c182ec7a8e5960a6

Thank you again for mentioning load, Meikel: it was very helpful in resolving the proxy/gen-class issue as well.

Just a single data point: the jar produced by the medium-sized project I've been using for testing the changes has shrunk from 1.8MB to less than 1MB. That's not the only reason this is a good change, but it's certainly a nice side-effect.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: [file:aIWFiWHeGr35ImeJe5cbLA]

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: [file:aI7Eu-HeGr35ImeJe5cbLA]

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: Patched attached. The compat one retains the current default behaviour [*transitive-compile* true], the other changes the default so that transitivity is a non-default option. At least of those I've spoken to about this, the latter is preferred.

The user impact of changing the default would be:

  1. The result of compiling from the REPL will change. Getting back current behaviour would require adding a [*transitive-compile* true] binding to the existing bindings one must set when compiling from the REPL.
  2. The same as #1 goes for those scripting AOT compilation via clojure.lang.Compile as well (whether by shell scripts, ant, etc).
  3. Those using lein, clojure-maven-plugin, gradle, and others will likely have a new option provided by those tools, and perhaps a different default than the language's. I suspect those using such tools would much prefer a change from the default behaviour in any case.
Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

hlship said: Just had a brain-storm:

How about an option to support transitive compilation, but only if the Clojure source file being compiled as a file: URL (i.e., its a local file on the file system, not a file stored in a JAR). That would make it easier to use compilation on the local project without transitively compiling imported libraries, such as clojure-contrib.

So transitive-compile should be a keyword, not a boolean, with values :all (for 1.1 behavior), :none (to compile only the exact specified namespaces) or :local (to compile transitively, but only for local files, not source files from JARs).

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: (Crossposted to the clojure-dev list)

I thought about this some, and I don't think that's a good idea, at least for now. I'm uncomfortable with semantics changing depending upon where code is being loaded from – which, depending upon a tool's implementation, might be undefined. E.g. if the com.foo.bar ns is available in source form in one directory, but as classes from a jar, and classpaths aren't being constructed in a stable fashion, then the results of compilation will change.

If we decide that special treatment depending upon the source of code is warranted in the future, that's a fairly straightforward thing to do w.r.t. the API – we could have :all and :local as you suggest, with nil representing :none.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

stu said: Rich is not comfortable enough with the implementation complexity of this patch (e.g. the guard clause for proxies and gen-class) to slide this in as a minor fix under the wire for 1.2.

Better to live with the pain we know a little longer than ship something we don't have enough experience with to be confident.

Comment by Chas Emerick [ 19/Nov/10 9:28 PM ]

Updated patch to cleanly apply to HEAD and address issues raised by screening done by Cosmin Stejerean. Also includes proper tests.

Note: this patch's tests require the fix for CLJ-432!

Comment by Stuart Halloway [ 29/Nov/10 7:18 AM ]

the "-resolved" patch resolves a conflict in main.clj

Comment by Stuart Halloway [ 29/Nov/10 7:25 AM ]

Several questions:

  1. I am getting an ant build error: "/Users/stuart/repos/clojure/build.xml:137: java.io.FileNotFoundException: Could not locate clojure/test_clojure/aot/nontransitive__init.class or clojure/test_clojure/aot/nontransitive.clj on classpath:"
  2. It feels icky to have a method named writeClassFile that, under some circumstances, does not write a class file, but instead loads it via a dynamic loader. Maybe this is just a naming issue.
  3. Are there any other ways to accomplish the goals of load-level? Or, taking the other side, if we are going to have a load-level, are there other possible consumers who might have different needs?
  4. (Minor) Why the use :only idiom instead of just require?
Comment by Stuart Sierra [ 10/Dec/10 3:34 PM ]

An alternative approach: patch write-classes-1.diff.gz

From my forked branch

What this patch does:

  • Keeps 'compile' and 'compile-files' exactly the same
  • Adds 'compile-write-classes' to write .class files for specifically named classes
  • Minor compiler changes to support this

This approach was prompted by the following observations:

  • Java interop is the dominant reason for needing .class files
  • Things other than namespaces can generate classes for Java interop:
    • deftype/defrecord
    • defprotocol
    • gen-class/gen-interface
  • For library releases, we want to control which .class files are emitted on a per-class basis, not per-namespace
  • Some legitimate uses of AOT compilation will want transitive compilation
    • Pre-compiling an entire application before release
Comment by Chas Emerick [ 10/Dec/10 4:04 PM ]

S. Halloway: My apologies, I didn't know you had commented. I thought that, having assigned this issue to myself, I'd be notified of updates.

FWIW, I aim to review your comments and SS' approach over the weekend.

Comment by Chas Emerick [ 16/Dec/10 7:36 AM ]

S. Halloway:

1. Certainly shouldn't happen. AFAIK, others have screened the patch, presumably with a successful build.
2. Agreed; given the approach, I think it's just a bad name.
3. Yes, I think S. Sierra's is one. See my next comment.
4. Because the :use form was already there. I've actually been using that form of :use more and more; I've found that easier than occasionally having to shuffle around specs between :use and :require. I think I'm aping Chris Houser in that regard.

Comment by Chas Emerick [ 16/Dec/10 9:00 AM ]

I think S. Sierra's approach is fundamentally superior what I offered. I have two suggestions: one slight perspective change (which implies no change in the actual implementation), and an idea for an even simpler approach (at least from a user perspective), in that order.

While interop is the driving requirement behind AOT, I absolutely do not want to have to keep an updated enumeration of all of the classes resulting from each and every defrecord et al. usages in my pom.xml/project.clj (and I wouldn't wish the task of ferreting those usages and their resulting classnames on any build tool author).

Right now, *compile-write-classes* is documented to be a set of classname strings, but could just as easily be any other function. *compile-write-classes* should be documented to accept any predicate function (renamed to e.g. *compile-write-class?*?). There's no reason why it shouldn't be bound to, e.g. #(re-matches #"foo\.bar\.[\w_]+$" %) if I know that all my records are defined in the foo.bar namespace.

To go along with that, I think some package/classname-globbing utilities along with corresponding options to clojure.lang.Compile would be most welcome. Classname munging rules are not exactly obvious, and it'd be good to make things a little easier for users in this regard.


Another alternative

If there's a closed set of forms that generate classes that one might reasonably be interested in having in a build result (outside of use cases for pervasive AOT), then why not have a simple option that only those forms utilize? gen-class and gen-interface already do this, but reusing the all-or-nothing *compile-files* binding; if they keyed off of a binding that implied a diminished scope (e.g. *compile-interop-forms* – which would be true if *compile-files* were true), then they'd do exactly what we wanted. Extending this approach to deftype (and therefore defrecord) should be straightforward.

An implementation of this would probably be somewhat more complicated than S. Sierra's patch, though not as complex as my original stab at the problem (i.e. no *load-level*). On the plus side:

1. No additional configuration for users or implementation work for build tool authors, aside from the addition of the boolean diminished-scope AOT option
2. Class file generation would remain opaque from a build process standpoint
3. Future/other class-generating forms (there are a few people futzing with ASM independently, etc) can make local decisions about whether or not to participate in interop-centric classfile generation. This might be particularly helpful if a given form emits multiple classes, making the determination of a classname-based filter fn less straightforward.

I can see wanting to further restrict AOT to specific classnames in certain circumstances, in which case the above and S. Sierra's patch might be complimentary.

Comment by Stuart Sierra [ 16/Dec/10 11:49 AM ]

I like the idea of *compile-interop-forms*. But is it always possible to determine what an "interop form" is? I think it is, I'm just not sure.

Comment by Allen Rohner [ 09/Oct/11 12:50 PM ]

I'm also in favor of compile-interop-forms. As far as determining, how about sticking metadata on the var?

(defmacro ^{:interop-form true} deftype ...)

Comment by Stuart Sierra [ 21/Oct/11 8:38 AM ]

Summary and design discussion on wiki at http://dev.clojure.org/display/design/Transitive+AOT+Compilation

Comment by Stuart Sierra [ 29/Nov/11 6:54 PM ]

New attachment compile-interop-1.patch has new approach: Add a third possible value for *compile-files*. True and false keep their original meanings, but :interop causes only interop-related forms to be written out as .class files. "Interop forms" are gen-class, gen-interface, deftype, defrecord, defprotocol, and definterface.

Pros:

  • doesn't change existing behavior
  • handles common case for non-transitive AOT (interop)
  • minimal changes to the compiler

Cons:

  • not flexible
Comment by Stuart Sierra [ 02/Dec/11 8:12 AM ]

Just realized my patch doesn't solve the transitive compilation problem. If library A loads library B, then compiling interop forms in A will also emit interop .class files in B.

Comment by Paudi Moriarty [ 01/Jan/13 3:55 AM ]

It's disappointing to see an important issue like this still unresolved after 2.5 years. This is a real pain for us. We have a large closed source project where shipping source is not an option. This forces us to manage the AOT'ing of dependencies due to the hard dependency on protocol interfaces introduced by transitive AOT compilation (see https://groups.google.com/forum/?fromgroups=#!topic/clojure-dev/r3A1JOIiwVU).

Comment by Andy Fingerhut [ 01/Jan/13 4:27 PM ]

Paul, do you have a suggestion for which of the approaches described in comments here, or on the wiki page http://dev.clojure.org/display/design/Transitive+AOT+Compilation would be preferable solution for you? Or perhaps even a patch that implements your preferred approach?

Comment by Howard Lewis Ship [ 04/Jan/13 4:18 PM ]

Andy,

I'm now consulting with Paudi's organization, so I think I can speak for him (I'm now the default buildmeister).

I like Stuart's :interop idea, but that is somewhat orthogonal to our needs.

I return to what I would like; compilation would compile specific namespaces; dependencies of those namespaces would not be compiled.

To be honest, I'm still a little hung up on the interop forms: especially defprotocol and friends; from a cursory glance, it appears that todays AOT compilation will compile the protocol into a Java class, then compile the namespace that references the protocol with the assumption that the protocol's Java class is available. When we use build rules to only package our namespace's class files into the output JAR, the code fails with a NoClassDefFoundError because the protocol really needs to be recompiled, at runtime compilation, into an in-memory Java class.

Obviously, supporting this correctly will be a challenge; the compiled bytecode for our namespace would ideally:
1) check to see if the Java class already exists and use it if so
2) load the necessary namespaces so as to force the creation of the Java class

I can imagine any number of ways to juggle things to make this work, so I won't suggest a specific implementation.

In the meantime, our workaround is to create a "stub" module as part of our build; it simply requires in the necessary namespaces (for example, org.clojure:core.cache); this forces an AOT compile of the dependencies and we have a special rule to package such dependencies in the stub module's output JAR. This may not be a scalable problem, and it is expensive to identify what 3rd party dependencies require this treatment.

Comment by Stuart Halloway [ 24/May/13 1:25 PM ]

I am marking this incomplete because there does not yet seem to be plurality, much less consensus or unanimity, on approach.

Personally I am in favor of a solution based on a predicate that gets handed the class name and compiled bits, and then can choose whether to write the class. Pretty close to Stuart Sierra's compile-write-classes. Might be possible to flow more information than classname to the predicate.

Comment by Alex Miller [ 21/Oct/13 2:35 PM ]

Removed the 1.6 release from this and added to Release.Next list to make this a priority for the next release.

Comment by Kevin Downey [ 21/Oct/13 3:42 PM ]

Howard,

I don't exactly understand your write up, I am reading the compiler, the emit-protocol macro, and emit-method-builder to try and understand it.

You might check to see if you have a situation similar to the following:

(ns a.b)

(defprotocol P1
  (pm [a]))

then either

(ns a.c
  (:import (a.b P1))

(defrecord R []
  P1
  (pm [x] x))

or

(ns a.c)

(defrecord R []
  a.b.P1
  (pm [x] x))

in both examples defrecord is actually getting the class behind the protocol instead of the protocol, the correct thing to do is

(ns a.c
  (:require [a.b :refer [P1]]))

(defrecord R []
  P1
  (pm [x] x))

This is an extremely common mistake people make when using protocols, unfortunately the flexibility of using interfaces directly in defrecord forms, and protocols being backed by interfaces means it is very easy to unwittingly make such a mistake. Both of the mistake examples could result in missing classes/namespace problems.

Comment by Michael Blume [ 08/Jan/15 4:26 PM ]

The write-classes patch still applies cleanly, I just tried it out, made a one-line change to lein-ring and got a war-file for my app with two generated classes instead of hundreds. This seems like a good fix to me. What do we need to do to get it into release?

Comment by Alex Miller [ 08/Jan/15 4:30 PM ]

As Stu said above, we do not have consensus on whether this is the right way to go with this. I am planning to look at all outstanding AOT tickets (including this one) early in 1.8.

Comment by Stuart Halloway [ 07/Sep/16 3:15 PM ]

I believe that we should close this ticket. AOT ought to make all the classfiles as it does today to avoid classloader inversion problems (see e.g. http://dev.clojure.org/jira/browse/CLJ-1544), and tools can us e.g. boot's sift to remove classes not part of the current project from delivered jars.

We might want to make some tools that identify jars that erroneously includes compiled classes from other namespaces, i.e. resulting from not using sift.

Comment by Kevin Downey [ 07/Sep/16 3:44 PM ]

something like boot's sift is not a complete solution.

lein his included that functionality for sometime, and back in 2010 it defaulted to removing classes transitively generated. that turned out to break things so it was disabled. the functionality still exists but defaults to off.

the setting if someone wants to try it: https://github.com/technomancy/leiningen/blob/master/sample.project.clj#L330
the issue that lead to it being turned off by default: https://github.com/technomancy/leiningen/issues/141





[CLJ-1879] reduce-kv on a PHMs doesn't consistently execute the intended fastpath Created: 09/Jan/16  Updated: 19/Aug/16

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

Type: Defect Priority: Major
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Attachments: Text File CLJ-1879.patch    
Approval: Vetted

 Description   

https://github.com/clojure/clojure/blob/010864f/src/clj/clojure/core.clj#L6553-L6562

Because PHMs implement clojure.lang.IKVReduce and IPersistentMap, they have nondeterministic dispatch through the protocol that backs reduce-kv (clojure.core.protocols/IKVReduce).

A potential way to solve this is to add an instance check for clojure.lang.IKVReduce inside `reduce-kv` (This is similar to how reduce checks for IReduceInit)



 Comments   
Comment by Nicola Mometto [ 11/Jan/16 9:23 AM ]

CLJ-1807 offers a generic solution for this class of problems





[CLJ-1872] empty? is broken for transient collections Created: 26/Dec/15  Updated: 19/Aug/16

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

Type: Defect Priority: Critical
Reporter: Leonid Bogdanov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Approval: Vetted

 Description   

Couldn't find whether it was brought up earlier, but it seems that empty? predicate is broken for transient collections

user=> (empty? (transient []))
IllegalArgumentException Don't know how to create ISeq from: clojure.lang.PersistentVector$TransientVector  clojure.lang.RT.seqFrom (RT.java:528)

user=> (empty? (transient ()))
ClassCastException clojure.lang.PersistentList$EmptyList cannot be cast to clojure.lang.IEditableCollection  clojure.core/transient (core.clj:3209)

user=> (empty? (transient {}))
IllegalArgumentException Don't know how to create ISeq from: clojure.lang.PersistentArrayMap$TransientArrayMap  clojure.lang.RT.seqFrom (RT.java:528)

user=> (empty? (transient #{}))
IllegalArgumentException Don't know how to create ISeq from: clojure.lang.PersistentHashSet$TransientHashSet  clojure.lang.RT.seqFrom (RT.java:528)

The workaround is to use (zero? (count (transient ...))) check instead.



 Comments   
Comment by Alex Miller [ 26/Dec/15 9:58 PM ]

Probably similar to CLJ-700.





[CLJ-1381] Improve support for extending protocols to primitive arrays Created: 13/Mar/14  Updated: 07/Jun/16

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: protocols

Approval: Vetted

 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
Comment by Alex Miller [ 12/Jan/16 3:16 PM ]

Dupe of CLJ-1790

Comment by Alex Miller [ 07/Jun/16 4:00 PM ]

On further inspection, I don't think this is a dupe of CLJ-1790 but merely a related problem.





[CLJ-1790] Error extending protocols to Java arrays Created: 29/Jul/15  Updated: 07/Jun/16

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

Type: Defect Priority: Major
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: compiler, protocols

Attachments: Text File 0001-CLJ-1790-emit-a-cast-to-the-interface-during-procol-.patch    
Patch: Code
Approval: Vetted

 Description   

First reported from core.matrix, but here's a smaller repro:

user=> (defprotocol p (f [_]))
p
user=> (fn [] (f (object-array [])))

VerifyError (class: user$eval15920$fn__15921, method: invoke signature: ()Ljava/lang/Object;) Incompatible object argument for function call  user/eval15920 (form-init9183379085801704163.clj:1)

Cause: The jvm verifier doesn't like situations where we have an array on the stack typed as such, and on a later codepath it is used as target for an invokeinterface even if that path is unreachable because of a previous instance check.

Here's an explanation of exactly our case in pseudo bytecode:

load obj // Object[]
 dup
 instanceof SomeInterface
 iftruejmp label1
 pop
 jmp end
label1:
 // here is where the verifier chokes.
 // it can figure out that the target is of type Object[] which can never be a SomeInterface
 // but it cannot figure out that this code path can never be reached because of the previous
 // instance check with jump
 // to fix this we need to insert an explicit checkcast to SomeInterface on the target
 invokeinterface SomeInterface/someMethod
end:
 return

Proposed: Insert an explicit checkcast to the interface on the target.

Also see: CLJ-1381

Patch: 0001-CLJ-1790emit-a-cast-to-the-interface-during-procol.patch



 Comments   
Comment by Nicola Mometto [ 06/Nov/15 3:53 PM ]

Mike Anderson does 1.8.0-beta2 fix this issue?
Alex Miller if core.matrix is still affected this must be fixed before 1.8.0 as it'd mean that direct linking is still broken

Comment by Nicola Mometto [ 06/Nov/15 6:26 PM ]

I could reproduce the bug with 1.8.0-beta2 btw

Comment by Nicola Mometto [ 06/Nov/15 7:27 PM ]

Apparently this is not a 1.8 regression.

At least 1.6 and 1.7 both manifest the same issue:

Clojure 1.6.0
user=> (defprotocol p (f [_]))
p
user=> (fn [] (f (object-array [])))

VerifyError (class: user$eval15920$fn__15921, method: invoke signature: ()Ljava/lang/Object;) Incompatible object argument for function call  user/eval15920 (form-init9183379085801704163.clj:1)

Comment by Michael Blume [ 06/Nov/15 8:24 PM ]

Do we know why core.matrix works with Clojure 1.6/1.7 then?

Comment by Nicola Mometto [ 06/Nov/15 9:09 PM ]

It doesn't.

Clojure 1.7.0
Java HotSpot(TM) 64-Bit Server VM 1.8.0_60-b27

user=> (require 'clojure.core.matrix.protocols)
nil
user=> (clojure.core.matrix.protocols/construct-matrix (object-array 1) [1])

VerifyError (class: user$eval6935, method: invoke signature: ()Ljava/lang/Object;) Incompatible object argument for function call  java.lang.Class.getDeclaredConstructors0 (Class.java:-2)
user=>

I attached a patch that fixes this issue.
It's caused by the jvm verifier understanding that the object on the stack is an array and thus can never be an instance of the protcol interface, but not being able to understant that the code path leading to the direct protocol interface method invocation can never be reached because of a branch guided by an instance check for that interface on the target

Comment by Mike Anderson [ 06/Nov/15 10:10 PM ]

Apologies, it is possible I just hadn't tested this code path thoroughly before.

It only seems to get triggered in certain circumstances, the following behaviour is interesting:

=> (let [o (identity (object-array 1))]
     (clojure.core.matrix.protocols/dimensionality o))
1
=> (let [o (object-array 1)]
     (clojure.core.matrix.protocols/dimensionality o))
VerifyError (class: clojure/core/matrix$eval17775, method: invokeStatic signature: ()Ljava/lang/Object;) Incompatible object argument for function call  java.lang.Class.getDeclaredConstructors0 (:-2)

Perhaps it only happens when the callsite has type information about the protocol parameter?

Comment by Nicola Mometto [ 07/Nov/15 4:53 AM ]

Correct, apparently the jvm verifier doesn't like situations where we have an array on the stack typed as such, and on a later codepath it is used as target for an invokeinterface even if that path is unreachable because of a previous instance check.

here's an explaination of exactly our case in pseudo bytecode:

..
 load obj // Object[]
 dup
 instanceof SomeInterface
 iftruejmp label1
 pop
 jmp end
label1:
 // here is where the verifier chokes.
 // it can figure out that the target is of type Object[] which can never be a SomeInterface
 // but it cannot figure out that this code path can never be reached because of the previous
 // instance check with jump
 // to fix this we need to insert an explicit checkcast to SomeInterface on the target
 invokeinterface SomeInterface/someMethod
end:
 return




[CLJ-445] Method/Constructor resolution does not factor in widening conversion of primitive args Created: 29/Sep/10  Updated: 04/Mar/16

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

Type: Enhancement Priority: Major
Reporter: Alexander Taggart Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: Text File clj-445-prim-conversion-update-2-patch.txt     Text File prim-conversion.patch     Text File prim-conversion-update-1.patch     Text File reorg-reflector.patch    
Approval: Vetted

 Description   

Problem:
When making java calls (or inlined functions), if both args and param are primitive, no widening conversion is used to locate the proper overloaded method/constructor.

Examples:

user=> (Integer. (byte 0))
java.lang.IllegalArgumentException: No matching ctor found for class java.lang.Integer (NO_SOURCE_FILE:0)
</code></pre>
The above occurs because there is no Integer(byte) constructor, though it should match on Integer(int).
<pre><code>user=> (bit-shift-left (byte 1) 1)
Reflection warning, NO_SOURCE_PATH:3 - call to shiftLeft can't be resolved.
2

In the above, a call is made via reflection to Numbers.shiftLeft(Object, Object) and its associated auto-boxing, instead of directly to the perfectly adequate Numbers.shiftLeft(long, int).

Workarounds:
Explicitly casting to the formal type.

Ancillary benefits of fixing:
It would also reduce the amount of method overloading, e.g., RT.intCast(char), intCast(byte), intCast(short), could all be removed, since such calls would pass to RT.intCast(int).



 Comments   
Comment by Assembla Importer [ 23/Oct/10 6:43 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/445
Attachments:
fixbug445.diff - https://www.assembla.com/spaces/clojure/documents/b6gDSUZOur36b9eJe5cbCb/download/b6gDSUZOur36b9eJe5cbCb

Comment by Assembla Importer [ 23/Oct/10 6:43 PM ]

ataggart said: [file:b6gDSUZOur36b9eJe5cbCb]

Comment by Assembla Importer [ 23/Oct/10 6:43 PM ]

ataggart said: Also fixes #446.

Comment by Stuart Halloway [ 03/Dec/10 12:50 PM ]

The patch is causing a test failure

[java] Exception in thread "main" java.lang.IllegalArgumentException: 
     More than one matching method found: equiv, compiling:(clojure/pprint/cl_format.clj:428)

Can you take a look?

Comment by Stuart Halloway [ 28/Jan/11 12:30 PM ]

The failing test happens when trying to find the correct equiv for signature (Number, long). Is the compiler wrong to propose this signature, or is the resolution method wrong in not having an answer? (It thinks two signatures are tied: (Object, long) and (Number, Number).)

Exception in thread "main" java.lang.IllegalArgumentException: More than one matching method found: equiv, compiling:(clojure/pprint/cl_format.clj:428)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6062)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6050)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.access$100(Compiler.java:35)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5492)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2372)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3277)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6057)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5527)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2385)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5527)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2385)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5527)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$FnMethod.parse(Compiler.java:4667)
	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3397)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6053)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.access$100(Compiler.java:35)
	at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:480)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler.eval(Compiler.java:6114)
	at clojure.lang.Compiler.load(Compiler.java:6545)
	at clojure.lang.RT.loadResourceScript(RT.java:340)
	at clojure.lang.RT.loadResourceScript(RT.java:331)
	at clojure.lang.RT.load(RT.java:409)
	at clojure.lang.RT.load(RT.java:381)
	at clojure.core$load$fn__1427.invoke(core.clj:5308)
	at clojure.core$load.doInvoke(core.clj:5307)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.pprint$eval3969.invoke(pprint.clj:46)
	at clojure.lang.Compiler.eval(Compiler.java:6110)
	at clojure.lang.Compiler.load(Compiler.java:6545)
	at clojure.lang.RT.loadResourceScript(RT.java:340)
	at clojure.lang.RT.loadResourceScript(RT.java:331)
	at clojure.lang.RT.load(RT.java:409)
	at clojure.lang.RT.load(RT.java:381)
	at clojure.core$load$fn__1427.invoke(core.clj:5308)
	at clojure.core$load.doInvoke(core.clj:5307)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.core$load_one.invoke(core.clj:5132)
	at clojure.core$load_lib.doInvoke(core.clj:5169)
	at clojure.lang.RestFn.applyTo(RestFn.java:143)
	at sun.reflect.GeneratedMethodAccessor11.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:597)
	at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:77)
	at clojure.lang.Reflector.invokeInstanceMethod(Reflector.java:28)
	at clojure.core$apply.invoke(core.clj:602)
	at clojure.core$load_libs.doInvoke(core.clj:5203)
	at clojure.lang.RestFn.applyTo(RestFn.java:138)
	at sun.reflect.GeneratedMethodAccessor11.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:597)
	at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:77)
	at clojure.lang.Reflector.invokeInstanceMethod(Reflector.java:28)
	at clojure.core$apply.invoke(core.clj:604)
	at clojure.core$use.doInvoke(core.clj:5283)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.main$repl.doInvoke(main.clj:196)
	at clojure.lang.RestFn.invoke(RestFn.java:422)
	at clojure.main$repl_opt.invoke(main.clj:267)
	at clojure.main$main.doInvoke(main.clj:362)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.lang.Var.invoke(Var.java:401)
	at clojure.lang.AFn.applyToHelper(AFn.java:163)
	at clojure.lang.Var.applyTo(Var.java:518)
	at clojure.main.main(main.java:37)
Caused by: java.lang.IllegalArgumentException: More than one matching method found: equiv
	at clojure.lang.Reflector.getMatchingParams(Reflector.java:639)
	at clojure.lang.Reflector.getMatchingParams(Reflector.java:578)
	at clojure.lang.Reflector.getMatchingMethod(Reflector.java:569)
	at clojure.lang.Compiler$StaticMethodExpr.<init>(Compiler.java:1439)
	at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:896)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	... 115 more
Comment by Alexander Taggart [ 08/Feb/11 6:27 PM ]

In working on implementing support for vararg methods, I found a number of flaws with the previous solutions. Please disregard them.

I've attached a single patch (reflector-compiler-numbers.diff) which is a rather substantial overhaul of the Reflector code, with some enhancements to the Compiler and Numbers code.

The patch notes:

  • Moved reflection functionality from Compiler to Reflector.
  • Reflector supports finding overloaded methods by widening conversion, boxing conversion, and casting.
  • During compilation Reflector attempts to find best wildcard match.
  • Reflector refers to *unchecked-math* when reflectively invoking methods and constructors.
  • Both Reflector and Compiler support variable arity java methods and constructor; backwards compatible with passing an array or nil in the vararg slot.
  • Added more informative error messages to Reflector.
  • Added tests to clojure.test-clojure.reflector.
  • Altered overloaded functions of clojure.lang.Numbers to service Object/double/long params; fixes some ambiguity issues and avoids unnecessary boxing in some cases.
  • Patch closes issues 380, 440, 445, 666, and possibly 259 (not enough detail provided).
Comment by Alexander Taggart [ 10/Feb/11 7:35 PM ]

Updated patch to fix a bug where a concrete class with multiple identical Methods (e.g., one from an interface, another from an abstract class) would result in ambiguity. Now resolved by arbitrary selection (this is what the original code did as well albeit not explicitly).

Comment by Alexander Taggart [ 25/Feb/11 9:29 PM ]

Updated patch to work with latest master branch.

Comment by Stuart Halloway [ 06/Mar/11 1:54 PM ]

patch appears to be missing test file clojure/test_clojure/reflector.clj.

Comment by Alexander Taggart [ 06/Mar/11 2:39 PM ]

Bit by git.

Patch corrected to contain clojure.test-clojure.reflector.

Comment by Stuart Halloway [ 11/Mar/11 10:30 AM ]

Rich: I verified that the patch applied but reviewed only briefly, knowing you will want to go over this one closely.

Comment by Stuart Halloway [ 11/Mar/11 10:55 AM ]

After applying this patch, I am getting method missing errors:

java.lang.NoSuchMethodError: clojure.lang.Numbers.lt(JLjava/lang/Object;)

but only when using compiled code, e.g. the same code works in the REPL and then fails after compilation. Haven't been able to isolate an example that I can share here yet, but hoping this will cause someone to have an "a, hah" moment...

Comment by Alexander Taggart [ 02/Apr/11 12:55 PM ]

The patch now contains only the minimal changes needed to support widening conversion. Cleanup of Numbers overloads, etc., can wait until this patch gets applied. The vararg support is in a separate patch on CLJ-440.

Comment by Christopher Redinger [ 15/Apr/11 12:50 PM ]

Please test patch

Comment by Christopher Redinger [ 21/Apr/11 11:02 AM ]

FYI: Patch applies cleanly on master and all tests pass as of 4/21 (2011)

Comment by Christopher Redinger [ 22/Apr/11 9:57 AM ]

This work is too big to take into the 1.3 beta right now. We'll revisit for a future release.

Comment by Alexander Taggart [ 28/Apr/11 1:19 PM ]

To better facilitate understanding of the changes, I've broken them up into two patches, each with a number of isolable, incremental commits:

reorg-reflector.patch: Moves the reflection/invocation code from Compiler to Reflector, and eliminates redundant code. The result is a single code base for resolving methods/constructors, which will allow for altering that mechanism without excess external coordination. This contains no behaviour changes.

prim-conversion.patch: Depends on the above. Alters the method/constructor resolution process:

  • more consistent with java resolution, especially when calling pre-1.5 APIs
  • adds support for widening conversion of primitive numerics of the same category (this is more strict than java, and more clojuresque)
  • adds support for wildcard matches at compile-time (i.e., you don't need to type-hint every arg to avoid reflection).

This also provides a base to add further features, e.g., CLJ-666.

Comment by Alexander Taggart [ 29/Apr/11 3:01 PM ]

It's documented in situ, but here are the conversion rules that the reflector uses to find methods:

  1. By Type:
    • object to ancestor type
    • primitive to a wider primitive of the same numeric category (new)
  2. Boxing:
    • boxed number to its primitive
    • boxed number to a wider primitive of the same numeric category (new for Short and Byte args)
    • primitive to its boxed value
    • primitive to Number or Object (new)
  3. Casting:
    • long to int
    • double to float
Comment by Alexander Taggart [ 10/May/11 3:13 PM ]

prim-conversion-update-1.patch applies to current master.

Comment by Alexander Taggart [ 11/May/11 2:15 PM ]

Created CLJ-792 for the reflector reorg.

Comment by Stuart Sierra [ 17/Feb/12 2:29 PM ]

prim-conversion-update-1.patch does not apply as of f5bcf64.

Is CLJ-792 now a prerequisite of this ticket?

Comment by Alexander Taggart [ 17/Feb/12 3:15 PM ]

Yes, after the original patch was deemed "too big".

After this much time with no action from TPTB, feel free to kill both tickets.

Comment by Andy Fingerhut [ 20/Feb/12 2:04 PM ]

Again, not sure if this is any help, but I've tested starting from Clojure head as of Feb 20, 2012, applying clj-792-reorg-reflector-patch2.txt attached to CLJ-792, and then applying clj-445-prim-conversion-update-2-patch.txt attached to this ticket, and the result compiles and passes all but 2 tests. I don't know whether those failures are easy to fix or not, or whether issues may have been introduced with these patches.

Comment by Nicola Mometto [ 30/Jan/15 7:25 AM ]

As of 1.7.0-alpha5 the two examples in the ticket description run without exceptions/reflection-warnings

user=> (set! *warn-on-reflection* true)
true
user=> (bit-shift-left (byte 1) 1)
2
user=> (Integer. (byte 0))
0
Comment by Petr Yanovich [ 04/Mar/16 7:17 AM ]

Hi!

(Integer. (Long. 1))
#=> 1
(Long. (Integer. 1))
#=> Exception

https://github.com/clojure/clojure/pull/59





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

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

Type: Defect Priority: Minor
Reporter: Adam Clements Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: android
Environment:

Android ART runtime


Attachments: Text File 0001-CLJ-1472-Locking-macro-without-explicit-monitor-ente.patch     Text File clj-1472-2.patch     Text File clj-1472.patch    
Patch: Code
Approval: Vetted

 Description   

Android ART runs compile time verification on bytecode and fails on any usage of the locking macro. The error looks like that seen in CLJ-1829 (in that case clojure.core.server/stop-server calls the locking macro):

10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: java.lang.VerifyError: Rejecting class clojure.core.server$stop_server because it failed compile-time verification (declaration of 'clojure.core.server$stop_server' appears in /data/app/com.clojure_on_android-1/base.apk)

Cause: From discussion on an Android issue (https://code.google.com/p/android/issues/detail?id=80823), it seems this is due to more strictly enforcing the "structural locking" provisions in the JVM spec (https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-2.html#jvms-2.11.10).

It appears that the mixture of monitor-enter, monitor-exit, and the try/finally block in `locking` create paths that ART is flagging as not having balanced monitorenter/monitorexit bytecode. Particularly, monitorenter and monitorexit themselves can throw (on a null locking object). The Java bytecode does some tricky exception table handling to cover these cases which (afaict) is not possible to do without modifying the Clojure compiler.

Approach: One possible approach is to have locking invoke the passed body in the context of a synchronized block in Java. This avoids the issue of the tricky bytecode by handing it over to Java but has unknown performance impacts.

Patch: clj-1472-2.patch

See also: Examination of the bytecode as compared to a java synchronized block shows up a number of differences:
https://gist.github.com/AdamClements/2ae6c4919964b71eb470



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

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

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

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

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

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

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

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

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

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

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

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

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

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

Comment by Adam Clements [ 28/Nov/14 11:03 AM ]

I have uploaded an alternative implementation of the locking macro (0001-CLJ-1472-Locking-macro-without-explicit-monitor-ente.patch) which cheats a little - the synchronized block is actually implemented in Java and so guarantees compatibility. This is at the cost of a little extra indirection and the naming/location could probably be better.

But it does fix the bug and work on all versions of android, android + art and the jvm. Would this approach be acceptable?

Comment by Kevin Downey [ 08/Dec/14 1:12 PM ]

I have yet to see any evidence that the bytecode clojure is generating in some way violates the jvm spec, so I suspect the issue is clojure requires a jvm to run, and android doesn't provide a jvm, just something that looks like one if you don't tread outside the beaten path.

Comment by Kevin Downey [ 08/Dec/14 1:27 PM ]

given the structured locking verbiage in https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-2.html#jvms-2.11.10, `(locking nil)` may generate bytecode whose runtime behavior violates structured locking. the first patch on this issue can cause the compiler to emit monitorenter/exit instructions in different methods, which definitely violates structured locking

Comment by Adam Clements [ 09/Dec/14 10:45 AM ]

Yes, the first patch was definitely wrong, I left it for some context to the conversation, but it's probably best to just remove it for clarity.

For anyone following this conversation who doesn't want to decompile and observe the bytecode, here's a gist with the difference between a java synchronized block and clojure locking https://gist.github.com/AdamClements/2ae6c4919964b71eb470

I'm finding it hard to work out where the deviation from the spec occurs too, though I can see the difference with the Java version, if anything, the Clojure version looks closer to what's described in the spec than the Java one!

If someone with more knowledge than me on the subject could engage on the AOSP bug https://code.google.com/p/android/issues/detail?id=80823 then perhaps we could settle this as an android bug which is too focussed on the java implementation rather than the JVM spec, or perhaps they'll find something that's wrong with the Clojure implementation. I have uploaded the original clojure behaviour and asked them for some more explanation on why it is failing.

Comment by Adam Clements [ 09/Dec/14 11:09 AM ]

The response from the ART guys about what they think we're violating is:

The section on "Structured locking" contains the following:

"[...] implementations [...] are permitted but not required to enforce
both of the following two rules guaranteeing structured locking. [...]"

ART currently enforces both rules at verification time, including

"At no point during a method invocation may the number of monitor exits
performed by T on M since the method invocation exceed the number of
monitor entries performed by T on M since the method invocation."

Comment by Adam Clements [ 09/Dec/14 11:32 AM ]

If for example instruction https://gist.github.com/AdamClements/2ae6c4919964b71eb470#file-test_locks-class-L24 or the monitor-enter itself on the next line were to fail, couldn't it could end up in the finally clause and attempt to release the lock even though it has never been captured?

I think this violates the structured locking rules in the jvm spec you linked to.

Comment by Kevin Downey [ 09/Dec/14 1:15 PM ]

an interesting question about structured locking, is does the specification refer to the static bytecode or the runtime behavior of the bytecode. given the bytecode linked(https://gist.github.com/AdamClements/2ae6c4919964b71eb470#file-test_locks-class-L24), the static bytecode has the same number of entries and exits, but the dynamic behavior may be different. I wonder which one the art guys claim to be enforcing at verification time (it seems like it would have to be the static bytecode, not the dynamic properties, but then they shouldn't be failing to verify this). looking at the google code issue, the comment https://code.google.com/p/android/issues/detail?id=80823#c6 was made by the same dev as https://code.google.com/p/android/issues/detail?id=80823#c3, so I sort of suspect there is some miscommunication going on. It is not clear in what context the dev is replying in, since in the previous comment you mention splitting monitor-enter and exit across methods. I think things would be much clearer if all patches, specialized clojure android builds, etc, were gotten rid of, then with a vanilla clojure jar you get a javap dump of what fails to verify, then just take that over to the android issue tracker and ask "hey, this fails to verify, why?"

Comment by Adam Clements [ 11/Dec/14 9:15 AM ]

Yeah, I shouldn't have confused it with the patched versions. The gist and the currently uploaded version use the vanilla clojure version of the locking macro now though.

I think the issue comes from the exception table and the instructions that covers. If line 24 can throw for example, you would end up at runtime with a monitor-exit, having never encountered a monitor-enter.

Comment by Ghadi Shayban [ 21/Dec/15 12:19 PM ]

According to Marcus Lagergren JRockit and Hotspot both account for lock releases being in a different method... Perhaps Android has a different (wrong) interpretation?

Comment by Alex Miller [ 21/Dec/15 3:33 PM ]

I added a new clj-1472.patch that fixes a few minor details I didn't like about the prior patch. However, it is still essentially the same change so I have retained the original author attribution.

Comment by Ghadi Shayban [ 21/Dec/15 4:14 PM ]

alex did you upload the right patch?

Comment by Alex Miller [ 21/Dec/15 4:58 PM ]

Ah, no messed that up. Will fix.

Comment by Nicola Mometto [ 22/Dec/15 3:54 AM ]

Alex, it's probably worth making that a `^:once fn*`

Comment by Ghadi Shayban [ 22/Dec/15 1:05 PM ]

From Android:

Android doesn't run Java bytecode, it runs Dex bytecode. The dexdump output of what your class translates to is interesting.

Neither is the JVMS interesting. Android isn't a Java Virtual Machine. We follow the JLS, but not the JVMS (how could we, when we don't run Java bytecode). As such, all appeals against it are irrelevant. We try to be compatible to the spirit of the JVMS wrt/ Dex bytecode, but if your source isn't Java, there are no guarantees.

Now, the verifiers were (and probably still are) broken, even against our (pretty bad) spec, and regrettably we aren't very consistent. In Marshmallow, for example, a lot of the code we can't correctly verify wrt/ structured locking is rejected as a VerifyError, which is not in the spirit of the JVMS. In the next release, this will be relaxed, however, and postponed to an actual check while running the code.

Regrettably there isn't anything we can do about old releases, you'll have to work around any issues. I'll try to take a look at your class when I find the time.

Sounds like making a workaround in Clojure is the least of all evils.

Comment by Alex Miller [ 22/Dec/15 1:46 PM ]

Added -2 patch with ^:once.

Comment by Alex Miller [ 12/Jan/16 10:30 AM ]

Our current belief is that Android is at fault here, so backlogging this for now.





[CLJ-1372] Inconsistent hash with java collections Created: 09/Mar/14  Updated: 12/Jan/16

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

Type: Defect Priority: Minor
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: collections, interop
Environment:

1.6.0 master


Attachments: Text File 0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M-alternative.patch     Text File 0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch     Text File 0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M-substring.patch     Text File 0005-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch     Text File 0006-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch     Text File 0007-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch     File clj-1372-2.diff     File clj-1372.diff    
Patch: Code and Test
Approval: Vetted

 Description   

c.c/hash always use hashCode for java collections, which is incompatible when comparing with Clojure collections, which use Murmur3.

user=> (== (hash (java.util.ArrayList. [1 2 3])) (hash [1 2 3]))
false
user=> (= (java.util.ArrayList. [1 2 3]) [1 2 3])
true

One way to fix it is to add a special case in Util/hasheq for java.util.Collections, as it is now for Strings.

Link to a discussion of this topic in the Clojure group: https://groups.google.com/forum/#!topic/clojure/dQhdwZsyIEw



 Comments   
Comment by Jozef Wagner [ 09/Mar/14 8:02 AM ]

Same problem for maps, so hasheq should have a special case for java.util.Map too.

Comment by Jozef Wagner [ 09/Mar/14 9:21 AM ]

Added patch with fix for j.u. Map, Set and List.

Comment by Andy Fingerhut [ 09/Mar/14 6:02 PM ]

Add patch clj-1372-2.diff that is identical to Jozef Wagner's clj-1372.diff, except it also adds some new tests that fail without his changes, and pass with them.

Comment by Alex Miller [ 10/Mar/14 9:31 AM ]

I think the contract on equiv/hasheq is more narrowly scoped than this and only applies if both collections are IPersistentCollection. In other words, I don't think this is wanted or required.

Note that the Java .equals/.hashCode contract is maintained here - these collections will compare as .equals() and do have the same .hashCode().

Comment by Jozef Wagner [ 10/Mar/14 9:38 AM ]

Without the patch the following statement is not valid: "If two objects are equal with c.c/=, than their hash returned by c.c/hash is the same number". We can say that this is valid only iff both objects are 'clojure' objects, but this goes against clojures interop principles (interop is easy, fast, no surprises).

Comment by Jozef Wagner [ 10/Mar/14 9:54 AM ]

Manifestation of this bug

user=> (assoc (hash-map [1 2 3] :foo) (java.util.ArrayList. [1 2 3]) :bar)
{[1 2 3] :bar, [1 2 3] :foo}
user=> (get (hash-map [1 2 3] :foo) (java.util.ArrayList. [1 2 3]))
nil
Comment by Alex Miller [ 10/Mar/14 10:58 AM ]

I agree that would be a nice thing to say without qualification.

There is a real cost to adding more branches in hasheq - adding those collection checks affects every hasheq. Running a full Clojure build, I see the following set of classes with >100 occurences where this happens (note that exactly 0 of these are the Java collections - this case doesn't exist in the Clojure build itself):

clojure.lang.Var 107001502
java.lang.Class 2651389
java.lang.Character 2076322 
java.util.UUID 435235 
java.util.Date 430956
clojure.lang.Compiler$LocalBinding 116830
java.lang.Boolean 112361
java.util.regex.Pattern 325

We'd be adding 4 more instanceof checks in the path of every one of those hasheqs. This would also likely blow any JVM inlining.

Rich says "all bets should be off for hasheq/equiv of non-values" where Java collections obviously fall into the class of "non-values".

Comment by Andy Fingerhut [ 10/Mar/14 11:04 AM ]

Would a doc patch be considered? Say one that modified the doc of clojure.core/hash to include a phrase indicating that it is only promised to be consistent with clojure.core/= for immutable values? It could even perhaps mention that Floats are out, too: see CLJ-1036

Comment by Alex Miller [ 10/Mar/14 12:00 PM ]

I think it would be preferred to do any detailed docs about hash at http://clojure.org/data_structures rather than in the docstring. Although the docstring on hash probably could use an update and a pointer to the web site after the latest changes.

Comment by Jozef Wagner [ 10/Mar/14 12:14 PM ]

Neverthless it is a breaking change from 1.5, and it should be mentioned in changelog. What still bugs me is that c.c/= is supported in such cases but the c.c/hash is not. If supporting c.c/hash is expensive, isn't it better to drop support for c.c/= in such cases? It will eliminate surprises such as:

user=> (apply distinct? (hash-set [1 2 3] (java.util.Collections/unmodifiableList [1 2 3])))
false
Comment by Alex Miller [ 10/Mar/14 2:05 PM ]

I'm not sure it's a "breaking" change if something not considered to be guaranteed changes. But I take your point.

I don't think it's feasible to drop = support for Clojure and Java collections - that seems important and useful. And if it were free to do so, I would like to be able to say without qualification that if equiv=true, then hasheq is the same.

It's unclear to me that the examples listed on this ticket are actually real problems people are likely to encounter. The main users of hasheq are hash map and hash set. So to manifest, you would need to be putting a mixture of Clojure and Java collections into one of those, in particular a mixture of collections that compare as equal.

Still thinking about it.

Comment by Jozef Wagner [ 10/Mar/14 3:27 PM ]

Sorry for spamming but there may be another option, to not fallback into hashCode in hasheq, but to instead throw in cases where hasheq is requested for non-values. This will lead to a cleaner separation of hash types. Of course it will prevent putting non-values into hash-set.

Comment by Alex Miller [ 10/Mar/14 3:34 PM ]

There is no simple check for "valueness" though?

Comment by Andy Fingerhut [ 10/Mar/14 3:37 PM ]

An idea, for what it might be worth: Add one test for instance of java.util.Collection in Util.hasheq method instead of 3 separate tests for Set, List, and Map. It doesn't cover Map.Entry.

Comment by Alex Miller [ 10/Mar/14 3:38 PM ]

Map doesn't extend Collection either.

Comment by Stuart Halloway [ 11/Mar/14 10:44 AM ]

I think this needs more consideration and should not hold up 1.6.

Comment by Andy Fingerhut [ 20/Mar/14 2:01 PM ]

Both patches clj-1372.diff and clj-1372-2.diff fail to apply cleanly as of latest Clojure master on Mar 20 2014. They did apply cleanly before the Mar 19 2014 commit, I believe, and the only issue appears to be a changed line of diff context. Given the discussion about whether such a change is desired, it sounds like more thought is needed before deciding what change should be made, if any.

Comment by Mike Anderson [ 11/May/14 2:31 PM ]

This is a pretty bad defect. It absolutely needs to be fixed. It's not really about whether using a mix of Clojure and Java collections is a likely use case or not (it probably isn't...), it's about providing consistent guarantees that people can rely upon.

For example, now I'm really unsure about whether some of the library functions I have that use sets or maps are broken or not. I'd be particularly worried about anything that implements object caches / memoisation / interning based on hashed values. Such code may now have some really nasty subtle defects.

Since they are library functions, I can't guarantee what kind of objects are passed in so the code has to work with all possible inputs (either that or I need to write a clear docstring and throw an exception if the input is not supported).

Comment by Michał Marczyk [ 12/May/14 11:29 PM ]

This patch (0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch) makes hasheq consistent with = for java.util.{List,Map,Map.Entry,Set}. Additionally it extends the special treatment of String (return hasheq of hashCode) to all types not otherwise handled (see below for a comment on this).

It is also available here:

https://github.com/michalmarczyk/clojure/tree/alien-hasheq-2

An earlier version is available here:

https://github.com/michalmarczyk/clojure/tree/alien-hasheq

If I understand correctly, what needs to be benchmarked is primarily the "dispatch time" for clojure.lang.Util/hasheq given a Clojure type. So, I ran a Criterium benchmark repeatedly hashing the same persistent hash map, on the theory that this will measure just the dispatch time on IHashEq instances. I then ran a separate benchmark hashing a PHM, a string and a long and adding up the results with unchecked-add. Hopefully this is a good start; I've no doubt additional benchmarks would be useful.

The results are somewhat surprising to me: hasheq on PHM is actually slightly faster in this benchmark on my build than on 1.6.0; the "add three hasheqs" benchmark is slightly faster on 1.6.0.

;;; 1.6.0

;;; NB. j.u.HM benchmark irrelevant
user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (clojure.lang.Util/hasheq phm)) (c/bench (clojure.lang.Util/hasheq juhm)))
WARNING: Final GC required 1.24405836928592 % of runtime
Evaluation count : 5549560980 in 60 samples of 92492683 calls.
             Execution time mean : 9.229881 ns
    Execution time std-deviation : 0.156716 ns
   Execution time lower quantile : 8.985994 ns ( 2.5%)
   Execution time upper quantile : 9.574039 ns (97.5%)
                   Overhead used : 1.741068 ns

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 6.2652 % Variance is slightly inflated by outliers
Evaluation count : 35647680 in 60 samples of 594128 calls.
             Execution time mean : 1.695145 µs
    Execution time std-deviation : 20.186554 ns
   Execution time lower quantile : 1.670049 µs ( 2.5%)
   Execution time upper quantile : 1.740329 µs (97.5%)
                   Overhead used : 1.741068 ns

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

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (unchecked-add (clojure.lang.Util/hasheq phm) (unchecked-add (clojure.lang.Util/hasheq "foo") (clojure.lang.Util/hasheq 123)))))
WARNING: Final GC required 1.028614538339401 % of runtime
Evaluation count : 1029948300 in 60 samples of 17165805 calls.
             Execution time mean : 56.797488 ns
    Execution time std-deviation : 0.732221 ns
   Execution time lower quantile : 55.856731 ns ( 2.5%)
   Execution time upper quantile : 58.469940 ns (97.5%)
                   Overhead used : 1.836671 ns

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

;;; patch applied

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] (assert (= (hash phm) (hash juhm))) (c/bench (clojure.lang.Util/hasheq phm)) (c/bench (clojure.lang.Util/hasheq juhm)))
Evaluation count : 5537698680 in 60 samples of 92294978 calls.
             Execution time mean : 8.973200 ns
    Execution time std-deviation : 0.157079 ns
   Execution time lower quantile : 8.733544 ns ( 2.5%)
   Execution time upper quantile : 9.289350 ns (97.5%)
                   Overhead used : 1.744772 ns
Evaluation count : 2481600 in 60 samples of 41360 calls.
             Execution time mean : 24.287800 µs
    Execution time std-deviation : 288.124326 ns
   Execution time lower quantile : 23.856445 µs ( 2.5%)
   Execution time upper quantile : 24.774097 µs (97.5%)
                   Overhead used : 1.744772 ns
nil

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (unchecked-add (clojure.lang.Util/hasheq phm) (unchecked-add (clojure.lang.Util/hasheq "foo") (clojure.lang.Util/hasheq 123)))))
WARNING: Final GC required 1.298136122909759 % of runtime
Evaluation count : 954751500 in 60 samples of 15912525 calls.
             Execution time mean : 61.681794 ns
    Execution time std-deviation : 0.712110 ns
   Execution time lower quantile : 60.622003 ns ( 2.5%)
   Execution time upper quantile : 62.904801 ns (97.5%)
                   Overhead used : 1.744772 ns

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

As a side note, the earlier version of the patch available on the other branch doesn't have a separate branch for String. This made hasheq faster for objects implementing IHashEq, but slowed down the "three hashes" benchmark roughly by a factor of 2.

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

Just for clarity, please refer to patches attached here by name so as time goes on we don't have to correlate attachment time with comment time.

I'm not particularly worried about the cost of things that implement IHashEq as they should be unaffected other than potential inlining issues. I am curious about the cost of hasheq for objects that fall through to the end of the cases and pay the cost for all of the checks. The list farther up in the comments is a good place to start - things like Class, Character, and Var (which could possibly be addressed in Var).

Comment by Michał Marczyk [ 12/May/14 11:47 PM ]

Good point, I've edited the above comment to include the patch name.

Thanks for the benchmarking suggestions – I'll post some new results in ~6 minutes.

Comment by Michał Marczyk [ 13/May/14 12:18 AM ]

First, for completeness, here's a new patch (0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M-alternative.patch) which doesn't do the extra murmuring for types not otherwise handled. It's slower for the single PHM case; see below for details. Also, here's the branch on GitHub:

https://github.com/michalmarczyk/clojure/tree/alien-hasheq-3

As for the new results, the perf hit is quite large, I'm afraid:

;;; with patch (murmur hashCode for default version)
user=> (let [class-instance java.lang.String character-instance \a var-instance #'hash] (c/bench (clojure.lang.Util/hasheq class-instance)) (c/bench (clojure.lang.Util/hasheq character-instance)) (c/bench (clojure.lang.Util/hasheq var-instance)))
WARNING: Final GC required 1.409118084170768 % of runtime
Evaluation count : 655363680 in 60 samples of 10922728 calls.
             Execution time mean : 96.459888 ns
    Execution time std-deviation : 1.019817 ns
   Execution time lower quantile : 95.079086 ns ( 2.5%)
   Execution time upper quantile : 98.684168 ns (97.5%)
                   Overhead used : 1.708347 ns
Evaluation count : 675919140 in 60 samples of 11265319 calls.
             Execution time mean : 88.965959 ns
    Execution time std-deviation : 0.825226 ns
   Execution time lower quantile : 87.817159 ns ( 2.5%)
   Execution time upper quantile : 90.755688 ns (97.5%)
                   Overhead used : 1.708347 ns
Evaluation count : 574987680 in 60 samples of 9583128 calls.
             Execution time mean : 103.881498 ns
    Execution time std-deviation : 1.103615 ns
   Execution time lower quantile : 102.257474 ns ( 2.5%)
   Execution time upper quantile : 106.071144 ns (97.5%)
                   Overhead used : 1.708347 ns

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

;;; 1.6.0
user=> (let [class-instance java.lang.String character-instance \a var-instance #'hash] (c/bench (clojure.lang.Util/hasheq class-instance)) (c/bench (clojure.lang.Util/hasheq character-instance)) (c/bench (clojure.lang.Util/hasheq var-instance)))
WARNING: Final GC required 1.3353133083866688 % of runtime
Evaluation count : 1829305260 in 60 samples of 30488421 calls.
             Execution time mean : 34.205701 ns
    Execution time std-deviation : 0.379106 ns
   Execution time lower quantile : 33.680636 ns ( 2.5%)
   Execution time upper quantile : 34.990138 ns (97.5%)
                   Overhead used : 1.718257 ns

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 1 (1.6667 %)
	low-mild	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
Evaluation count : 1858100340 in 60 samples of 30968339 calls.
             Execution time mean : 30.401309 ns
    Execution time std-deviation : 0.213878 ns
   Execution time lower quantile : 30.095976 ns ( 2.5%)
   Execution time upper quantile : 30.871497 ns (97.5%)
                   Overhead used : 1.718257 ns
Evaluation count : 1592932200 in 60 samples of 26548870 calls.
             Execution time mean : 36.292934 ns
    Execution time std-deviation : 0.333512 ns
   Execution time lower quantile : 35.795063 ns ( 2.5%)
   Execution time upper quantile : 36.918183 ns (97.5%)
                   Overhead used : 1.718257 ns

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

One PHM and Class/Character/Var results with the new patch (no extra murmur step in the default case):

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (unchecked-add (clojure.lang.Util/hasheq phm) (unchecked-add (clojure.lang.Util/hasheq "foo") (clojure.lang.Util/hasheq 123)))))
WARNING: Final GC required 1.258952964663877 % of runtime
Evaluation count : 1007768460 in 60 samples of 16796141 calls.
             Execution time mean : 58.195608 ns
    Execution time std-deviation : 0.482804 ns
   Execution time lower quantile : 57.655857 ns ( 2.5%)
   Execution time upper quantile : 59.154655 ns (97.5%)
                   Overhead used : 1.567532 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil
user=> (let [class-instance java.lang.String character-instance \a var-instance #'hash] (c/bench (clojure.lang.Util/hasheq class-instance)) (c/bench (clojure.lang.Util/hasheq character-instance)) (c/bench (clojure.lang.Util/hasheq var-instance)))
Evaluation count : 647944080 in 60 samples of 10799068 calls.
             Execution time mean : 91.275863 ns
    Execution time std-deviation : 0.659943 ns
   Execution time lower quantile : 90.330980 ns ( 2.5%)
   Execution time upper quantile : 92.711120 ns (97.5%)
                   Overhead used : 1.567532 ns
Evaluation count : 699506160 in 60 samples of 11658436 calls.
             Execution time mean : 84.564131 ns
    Execution time std-deviation : 0.517071 ns
   Execution time lower quantile : 83.765607 ns ( 2.5%)
   Execution time upper quantile : 85.569206 ns (97.5%)
                   Overhead used : 1.567532 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
Evaluation count : 594919980 in 60 samples of 9915333 calls.
             Execution time mean : 100.336792 ns
    Execution time std-deviation : 0.811312 ns
   Execution time lower quantile : 99.313490 ns ( 2.5%)
   Execution time upper quantile : 102.167675 ns (97.5%)
                   Overhead used : 1.567532 ns

Found 3 outliers in 60 samples (5.0000 %)
	low-severe	 3 (5.0000 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil
Comment by Michał Marczyk [ 13/May/14 1:05 AM ]

Here's a new patch (0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M-substring.patch) that takes the outrageous approach of replacing the Iterable/Map/Entry test with a .startsWith("java.util.") on the class name. (I experimented with .getClass().getPackage(), but the performance of that was terrible.) The branch is here:

https://github.com/michalmarczyk/clojure/tree/alien-hasheq-4

Hash perf on the "fall-through" cases with this patch seems to be very good:

user=> (let [class-instance java.lang.String character-instance \a var-instance #'hash] (c/bench (clojure.lang.Util/hasheq class-instance)) (c/bench (clojure.lang.Util/hasheq character-instance)) (c/bench (clojure.lang.Util/hasheq var-instance)))
WARNING: Final GC required 1.31690036780011 % of runtime
Evaluation count : 1661453640 in 60 samples of 27690894 calls.
             Execution time mean : 35.099750 ns
    Execution time std-deviation : 0.422800 ns
   Execution time lower quantile : 34.454839 ns ( 2.5%)
   Execution time upper quantile : 35.953584 ns (97.5%)
                   Overhead used : 1.556642 ns
Evaluation count : 1630167600 in 60 samples of 27169460 calls.
             Execution time mean : 35.487409 ns
    Execution time std-deviation : 0.309872 ns
   Execution time lower quantile : 35.083030 ns ( 2.5%)
   Execution time upper quantile : 36.190015 ns (97.5%)
                   Overhead used : 1.556642 ns

Found 4 outliers in 60 samples (6.6667 %)
	low-severe	 3 (5.0000 %)
	low-mild	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
Evaluation count : 1440434700 in 60 samples of 24007245 calls.
             Execution time mean : 40.894457 ns
    Execution time std-deviation : 0.529510 ns
   Execution time lower quantile : 40.055991 ns ( 2.5%)
   Execution time upper quantile : 41.990985 ns (97.5%)
                   Overhead used : 1.556642 ns
nil
Comment by Michał Marczyk [ 13/May/14 1:28 AM ]

The new patch (...-substring.patch) returns hashCode for java.util.** classes other than List, Map, Map.Entry and Set, of course, so no behaviour change there.

Here are the benchmarks for repeated PHM lookups (slightly slower than 1.6.0 apparently, though within 1 ns) and the "add three hasheqs" benchmark (66 ns with patch vs. 57 ns without):

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] (assert (= (hash phm) (hash juhm))) (c/bench (clojure.lang.Util/hasheq phm)) (c/bench (clojure.lang.Util/hasheq juhm)))
Evaluation count : 5183841240 in 60 samples of 86397354 calls.
             Execution time mean : 10.076893 ns
    Execution time std-deviation : 0.182592 ns
   Execution time lower quantile : 9.838456 ns ( 2.5%)
   Execution time upper quantile : 10.481086 ns (97.5%)
                   Overhead used : 1.565749 ns
Evaluation count : 3090420 in 60 samples of 51507 calls.
             Execution time mean : 19.596627 µs
    Execution time std-deviation : 224.380257 ns
   Execution time lower quantile : 19.288347 µs ( 2.5%)
   Execution time upper quantile : 20.085620 µs (97.5%)
                   Overhead used : 1.565749 ns
nil

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (unchecked-add (clojure.lang.Util/hasheq phm) (unchecked-add (clojure.lang.Util/hasheq "foo") (clojure.lang.Util/hasheq 123)))))
WARNING: Final GC required 1.418253438197936 % of runtime
Evaluation count : 879210900 in 60 samples of 14653515 calls.
             Execution time mean : 66.939309 ns
    Execution time std-deviation : 0.747984 ns
   Execution time lower quantile : 65.667310 ns ( 2.5%)
   Execution time upper quantile : 68.155046 ns (97.5%)
                   Overhead used : 1.724002 ns
nil

It is important to note that I have obtained the no-patch result for the "three hasheqs" benchmarks on a fresh JVM when benchmarking 1.6.0, so that's also how I repeated the benchmark with the patch applied. Hashing many different types changes the results noticeably – presumably HotSpot backs off from some optimizations after seeing several different types passed in to hasheq?

Comment by Michał Marczyk [ 13/May/14 8:04 AM ]

Here's a new patch (0005-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch) that introduces a new isAlien static method that checks for instanceof Map/Map.Entry/Iterable and uses this method to test for "alien collection".

Initial benchmarking results are promising:

;;; "fall-through" benchmark
user=> (let [class-instance java.lang.String character-instance \a var-instance #'hash] (c/bench (clojure.lang.Util/hasheq class-instance)) (c/bench (clojure.lang.Util/hasheq character-instance)) (c/bench (clojure.lang.Util/hasheq var-instance)))
WARNING: Final GC required 1.258979068087473 % of runtime
Evaluation count : 1598432100 in 60 samples of 26640535 calls.
             Execution time mean : 36.358882 ns
    Execution time std-deviation : 0.566925 ns
   Execution time lower quantile : 35.718889 ns ( 2.5%)
   Execution time upper quantile : 37.414722 ns (97.5%)
                   Overhead used : 1.823120 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
Evaluation count : 1626362460 in 60 samples of 27106041 calls.
             Execution time mean : 35.426993 ns
    Execution time std-deviation : 0.294517 ns
   Execution time lower quantile : 35.047064 ns ( 2.5%)
   Execution time upper quantile : 36.058667 ns (97.5%)
                   Overhead used : 1.823120 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
Evaluation count : 1461423180 in 60 samples of 24357053 calls.
             Execution time mean : 39.541873 ns
    Execution time std-deviation : 0.423707 ns
   Execution time lower quantile : 38.943560 ns ( 2.5%)
   Execution time upper quantile : 40.499433 ns (97.5%)
                   Overhead used : 1.823120 ns

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

;;; "three hasheqs" benchmark
user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (unchecked-add (clojure.lang.Util/hasheq phm) (unchecked-add (clojure.lang.Util/hasheq "foo") (clojure.lang.Util/hasheq 123)))))
WARNING: Final GC required 1.5536755331464491 % of runtime
Evaluation count : 820376460 in 60 samples of 13672941 calls.
             Execution time mean : 71.999365 ns
    Execution time std-deviation : 0.746588 ns
   Execution time lower quantile : 70.869739 ns ( 2.5%)
   Execution time upper quantile : 73.565908 ns (97.5%)
                   Overhead used : 1.738155 ns

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil
Comment by Michał Marczyk [ 13/May/14 8:28 AM ]

Ah, I left out the repeated phm hasheq lookup + hasheq of a java.util.HashMap instance pair of benchmarks from the above – here it is for completeness (no surprises though):

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] (assert (= (hash phm) (hash juhm))) (c/bench (clojure.lang.Util/hasheq phm)) (c/bench (clojure.lang.Util/hasheq juhm)))
WARNING: Final GC required 1.260853406580491 % of runtime
Evaluation count : 5369135760 in 60 samples of 89485596 calls.
             Execution time mean : 10.380464 ns
    Execution time std-deviation : 3.407284 ns
   Execution time lower quantile : 9.510624 ns ( 2.5%)
   Execution time upper quantile : 11.461485 ns (97.5%)
                   Overhead used : 1.566301 ns

Found 5 outliers in 60 samples (8.3333 %)
	low-severe	 3 (5.0000 %)
	low-mild	 2 (3.3333 %)
 Variance from outliers : 96.4408 % Variance is severely inflated by outliers
Evaluation count : 3078180 in 60 samples of 51303 calls.
             Execution time mean : 19.717981 µs
    Execution time std-deviation : 209.896848 ns
   Execution time lower quantile : 19.401811 µs ( 2.5%)
   Execution time upper quantile : 20.180163 µs (97.5%)
                   Overhead used : 1.566301 ns

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil
Comment by Alex Miller [ 13/May/14 9:17 AM ]

Please don't submit any patches that change hashcode for anything other than making Java collections match Clojure collections - any other change is out of scope of this ticket.

In general, I would prefer just the execution time mean report for the moment rather than everything - the full criterium output makes these comments much harder to read and compare.

Comment by Alex Miller [ 13/May/14 9:33 AM ]

Could I get a summary of approaches, and a timing of 1.6.0 vs each patch for a consistent set of tests - say time of hash for Long, PHM, juHM, Class, and the "three hasheqs" test?

Comment by Rich Hickey [ 13/May/14 9:47 AM ]

"Hashing many different types changes the results noticeably – presumably HotSpot backs off from some optimizations after seeing several different types passed in to hasheq?"

Right - if your benchmarks do not treat this site as megamorphic you will get all sorts of distorted results.

Comment by Michał Marczyk [ 14/May/14 3:15 AM ]

Ok, I have what I think is an improved microbenchmark for this: xor of hasheqs for a long, a double, a string, a class, a character and a PHM (single instance, so it'll be a hash lookup). The results are not very encouraging.

Single form including the require to make it convenient to run; also bundled is a j.u.HashMap (128 entries) hasheq benchmark:

(do
  (require '[criterium.core :as c])
  (let [l    41235125123
        d    123.456
        s    "asdf;lkjh"
        k    BigInteger
        c    \S
        phm  (apply hash-map (interleave (range 128) (range 128)))
        juhm (java.util.HashMap. phm)
        f    (fn f []
               (-> (clojure.lang.Util/hasheq l)
                   (bit-xor (clojure.lang.Util/hasheq d))
                   (bit-xor (clojure.lang.Util/hasheq s))
                   (bit-xor (clojure.lang.Util/hasheq k))
                   (bit-xor (clojure.lang.Util/hasheq c))
                   (bit-xor (clojure.lang.Util/hasheq phm))))]
    (c/bench (f))
    (c/bench (hash juhm))))

Mean execution time as reported by Criterium:

version xor (ns) j.u.HM (µs)
unpatched 1.6.0 148.128748 1.701640
0005 patch 272.039667 21.201178
original patch 268.670316 21.169436
-alternative patch 271.747043 20.755397

The substring patch is broken (see below), so I skipped it. The patch I'm describing as the "original" one is attached as 0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch.

Decisions common to all the patches:

1. One extra if statement in hasheq just above the default return with a three-way instanceof check.

2. The types tested for are j.u.Iterable, j.u.Map.Entry and j.u.Map.

3. Murmur3.hashOrdered takes Iterable, so that's why it's on the list. Map does not extend Iterable, so it's listed separately. Map.Entry is on the list, because ultimately the way to hash maps is to iterate over and hash their entries.

4. The actual hashing of the "alien" / host types is done by a separate static method – clojure.lang.Util.doalienhasheq – on the theory that this will permit hasheq to be inlined more aggressively and limit the worst of the perf hit to alien collections.

5. doalienhasheq checks for Map, Map.Entry, Set and List; entries are converted to lists for hashing, maps are hashed through entry sets and lists and sets are passed directly to Murmur3.

6. There is also a default case for other Iterable types – we must return hashCode or the result of composing some other function with hashCode for these, since we use equals to test them for equivalence.

The 0005 patch has hasheq call a separate private static method to perform the three-way type check, whereas the others put the check directly in the actual if test. The -alternative patch and the 0005 patch return hashCode in the default case, whereas the original patch composes Murmur3.hashInt with hashCode.

The substring patch only works for java.util.** classes and so doesn't solve the problem (it wouldn't correctly hash Guava collections, for example).

All of the patches change c.l.Util.hasheq and add one or two new static methods to clojure.lang.Util that act as helpers for hasheq. None of them changes anything else. Murmuring hashCode was a performance experiment that appeared to have a slight positive impact on some of the "fast cases" (in fact it's still the best performer among the current three patches in the microbenchmark presented above, although the margin of victory is of course extremely tiny). Thus I think all the current patches are in fact limited in scope to changes directly relevant to the ticket; the -alternative patch and the 0005 patch certainly are.

Comment by Michał Marczyk [ 14/May/14 3:29 AM ]

For completeness, branching on Map, Set etc. directly in hasheq, as with Jozef's original patch, results in the following timings in the microbenchmark introduced in my previous comment:

xor 315.866626 ns
juhm 18.520133 µs
Comment by Michał Marczyk [ 14/May/14 4:01 AM ]

New patch (0006) that leaves out the Map.Entry check; instead, two methods are introduced in the Murmur3 class to handle j.u.maps.

Java map entries aren't really integrated into Clojure – you can't use them like vectors, can't call seq on them etc. – so I don't think they need to match Clojure map entries in hasheq as long as j.u.maps do.

Timings:

xor 233.341689 ns
juhm 9.104637 µs
Comment by Michał Marczyk [ 14/May/14 4:17 AM ]

Checking for Map/Iterable in-line doesn't seem to affect xor benchmark results very much, but makes juhm hashing quicker. This is rather surprising to me. In any case, here's a new patch (0007) and the timings:

xor 233.062337 ns
juhm 8.629149 µs
Comment by Alex Miller [ 14/May/14 7:17 AM ]

What are equivalent timings without the patch?

Comment by Michał Marczyk [ 14/May/14 7:43 AM ]

They're listed in the table in the comment introducing the benchmark – 148.128748 ns for xor, 1.701640 µs for juhm.

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

What if we override hasheq for different types instead of using instanceof?

Comment by Michał Marczyk [ 14/May/14 12:50 PM ]

Overloaded methods are resolved statically, so there's no avoiding testing for type in the Object overload.

A more specific overload could be used to speed up hashing for its parameter type given a type hint or for literals, since the compiler would emit calls to that overload given appropriate compile-time information. There wouldn't be any speed-up in "implicit" hashing during hash map / set ops, however.

Comment by Daniel Compton [ 12/Nov/14 9:19 PM ]

This hit me when upgrading Factual/skuld from 1.5.1 to 1.6. clojure.data.fressian serialises c.l.PersistentHashSet sets into java.util.HashSet. This breaks equality checking in https://github.com/Factual/skuld/blob/b720feb142e6d274e85be208dc1d6d8634801719/test/skuld/net_test.clj#L8-L29 as we are comparing a set of maps where the original set contains a PersistentSet and the serialised and deserialised one contains a HashSet.

Comment by Daniel Compton [ 12/Nov/14 11:54 PM ]

This has come up again for me, details are in http://dev.clojure.org/jira/browse/DFRS-7

Comment by Max Penet [ 16/Mar/15 11:22 AM ]

This bit me again today as well (wasted a lot of time before figuring this one out). Any chance we'll have a patch for 1.7?

Comment by Alex Miller [ 16/Mar/15 12:18 PM ]

afaik, we are still in search of an approach with tolerable performance impacts before this can be considered.

Comment by Alex Miller [ 12/Jan/16 10:01 AM ]

While we are not opposed to addressing this ticket, we are not interested in addressing it at the expense of performance in comparisons that we care about more, and none of the current proposals meets that bar. For now, I am moving this to Backlog but would pull it out if a solution presents itself.





[CLJ-992] `iterate` reducer Created: 10/May/12  Updated: 09/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reducers

Attachments: Text File 0001-Add-reducers-iterate.patch     Text File iterate-reducer.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Added a reducer implementation mirroring clojure.core/iterate.

Patch: iterate-reducer.patch

Screened by:



 Comments   
Comment by Alan Malloy [ 10/May/12 9:50 PM ]

Should I have made this implement Seqable as well? It wasn't clear to me, because as far as I could see this was the only function in clojure.core.reducers that's generating a brand-new sequence rather than transforming an existing one.

Comment by Alan Malloy [ 10/May/12 10:24 PM ]

Previous version neglected to include the seed value of the iteration in the reduce.

Comment by Jason Jackson [ 11/May/12 11:23 AM ]

Currying iterate seems useless, albeit not harmful.

While implementing repeat, I couldn't use currying. Because 1-arity is already reserved for infinite repeat ([n x] and [x], not [n x] and [n] if currying)

How about we just support currying for functions where last param is reducible?

Comment by Alan Malloy [ 18/Aug/12 7:16 PM ]

This new patch replaces the previous patch. As requested, I am splitting up the large issue CLJ-993 into smaller tickets.

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 CLJ-1046, and before CLJ-993.

Comment by Ghadi Shayban [ 09/Jan/16 5:34 PM ]

iterate is now reducible





[CLJ-274] cannot close over mutable fields (in deftype) Created: 23/Feb/10  Updated: 12/Sep/15

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

Type: Defect Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: deftype

Approval: Vetted

 Description   

Simplest case:

user=>
(deftype Bench [#^{:unsynchronized-mutable true} val]
  Runnable
  (run [_]
    (fn [] (set! val 5))))

java.lang.IllegalArgumentException: Cannot assign to non-mutable: val (NO_SOURCE_FILE:5)

Functions should be able to mutate mutable fields in their surrounding deftype (just like inner classes do in Java).

Filed as bug, because the loop special form expands into a fn form sometimes:

user=>
(deftype Bench [#^{:unsynchronized-mutable true} val]
  Runnable
  (run [_]
    (let [x (loop [] (set! val 5))])))
java.lang.IllegalArgumentException: Cannot assign to non-mutable: val (NO_SOURCE_FILE:9)


 Comments   
Comment by Assembla Importer [ 01/Oct/10 9:35 AM ]

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

Comment by Assembla Importer [ 01/Oct/10 9:35 AM ]

donmullen said: Updated each run to [_] for new syntax.

Now gives exception listed.

Comment by Assembla Importer [ 01/Oct/10 9:35 AM ]

richhickey said: We're not going to allow closing over mutable fields. Instead we'll have to generate something other than fn for loops et al used as expressions. Not going to come before cinc

Comment by Nicola Mometto [ 30/Jan/15 7:37 AM ]

The patch for CLJ-1226 makes this work:

(deftype Bench [#^{:unsynchronized-mutable true} val]
Runnable
(run [_]
(let [x (loop [] (set! (.val _) 5))])))

If there's interest, I could provide a patch that converts closed over mutable field access by generated fns (for loop/try) into field access on closed over "this", i.e. val -> (.val this)

Comment by Nicola Mometto [ 30/Jan/15 7:39 AM ]

Related tickets: CLJ-1075 CLJ-1023

Comment by Nicola Mometto [ 12/Sep/15 5:35 AM ]

CLJ-701 could probably make the loop case working





[CLJ-1130] When unable to match a static method, report arity caller was looking for, avoid misleading field error Created: 17/Dec/12  Updated: 21/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs, ft

Attachments: Text File clj-1130-v1.txt     File clj-1130-v2.diff     File clj-1130-v2-ignore-ws.diff     Text File clj-1130-v2.txt     File clj-1130-v3.diff     File clj-1130-v4.diff     File clj-1130-v5.diff    
Patch: Code and Test
Approval: Vetted

 Description   

Incorrectly invoking a static method with 0 parameters yields a NoSuchFieldException:

user=> (Long/parseLong)
CompilerException java.lang.NoSuchFieldException: parseLong, compiling:(NO_SOURCE_PATH:1:1) 
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: No matching method: parseLong, compiling:(NO_SOURCE_PATH:2:1)

Approach: Error reporting enhanced to report desired arg count and to avoid a field exception confusion if 0 arg method.

user=> (Long/parseLong)
CompilerException java.lang.IllegalArgumentException: No matching method parseLong found taking 0 args for class java.lang.Long, compiling:(NO_SOURCE_PATH:1:1)
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: No matching method parseLong found taking 3 args for class java.lang.Long, compiling:(NO_SOURCE_PATH:2:1)

Patch: clj-1130-v5.diff

Screened by: Alex Miller



 Comments   
Comment by Michael Drogalis [ 06/Jan/13 6:44 PM ]

It looks like it's first trying to resolve a field by name, since field access with / is legal. For example:

user=> (Integer/parseInt)
CompilerException java.lang.NoSuchFieldException: parseInt, compiling:(NO_SOURCE_PATH:1)

user=> (Integer/MAX_VALUE)
2147483647

Would trying to resolve a method before a field fix this?

Comment by Alex Miller [ 03/Sep/13 10:10 AM ]

Similarities to CLJ-1248 (there a warning, here an error).

Comment by Andy Fingerhut [ 09/Sep/13 12:36 AM ]

Patch clj-1130-v1.txt changes the error message in a situation when one attempts to invoke a static method with no args, and there is no such 0-arg static method. The message now says that there is no such method with that name and 0 args, rather than that there is no such static field with that name.

Comment by Alex Miller [ 04/Oct/13 3:56 PM ]

I updated the patch to simplify it a bit but more importantly to remove the check by exception and instead use the Reflector method that can answer this question.

Comment by Andy Fingerhut [ 12/Oct/13 3:11 PM ]

Alex, thank you for the improvements to the code. It looks better to me.

Comment by Rich Hickey [ 25/Oct/13 7:30 AM ]

due to indentation changes, this patch appears to touch much more than it probably does, making it difficult to approve.

Comment by Andy Fingerhut [ 25/Oct/13 10:59 AM ]

Any suggestions on what can be done to make progress here? Would it help to attach a patch made with "-w" option to ignore lines that differ only in whitespace? Provide git diff command line options that do this, after the patch is applied to your local workspace? Make a patch that leaves the indentation 'incorrect' after the change (involuntary shudder)?

Comment by Alex Miller [ 25/Oct/13 11:17 AM ]

The indentation has intentionally changed because the if/else structure has changed. I don't think making the patch incorrect to reduce changes is a good idea.

Comment by Andy Fingerhut [ 25/Oct/13 11:32 AM ]

Well, the 'incorrect' was in quotes because I was asking about a proposed patch that had the correct logic, but misleading indentation. Agreed it isn't a good idea, hence the shudder. I'm just brainstorming ideas to make the patch less difficult to approve.

Comment by Howard Lewis Ship [ 25/Oct/13 11:43 AM ]

At some point, you may need to bite the bullet and reformat some of the Clojure code .... Compiler.java had a crazy mix of tabs, spaces, and just completely wrong stuff.

Comment by Alex Miller [ 03/Nov/13 10:47 PM ]

Re-marking screened. Not sure what else to do.

Comment by Andy Fingerhut [ 04/Nov/13 8:35 AM ]

clj-1130-v2-ignore-ws.diff is identical to clj-1130-v2.diff, except it was produced with a command that ignores differences in a line due only to whitespace, i.e.: 'git format-patch master --stdout -w > clj-1130-v2-ignore-ws.diff'

It is not intended as the patch to be applied. It is only intended to make it easier to see that many of the lines in clj-1130-v2.diff are truly only differences in indentation.

Comment by Alex Miller [ 04/Nov/13 8:55 AM ]

Thanks Andy...

Comment by Rich Hickey [ 22/Nov/13 7:59 AM ]

This patch ignores the fact that method is checked for first above:

if(c != null)
  maybeField = Reflector.getMethods(c, 0, munge(sym.name), true).size() == 0;

Which is why the field code is unconditional. I'm fine with making errors better, but changing logic as well deserves more scrutiny.

Comment by Alex Miller [ 06/Dec/13 9:01 PM ]

This patch is intentionally trying to avoid calling StaticFieldExpr in the field code as that is where the (Long/parseLong) case (erroneously calling an n-arity static method with 0 args) will throw a field-oriented exception instead of a method-oriented exception. By adding the extra check here, this case falls through into the method case and throws later on calling StaticMethodExpr instead.

The early check is a check for methods of the specified arity. The later check is for the existence of a field of matching name. Combined, they lead to a better error message.

However, another alternative is to set maybeField in the first check based on field existence, not on invocation arity. That just improves the maybeField informaiton and the existing code then naturally throws the correct exception (and the patch is much simpler).

The similar case for calling n-arity instance methods with 0-arity has the same problem for the same reason:

user=> (.setTime (java.util.Date.))
IllegalArgumentException No matching field found: setTime for class java.util.Date  clojure.lang.Reflector.getInstanceField (Reflector.java:271)

Thus we can also adjust the other call that sets maybeField (which now is much less maybe).

I will attach a patch that covers these cases and update the ticket for someone to screen.

Comment by Stuart Sierra [ 08/Dec/13 12:24 PM ]

Screened. The patch clj-1130-v3.diff works as advertised.

This patch only improves error messages for cases when the type of the
target object is known to the compiler. In reflective calls, the error
messages are still the same.

Example, after this patch, given these definitions:

(def v 42)
(defn untagged-f [] 42)
(defn ^Long tagged-f [] 42)

The following expressions produce new error messages:

(.foo v 1)
;; IllegalArgumentException No matching method found: foo taking 1 args
;; for class java.lang.Long clojure.lang.Reflector.invokeMatchingMethod
;; (Reflector.java:53)

(.foo (tagged-f))
;; IllegalArgumentException No matching method found: foo taking 0 args
;; for class java.lang.Long clojure.lang.Reflector.invokeMatchingMethod
;; (Reflector.java:53)

These expressions still use the old error messages:

(.foo v)
;; IllegalArgumentException No matching field found: foo for class
;; java.lang.Long clojure.lang.Reflector.getInstanceField
;; (Reflector.java:271)

(.foo (untagged-f))
;; IllegalArgumentException No matching field found: foo for class
;; java.lang.Long clojure.lang.Reflector.getInstanceField
;; (Reflector.java:271)
Comment by Rich Hickey [ 03/Jan/14 8:41 AM ]

Changing the logic to get a different error message is something that needs to be done with great care. This now seems to prefer fields over methods, changing the semantics.

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

v4 patch simply enhances error messaages

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

clj-1130-v4.diff has the same patch repeated twice in the file. clj-1130-v5.diff is identical, except deleting the redundant copy.





[CLJ-1005] Use transient map in zipmap Created: 30/May/12  Updated: 19/Jul/15

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: 14
Labels: ft, 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   

#'zipmap constructs a map without transients, where transients could improve performance.

Approach: Use a transient map internally, along with iterators for the keys and values. A persistent map is returned as before. The definition is also moved so that it resides below that of #'transient.

Performance:

(def xs (range 16384))
(def ys (range 16))

expression 1.7.0-beta3 +patch  
(zipmap xs xs) 4.50 ms 2.12 ms large map
(zipmap ys ys) 2.75 us 2.07 us small map

Patch: CLJ-1005-zipmap-iterators.patch

Screened by: Alex Miller



 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-1551] Consider transducer support for primitives Created: 07/Oct/14  Updated: 12/Jul/15

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: transducers

Approval: Vetted

 Description   

Need to consider how we can support primitives for transducers. In particular it may be that IFn needs overloading for L/D in addition to O.






[CLJ-415] smarter assert (prints locals) Created: 29/Jul/10  Updated: 06/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs

Attachments: Text File clj-415-assert-prints-locals-v1.txt     Text File CLJ-415-v2.patch    
Patch: Code
Approval: Vetted
Waiting On: Rich Hickey

 Description   

Here is an implementation you can paste into a repl. Feedback wanted:

(defn ^{:private true} local-bindings
  "Produces a map of the names of local bindings to their values."
  [env]
  (let [symbols (map key env)]
    (zipmap (map (fn [sym] `(quote ~sym)) symbols) symbols)))

(defmacro assert
  "Evaluates expr and throws an exception if it does not evaluate to
 logical true."
  {:added "1.0"}
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep# (System/getProperty "line.separator")]
           (throw (AssertionError. (apply str "Assert failed: " (pr-str '~x) sep#
                                          (map (fn [[k# v#]] (str "\t" k# " : " v# sep#)) ~bindings)))))))))


 Comments   
Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

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

Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

alexdmiller said: A simple example I tried for illustration:

user=> (let [a 1 b 2] (assert (= a b)))
#<CompilerException java.lang.AssertionError: Assert failed: (= a b)
 a : 1
 b : 2
Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

fogus said: Of course it's weird if you do something like:

(let [x 1 y 2 z 3 a 1 b 2 c 3] (assert (= x y)))
java.lang.AssertionError: Assert failed: (= x y)
 x : 1
 y : 2
 z : 3
 a : 1
 b : 2
 c : 3
 (NO_SOURCE_FILE:0)
</code></pre>

So maybe it could be slightly changed to:
<pre><code>(defmacro assert
  "Evaluates expr and throws an exception if it does not evaluate to logical true."
  {:added "1.0"}
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep#  (System/getProperty "line.separator")
               form# '~x]
           (throw (AssertionError. (apply str "Assert failed: " (pr-str form#) sep#
                                          (map (fn [[k# v#]] 
                                                 (when (some #{k#} form#) 
                                                   (str "\t" k# " : " v# sep#))) 
                                               ~bindings)))))))))
</code></pre>

So that. now it's just:
<pre><code>(let [x 1 y 2 z 3 a 1 b 2 c 3] (assert (= x y)))
java.lang.AssertionError: Assert failed: (= x y)
 x : 1
 y : 2
 (NO_SOURCE_FILE:0)

:f

Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

fogus said: Hmmm, but that fails entirely for: (let [x 1 y 2 z 3 a 1 b 2 c 3] (assert (= [x y] [a c]))). So maybe it's better just to print all of the locals unless you really want to get complicated.
:f

Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

jawolfe said: See also some comments in:

http://groups.google.com/group/clojure-dev/browse_frm/thread/68d49cd7eb4a4899/9afc6be4d3f8ae27?lnk=gst&q=assert#9afc6be4d3f8ae27

Plus one more suggestion to add to the mix: in addition to / instead of printing the locals, how about saving them somewhere. For example, the var assert-bindings could be bound to the map of locals. This way you don't run afoul of infinite/very large sequences, and allow the user to do more detailed interrogation of the bad values (especially useful when some of the locals print opaquely).

Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

stuart.sierra said: Another approach, which I wil willingly donate:
http://github.com/stuartsierra/lazytest/blob/master/src/main/clojure/lazytest/expect.clj

Comment by Jeff Weiss [ 15/Dec/10 1:33 PM ]

There's one more tweak to fogus's last comment, which I'm actually using. You need to flatten the quoted form before you can use 'some' to check whether the local was used in the form:

(defmacro assert
  "Evaluates expr and throws an exception if it does not evaluate to logical true."
  {:added "1.0"}
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep#  (System/getProperty "line.separator")
               form# '~x]
           (throw (AssertionError. (apply str "Assert failed: " (pr-str form#) sep#
                                          (map (fn [[k# v#]] 
                                                 (when (some #{k#} (flatten form#)) 
                                                   (str "\t" k# " : " v# sep#))) 
                                               ~bindings)))))))))
Comment by Stuart Halloway [ 04/Jan/11 8:31 PM ]

I am holding off on this until we have more solidity around http://dev.clojure.org/display/design/Error+Handling. (Considering, for instance, having all exceptions thrown from Clojure provide access to locals.)

When my pipe dream fades I will come back and screen this before the next release.

Comment by Stuart Halloway [ 28/Jan/11 1:14 PM ]

Why try to guess what someone wants to do with the locals (or any other context, for that matter) when you can specify a callback (see below). This would have been useful last week when I had an assertion that failed only on the CI box, where no debugger is available.

Rich, at the risk of beating a dead horse, I still think this is a good idea. Debuggers are not always available, and this is an example of where a Lisp is intrinsically capable of providing better information than can be had in other environments. If you want a patch for the code below please mark waiting on me, otherwise please decline this ticket so I stop looking at it.

(def ^:dynamic *assert-handler* nil)

(defn ^{:private true} local-bindings
  "Produces a map of the names of local bindings to their values."
  [env]
  (let [symbols (map key env)]
    (zipmap (map (fn [sym] `(quote ~sym)) symbols) symbols)))

(defmacro assert
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep#  (System/getProperty "line.separator")
               form# '~x]
           (if *assert-handler*
             (*assert-handler* form# ~bindings)
             (throw (AssertionError. (apply str "Assert failed: " (pr-str form#) sep#
                                            (map (fn [[k# v#]] 
                                                   (when (some #{k#} (flatten form#)) 
                                                     (str "\t" k# " : " v# sep#))) 
                                                 ~bindings))))))))))
Comment by Jeff Weiss [ 27/May/11 8:16 AM ]

A slight improvement I made in my own version of this code: flatten does not affect set literals. So if you do (assert (some #{x} [a b c d])) the value of x will not be printed. Here's a modified flatten that does the job:

(defn symbols [sexp]
  "Returns just the symbols from the expression, including those
   inside literals (sets, maps, lists, vectors)."
  (distinct (filter symbol? (tree-seq coll? seq sexp))))
Comment by Andy Fingerhut [ 18/Nov/12 1:06 AM ]

Attaching git format patch clj-415-assert-prints-locals-v1.txt of Stuart Halloway's version of this idea. I'm not advocating it over the other variations, just getting a file attached to the JIRA ticket.

Comment by Michael Blume [ 05/Jul/15 7:53 PM ]

Previous patch was incompatible with CLJ-1005, which moves zipmap later in clojure.core. Rewrote to use into.

Comment by Michael Blume [ 06/Jul/15 3:30 PM ]

Both patches are somehow incompatible with CLJ-1224. When building my compojure-api project I get

Exception in thread "main" java.lang.UnsupportedOperationException: Can't type hint a primitive local, compiling:(schema/core.clj:680:27)
	at clojure.lang.Compiler.analyze(Compiler.java:6569)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$MapExpr.parse(Compiler.java:3050)
	at clojure.lang.Compiler.analyze(Compiler.java:6558)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3791)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6751)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$NewExpr$Parser.parse(Compiler.java:2614)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$ThrowExpr$Parser.parse(Compiler.java:2409)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5889)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2785)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5889)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6205)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2777)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2785)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2785)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5891)
	at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5322)
	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3925)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6747)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5891)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6205)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2777)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5891)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6205)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5891)
	at clojure.lang.Compiler$NewInstanceMethod.parse(Compiler.java:8165)
	at clojure.lang.Compiler$NewInstanceExpr.build(Compiler.java:7672)
	at clojure.lang.Compiler$NewInstanceExpr$DeftypeParser.parse(Compiler.java:7549)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5889)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6205)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5891)
	at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5322)
	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3925)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6747)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.eval(Compiler.java:6805)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:436)
	at plumbing.fnk.schema$eval12507$loading__5352__auto____12508.invoke(schema.clj:1)
	at plumbing.fnk.schema$eval12507.invoke(schema.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at plumbing.core$eval12244$loading__5352__auto____12245.invoke(core.clj:1)
	at plumbing.core$eval12244.invoke(core.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:930)
	at compojure.api.meta$eval11960$loading__5352__auto____11961.invoke(meta.clj:1)
	at compojure.api.meta$eval11960.invoke(meta.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:703)
	at compojure.api.core$eval11954$loading__5352__auto____11955.invoke(core.clj:1)
	at compojure.api.core$eval11954.invoke(core.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at compojure.api.sweet$eval11948$loading__5352__auto____11949.invoke(sweet.clj:1)
	at compojure.api.sweet$eval11948.invoke(sweet.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:703)
	at com.climate.scouting.resources$eval10202$loading__5352__auto____10203.invoke(resources.clj:1)
	at com.climate.scouting.resources$eval10202.invoke(resources.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5761)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:703)
	at com.climate.scouting.service$eval8256$loading__5352__auto____8257.invoke(service.clj:1)
	at com.climate.scouting.service$eval8256.invoke(service.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at com.climate.scouting.dev_server$eval8250$loading__5352__auto____8251.invoke(dev_server.clj:1)
	at com.climate.scouting.dev_server$eval8250.invoke(dev_server.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at user$eval58$fn__69.invoke(form-init9085313321330645488.clj:1)
	at user$eval58.invoke(form-init9085313321330645488.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6798)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.Compiler.loadFile(Compiler.java:7191)
	at clojure.main$load_script.invoke(main.clj:275)
	at clojure.main$init_opt.invoke(main.clj:280)
	at clojure.main$initialize.invoke(main.clj:308)
	at clojure.main$null_opt.invoke(main.clj:343)
	at clojure.main$main.doInvoke(main.clj:421)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.UnsupportedOperationException: Can't type hint a primitive local
	at clojure.lang.Compiler$LocalBindingExpr.<init>(Compiler.java:5792)
	at clojure.lang.Compiler.analyzeSymbol(Compiler.java:6929)
	at clojure.lang.Compiler.analyze(Compiler.java:6532)
	... 299 more
Failed.
Comment by Andy Fingerhut [ 06/Jul/15 4:02 PM ]

Michael, are you finding these incompatibilities between patches because you want to run a modified version of Clojure with all of these patches? Understood, if so.

If you are looking for pairs of patches that are incompatible with each other, I'd recommend a different hobby They get applied at the rate of about 9 per month, on average, so there should be plenty of time to resolve inconsistencies between them later.





[CLJ-1148] adds docstring support to defonce Created: 17/Jan/13  Updated: 29/Apr/15

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: 10
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-6.patch

Screened by:



 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.

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

Linus, while JIRA can handle multiple attachments on the same ticket with identical names, it can be confusing to do so. Would you mind renaming or removing the ones you have added with duplicate names, e.g. delete obsolete ones, perhaps? Instructions for deleting patches are in the "Removing patches" section on this wiki page: http://dev.clojure.org/display/community/Developing+Patches





[CLJ-1099] better error message when passing non-seq to seq Created: 01/Nov/12  Updated: 24/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: checkargs

Attachments: Text File better-error-message-for-seq.patch    
Patch: Code
Approval: Vetted

 Description   

Design discussion here.

This patch improves Clojure's error message for a single common error: passing a non-seq where a seq is neede. More importantly, it is intended as a prototype for other similar improvements in the future.

Error message before:

(cons 1 2)
=> IllegalArgumentException Don't know how to create ISeq from: java.lang.Long

Error message after:

user=> (cons 1 2)
ExceptionInfo Don't know how to create ISeq from: java.lang.Long
user=> (ex-data *e)
{:instance 2}

Patch: better-error-message-for-seq.patch
NOTE: This patch was reverted as it affected the inlining of RT.seqFrom().



 Comments   
Comment by Michael Klishin [ 12/Nov/12 10:34 AM ]

Wouldn't it be better to make it read "Don't know how to create ISeq from: 2 (java.lang.Long)"? How many beginners will figure
out ex-data exists and how to use it?

Comment by Stuart Halloway [ 12/Apr/13 11:36 AM ]

Hi Michael,

ex-info messages should not, in general, pr-str things into their bodies. This raises the question of print-length and print-level in a place where the user doesn't have good control, while the whole point of ex-info is to be in the data business, not the string business. Users can control printing from ex-data any way they like.

There are two possible ways to make beginners aware of ex-data: Tell them about it in one (or a few places) in docs, or in an infinite number of places saying "This would have been useful here, but we didn't use it because you might not know about it." I prefer the former.

That said, I think it would be great to increase the visibility of ex-info and ex-data early on in documentation for beginners, and to make sure that things like exception printing in logs are flexible enough not to lose the benefits of ex-info.

Comment by Andy Fingerhut [ 25/Mar/14 5:14 PM ]

Just a comment that this fix was committed before release 1.6.0, and then reverted very shortly before release 1.6.0. I believe the reason for reverting was due to concerns that this change made performance about 5% slower in some relatively common cases, with a suspicion that it could have affected inlining of the seqFrom method.

Not sure whether the ticket should be reopened or not.





[CLJ-771] Move unchecked-prim casts to clojure.unchecked Created: 07/Apr/11  Updated: 30/Jan/15

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

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

Attachments: Text File clj-771-move-unchecked-casts-patch-v5.txt     Text File move-unchecked-casts.patch     Text File move-unchecked-casts-v2.patch    
Patch: Code and Test
Approval: Vetted
Waiting On: Rich Hickey

 Description   

Per Rich's comment in CLJ-767:

Moving unchecked coercions into unchecked ns is ok



 Comments   
Comment by Alexander Taggart [ 29/Apr/11 3:41 PM ]

Requires that patch on CLJ-782 be applied first.

Comment by Stuart Sierra [ 31/May/11 10:43 AM ]

Applies on master as of commit 66a88de9408e93cf2b0d73382e662624a54c6c86

Comment by Rich Hickey [ 09/Dec/11 8:40 AM ]

still considering when to incorporate this

Comment by John Szakmeister [ 19/May/12 9:36 AM ]

v2 of the patch applies to master as of commit eccde24c7fb63679f00c64b3c70c03956f0ce2c3

Comment by Andy Fingerhut [ 07/Sep/12 12:40 AM ]

Patch clj-771-move-unchecked-casts-patch-v3.txt dated Sep 6 2012 is the same as Alexander Taggart's patch move-unchecked-casts.patch except that it has been updated to apply cleanly to latest Clojure master.

Comment by Andy Fingerhut [ 20/Oct/12 12:18 PM ]

Patch clj-771-move-unchecked-casts-patch-v4.txt dated Oct 20 2012 is the same as Alexander Taggart's patch move-unchecked-casts.patch except that it has been updated to apply cleanly to latest Clojure master.

Comment by Andy Fingerhut [ 01/Jan/13 11:37 AM ]

The patch clj-771-move-unchecked-casts-patch-v4.txt applies cleanly to latest master and passes all tests. Rich marked this ticket as Incomplete on Dec 9 2011 with the comment "still considering when to incorporate this" above. Is it reasonable to change it back to Vetted or Screened so it can be considered again, perhaps after Release 1.5 is made?

Comment by Andy Fingerhut [ 13/Feb/13 12:50 AM ]

Patch clj-771-move-unchecked-casts-patch-v5.txt dated Feb 12 2013 is the same as Alexander Taggart's patch move-unchecked-casts.patch except that it has been updated to apply cleanly to latest Clojure master.





[CLJ-865] Macroexpansion discards &form metadata Created: 26/Oct/11  Updated: 05/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 21
Labels: Compiler

Attachments: Text File 0001-Add-test-for-macroexpansion-metadata-preservation.patch     Text File 0002-Preserve-form-metadata-on-macroexpanded-forms.patch     Text File 0003-Make-defmacro-preserve-form-metadata.patch     Text File 0004-Another-stab-at-implementing-this.patch     File 2013-10-11_CLJ-865_Fix-With-Tests.diff     Text File clj-865.patch     Text File clj865.patch     Text File clj-865-updated-v2-patch.txt     Text File updated.patch    
Patch: Code and Test
Approval: Vetted

 Description   

This patch changes the behavior of metadata when used in conjunction with macros. The metadata &form is now merged with the metadata of the macro call sexpr. This allows users to either type-hint the inner or the outer form in a macro call and have somewhat better results. In the past, the metadata from the macroexpand was used as-is. This disallowed code like the following, to work without reflection:

(.trim ^String (when true "hello "))

Patch: 2013-10-11_CLJ-865_Fix-With-Tests.diff
Screened by: Timothy Baldridge

--------- Implementation Details ----------

As discussed in http://groups.google.com/group/clojure/browse_thread/thread/2690cb6ca0e8beb8 there is a "surprise factor" when type-hinting an expression that represents a macro, such as with (.length ^String (doto (identity "x") prn)). Here the doto macro discards the metadata on &form, causing a reflective lookup. This has the effect that while expressions representing function calls can be type-hinted, expressions representing macros in general cannot. The doto macro could be rewritten to respect its &form metadata, but doing this for every macro in existence would be tedious and error-prone. Instead, I propose a change to the compiler, to cause macroexpansion to hang onto the metadata automatically.

The first patch attached adds a test for the behavior I propose: this test fails. After applying the second patch, the test passes.

There are a couple points that merit further consideration before accepting my patch:

  • I'm not sure I actually got the Java code formatted correctly. My editor is not well-configured to get the clojure/core style right automatically.
  • My solution is to take the &form metadata, drop :line/:file keys, and then merge with the returned metadata, with &form taking precedence. I'm not sure whether this is the right approach in all cases, even though it works for :tag metadata.
  • I achieved this with a change to the compiler, which makes it fairly heavy-weight. It should be possible to instead adjust defmacro if changes to the compiler are not desirable. However, I believe this would involve substantially more work and be harder to test (for example, multiple arities complicate things). It seems nicer to treat the macroexpansion as a black box and then make metadata tweaks to the result, rather than modifying their actual defmacro code.
  • If a macro expands to something that is not an IObj, such as an Integer, then my patch silently discards the caller's metadata. Would it be better to throw an exception?


 Comments   
Comment by Alan Malloy [ 28/Oct/11 1:12 AM ]

So I went ahead and did the work of making this change in clojure.core/defmacro instead of clojure.lang.Compiler/macroexpand1. It was even worse than I expected: I didn't realize we don't yet have syntax-quote or apply at this stage in bootstrapping, so writing a non-trivial macroexpansion requires a huge amount of (list `foo (list `bar 'local-name)) and so forth.

I'm sure the version I wrote is not optimal, but it seemed simpler to piggyback on defn, and then use alter-var-root to shim the metadata management in, than it would have been to expand to the correct thing in the first place.

Anyway, attached patch #3 could be applied instead of #2 to resolve the issue in clojure.core instead of clojure.lang. The tests added in patch #1 pass either way.

Comment by Alan Malloy [ 13/Nov/11 8:29 PM ]

I realized I can do this with a named private function instead of an anonymous function, reducing the amount of mess defmacro itself has to generate. Patch 4 is, I think, strictly better than Patch 3, if a Clojure implementation is preferred to one in Java.

Comment by Chouser [ 20/Nov/11 10:43 PM ]

I prefer patch 0002 in Java over either 0003 or 0004. Patch 0002 keeps the knowledge of how to invoke macro fns (specifically the extra &form and &env args) in one place, macroexpand1 rather than duplicating that knowledge in core.clj as well. Note patch 0001 is just tests.

The proposed default macroexpansion behavior is more useful than what we currently have, but there are two details I'd like to think about a bit more:

1) In exchange for a more useful default, macro writers lose the ability to consume their &form metadata and have control over the resulting form metadata without the &form metadata overridding it. That is, macros are no longer in complete control of their output form.

2) Rule (1) above has hardcoded exceptions for :line and :file, where &form metadata is unable to override the results returned by the macro.

Comment by Alan Malloy [ 01/Jun/12 2:04 PM ]

This patch incorporates all previous patches to this issue.

On the clj-dev mailing list, Andy Fingerhut suggested a new metadata key for allowing the macro author to specify "I've looked at their &form metadata, and this form is exactly what I want to expand to, please don't change the metadata any further." I've implemented this, and I think it addresses Chouser's concern about needing a way to "break out" of the improved-default behavior.

One open question is, is :explicit-meta the right key to use? I spent some time tracking down a bug caused by my forgetting the keyword and using :explicit-metadata in my test; perhaps something more difficult to get confused by is available.

Comment by Andy Fingerhut [ 14/Aug/13 8:05 PM ]

clj-865-updated-v2-patch.txt dated Aug 14 2013 is identical to Alan Malloy's updated.patch dated Jun 1 2012. I simply updated the patch to apply cleanly to latest master after some context lines in the test file macros.clj had gone bad due to recent commits.

Comment by Timothy Baldridge [ 11/Oct/13 9:23 AM ]

Added updated patch that works against master, and also removes COLUMN_KEY from the macro's metadata

Comment by Timothy Baldridge [ 11/Oct/13 12:50 PM ]

Added patch that contains all fixes plus a few more tests.

Comment by Rich Hickey [ 22/Nov/13 11:19 AM ]

Since this could break things, we could just take metadata on the macro name to ask for this:

(defmacro ^:keep-meta simple-macro [f arg]
  `(~f ~arg))

or something

Comment by Alan Malloy [ 03/Dec/13 1:24 AM ]

Sure, I'll put together that patch. I'm worried, though, that if it's not the default, it will just never get used, and we'll be in effectively the same situation we are now, where no macros do this right. I don't foresee anyone going through their libraries to add ^:keep-meta on every macro.

Comment by Alan Malloy [ 03/Dec/13 2:20 AM ]

I updated the patch to behave as Rich requested, but it caused a test regression that I can't figure out, in the handling of either refer or private vars. Hopefully someone else can run the tests and figure out what is missing here; my change is supposed to be opt-in, and I can't see where I've gone wrong.

Comment by Andy Fingerhut [ 07/Dec/13 10:31 AM ]

Alan, your patch clj865.patch dated Dec 3, 2013 has some HTML cruft at the beginning and end, but even after removing that it does not apply cleanly to the latest Clojure master as of today. I understand that you say it needs more work, but it would be easier for others who wish to try it out if it applied cleanly.

Comment by Alan Malloy [ 10/Dec/13 1:06 PM ]

Sorry Andy, and thanks for noticing. I haven't been on a very developer-friendly computer recently, but I'll try to fix the patch tonight.

Comment by Alan Malloy [ 11/Dec/13 10:26 AM ]

Here's a fix to the patch. I verified that this applies cleanly to current master.

Comment by Alan Malloy [ 11/Dec/13 10:27 AM ]

To clarify, it's the file named clj-865.patch. I didn't realize JIRA wouldn't make it clear which file I uploaded along with the comment.

Comment by Andy Fingerhut [ 05/Dec/14 1:54 PM ]

As of Eastwood version 0.2.0, it includes a new warning :unused-meta-on-macro that will warn whenever metadata is applied to a macro invocation, with the known exception of clojure.core/fn, which explicitly uses metadata applied to it. https://github.com/jonase/eastwood#unused-meta-on-macro





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

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

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

Requires Java >=1.7!


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

 Description   

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

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

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

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



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

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

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

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

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

Clojure 1.7.0-master-SNAPSHOT.

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

Found 2 outliers in 60 samples (3.3333 %)
low-severe 2 (3.3333 %)
Variance from outliers : 1.6389 % Variance is slightly inflated by outliers

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

CHANGE:

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

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

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

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

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

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

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

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

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

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

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

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

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

I'll contact you via mail!

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

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

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

New issue is at CLJ-1452.

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

Patch 0001-rand-using-ThreadLocalRandom-and-tests-for-random.patch dated May 11 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





[CLJ-124] GC Issue 120: Determine mechanism for controlling automatic shutdown of Agents, with a default policy and mechanism for changing that policy as needed Created: 17/Jun/09  Updated: 23/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Chas Emerick Assignee: Alex Miller
Resolution: Unresolved Votes: 12
Labels: agents

Attachments: Text File clj-124-daemonthreads-v1.patch     Text File clj-124-v1.patch    
Patch: Code
Approval: Vetted
Waiting On: Rich Hickey

 Description   

The original description when this ticket was vetted is below, starting with "Reported by cemer...@snowtide.com, June 01, 2009". This prefix attempts to summarize the issue and discussion.

Description:

Several Clojure functions involving agents and futures, such as future, pmap, clojure.java.shell/sh, and a few others, create non-daemon threads in the JVM in an ExecutorService called soloExecutor created via Executors#newCachedThreadPool. The javadocs for this method here http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executors.html#newCachedThreadPool%28%29 say "Threads that have not been used for sixty seconds are terminated and removed from the cache." This causes a 60-second wait after a Clojure program is done before the JVM process exits. Questions about this confusing behavior come up a couple of times per year on the Clojure Google group. Search for "shutdown-agents" to find most of these occurrences, since calling (shutdown-agents) at the end of one's program typically eliminates this 60-second wait.

Example:

% java -cp clojure.jar clojure.main -e "(println 1)"
1
[ this case exits quickly ]

% java -cp clojure.jar clojure.main -e "(println @(future 1))"
1
[ 60-second pause before process exits, at least on many platforms and JVMs ]

Summary of comments before July 2014:

Most of the comments on this ticket on or before August 23, 2010 were likely spread out in time before being imported from the older ticket tracking system into JIRA. Most of them refer to an older suggested patch that is not in JIRA, and compilation problems it had with JDK 1.5, which is no longer supported by Clojure 1.6.0. I think these comments can be safely ignored now.

Alex Miller blogged about this and related issues here: http://tech.puredanger.com/2010/06/08/clojure-agent-thread-pools/

Since then, two of the suggestions Alex raised have been addressed. One by CLJ-378 and one by the addition of set-agent-send-executor! and similar functions to Clojure 1.5.0: https://github.com/clojure/clojure/blob/master/changes.md#23-clojurecoreset-agent-send-executor-set-agent-send-off-executor-and-send-via

One remaining issue is the topic of this ticket, which is how best to avoid this 60-second pause.

Approach #1: automatically shut down agents

One method is mentioned in Chas Emerick's original description below, suggested by Rich Hickey, but perhaps long enough ago he may no longer endorse it: Create a Var *auto-shutdown-agents* that when true (the default value), clojure.lang.Agent shutdown() is called after the clojure.main entry point. This removes the surprising wait for common methods of starting Clojure, while allowing expert users to change that value to false if desired.

Approach #2: create daemon threads by default

Another method mentioned by several people in the comments is to change the threads created in agent thread pools to daemon threads by default, and perhaps to deprecate shutdown-agents or modify it to be less dangerous. That approach is discussed a bit more in Alex's blog post linked above, and in a comment from Alexander Taggart on July 11, 2011 below.

Approach #3:

The only other comment before 2014 that is not elaborated in this summary is shoover's suggestion: There are already well-defined and intuitive ways to block on agents and futures. Why not deprecate shutdown-agents and force users to call await and deref if they really want to block? In the pmap situation one would have to evaluate the pmap form.

Approach #4: Create a cached thread pool with a timeout much lower than 60 seconds

This could be done by using one of the ThreadPoolExecutor constructors with a keepAliveTime parameter of the desired time.

Patch: clj-124-v1.patch clj-124-daemonthreads-v1.patch

At most one of these patches should be considered, depending upon the desired approach to take.

Patch clj-124-v1.patch implements appproach #1 using *auto-shutdown-agents*. See the Jul 31 2014 comment when this patch was added for some additional details.

Patch clj-124-daemonthreads-v1.patch implements approach #2 and is straightforward.

Reported by cemer...@snowtide.com, Jun 01, 2009

There has been intermittent chatter over the past months from a couple of
people on the group (e.g.
http://groups.google.com/group/clojure/browse_thread/thread/409054e3542adc1f)
and in #clojure about some clojure scripts hanging, either for a constant
time (usually reported as a minute or so with no CPU util) or seemingly
forever (or until someone kills the process).

I just hit a similar situation in our compilation process, which invokes
clojure.lang.Compile from ant.  The build process for this particular
project had taken 15 second or so, but after adding a couple of pmap calls,
that build time jumped to ~1:15, with roughly zero CPU utilization over the
course of that last minute.

Adding a call to Agent.shutdown() in the finally block in
clojure.lang.Compile/main resolved the problem; a patch including this
change is attached.  I wouldn't suspect anyone would have any issues with
such a change.

-----
In general, it doesn't seem like everyone should keep tripping over this
problem in different directions.  It's a very difficult thing to debug if
you're not attuned to how clojure's concurrency primitives work under the
hood, and I would bet that newer users would be particularly affected.

After discussion in #clojure, rhickey suggested adding a
*auto-shutdown-agents* var, which:

- if true when exiting one of the main entry points (clojure.main, or the
legacy script/repl entry points), Agent.shutdown() would be called,
allowing for the clean exit of the application

- would be bound by default to true

- could be easily set to false for anyone with an advanced use-case that
requires agents to remain active after the main thread of the application
exits.

This would obviously not help anyone initializing clojure from a different
entry point, but this may represent the best compromise between
least-surprise and maximal functionality for advanced users.

------

In addition to the above, it perhaps might be worthwhile to change the
keepalive values used to create the Threadpools used by c.l.Actor's
Executors.  Currently, Actor uses a default thread pool executor, which
results in a 60s keepalive.  Lowering this to something much smaller (1s?
5s?) would additionally minimize the impact of Agent's threadpools on Java
applications that embed clojure directly (and would therefore not benefit
from *auto-shutdown-agents* as currently conceived, leading to puzzling
'hanging' behaviour).  I'm not in a position to determine what impact this
would have on performance due to thread churn, but it would at least
minimize what would be perceived as undesirable behaviour by users that are
less familiar with the implementation details of Agent and code that
depends on it.

Comment 1  by cemer...@snowtide.com, Jun 01, 2009

Just FYI, I'd be happy to provide patches for either of the suggestions mentioned
above...


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

Converted from http://www.assembla.com/spaces/clojure/tickets/124
Attachments:
compile-agent-shutdown.patch - https://www.assembla.com/spaces/clojure/documents/a56S2ow4ur3O2PeJe5afGb/download/a56S2ow4ur3O2PeJe5afGb
124-compilation.diff - https://www.assembla.com/spaces/clojure/documents/aqn0IGxZSr3RUGeJe5aVNr/download/aqn0IGxZSr3RUGeJe5aVNr

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

oranenj said: [file:a56S2ow4ur3O2PeJe5afGb]

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

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

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

cemerick said: (In [[r:fa3d24973fc415b35ae6ec8d84b61ace76bd4133]]) Add a call to Agent.shutdown() at the end of clojure.lang.Compile/main Refs #124

Signed-off-by: Chouser <chouser@n01se.net>

Branch: master

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

chouser@n01se.net said: I'm closing this ticket to because the attached patch solves a specific problem. I agree that the idea of an auto-shutdown-agents var sounds like a positive compromise. If Rich wants a ticket to track that issue, I think it'd be best to open a new ticket (and perhaps mention this one there) rather than use this ticket to track further changes.

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

scgilardi said: With both Java 5 and Java 6 on Mac OS X 10.5 Leopard I'm getting an error when compiling with this change present.

Java 1.5.0_19
Java 1.6.0_13

For example, when building clojure using "ant" from within my clone of the clojure repo:

[java] java.security.AccessControlException: access denied (java.lang.RuntimePermission modifyThread)
[java] at java.security.AccessControlContext.checkPermission(AccessControlContext.java:264)
[java] at java.security.AccessController.checkPermission(AccessController.java:427)
[java] at java.util.concurrent.ThreadPoolExecutor.shutdown(ThreadPoolExecutor.java:894)
[java] at clojure.lang.Agent.shutdown(Agent.java:34)
[java] at clojure.lang.Compile.main(Compile.java:71)

I reproduced this on two Mac OS X 10.5 machines. I'm not aware of having any enhanced security policies along these lines on my machines. The compile goes fine for me with Java 1.6.0_0 on an Ubuntu box.

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

chouser@n01se.net said: I had only tested it on my ubuntu box – looks like that was openjdk 1.6.0_0. I'll test again with sun-java5 and sun-java6.

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

chouser@n01se.net said: 1.6.0_13 worked fine for me on ubuntu, but 1.5.0_18 generated an the exception Steve pasted. Any suggestions? Should this patch be backed out until someone has a fix?

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

achimpassen said: [file:aqn0IGxZSr3RUGeJe5aVNr]

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

chouser@n01se.net said: With Achim's patch, clojure compiles for me on ubuntu using java 1.5.0_18 from sun, and still works on 1.6.0_13 sun and 1.6.0_0 openjdk. I don't know anything about ant or the security error, but this is looking good to me.

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

achimpassen said: It works for me on 1.6.0_13 and 1.5.0_19 (32 and 64 bit) on OS X 10.5.7.

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

chouser@n01se.net said: (In [[r:895b39dabc17b3fd766fdbac3b0757edb0d4b60d]]) Rev fa3d2497 causes compile to fail on some VMs – back it out. Refs #124

Branch: master

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

mikehinchey said: I got the same compile error on both 1.5.0_11 and 1.6.0_14 on Windows. Achim's patch fixes both.

See the note for "permissions" on http://ant.apache.org/manual/CoreTasks/java.html . I assume ThreadPoolExecutor.shutdown is the problem, it would shutdown the main Ant thread, so Ant disallows that. Forking avoids the permissions limitation.

In addition, since the build error still resulted in "BUILD SUCCESSFUL", I think failonerror="true" should also be added to the java call so the build would totally fail for such an error.

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

chouser@n01se.net said: I don't know if the <java fork=true> patch is a good idea or not, or if there's a better way to solve the original problem.

Chas, I'm kicking back to you, but I guess if you don't want it you can reassign to "nobody".

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

richhickey said: Updating tickets (#8, #42, #113, #2, #20, #94, #96, #104, #119, #124, #127, #149, #162)

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

shoover said: I'd like to suggest an alternate approach. There are already well-defined and intuitive ways to block on agents and futures. Why not deprecate shutdown-agents and force users to call await and deref if they really want to block? In the pmap situation one would have to evaluate the pmap form.

The System.exit problem goes away if you configure the threadpools to use daemon threads (call new ThreadPoolExecutor and pass a thread factory that creates threads and sets daemon to true). That way the user has an explicit means of blocking and System.exit won't hang.

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

alexdmiller said: I blogged about these issues at:
http://tech.puredanger.com/2010/06/08/clojure-agent-thread-pools/

I think that:

  • agent thread pool threads should be named (see ticket #378)
  • agent thread pools must be daemon threads by default
  • having ways to specify an customized executor pool for an agent send/send-off is essential to customize threading behavior
  • (shutdown-agents) should be either deprecated or made less dangerous
Comment by Alexander Taggart [ 11/Jul/11 9:33 PM ]

Rich, what is the intention behind using non-daemon threads in the agent pools?

If it is because daemon threads could terminate before their work is complete, would it be acceptable to add a shutdown hook to ensure against such premature termination? Such a shutdown hook could call Agent.shutdown(), then awaitTermination() on the pools.

Comment by Christopher Redinger [ 27/Nov/12 3:47 PM ]

Moving this ticket out of approval "OK" status, and dropping the priority. These were Assembla import defaults.

Also, Chas gets to be the Reporter now.

Comment by Chas Emerick [ 27/Nov/12 5:56 PM ]

Heh, blast from the past.

The comment import appears to have set their timestamps to the date of the import, so the conversation is pretty hard to follow, and obviously doesn't benefit from the intervening years of experience. In addition, there have been plenty of changes to agents, including some recent enhancements that address some of the pain points that Alex Miller mentioned above.

I propose closing this as 'invalid' or whatever, and opening one or more new issues to track whatever issues still persist (presumably based on fresh ML discussion, etc).

Comment by Andy Fingerhut [ 27/Nov/12 6:11 PM ]

Rereading the original description of this ticket, without reading all of the comments that follow, that description is still right on target for the behavior of latest Clojure master today.

People send messages to the Clojure Google group every couple of months hitting this issue, and one even filed CLJ-959 because of hitting it. I have updated the examples on ClojureDocs.org for future, and also for pmap and clojure.java.shell/sh which use future in their implementations, to warn people about this and explain that they should call (shutdown-agents), but making it unnecessary to call shutdown-agents would be even better, at least as the default behavior. It sounds fine to me to provide a way for experts on thread behavior to change that default behavior if they need to.

Comment by Andy Fingerhut [ 31/Jul/14 6:39 PM ]

Patch clj-124-v1.patch dated Jul 31 2014 implements the approach of calling clojure.lang.Agent#shutdown when the new Var *auto-shutdown-agents* is true, which is its default value.

I don't see any benefit to making this Var dynamic. Unless I am missing something, only the root binding value is visible after clojure.main/main returns, not any binding that would be pushed on top of that if it were dynamic. It seems to require alter-var-root to change it to false in a way that this patch would avoid calling clojure.lang.Agent#shutdown.

This patch only adds the shutdown call to clojure.main#main, but can easily be added to the legacy_repl and legacy_script methods if desired.

Comment by Andy Fingerhut [ 23/Aug/14 11:49 AM ]

Patch clj-124-daemonthreads-v1.patch dated Aug 23 2014 simply modifies the ThreadFactory so that every thread created in an agent thread pool is a daemon thread.





[CLJ-250] debug builds Created: 27/Jan/10  Updated: 23/Oct/13

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

Type: Enhancement Priority: Minor
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: build

Approval: Vetted
Waiting On: Rich Hickey

 Description   

This ticket includes two patches:

  1. a patch to set assert when clojure.lang.RT loads, based on the presence of system property clojure.debug
  2. expand error messages in assert to include local-bindings</code> (a new macro which wraps the implicit <code>&env)

Things to consider before approving these patches:

  1. should there be an easy Clojure-level way to query if debug is enabled? (checking assert isn't the same, as debug should eventually drive other features)
  2. assertions will now be off by default – this is a change!
  3. is the addition of the name local-bindings to clojure.core cool?


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

Converted from http://www.assembla.com/spaces/clojure/tickets/250
Attachments:
add-clojure-debug-flag.patch - https://www.assembla.com/spaces/clojure/documents/aUWn50c64r35E-eJe5aVNr/download/aUWn50c64r35E-eJe5aVNr
assert-report-locals.patch - https://www.assembla.com/spaces/clojure/documents/aUWqLSc64r35E-eJe5aVNr/download/aUWqLSc64r35E-eJe5aVNr

Comment by Stuart Halloway [ 07/Dec/10 8:23 PM ]

Ignore the old patches. Considering the following implementation, please review and then move ticket to waiting on Stu:

  1. RT will check system property "clojure.debug", default to false
  2. property will set the root binding for the current *assert*, plus a new *debug* flag. (Debug builds can and will drive other things than just asserts.)
  3. does Compile.java need to push *assert* or *debug* as thread local bindings, or can they be root-bound only when compiling clojure?
  4. will add *debug* binding to clojure.main/with-bindings. Anywhere else?
  5. build.xml should not have to change – system properties will flow through (and build.xml may not be around much longer anyway)
  6. once we agree on the approach, I will ping maven plugin and lein owners so that they flow the setting through
  7. better assertion messages will be a separate ticket
  8. what is the interaction between *debug* and *unchecked-math*? Change checks to (and *unchecked-math* (not *debug*))}?
Comment by Rich Hickey [ 08/Dec/10 11:00 AM ]

#3 - root bound only
#4 - should not be in with-bindings for same reason as #3 - we don't want people to set! *debug* nor *assert*
#8 - yes, wrapping that in a helper fn

#6 - my biggest reservation is that this isn't yet informed by maven best practices

Comment by Stuart Sierra [ 08/Dec/10 2:09 PM ]

System properties can be passed through Maven, so I do not anticipate this being a problem.

However, I would prefer *assert* to remain true by default.

Comment by Chas Emerick [ 09/Dec/10 7:19 AM ]

SS is correct about this approach not posing any issue for Maven. In addition, the build could easily be set up to always emit two jars, one "normal", one "debug".

I'd suggest that, while clojure.debug might have broad effect, additional properties should be available to provide fine-grained control over each of the additional "debug"-related parameterizations that might become available in the future.


I'd like to raise a couple of potentially tangential concerns (after now thinking about assertions for a bit in the above context), some or all of which may simply be a result of my lack of understanding in various areas.

Looking at where assert is used in core.clj (only two places AFAICT: validating arguments to derive and checking pre- and post-conditions in fn), it would seem unwise to make it false by default. i.e. non-Named values would be able to get into hierarchies, and pre- and post-conditions would simply be ignored.

It's my understanding that assertions (talking here of the JVM construct, from which Clojure reuses AssertionError) should not be used to validate arguments to public API functions, or used to validate any aspect of a function's normal operation (i.e. "where not to use assertions"). That would imply that derive should throw IllegalArugmentException when necessary, and fn pre- and post-conditions should perhaps throw IllegalStateException – or, in any case, something other than AssertionError via assert. This would match up far better with most functions in core using assert-args rather than assert, the former throwing IllegalArgumentException rather than AssertionError.

That leads me to the question: is assert (and *assert*) intended to be a Clojure construct, or a quasi-interop form?

If the former, then it can roughly have whatever semantics we want, but then it seems like it should not be throwing AssertionError.

If the latter, then AssertionError is appropriate on the JVM, but then we need to take care that assertions can be enabled and disabled at runtime (without having to switch around different builds of Clojure), ideally using the host-defined switches (e.g. -ea and friends) and likely not anything like *assert*. I don't know if this is possible or practical at this point (I presume this would require nontrivial compiler changes).


Hopefully the above is not water under the bridge at this point. Thank you in advance for your forbearance.

Comment by Rich Hickey [ 09/Dec/10 8:08 AM ]

Thanks for the useful input Chas. Nothing is concluded yet. I think we should step back and look at the objective here, before moving forward with a solution. Being a dynamic language, there are many things we might want to validate about our programs, where the cost of checking is something we are unwilling to pay in production.

Being a macro, assert has the nice property that, should *assert* not be true during compilation, it generates nil, no conditional test at all. Thus right now it is more like static conditional compilation.

Java assert does have runtime toggle-ability, via -ea as you say. I haven't looked at the bytecode generated for Java asserts, but it might be possible for Clojure assert to do something similar, if the runtime overhead is negligible. It is quite likely that HotSpot has special code for eliding the assertion bytecode given a single check of some flag. I'm just not sure that flag is Class.desiredAssertionStatus.

Whether this turns into changes in assert or pre/post conditions, best practices etc is orthogonal and derived. Currently we don't have a facility to run without the checks. We need to choose between making them disappear during compilation (debug build) or runtime (track -ea) or both. Then we can look at how that is reflected in assert/pre-post and re-examine existing use of both. The "where not to use assertions" doc deals with them categorically, but in not considering their cost, seems unrealistic IMO.

I'd appreciate it if someone could look into how assert is generated and optimized by Java itself.

Comment by Chas Emerick [ 09/Dec/10 5:04 PM ]

Bytecode issues continue to be above my pay grade, unfortunately…

A few additional thoughts in response that you may or may not be juggling already:

assert being a macro makes total sense for what it's doing. Trouble is, "compile-time" is a tricky concept in Clojure: there's code-loading-time, AOT-compile-time, and transitively-AOT-compile-time. Given that, it's entirely possible for an application deployed to a production environment that contains a patchwork of code or no code produced by assert usages in various libraries and namespaces depending upon when those libs and ns' were loaded, AOT-compiled, or their dependents AOT-compiled, and the value of *assert* at each of those times. Of course, this is the case for all such macros whose results are dependent upon context-dependent state (I think this was a big issue with clojure.contrib.logging, making it only usable with log4j for a while).

What's really attractive about the JVM assertion mechanism is that it can be parameterized for a given runtime on a per-package basis, if desired. Reimplementing that concept so that assert can be *ns*-sensitive seems like it'd be straightforward, but the compile-time complexity already mentioned remains, and the idea of having two independently-controlled assertion facilities doesn't sound fun.

I know nearly nothing about the CLR, but it would appear that it doesn't provide for anything like runtime-controllable assertions.

Comment by Stuart Halloway [ 29/Dec/10 3:17 PM ]

The best (dated) evidence I could find says that the compiler sets a special class static final field $assertionsDisabled based on the return of desiredAssertionStatus. HotSpot doesn't do anything special with this, dead code elimination simply makes it go away. The code indeed compiles this way:

11: getstatic #6; //Field $assertionsDisabled:Z
14: ifne 33
17: lload_1
18: lconst_0
19: lcmp
20: ifeq 33
23: new #7; //class java/lang/AssertionError
26: dup
27: ldc #8; //String X should be zero
29: invokespecial #9; //Method java/lang/AssertionError."<init>":(Ljava/lang/Object;)V
32: athrow

Even if we were 100% sure that assertion removal was total, I would still vote for a separate Clojure-level switch, for the following reasons:

  1. I have a real and pressing need to disable some assertions, and I don't need the Java interop at all. Arguably others will be in the same boat.
  2. there will be multiple debugging facilities over time, and having a top-level debug switch is convenient for Clojure users.
  3. Java dis/enabling via command line flags is still possible as a separate feature. We could add this later as a (small) breaking change to our assert, or have a separate java-assert interop form. I am on the fence about which way to go here.
  4. I believe it is perfectly fine to throw an AssertionError from a non-Java-assertion-form. We don't believe in a world of a static exception hierarchy, and an assertion in production is a critical failure no matter what you call it. Even Scala does it http://daily-scala.blogspot.com/2010/03/assert-require-assume.html

Rich: awaiting your blessing to move forward on this.

Comment by Rich Hickey [ 07/Jan/11 9:42 AM ]

The compiler sets $assertionsDisabled when, in static init code? Is there special support in the classloader for it? Is there a link to the best dated evidence you found?

Comment by Stuart Halloway [ 07/Jan/11 9:51 AM ]
  1. Yes, in static init code
  2. There is no special support in the classloader, per Brian Goetz (private correspondence) last week. But dead code elimination is great: "The run-time cost of disabled assertions should indeed be zero for compiled code"
Comment by Stuart Halloway [ 07/Jan/11 9:57 AM ]

Link: Google "java assert shirazi". (Not posting link because I can't tell in 10 secs whether it includes my session information.)

Comment by Alexander Kiel [ 14/Mar/13 1:28 PM ]

Is there anything new on this issue? I also look for a convenient way to disable assertions in production.

Comment by Michał Łopuszyński [ 11/Oct/13 4:21 AM ]

I am also interested in any news on this issue.
Convenient way to enable/disable assertions at runtime (preferably via -ea/-da options) would be a great feature!

Comment by Michał Łopuszyński [ 23/Oct/13 3:12 AM ]

Btw. there is a library for runtime-toggable assertions available via clojars
https://github.com/pjstadig/assertions
This helped me a great deal.





[CLJ-346] (pprint-newline :fill) is not handled correctly Created: 12/May/10  Updated: 03/Sep/13

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

Type: Defect Priority: Minor
Reporter: Assembla Importer Assignee: Tom Faulhaber
Resolution: Unresolved Votes: 0
Labels: print

Approval: Vetted
Waiting On: Tom Faulhaber

 Description   

Filled pretty printing (where we try to fit as many elements on a line as possible) is being too aggressive as we can see when we try to print the following array:

user> (binding [*print-right-margin* 20] (pprint (int-array (range 10))))

Produces:

[0,
1,
2,
3,
4, 5, 6, 7, 8, 9]

Rather than

[0, 1, 2, 3, 4,
5, 6, 7, 8, 9]

or something like that. (I haven't worked through the exact correct representation for this case).

We currently only use :fill style newlines for native java arrays.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 8:01 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/346
Attachments:
0347-pprint-update-2.diff - https://www.assembla.com/spaces/clojure/documents/diLxv6y4Sr35GVeJe5cbLr/download/diLxv6y4Sr35GVeJe5cbLr

Comment by Assembla Importer [ 24/Aug/10 8:01 AM ]

stu said: [file:diLxv6y4Sr35GVeJe5cbLr]

Comment by Assembla Importer [ 24/Aug/10 8:01 AM ]

stu said: The second patch includes the first, and adds another test.

Comment by Assembla Importer [ 24/Aug/10 8:01 AM ]

tomfaulhaber said: This patch was attached to the wrong bug. It should be attached to bug #347. There is no fix for this bug yet.

Comment by Rich Hickey [ 05/Nov/10 8:07 AM ]

Is this current?

Comment by Stuart Halloway [ 29/Nov/10 8:48 PM ]

Tom, this patch doesn't apply, and I am not sure why. Can you take a look?





[CLJ-291] (take-nth 0 coll) redux... Created: 08/Apr/10  Updated: 27/Jul/13

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

Type: Enhancement Priority: Minor
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Approval: Vetted

 Description   

I dont seem to be able make the old ticket uninvalid so here goes
(take-nth 0 coll) causes (at least on Solaris) infinite space and time consumption
It's not a printout error as the following code causes the problem too

(let [j 0
firstprod (apply * (doall (map #(- 1 %) (take-nth j (:props mix)))))]) ; from my parameter update function

I used jvisualvm and the jvm is doing some RNI call - no clojure code is running at all
If left alone it will crash the jvm with all heap space consumed
0 is an InvalidArgument for take-nth
I wouldnt mind if it produced an infinite lazy sequence of nils even though thats wrong
It doesnt do this though it actively destroys the JVM
Its a bug nasty destructive and it took me half a day to figure out what was going on
please let someone fix it!



 Comments   
Comment by Assembla Importer [ 17/Oct/10 9:47 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/291
Attachments:
fixbug291.diff - https://www.assembla.com/spaces/clojure/documents/dfNhoS2Cir3543eJe5cbLA/download/dfNhoS2Cir3543eJe5cbLA

Comment by Assembla Importer [ 17/Oct/10 9:47 AM ]

bhurt said: Before this bug gets marked as invalid as well, let me point out that the problem here is that (take-nth 0 any-list) is a meaningless construction- the only question is what to do when this happens. IMHO, the correct behavior is to throw an exception.

Comment by Assembla Importer [ 17/Oct/10 9:47 AM ]

ataggart said: [file:dfNhoS2Cir3543eJe5cbLA]: throws IllegalArgumentException on negative step size

Comment by Stuart Halloway [ 29/Oct/10 10:36 AM ]

Does calling (take-nth 0 ...) cause the problem, or only realizing the result?

Comment by Chouser [ 29/Oct/10 11:06 AM ]

I'm not seeing a problem. Calling take-nth and even partially consuming the seq it returns works fine for me:

(take 5 (take-nth 0 [1 2 3]))
;=> (1 1 1 1 1)

Note however that it is returning an infinite lazy seq. The example in the issue description seems to include essentially (doall <infinite-lazy-seq>) which does blow the heap:

(doall (range))

This issue still strikes me as invalid.

Comment by Rich Hickey [ 29/Oct/10 11:06 AM ]

(take-nth 0 ...) returns an infinite sequence of the first item:

(take 12 (take-nth 0 [1 2 3]))
=> (1 1 1 1 1 1 1 1 1 1 1 1)

Is something other than this happening on Solaris?





[CLJ-211] Support arbitrary functional destructuring via -> and ->> Created: 17/Nov/09  Updated: 27/Jul/13

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

Type: Enhancement Priority: Minor
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Approval: Vetted

 Description   

Support arbitrary functional destructuring, that is the use of
any function in any destructuring form to help unpack data in
arbitrary ways.

The discussion began here:
http://clojure-log.n01se.net/date/2009-11-17.html#09:31c

The attached patch implements the spec described here:
http://clojure-log.n01se.net/date/2009-11-17.html#10:50a

That is, the following examples would now work:

user=> (let [(-> str a) 1] a)
"1"

user=> (let [[a (-> str b) c] [1 2]] (list a b c))
(1 "2" nil)

user=> (let [(->> (map int) [a b]) "ab"] (list a b))
(97 98)



 Comments   
Comment by Assembla Importer [ 28/Sep/10 6:57 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/211
Attachments:
destructuring-fns.diff - https://www.assembla.com/spaces/clojure/documents/aHWQ_W06Kr3O89eJe5afGb/download/aHWQ_W06Kr3O89eJe5afGb

Comment by Assembla Importer [ 28/Sep/10 6:57 AM ]

chouser@n01se.net said: [file:aHWQ_W06Kr3O89eJe5afGb]: [PATCH] Support -> and ->> in destructuring forms.

Comment by Assembla Importer [ 28/Sep/10 6:57 AM ]

cgrand said: I think the current patch suffers from the problem described here http://groups.google.com/group/clojure-dev/msg/80ba7fad2ff04708 too.

Comment by Assembla Importer [ 28/Sep/10 6:57 AM ]

richhickey said: so, don't use syntax-quote, just use clojure.core/->

Comment by Assembla Importer [ 28/Sep/10 6:57 AM ]

chouser@n01se.net said: Only -> and ->> are actually legal here anyway – if you've locally bound foo to -> there's not really any good reason to think (fn [(foo inc a)] a) should work. And if you've redefined -> or ->> to mean something else in your ns, do we need to catch that at compile time, or is it okay to emit the rearranged code and see what happens?

In short, would '#{> ->> clojure.core/> clojure.core/->>} be sufficient?

Comment by Assembla Importer [ 28/Sep/10 6:57 AM ]

cgrand said: Only -> and ->> are legal here but what if they are aliased or shadowed? Instead of testing the symboil per se I would check, if:

  • the symbol is not in &env
  • the symbol resolve to #'clojure.core/> or #'clojure.core/>>
(when-not (&env (first b)) (#{#'clojure.core/-> #'clojure.core/->>} (resolve (first b))))

but it requires to change destructure's sig to pass the env around

Comment by Stuart Halloway [ 03/Dec/10 1:03 PM ]

Rich: Are you assigned to this by accident? If so, please deassign yourself.





[CLJ-84] GC Issue 81: compile gen-class fail when class returns self Created: 17/Jun/09  Updated: 27/Jul/13

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

Type: Defect Priority: Minor
Reporter: Assembla Importer Assignee: Rich Hickey
Resolution: Unresolved Votes: 0
Labels: None

Approval: Vetted
Waiting On: Chouser

 Description   
Reported by davidhaub, Feb 14, 2009

When attempting to compile the following program, clojure fails with a
ClassNotFoundException.  It occurs because one of the methods returns the
same class that is being generated.  If the returnMe method below is
changed to return an Object, the compile succeeds.

Beware when testing! If the classpath contains a class file (say from a
prior successful build when the returnMe method was changed to return an
object), the compile will succeed.  Always clear out the
clojure.compile.path prior to compiling.

;badgenclass.clj
(ns badgenclass
  (:gen-class
     :state state
     :methods
     [[returnMe [] badgenclass]]
     :init init))
(defn -init []
  [[] nil])

(defn -returnMe [this]
  this)

#!/bin/sh
rm -rf classes
mkdir classes
java -cp lib/clojure.jar:classes:. -Dclojure.compile.path=classes \
clojure.lang.Compile badgenclass


Comment 1 by chouser, Mar 07, 2009

Attached is a patch that accepts strings or symbols for parameter and return class
names, and generates the appropriate bytecode without calling Class/forName.  It
fixes this issue, but because 'ns' doesn't resolve :gen-class's arguments, class
names aren't checked as early anymore.  :gen-class-created classes with invalid
parameter or return types can even be instantiated, and no error will be reported
until the broken method is called.

One possible alternative would be to call Class/forName on any symbols given, but
allow strings to use the method given by this patch.  To return your own type, you'd
need a method defined like:

  [returnMe [] "badgenclass"]

Any thoughts?


 Comments   
Comment by Assembla Importer [ 28/Sep/10 5:09 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/84
Attachments:
genclass-allow-unresolved-classname.patch - https://www.assembla.com/spaces/clojure/documents/cWS6Aww30r3RbzeJe5afGb/download/cWS6Aww30r3RbzeJe5afGb

Comment by Assembla Importer [ 28/Sep/10 5:09 PM ]

oranenj said: [file:cWS6Aww30r3RbzeJe5afGb]: on comment 1

Comment by Assembla Importer [ 28/Sep/10 5:09 PM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

Comment by Stuart Halloway [ 30/Oct/10 11:58 AM ]

The approach take in the initial patch (delaying resolution of symbols into classes) is fine: gen-class makes no promise about when this happens, and the dynamic approach feels more consistent with Clojure. I think the proposed (but not implemented) use of string/symbol to control when class names are resolved is a bad idea: magical and not implied by the types.

Needed:

  • update the patch to apply cleanly
  • consider whether totype could live in clojure.reflect.java. (Beware load order dependencies.)
Comment by Chouser [ 30/Oct/10 9:29 PM ]

Wow, 18-month-old patch, back to haunt me for Halloway'een

So what does it mean that the assignee is Rich, but it's waiting on me?

Comment by Stuart Halloway [ 01/Nov/10 9:17 AM ]

I am using Approval = Incomplete plus Waiting On = Someone to let submitters know that there is feedback waiting, and that they can move the ticket forward by acting on it. The distinction is opportunity to contribute (something has been provided to let you move forward) vs. expectation of contribution.





[CLJ-69] GC Issue 66: Add "keyset" to Clojure; make .keySet for APersistentMap return IPersistentSet Created: 17/Jun/09  Updated: 27/Jul/13

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

Type: Enhancement Priority: Minor
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Approval: Vetted

 Description   
Reported by wolfe.a.jason, Feb 04, 2009

Describe the feature/change.

Add "keyset" to Clojure; make .keySet for APersistentMap return an
IPersistentSet

Was this discussed on the group? If so, please provide a link to the
discussion:

http://groups.google.com/group/clojure/browse_thread/thread/66e708e477ae992f/ff3d8d588068b60e?hl=en#ff3d8d588068b60e

-----------------------------------------------------

A patch is attached.  Some notes:

I chose to add a "keyset" function, rather than change the existing "keys",
so as to avoid breaking anything.

The corresponding RT.keyset function just calls .keySet on the argument.
I would have liked to have "keyset" return an IPersistentSet even when
passed a (non-Clojure) java.util.Map, but this seems impossible to do in
sublinear time because of essentially the same limitation mentioned in the
above thread (the Map interface does not support getKey() or entryAt()) --
assuming, again, that "get" is supposed to return the actual (identical?)
key in a set, and not just an .equal key.

I then changed the implementation of .keySet for APersistentMap to
essentially copy APersistentSet.  A more concise alternative would have
been to extend APersistentSet and override the .get method, but that made
me a bit nervous (since if APeristentSet changed this could break). 

Anyway, this is my first patch for the Java side of Clojure, and I'm not
yet solid on the conventions and aesthetics, so
comments/questions/criticisms/requests for revisions are very welcome.


 Comments   
Comment by Assembla Importer [ 28/Sep/10 7:00 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/69
Attachments:
keyset.patch - https://www.assembla.com/spaces/clojure/documents/dKgE6mw3Gr3O2PeJe5afGb/download/dKgE6mw3Gr3O2PeJe5afGb

Comment by Assembla Importer [ 28/Sep/10 7:00 AM ]

oranenj said: [file:dKgE6mw3Gr3O2PeJe5afGb]

Comment by Assembla Importer [ 28/Sep/10 7:00 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

Comment by Stuart Halloway [ 03/Dec/10 1:12 PM ]

patch not in correct format





[CLJ-326] add :as-of option to refer Created: 30/Apr/10  Updated: 26/Jul/13

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

Type: Enhancement Priority: Minor
Reporter: Christophe Grand Assignee: Christophe Grand
Resolution: Unresolved Votes: 0
Labels: None

Approval: Vetted

 Description   

Discussed here: http://groups.google.com/group/clojure-dev/msg/74af612909dcbe56

:as-of allows library authors to specify a known subset of vars to refer from clojure (or any other library which would use :added metadata).

(ns foo (:refer-clojure :as-of "1.1")) is equivalent to (ns foo (:refer-clojure :only [public-documented-vars-which-already-existed-in-1.1]))



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

Converted from http://www.assembla.com/spaces/clojure/tickets/326
Attachments:
add-as-of-to-refer.patch - https://www.assembla.com/spaces/clojure/documents/a8SumUvcOr37SmeJe5cbLA/download/a8SumUvcOr37SmeJe5cbLA

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

cgrand said: [file:a8SumUvcOr37SmeJe5cbLA]: requires application of #325

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

richhickey said: Do we still need this?





[CLJ-348] reify allows use of qualified name as method parameter Created: 13/May/10  Updated: 26/Jul/13

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

Type: Defect Priority: Minor
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Approval: Vetted

 Description   

This should complain about using a fully-qualified name as a parameter:

(defmacro lookup []
`(reify clojure.lang.ILookup
(valAt [_ key])))

Instead it simply ignores that parameter in the method body in favour of clojure.core/key.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 8:03 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/348
Attachments:
0001-Add-a-test-for-348-reify-shouldn-t-accept-qualified-.patch - https://www.assembla.com/spaces/clojure/documents/d2xUJIxTyr36fseJe5cbLA/download/d2xUJIxTyr36fseJe5cbLA

Comment by Assembla Importer [ 24/Aug/10 8:03 AM ]

technomancy said: [file:d2xUJIxTyr36fseJe5cbLA]: A test to expose the unwanted behaviour

Comment by Assembla Importer [ 24/Aug/10 8:03 AM ]

richhickey said: I'm not sure the bug is what you say it is, or the resolution should be what you suggest. The true problem is the resolution of key when qualified. Another possibility is to ignore the qualifier there.

Comment by Assembla Importer [ 24/Aug/10 8:03 AM ]

technomancy said: Interesting. So it's not appropriate to require auto-gensym here? Why are the rules different for reify methods vs proxy methods?

> (defmacro lookup []
`(proxy [clojure.lang.ILookup] []
(valAt [key] key)))
> (lookup)

Can't use qualified name as parameter: clojure.core/key
[Thrown class java.lang.Exception]





[CLJ-1104] Concurrent with-redefs do not unwind properly, leaving a var permanently changed Created: 07/Nov/12  Updated: 26/Jul/13

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

Type: Enhancement Priority: Minor
Reporter: Jason Wolfe Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Mac OS, Java 6


Attachments: Text File clj-1104-doc-unsafety-of-concurrent-with-redefs-v1.txt    
Patch: Code
Approval: Vetted

 Description   

On 1.4 and latest master:

user> (defn ten [] 10)
#'user/ten
user> (doall (pmap #(with-redefs [ten (fn [] %)] (ten)) (range 20 100)))
(20 21 22 23 24 25 34 27 28 29 30 31 32 33 34 35 36 37 38 39 39 35 42 43 44 45 48 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 79 87 88 89 90 91 92 93 94 95 97 92 98 99)
user> (ten)
79

Not sure if this is a bug per se, but the doc doesn't mention lack of concurrency safety and my expectation was that the original value would always be restored after any (arbitrarily interleaved) sequence of with-redefs calls.



 Comments   
Comment by Tim McCormack [ 07/Nov/12 8:50 PM ]

The with-redefs doc (v1.4.0) says "These temporary changes will be visible in all threads." That sounds non-thread-safe to me.

In general, changes to var root bindings are not thread safe.

Comment by Herwig Hochleitner [ 08/Nov/12 9:17 AM ]

As I understand it, with-redefs is mainly used in test suites to mock out vars. It was introduced when vars became static by default and a lot of testsuites had been using binding for mocking. Maybe the docstring should be amended with something along the lines of: When using this you have to ensure that only a single thread is interacting with redef'd vars.

Comment by Stuart Halloway [ 25/Nov/12 6:41 PM ]

Behavior find as is, doc string change would be fine.

Comment by Andy Fingerhut [ 25/Nov/12 6:57 PM ]

Patch clj-1104-doc-unsafety-of-concurrent-with-redefs-v1.txt dated Nov 25 2012 updates doc string of with-redefs to make it clear that concurrent use is unsafe.





[CLJ-731] Create macro to variadically unroll a combinator function definition Created: 26/Jan/11  Updated: 26/Jul/13

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

Type: Enhancement Priority: Minor
Reporter: Fogus Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Approval: Vetted

 Description   

Clojure contains a set of combinators that are implemented in a similar, but slightly different way. That is, they are implemented as a complete set of variadic overloads on both the call-side and also on the functions that they return. Visually, they all tend to look something like:

(defn foo
  ([f]
     (fn
       ([] do stuff...)
       ([x] do stuff...)
       ([x y] do stuff...)
       ([x y z] do stuff...)
       ([x y z & args] do variadic stuff...)))
  ([f1 f2]
     (fn
       ([] do stuff...)
       ([x] do stuff...)
       ([x y] do stuff...)
       ([x y z] do stuff...)
       ([x y z & args] do variadic stuff...)))
  ([f1 f2 f3]
     (fn
       ([] do stuff...)
       ([x] do stuff...)
       ([x y] do stuff...)
       ([x y z] do stuff...)
       ([x y z & args] do variadic stuff...)))
  ([f1 f2 f3 & fs]
     (fn
       ([] do stuff...)
       ([x] do stuff...)
       ([x y] do stuff...)
       ([x y z] do stuff...)
       ([x y z & args] do variadic stuff...))))

To build this type of function for each combinator is tedious and error-prone.

There must be a way to implement a macro that takes a "specification" of a combinator including:

1. name
2. docstring
3. do stuff template
4. do variadic stuff template

And builds something like the function foo above. This macro should be able to implement the current batch of combinators (assuming that http://dev.clojure.org/jira/browse/CLJ-730 is completed first for the sake of verification).



 Comments   
Comment by Stuart Halloway [ 28/Jan/11 9:03 AM ]

This seems useful. Rich, would you accept a patch?

Comment by Stuart Halloway [ 28/Jan/11 9:40 AM ]

Nevermind, just saw that Rich already suggested this on the dev list. Patch away.





Generated at Tue Sep 27 15:51:10 CDT 2016 using JIRA 4.4#649-r158309.