<< Back to previous view

[CLJ-1499] Replace seq-based iterators with direct iterators for all non-seq collections that use SeqIterator Created: 08/Aug/14  Updated: 01/Oct/14

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

Type: Enhancement Priority: Critical
Reporter: Rich Hickey Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

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

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

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

Patch does not change use of SeqIterator in:

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

Patch: clj-1499-all.diff



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

The list of non-seqs that uses SeqIterator are:

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

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

  • ASeq
  • LazySeq.java

LazyTransformer$MultiStepper - not sure

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

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

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

PHM patch

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

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

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

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

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

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

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

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

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

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

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

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





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

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

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

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

 Description   

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

Exception in thread "main" java.io.FileNotFoundException: Could not locate a/b_c__init.class or a/b_c.clj on classpath:
...

Proposed:
Fix the bad ending colon in this sentence and add a second sentence only when the file name has an _ in it: "Please check that namespaces with dashes use underscores in the Clojure file name."

Patch: clj-1297-v3.patch

Screened by:



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

I would have saved a few hours as well.

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

This patch changes clojure.core/load such that:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

With latest:

user=> (require 'a-b)
FileNotFoundException Could not locate a_b__init.class or a_b.clj on classpath:   Perhaps a file you are attempting to load needs - chars in name replaced with _  clojure.core/load-one/fn--5135 (core.clj:5606)

That looks goofy due to the base message in RT.load(): throw new FileNotFoundException(String.format("Could not locate %s or %s on classpath: ", classfile, cljfile));

I would like to:
1) Fix that RT.load() message to not end with a colon: "Could not locate %s or %s on classpath."
2) Instead of changing load-one, just add the additional sentence in RT.load(). Second optional sentence could be: "Please check that namespaces with dashes use underscores in the Clojure file name."

Final message would then look like:

"Could not locate a_b__init.class or a_b.clj on classpath. Please check that namespaces with dashes use underscores in the Clojure file name."





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

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

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

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

 Description   

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

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

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

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

Patch: range-patch3.diff



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

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

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

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

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

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

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

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

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

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

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

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

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





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

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

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

Approval: Triaged

 Description   

See the example in this repo:

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

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

The bug seems to involve:

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





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

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

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

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

 Description   

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

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

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

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

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

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



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

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

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

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

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

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

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

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

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

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

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

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

-> #user/hinted

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

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

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

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

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

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

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

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

Attached two patches implementing two different solutions:

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

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

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

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

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

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

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

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

Andy, can you file that as a separate ticket?

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

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

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

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

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

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





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

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

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


 Description   

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

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

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



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

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

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

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

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

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

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

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

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





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

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

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

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

 Description   

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

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

Patch: clj-1537-v2.diff



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

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

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

Should these changes be deref'ing ret?

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





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

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

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

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

 Description   

AOT-compilation drops namespace metadata.

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

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



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

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

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

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

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

juergenhoetzel said: This is still a issue on

Clojure 1.2.0-master-SNAPSHOT

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

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

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

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

BTW, I verified that this still exists in 1.6.0.

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

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

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

Alex Miller:

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

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

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

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

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

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

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

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

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

Here's an explaination of why this bug happens:

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

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





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

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

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

Approval: Triaged

 Description   

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

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

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






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

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

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

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

 Description   

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

Reproduction steps:
Compare the following calls:

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

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

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

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

Patch
CLJ-1250-08-29.patch

Approach:

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

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

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

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

Adds two helpers to emitClearThis and inTailCall.

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

Alternate Approaches:

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

(defrecord Reducer [coll xf])

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

(def rreducer ->Reducer) 

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

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

2) Alternate approach

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

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

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

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



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

Fixed indentation in description.

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

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

Will upload a diff of the bytecode soon.

Any reason this juicy bug was taken off 1.6?

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

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

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

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

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

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

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

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

I agree re: another special case in the compiler.

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

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

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

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

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

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

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

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

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

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

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

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

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

Updating patch after murmur changes

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

hah, sorry for the typo on the name

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

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

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

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

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

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

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

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

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

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

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

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

Attached new squashed patch with a couple of tests.

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

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

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

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

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

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

Agreed, Ghadi that last rebase looks wrong.

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

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

Apologies that ticket is longer than War & Peace

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

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

Compiler.java

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

compilation.clj

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

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

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

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

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

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





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

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

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

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

 Description   

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

This Discussion questioned the current implentation.

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

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



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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





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

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

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

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



 Description   

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

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

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

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

Expected same behavior and hence correct output in Clojure.



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

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

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





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

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

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


 Description   

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

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

and only named classes can run off main function.

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

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

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

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

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

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

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

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

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

thank



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

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

See: http://clojure.org/repl_and_main

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

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

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

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

checkout the `-m` option





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

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

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


 Description   

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

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

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



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

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





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

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

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


 Description   

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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






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

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

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

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

 Description   

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



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

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

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

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

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

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

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

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

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

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

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

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

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

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

Hi Aaron,

Thanks for looking into this!

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

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

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

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

Clearly the semantics are preserved provided transients satisfy their contract.

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

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

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

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

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

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

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

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





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

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

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


 Description   

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

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

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






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

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

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

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

 Description   

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

Patch: 0001-atom-interface.patch

Screened by: Alex Miller

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



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

This ticket needs some work before it can be screened:

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

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





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

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

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

Approval: Vetted

 Description   

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






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

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

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

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

 Description   

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

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

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

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

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

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



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

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





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

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

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

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

 Description   

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

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

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

Approach: apply var and form meta to invokePrim expression

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



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

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

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

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

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

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

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

Also needs tests in the patch.

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

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





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

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

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

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

 Description   

Consider the following example.

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

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

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

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

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

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

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

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



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

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

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

Not challenging the premise at all but workaround:

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

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

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

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

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

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

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

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

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

Dupe of CLJ-1533





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

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

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

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


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

 Description   

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

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

Reproduce:

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

Cause:

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

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

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

Proposed:

Instead produce the equivalent of this in the static initializer:

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

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

Patch: clj-1157-v2.diff

Screened by:



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

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

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

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

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

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

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

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

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

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

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





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

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

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

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

 Description   

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

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

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

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

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

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

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

The compiler can easily avoid these by counting forms.

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



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

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

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

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

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

Newest patch also contains a few tests.

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

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

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

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

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

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





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

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

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

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

 Description   

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

Patch: clj-1313-v2.diff



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

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

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

please update to apply cleanly on master

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

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





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

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

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

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

 Description   

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

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

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

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



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

One workaround:

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

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

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

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

(iterator [] (.iterator pmap)

to the bean proxy definition?

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

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

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

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

Added new patch that just adds iterator to bean.





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

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

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

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

 Description   

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

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


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

Patch clj-1501.diff with tests added





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

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

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

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

 Description   

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

For example:

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

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

Demonstration script: demo1.clj

Patch: 0001-CLJ-1093-v3.patch

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

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

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

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

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



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

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

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

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

Attached a fix.

This also fixes AOT compiling of code like:

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

Cleaned up patch

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

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

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

Updated patch without whitespace changes

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

Thanks, that's helpful.

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

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

Some questions we would like to answer:

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

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

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

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

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

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

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

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

I can add some tests to test that

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

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

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

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

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

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

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

Summarizing some cases here from before/after the patch:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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





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

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

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

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

 Description   

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

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

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






Generated at Wed Oct 01 15:27:01 CDT 2014 using JIRA 4.4#649-r158309.