<< Back to previous view

[CLJ-2192] When data fails to conform to `map-of` spec, `:in` path does not point to the invalid (inner) value Created: 27/Jun/17  Updated: 21/Sep/17

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

Type: Defect Priority: Major
Reporter: Ben Brinckerhoff Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

org.clojure/clojure "1.9.0-alpha17", org.clojure/clojurescript "1.9.542", org.clojure/core.specs.alpha "0.1.10"



 Description   

Repro:

(require '[clojure.spec.alpha :as s]
(s/def :foo/user-map (s/map-of string? int?))
(s/explain-data :foo/user-map {"hi" "foo"})
;; Actual value:
;; #:cljs.spec.alpha{:problems
;;                 ({:path [1],
;;                   :pred int?,
;;                   :val "foo", 
;;                   :via [:foo/user-map], 
;;                   :in ["hi" 1]})}
;; Expected: the `:in` value to be ["hi"] ? 
(s/explain-data :foo/user-map {:hi 2})
;; Actual value: 
;; #:cljs.spec.alpha{:problems
;;                 ({:path [0],
;;                   :pred string?,
;;                   :val :hi, 
;;                   :via [:foo/user-map], 
;;                   :in [:hi 0]})}
;; Expected: I'm not sure, since a path can't "point to" a key

Motivation: given some top-level data (in this case, `{"hi" "foo"}`) and an `:in` path, I would like to be able to find the problematic data (in this case, `"foo"`).

In the case where the value of a map does not conform, the `:in` path is not compatible with functions like `get-in`, but it could be.

In the case where the key of a map does not conform, there is no way to "point to" a key using `get-in`, so I'm not sure what the right fix is.

I don't know that compatibility with `get-in` is a requirement: if spec provided a function that accomplished the same thing with "spec" paths (i.e. ones that could point to keys), that would be fine.



 Comments   
Comment by juan pedro monetta sanchez [ 12/Jul/17 9:58 AM ]

I think that more important than get-in compatible is a way of matching :clojure.spec.alpha/problems inside :clojure.spec.alpha/value independent of the spec that lead to the problem.
For this I think it's important to know if the problem is with the key or the value.

Currently s/map-of reports a path taking into account the map-entry vec, so 0 will be the key and 1 the value.

The problem with what I'm trying to implement is the :in is s/keys which only reports the key.

So when you see a problem in [::k 1] you don't know if it's a problem in the map value or the value is a seq and the problem is in the value at pos 1 in that seq.

Comment by Ben Brinckerhoff [ 21/Sep/17 8:56 AM ]

I agree with what is said above, and I'd like to expand on it after having more experience using the in path.

AIUI, clojure.spec is not intended to provide easy-to-read error messages. Rather, it is intended to provide a solid foundation so libraries can present errors in a variety of ways.

In my experience in trying to present errors in a different way (Expound), I can report that one of the biggest challenges in trying to interpret the in path.

There are two cases in particular that are challenging: integer indexes that indicate "key" or "value" failures in a map-of spec, and integer indexes that indicate positions in "key/value" vectors in coll-of specs.

I may be just failing to understand the way the 'in' path works, but anecdotally, this is a source of confusion for other library authors. https://groups.google.com/forum/#!topic/clojure/ppnWBJhz-R4 . Additionally, since this path is shown to users when using explain, a less ambiguous path would help all spec users better understand their errors.

Would it be possible to create unambiguous paths that do not require the reader to know anything about how the path is used? As noted above, the path [:key 1] can only be disambiguated in by knowing something about the failing data structure. This would likely require replacing the integer indexes with custom records. That would reduce conciseness and readability, but this might be able to be alleviated with new reader macros?





[CLJ-2237] Provide a type predicate for ex-info? Created: 18/Sep/17  Updated: 19/Sep/17

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

Type: Enhancement Priority: Minor
Reporter: Christian Romney Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File Add-ex-info-type-predicate.patch    
Patch: Code and Test

 Description   

A type predicate for ExceptionInfo would be as useful when writing specs as the other nice additions landing in Clojure 1.9 (e.g. nat-int?, pos-int?, etc).



 Comments   
Comment by Alex Miller [ 18/Sep/17 9:39 PM ]

It would be helpful to list an example of some code where this would come in handy (I think examples do exist in catch handling) and to give some indication of how common it is.

Comment by Christian Romney [ 19/Sep/17 7:44 AM ]

One place where it can be useful is specing data that's pulled from a channel. http://swannodette.github.io/2013/08/31/asynchronous-error-handling. David's example immediately throws the exception via the <? macro, but the fact that exception info might be put on the channel at all means it can be seen by functions we might have in a transducer. These functions can leverage spec, where this predicate would come in very handy. I'd paste code, but the example immediately at hand is proprietary. Is this a sufficiently good description, or does the case need more support?

Also, while David's example is CLJS I've used this pattern in Clojure core.async code (e.g. asynchronous Pedestal interceptors).

Comment by Christian Romney [ 19/Sep/17 12:17 PM ]

Also, from an aesthetic[1] point of view adding a type predicate (ex-info?) for a pretty important core Clojure abstraction that already has a constructor (ex-info) and an accessor (ex-data) just seems to round out / complete the API. Not sure how that compelling you or Rich might find that argument, but I figured it couldn't hurt the case.

[1] My sense of aesthetic here is colored by some of the ideas in Mitch Wand & Dan Friedman's Essentials of Programming Languages.





[CLJ-2239] Link to Google Guava API broken in clojure.java.javadoc/*remote-javadocs* Created: 19/Sep/17  Updated: 19/Sep/17

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

Type: Defect Priority: Minor
Reporter: David Bürgin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File javadoc-update-guava-url.patch    
Patch: Code
Approval: Screened

 Description   

googlecode.com is no longer in service. Thus using javadoc on Google Guava classes always results in a 404 error page.

The change proposed here updates the URL to point to the current release of Guava, release 23.0.

Unfortunately, the new URL contains the release version, so will be out-of-date soon as Guava has a higher release cadence (and there can be breakage). The alternative is to simply drop support for Guava in javadoc.



 Comments   
Comment by David Bürgin [ 19/Sep/17 12:01 PM ]

Created by recently merged CLJ-1398.

Comment by Alex Miller [ 19/Sep/17 12:04 PM ]

Ha, I think that happened since the original patch was written for 1.9. Thanks...

Comment by Alex Miller [ 19/Sep/17 12:05 PM ]

Fix url updated in 1.9-alpha20.





[CLJ-2065] reduce-kv fails on subvec Created: 20/Nov/16  Updated: 19/Sep/17

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

Type: Defect Priority: Minor
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Attachments: Text File CLJ-2065-reduce-kv-for-SubVector.patch     Text File CLJ-2065-Support-IKVReduce-on-SubVector-2.patch    
Patch: Code and Test
Approval: Triaged

 Description   

reduce-kv works as expected on vectors with the element index passed as the "key" argument. However, it fails with a subvec because clojure.lang.APersistentVector$SubVector does not implement IKVReduce.

(reduce-kv + 0 [1 2 3])
9

(reduce-kv + 0 (subvec [1 2 3] 1))
IllegalArgumentException No implementation of method: :kv-reduce of protocol: #'clojure.core.protocols/IKVReduce found for class: clojure.lang.APersistentVector$SubVector clojure.core/-cache-protocol-fn (core_deftype.clj:583)

One work around is to copy the subvec into a vector:

(reduce-kv + 0 (into [] (subvec [1 2 3] 1)))
6

Note however, the `vec` would not work here. Since Clojure 1.7, vec will return a subvec rather than copying.

(reduce-kv + 0 (vec (subvec [1 2 3] 1)))
IllegalArgumentException No implementation of method: :kv-reduce of protocol: #'clojure.core.protocols/IKVReduce found for class: clojure.lang.APersistentVector$SubVector clojure.core/-cache-protocol-fn (core_deftype.clj:583)

The Clojure user expects that a subvector supports all of the normal vector operations and this exception is confusing. The cause of the problem is that subvec returns a clojure.lang.APersistentVector$SubVector which does not implement clojure.lang.IKVReduce. PersistentVector inherits from APersistentVector and implements IKVReduce but SubVector doesn't get any IKVReduce support. This was probably just an oversight.

There are two patches attached. The first fixed the problem by extending the IKVReduce protocol in core.clj. The second, as suggested by Alex Miller, just adds the Java implementation of the IKVReduce interface directly to APersistentVector$SubVector. The same test is included in both patches.



 Comments   
Comment by Steve Miner [ 20/Nov/16 12:53 PM ]

Here is my current work-around:

(extend-type clojure.lang.APersistentVector$SubVector
  clojure.core.protocols/IKVReduce
  (kv-reduce [subv f init]
    (transduce (map-indexed vector)
               (fn ([ret] ret) ([ret [k v]] (f ret k v)))
               init
               subv)))

In my tests it was usually faster to copy the subvec into a regular vector but I like the look of the transduce fix. It would probably be faster to add a native Java implementation in APersistentVector.java. I'm willing to do the work if the Clojure/core team wants a patch.

Comment by Steve Miner [ 09/Mar/17 1:38 PM ]

Revised work-around using more interop for better performance. Comparable to the speed of normal vector reduce-kv.

(when-not (satisfies?   clojure.core.protocols/IKVReduce (subvec [1] 0))
  (extend-type clojure.lang.APersistentVector$SubVector
    clojure.core.protocols/IKVReduce
    (kv-reduce
      [subv f init]
      (let [cnt (.count subv)]
        (loop [k 0 ret init]
          (if (< k cnt)
            (let [val (.nth subv k)
                  ret (f ret k val)]
              (if (reduced? ret)
                @ret
                (recur (inc k) ret)))
            ret))))))
Comment by Steve Miner [ 18/Sep/17 10:35 AM ]

support IKVReduce for SubVector

Comment by Steve Miner [ 18/Sep/17 10:36 AM ]

Patch also adds test for reduce-kv on regular vector and subvector.

Comment by Alex Miller [ 18/Sep/17 10:48 AM ]

Why not implement IKVReduce directly in APersistentVector$SubVector?

Comment by Steve Miner [ 18/Sep/17 11:33 AM ]

Java implementation of IKVReduce on SubVector

Comment by Steve Miner [ 18/Sep/17 11:37 AM ]

Yes, that makes sense. I hesitated because I didn't want to rework the hierarchy to make SubVector a subclass of PersistentVector but that wasn't necessary to fix the bug. CLJ-2065-Support-IKVReduce-on-SubVector.patch is just the Java side, plus the same test.

Comment by Steve Miner [ 19/Sep/17 7:58 AM ]

Updated patch to inline nth() call, avoiding unnecessary bounds checking.





[CLJ-2238] NPE when print-duping Void/TYPE Created: 18/Sep/17  Updated: 18/Sep/17

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print

Attachments: Text File 0001-CLJ-2238-handle-Void-TYPE-in-print-dup.patch    
Patch: Code
Approval: Triaged

 Description   
user=> (binding [*print-dup* true] (print Void/TYPE))

NullPointerException   java.io.PrintWriter.write (PrintWriter.java:473)
#=(identity user=>

Patch: 0001-CLJ-2238-handle-Void-TYPE-in-print-dup.patch






[CLJ-1814] Make `satisfies?` as fast as a protocol method call Created: 11/Sep/15  Updated: 18/Sep/17

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

Type: Enhancement Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 10
Labels: performance, protocols

Attachments: Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me.patch     Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v2.patch     Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v3.patch     Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v4.patch     Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v5.patch     Text File CLJ-1814-v6.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Currently `satisfies?` doesn't use the same impl cache used by protocol methods, making it too slow for real world usage.

With:

(defprotocol p (f [_]))
(deftype x [])
(deftype y [])
(extend-type x p (f [_]))

Before patch:

(let [s "abc"] (bench (instance? CharSequence s))) ;; Execution time mean : 1.358360 ns
(let [x (x.)] (bench (satisfies? p x))) ;; Execution time mean : 112.649568 ns
(let [y (y.)] (bench (satisfies? p y))) ;; Execution time mean : 2.605426 µs

Cause: `satisfies?` calls `find-protocol-impl` to see whether an object implements a protocol, which checks for whether x is an instance of the protocol interface or whether x's class is one of the protocol implementations (or if its in an inheritance chain that would make this true). This check is fairly expensive and not cached.

Proposed: Extend the protocol's method impl cache to also handle (and cache) instance checks (including negative results).

After patch:

(let [x (x.)] (bench (satisfies? p x))) ;; Execution time mean : 79.321426 ns
(let [y (y.)] (bench (satisfies? p y))) ;; Execution time mean : 77.410858 ns

Patch: CLJ-1814-v6.patch



 Comments   
Comment by Michael Blume [ 11/Sep/15 4:17 PM ]

Nice. Honeysql used to spend 80-90% of its time in satisfies? calls before we refactored them out.

Comment by Michael Blume [ 24/Sep/15 3:55 PM ]

I realize this is a deeply annoying bug to reproduce, but if I clone core.match, point its Clojure dependency to 1.8.0-master-SNAPSHOT, start a REPL, connect to the REPL from vim, and reload clojure.core.match, I get

|| java.lang.Exception: namespace 'clojure.tools.analyzer.jvm.utils' not found, compiling:(clojure/tools/analyzer/jvm.clj:9:1)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5647| clojure.core$throw_if.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5733| clojure.core$load_lib.invokeStatic
|| clojure.core$load_lib.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:142)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5765| clojure.core$load_libs.invokeStatic
|| clojure.core$load_libs.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:137)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5787| clojure.core$require.invokeStatic
|| clojure.core$require.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:703)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.analyzer.jvm/0.6.5/tools.analyzer.jvm-0.6.5.jar::clojure/tools/analyzer/jvm.clj|9| clojure.tools.analyzer.jvm$eval4968$loading__5561__auto____4969.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.analyzer.jvm/0.6.5/tools.analyzer.jvm-0.6.5.jar::clojure/tools/analyzer/jvm.clj|9| clojure.tools.analyzer.jvm$eval4968.invokeStatic
|| clojure.tools.analyzer.jvm$eval4968.invoke(jvm.clj)
|| clojure.lang.Compiler.eval(Compiler.java:6934)
|| clojure.lang.Compiler.eval(Compiler.java:6923)
|| clojure.lang.Compiler.load(Compiler.java:7381)
|| clojure.lang.RT.loadResourceScript(RT.java:372)
|| clojure.lang.RT.loadResourceScript(RT.java:363)
|| clojure.lang.RT.load(RT.java:453)
|| clojure.lang.RT.load(RT.java:419)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5883| clojure.core$load$fn__5669.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5882| clojure.core$load.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5683| clojure.core$load_one.invokeStatic
|| clojure.core$load_one.invoke(core.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5728| clojure.core$load_lib$fn__5618.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5727| clojure.core$load_lib.invokeStatic
|| clojure.core$load_lib.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:142)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5765| clojure.core$load_libs.invokeStatic
|| clojure.core$load_libs.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:137)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5787| clojure.core$require.invokeStatic
|| clojure.core$require.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:457)
src/main/clojure/clojure/core/match.clj|1| clojure.core.match$eval4960$loading__5561__auto____4961.invoke
src/main/clojure/clojure/core/match.clj|1| clojure.core.match$eval4960.invokeStatic
|| clojure.core.match$eval4960.invoke(match.clj)
|| clojure.lang.Compiler.eval(Compiler.java:6934)
|| clojure.lang.Compiler.eval(Compiler.java:6923)
|| clojure.lang.Compiler.load(Compiler.java:7381)
|| clojure.lang.RT.loadResourceScript(RT.java:372)
|| clojure.lang.RT.loadResourceScript(RT.java:363)
|| clojure.lang.RT.load(RT.java:453)
|| clojure.lang.RT.load(RT.java:419)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5883| clojure.core$load$fn__5669.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5882| clojure.core$load.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5683| clojure.core$load_one.invokeStatic
|| clojure.core$load_one.invoke(core.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5728| clojure.core$load_lib$fn__5618.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5727| clojure.core$load_lib.invokeStatic
|| clojure.core$load_lib.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:142)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5765| clojure.core$load_libs.invokeStatic
|| clojure.core$load_libs.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:137)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5787| clojure.core$require.invokeStatic
|| clojure.core$require.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:421)
|| clojure.core.match$eval4949.invokeStatic(form-init2494799382238714928.clj:1)
|| clojure.core.match$eval4949.invoke(form-init2494799382238714928.clj)
|| clojure.lang.Compiler.eval(Compiler.java:6934)
|| clojure.lang.Compiler.eval(Compiler.java:6897)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|3096| clojure.core$eval.invokeStatic
|| clojure.core$eval.invoke(core.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|240| clojure.main$repl$read_eval_print__7404$fn__7407.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|240| clojure.main$repl$read_eval_print__7404.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|258| clojure.main$repl$fn__7413.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|258| clojure.main$repl.invokeStatic
|| clojure.main$repl.doInvoke(main.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:1523)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|58| clojure.tools.nrepl.middleware.interruptible_eval$evaluate$fn__637.invoke
|| clojure.lang.AFn.applyToHelper(AFn.java:152)
|| clojure.lang.AFn.applyTo(AFn.java:144)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|645| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|1874| clojure.core$with_bindings_STAR_.invokeStatic
|| clojure.core$with_bindings_STAR_.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:425)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|56| clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invokeStatic
|| clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invoke(interruptible_eval.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|191| clojure.tools.nrepl.middleware.interruptible_eval$interruptible_eval$fn__679$fn__682.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|159| clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__674.invoke
|| clojure.lang.AFn.run(AFn.java:22)
|| java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
|| java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
|| java.lang.Thread.run(Thread.java:745)

Same thing with reloading a namespace in my own project which depends on clojure.core.match

Comment by Nicola Mometto [ 24/Sep/15 3:59 PM ]

is it possible that AOT is involved?

Comment by Michael Blume [ 24/Sep/15 5:31 PM ]

Narrowed it down a little, if I check out tools.analyzer.jvm, open a REPL, and do (require 'clojure.tools.analyzer.jvm.utils) I get

|| java.lang.ClassCastException: java.lang.Class cannot be cast to clojure.asm.Type, compiling:(utils.clj:260:13)
|| clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3642)
|| clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3636)
|| clojure.lang.Compiler$DefExpr.eval(Compiler.java:450)
|| clojure.lang.Compiler.eval(Compiler.java:6939)
|| clojure.lang.Compiler.load(Compiler.java:7381)
|| clojure.lang.RT.loadResourceScript(RT.java:372)
|| clojure.lang.RT.loadResourceScript(RT.java:363)
|| clojure.lang.RT.load(RT.java:453)
|| clojure.lang.RT.load(RT.java:419)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5883| clojure.core$load$fn__5669.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5882| clojure.core$load.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5683| clojure.core$load_one.invokeStatic
|| clojure.core$load_one.invoke(core.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5728| clojure.core$load_lib$fn__5618.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5727| clojure.core$load_lib.invokeStatic
|| clojure.core$load_lib.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:142)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5765| clojure.core$load_libs.invokeStatic
|| clojure.core$load_libs.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:137)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5787| clojure.core$require.invokeStatic
|| clojure.core$require.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:421)
|| clojure.tools.analyzer.jvm.utils$eval4392.invokeStatic(form-init8663423518975891793.clj:1)
|| clojure.tools.analyzer.jvm.utils$eval4392.invoke(form-init8663423518975891793.clj)
|| clojure.lang.Compiler.eval(Compiler.java:6934)
|| clojure.lang.Compiler.eval(Compiler.java:6897)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|3096| clojure.core$eval.invokeStatic
|| clojure.core$eval.invoke(core.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|240| clojure.main$repl$read_eval_print__7404$fn__7407.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|240| clojure.main$repl$read_eval_print__7404.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|258| clojure.main$repl$fn__7413.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|258| clojure.main$repl.invokeStatic
|| clojure.main$repl.doInvoke(main.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:1523)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|58| clojure.tools.nrepl.middleware.interruptible_eval$evaluate$fn__637.invoke
|| clojure.lang.AFn.applyToHelper(AFn.java:152)
|| clojure.lang.AFn.applyTo(AFn.java:144)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|645| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|1874| clojure.core$with_bindings_STAR_.invokeStatic
|| clojure.core$with_bindings_STAR_.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:425)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|56| clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invokeStatic
|| clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invoke(interruptible_eval.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|191| clojure.tools.nrepl.middleware.interruptible_eval$interruptible_eval$fn__679$fn__682.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|159| clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__674.invoke
|| clojure.lang.AFn.run(AFn.java:22)
|| java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
|| java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
|| java.lang.Thread.run(Thread.java:745)

I don't see where AOT would be involved?

Comment by Nicola Mometto [ 27/Sep/15 2:28 PM ]

Michael Blume The updated patch should fix the issue you reported

Comment by Michael Blume [ 28/Sep/15 12:39 PM ]

Cool, thanks =)

New patch no longer deletes MethodImplCache, which is not used – is that deliberate?

Comment by Alex Miller [ 02/Nov/15 3:08 PM ]

It would be cool if there was a bulleted list of the things changed in the patch in the description. For example: "Renamed MethodImplCache to ImplCache", etc. That helps makes it easier to review.

Comment by Nicola Mometto [ 02/Nov/15 3:35 PM ]

Attached is an updated patch that doesn't replace MethodImplCache with ImplCache but simply reuses MethodImplCache, reducing the impact of this patch and making it easier (and safer) to review.

Comment by Alex Miller [ 07/Jun/16 11:42 AM ]

Bumping priority as this is used in new inst? predicate - see https://github.com/clojure/clojure/commit/58227c5de080110cb2ce5bc9f987d995a911b13e

Comment by Alex Miller [ 29/Jun/17 2:31 PM ]

I ran the before and after tests with the v3 patch. Before times matched pretty closely but I could not replicate the after results. I got this which is actually much worse in the not found case:

(let [x (x.)] (bench (satisfies? p x))) ;; Execution time mean : 76.833504 ns
(let [y (y.)] (bench (satisfies? p y))) ;; Execution time mean : 20.570007 µs
Comment by Nicola Mometto [ 29/Jun/17 4:09 PM ]

v4 patch fixes the regression on the not-found case, not sure how that happened, apologies.
Here are the timings I'm getting now:

clojure master:

user=> (let [x (x.)] (bench (satisfies? p x)))
Evaluation count : 604961580 in 60 samples of 10082693 calls.
             Execution time mean : 112.649568 ns
    Execution time std-deviation : 12.216782 ns
   Execution time lower quantile : 99.299203 ns ( 2.5%)
   Execution time upper quantile : 144.265205 ns (97.5%)
                   Overhead used : 1.898271 ns

Found 3 outliers in 60 samples (5.0000 %)
	low-severe	 2 (3.3333 %)
	low-mild	 1 (1.6667 %)
 Variance from outliers : 73.7545 % Variance is severely inflated by outliers
nil
user=> (let [y (y.)] (bench (satisfies? p y)))
Evaluation count : 22676100 in 60 samples of 377935 calls.
             Execution time mean : 2.605426 µs
    Execution time std-deviation : 141.100070 ns
   Execution time lower quantile : 2.487234 µs ( 2.5%)
   Execution time upper quantile : 2.873045 µs (97.5%)
                   Overhead used : 1.898271 ns

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

master + v4:

user=> (let [x (x.)] (bench (satisfies? p x)))
Evaluation count : 731759100 in 60 samples of 12195985 calls.
             Execution time mean : 79.321426 ns
    Execution time std-deviation : 3.959245 ns
   Execution time lower quantile : 75.365187 ns ( 2.5%)
   Execution time upper quantile : 87.986479 ns (97.5%)
                   Overhead used : 1.905711 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 35.2614 % Variance is moderately inflated by outliers
nil
user=> (let [y (y.)] (bench (satisfies? p y)))
Evaluation count : 771220140 in 60 samples of 12853669 calls.
             Execution time mean : 77.410858 ns
    Execution time std-deviation : 1.407926 ns
   Execution time lower quantile : 75.852530 ns ( 2.5%)
   Execution time upper quantile : 80.759226 ns (97.5%)
                   Overhead used : 1.897646 ns

Found 4 outliers in 60 samples (6.6667 %)
	low-severe	 3 (5.0000 %)
	low-mild	 1 (1.6667 %)
 Variance from outliers : 7.7866 % Variance is slightly inflated by outliers

So to summarize:
master found = 112ns
master not-found = 2.6us

master+v4 found = 79ns
master+v4 not-found = 77ns

Comment by Michael Blume [ 14/Jul/17 5:12 PM ]

For records that have been declared with an implementation of a particular protocol, and which therefore implement the corresponding interface, would it make sense to use an (instance?) check against that interface as a fast path?

Comment by Alex Miller [ 04/Aug/17 11:28 AM ]

Michael - that check is already in there

Nicola - I have a few comments/questions:

1. I don't get what the purpose of the NIL stuff is - could you explain that?
2. In the case where x is an instance of the interface, the old code returned x and the new code in find-protocol-impl* returns interface. Why the change?
3. This: (alter-var-root (:var protocol) assoc :impl-cache (expand-method-impl-cache cache c impl)) is not thread-safe afaict - I think simultaneous misses in two different threads for different impls would cause the cache to only have one of them. This is probably unlikely, and probably not a big deal since the cache will just be updated on the next call (not give a wrong answer), but wanted to mention it. I don't see any easy way to avoid it without a lot of changes.

Comment by Nicola Mometto [ 04/Aug/17 6:14 PM ]

Alex, thanks for looking at this,
1- The NIL object is a placeholder in the method impl cache for nil, as `find-and-cache-protocol-impl` tests for `nil?` to know if the dispatch has been cached or not

2- The change is purely a consistency one, making find-and-cache-protocol-impl always return a class/interface rather than either a class/interface or a concrete instance. Behaviourally, it doesn't change anything since the two consumers of `find-protocol-impl`, namely `find-protocol-method` and `satisfies?` don't care what that value is in that case

3- Yes you are correct that it is not thread safe, however I think it's a decent tradeoff as it doesn't cause any incorrect behaviour and at worse would cause an extra cache miss, making it thread safe would mean an extra performance penalty in every cache hit/miss

Comment by Michael Blume [ 16/Aug/17 1:44 PM ]

Nicola Mometto I'm seeing a change in behavior from this patch

(defprotocol BoolProtocol
  (proto-fn [this]))

(extend-protocol BoolProtocol
  Object
  (proto-fn [x] "Object impl")

  nil
  (proto-fn [x] "Nil impl"))

(proto-fn false)

returns "Object impl" with Clojure master and "Nil impl" with this patch

Comment by Nicola Mometto [ 17/Aug/17 3:08 AM ]

That's not good, I'll take a look later today, thanks

Comment by Nicola Mometto [ 18/Aug/17 3:29 AM ]

This was an issue with how `nil` was cached, I decided to special-case `nil` to skip the method cache, removing the need for all the `NIL` funny business and fixing this bad interaction between `false` and `nil`.

Comment by Michael Blume [ 18/Aug/17 1:17 PM ]

Not sure if it's in scope for this ticket, but given that this wasn't caught, there should probably be more protocol dispatch tests

Comment by Alex Miller [ 20/Aug/17 5:00 PM ]

yes, should definitely add

Comment by Michael Blume [ 21/Aug/17 12:52 PM ]

Patch with test

Comment by Michael Blume [ 21/Aug/17 12:56 PM ]

Verified that test fails with v4 patch:

[java] Testing clojure.test-clojure.protocols
     [java]
     [java] FAIL in (test-default-dispatch) (protocols.clj:695)
     [java] expected: (= "Object impl" (test-dispatch false))
     [java]   actual: (not (= "Object impl" "Nil impl"))
     [java]
     [java] FAIL in (test-default-dispatch) (protocols.clj:695)
     [java] expected: (= "Object impl" (test-dispatch true))
     [java]   actual: (not (= "Object impl" "Nil impl"))
Comment by Michael Blume [ 18/Sep/17 2:19 PM ]

Has this patch missed the 1.9 train? There was a fix we were hoping to make in HoneySQL that I'd hesitate to make with satisfies? as slow as it is.

Comment by Alex Miller [ 18/Sep/17 2:48 PM ]

Not necessarily. We don't add features after 1.9 but perf stuff like this is still possible. It's been vetted by Rich. It's in my list of stuff to screen.





[CLJ-2077] Clojure can't be loaded from the boot classpath under java 9 Created: 06/Dec/16  Updated: 18/Sep/17  Resolved: 18/Sep/17

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

Type: Defect Priority: Major
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Completed Votes: 6
Labels: java9

Attachments: Text File clj-2077-2.patch     Text File clj-2077-3.patch     Text File clj-2077-4.patch     Text File CLJ-2077.patch    
Patch: Code
Approval: Ok

 Description   

As part of the changes for the jigsaw module system in Java 9, the
java packages available to the boot classloader are now a subset of
the the full java distribution. This means that classes loaded via the
boot classloader cannot access any classes outside of that subset.

In the boot classloader only the java.base module is available. For a complete list of module/package listings see http://cr.openjdk.java.net/~mr/jigsaw/ea/module-summary.html

Clojure itself uses java.sql.Timestamp in clojure.instant to provide print-method and print-dup implementations for java.sql.Timestamp.

This can be seen with (using Clojure 1.4.0 or higher, and a early-access build
of Java 9, most recently tested with 9-ea+147):

java -Xbootclasspath/a:clojure.jar clojure.main -e "(require 'clojure.instant)"

This affects any clojure-based tool that puts itself on the boot
classpath in order to gain a startup time boost (both lein
and boot are affected currently).

Proposed: If java.sql.Timestamp is not available, do not load instant.clj or install it in the default data readers.

Patch: clj-2077-4.patch

Screener Notes: This looks correct and does not break normal usage. Should be tested in the bootclasspath scenarios people have problems with.



 Comments   
Comment by Toby Crawley [ 06/Dec/16 12:34 PM ]

More details on the underlying change that is triggering this are available at http://openjdk.java.net/jeps/261 (search for java.sql to find the relevant section).

Comment by Alex Miller [ 06/Dec/16 8:41 PM ]

Does this need to be a ticket here? Or is this really an issue for build tools?

Comment by Toby Crawley [ 08/Dec/16 4:30 PM ]

That depends on if we want using Clojure from the boot classpath to be an acceptable use case. If not, then I agree, it is just an issue for tooling.

Comment by Toby Crawley [ 09/Dec/16 2:21 PM ]

I realized today that this issue doesn't actually affect boot, since it doesn't use the bootclasspath. So lein is the only tooling I know of that is affected by this.

Comment by Ivan Pierre [ 12/Dec/16 4:59 AM ]

https://docs.oracle.com/javase/8/docs/api/java/sql/Timestamp.html
Would it be possible to use java.util.Date instead. Alas it's not possible to downcast :
Due to the differences between the Timestamp class and the java.util.Date class mentioned above, it is recommended that code not views Timestamp values generically as an instance of java.util.Date. The inheritance relationship between Timestamp and java.util.Date really denotes implementation inheritance, and not type inheritance.

Comment by Ivan Pierre [ 12/Dec/16 8:22 AM ]

The problem with Date is that it doesn't deal with nanoseconds. But Timestamp is created by a long giving the TimeDate value in milliseconds.
The use of setNano and getNano are only to store the SQL TIMESTAMP. Wouldn't it be better to deal with this value in another way?

The other way is to take just what we need from TimeStamp, and it's just a little thing, I'll try to compile with that to see if some other thing comes after...

Test code : https://gist.github.com/ivanpierre/b0ea937dac97d910a7c3c1e5774028e0

Comment by Ivan Pierre [ 12/Dec/16 1:13 PM ]

Ok, I pass to the GNU version of Timestamp. The code is neater. I mixed some of Sun's for more consistency. I dropped the string management of dates as Clojure will do it in clojure.instant.

It still works. I had a doubt...

If I type (clojure.lang.TimeStamp. 3678141) the response will be :
==> #inst "1970-01-01T01:01:18.141000000-00:00" with a nano of
141000000

But is if set nano to 1 : (.setNanos (clojure.lang.TimeStamp. 3678141) 1) the response is : #inst "1970-01-01T01:01:18.000000001-00:00"

This is correct, but it's a little disturbing to see my nice .141 disappear...

I put a fork on my GitHub. Last commit : https://github.com/ivanpierre/clojure/commit/749a0184ee7409290dad9ff353605fcaabd64f69

So, good, now pass to Leinigen...

Comment by Alex Miller [ 13/Dec/16 10:32 AM ]

I think the first question here is: do we expect that Clojure should be loadable from the bootclasspath?

While I realize this is a hack people use, my initial answer would be no (that was never a design constraint afaik). But will need to defer to Rich on that.

Comment by Ivan Pierre [ 13/Dec/16 10:41 AM ]

Well Leiningen do it, even with the lein repl. I test to see if TimeStamp is the only thing.
Alas Leinigen is not 1.9 compatible, so I have to go down to version 1.8 to make the tests. (problem of conflict between clojure.spec and hara library)
A funny thing would be to pass the whole Clojure test in bootstrap so we would know...

Comment by Ivan Pierre [ 13/Dec/16 1:06 PM ]

The java Timestamp could be directly integrated into clojure.instant as it's a new datatype. So no need to worry about copyright stuff, and integer it in a complete manner to accept SQL TIMESTAMP, and some correct protocols.
The worst is that looking across the DBs documentation doesn't help a lot and some are quite contradictory.

Comment by Phil Hagelberg [ 03/Apr/17 1:05 PM ]

> I think the first question here is: do we expect that Clojure should be loadable from the bootclasspath?

I don't know if it's the right thing for Clojure to be able to be loaded from the bootclasspath, but as an additional point of data: Leiningen takes 2.5 seconds to load on my machine from the bootclasspath on Java 8. Without the bootclasspath, it takes 3.4 seconds on Java 8 and 4.5 seconds on Java 9. (This is just for one Clojure instance, not launching a project subprocess.) So in this case it nearly doubles the time, which is consistently in the top 5 complaints about working with Clojure.

I would not be surprised if other Clojure-based tools (IDEs/editors perhaps) would want to use the bootclasspath but I don't have any data on that. I know there are people using the bootclasspath for production servers, but they probably wouldn't be as upset about adding a few seconds as people using it on their own machine.

I would be happy to write the few lines of code needed to defer all references to the inaccessible classes until runtime if it's decided that's the way to go.

Comment by Ghadi Shayban [ 03/Apr/17 3:13 PM ]

Phil - would you mind sharing your environment and testing code? I see a slowdown without bootclasspath but not nearly as dramatic as what you see. The Java 9 builds you are using probably have extra debugging enabled.

Comment by Phil Hagelberg [ 25/Apr/17 10:08 PM ]

Ghadi: I'm running on a Zulu build of Java 9 since I wasn't able to find binaries for Debian of the official OpenJDK ones. It's possible there's extra debugging overhead, but it's more likely it's just because I'm running on old hardware.

$ java -version
openjdk version "9-ea"
OpenJDK Runtime Environment (Zulu 9.0.0.10-linux64) (build 9-ea+155-pre-release-eval)
OpenJDK 64-Bit Server VM (Zulu 9.0.0.10-linux64) (build 9-ea+155, mixed mode, pre-release-eval)

Comment by Alex Miller [ 25/Apr/17 10:52 PM ]

I'm moving this into Clojure 1.9. Not implying any decisions, just so we look at it before any 1.9 release decisions.

Comment by Alex Miller [ 12/May/17 3:25 PM ]

Would like to see a patch for this that makes loading/definition of things not in java.base conditional such that this would not fail. If someone wants to work on that, would be happy to see it. If not, I will look at it eventually.

Comment by Ghadi Shayban [ 17/Aug/17 5:20 PM ]

Adding a patch that conditionally loads clojure.instant

test with:

java -Xbootclasspath/a:$PWD/target/clojure-1.9.0-master-SNAPSHOT.jar:$HOME/.m2/repository/org/clojure/spec.alpha/0.1.123/spec.alpha-0.1.123.jar:$HOME/.m2/repository/org/clojure/core.specs.alpha/0.1.10/core.specs.alpha-0.1.10.jar clojure.main
Comment by Rich Hickey [ 06/Sep/17 7:10 AM ]

Could we please get a function or macro that encapsulates the check/catch so we don't have N of these to maintain should there eventually be a better way?

Comment by Alex Miller [ 06/Sep/17 3:50 PM ]

Updated patch to encapsulate class existence check in "when-class" (and changed one existing usage of same pattern).

Comment by Alex Miller [ 08/Sep/17 10:19 AM ]

Added -3 patch that applies cleanly to latest master.

Comment by Alex Miller [ 08/Sep/17 12:51 PM ]

whoops, swapped the data readers in -3. fixed in -4.

Comment by Andrew Rosa [ 08/Sep/17 6:45 PM ]

Please correct me if I'm wrong, but as far as I can understand the issue is only related to code}}java.sql.Timestamp{{/code. The path removes the whole "instant.clj" file, but only a small portion of it is related to the problematic class.

Wouldn't be better to split it apart and conditionally load only the code}}java.sql.Timestamp{{/code portion of the code, like is already done with 1.8's code}}java.time.Instant{{/code on "code_instant18.clj"?

Comment by Alex Miller [ 11/Sep/17 1:29 PM ]

Hey Andrew, this is possible, and I went down this road a bit, but in the end I did not find the complexity of splitting it to be worth the effort. While you can work around the constructor-side of the reader parts as Timestamp is not used by default there, the Timestamp use is squarely in the middle of print support. A tagged literal that you can read but not print is of limited use so it seemed best to just omit the entire tag.

Comment by Alex Miller [ 18/Sep/17 1:33 PM ]

Released in 1.9.0-beta1.





[CLJ-2235] Add named loop + recur-to facility for nested loops Created: 10/Sep/17  Updated: 17/Sep/17

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

Type: Feature Priority: Major
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler

Attachments: Text File 0001-CLJ-2235-implement-named-loop-recur-to.patch     Text File 0001-CLJ-2235-implement-named-loop-recur-to-separate-special-form.patch     Text File 0001-CLJ-2235-recur-to-keyword-loop-names.patch     Text File 0002-CLJ-2235-implement-recur-recur-to-as-separate-specia.patch     Text File 0002-CLJ-2235-use-keywords-as-loop-names.patch    
Approval: Triaged

 Description   

Copied from the proposal email to the Clojure/dev Google group:

https://groups.google.com/d/msg/clojure-dev/zlMGmv60MVA/beyIRTrhAgAJ

Hi,

Currently loop/recur is limited to "single-layered" loops: loop forms can occur inside other loop forms, but there is no facility for "recuring to an outer loop".

A few years ago I posted a proposal to add support for nested loops to Clojure with a proof-of-concept patch to ClojureScript with syntax and semantics that I think suffice to make nested loops feel natural while remaining a natural extension of the core loop/recur model, with the same explicit tail recursion benefits:

(loop foo [...]    ; named loop
  ...
  (loop [...]      ; intervening loop
    ...
    (loop [...]    ; innermost loop
      ...
      (recur-to foo ...)))) ; recur to the named loop;
                            ; NB. this must be in tail position
                            ; w.r.t. *all* three loops

https://groups.google.com/d/msg/clojure-dev/imtbY1uqpIc/8DWLw8Ymf4IJ
https://dev.clojure.org/display/design/Named+loops+with+recur-to
https://github.com/michalmarczyk/clojurescript/tree/recur-to
https://github.com/michalmarczyk/clojurescript/commit/feba0a078138da08d584a67e671415fc403fa093

I have now implemented a complete patch enabling the proposed feature in Clojure (the first link is to a branch based on current master, that is, the "prepare for next development iteration" commit after 1.9.0-alpha17; the second is to the current tip of this branch for future reference):

https://github.com/michalmarczyk/clojure/tree/recur-to

https://github.com/michalmarczyk/clojure/commit/212ea06d21d3b336ac35480c99170e81defaf956

I also opened a ticket in JIRA so as to have a place to post the above in the form of a patch file:

https://dev.clojure.org/jira/browse/CLJ-2235

The remainder of this email sets out the proposal in more detail, states its key properties in a somewhat rigorous form, briefly summarizes the implementation approach and discusses certain design choices made in the patch.

Overview
========

The idea is that one could write e.g.

(let [m (two-dimensional-matrix)]
  (loop iloop [i 0]           ; named loop
    (if (< i i-lim)
      (loop [j 0]             ; nested anonymous loop
        (if (< j j-lim)
          (do
            (work)
            (recur (inc j)))  ; recur to the innermost loop
          (recur-to iloop (inc i)))) ; recur to iloop
      (done))))

and, provided that each recur-to form is in tail position with respect to all its enclosing loop forms up to and including its target and the number of arguments passed to each recur-to form matches the number of loop locals of the target loop (plus one for the leading loop name argument), this should compile and behave much like nested loops in Java.

The proposed syntax is modelled on Scheme's named lets, although semantically
those are quite different - this proposal is strictly limited to expanding the loop/recur model to nested loops in a natural way. Of course named fn forms ought also to be valid recur-to targets.

Key properties of named loops and recur-to
==========================================

With the above-linked patch in place, the following rules are enforced at compilation time:

1. Each recur-to form must be in tail position with respect to all its enclosing loop forms, whether named or not, up to and including its target (which may be a named loop or fn form).

2. It is an error to specify a recur-to target which does not occur among the names of the recur-to form's enclosing loop or fn forms with respect to which it is in tail position.

3. It is not possible to recur-to across try.

4. The number of arguments passed to recur-to beyond the initial target/label argument must match the number of formal parameters of the target loop or fn form.

5. Shadowing loop names is permissible; recur-to can only target the innermost loop of the given name among those with respect to which it is in tail position. Loop locals introduced by a shadowed named loop remain visible within the shadowing loop (unless they are themselves shadowed by identically named locals).

NB. loop names are not locals. In particular, they neither shadow nor are shadowed by locals of the same name. This point merits a longer discussion; see the section on design choices at the end of this email.

The innermost loop or fn form can always be targeted using plain recur, whether it is named or not. Additionally (recur-to nil ...) is equivalent to (recur ...) (even when the innermost loop or fn form is actually named), and (loop nil [...] ...) is equivalent to (loop [...]).

Summary of the implementation approach
======================================

The patch modifies the handling of loop labels in the compiler and implements the few necessary tweaks to the loop macro.

It also introduces an optional name argument to the loop* special form. (It is optional primarily so as to avoid breaking any non-core macros that emit loop* directly.)

Finally, it renames the recur special form to recur*; recur and recur-to become macros defined in clojure.core. See the section on design choices below for alternative approaches.

Design choices
==============

1. During development, purely as a matter of convenience at that stage, I had a separate loop-as macro that accepted a name argument. I thought it reasonable to add the naming feature directly to loop, particularly since fn already takes an optional name. Still, loop-as is a valid alternative design.

2. Should it be desirable to avoid renaming the existing recur special form to recur* and reimplementing recur as a macro, a new recur-to special form could be added. (Alternatively, one could keep recur as it is while adding recur-to as a macro delegating to a new recur* special form.)

3. Should it be desirable to preserve the option of treating loop names as locals in the future, it would probably be preferable to make them shadow and be shadowed by locals now, as otherwise elevating them to the status of locals at a later point would be a breaking change. To give an example of why such a future change might be useful, if tail call elimination support arrives in a future JDK spec, one might consider whether it'd be useful to adopt a Scheme-like approach with the loop name treated as a local bound to a function with a single arglist corresponding to the loop locals of the named loop; the closure allocations this would entail could perhaps be optimized away if the local is never referenced.

It bears pointing out that if TCE support does materialize, it will enable a range of alternative designs. For example, Scheme-style named lets could then be introduced as a feature of the let macro. Thus it seems to me that it is reasonable to restrict loop/recur/recur-to to label+goto-style looping only, even in a hypothetical future with VM TCE support, and that there is no reason to afford local-like treatment to loop names; hence the current no-shadowing-interactions approach of the patch.

Cheers,
Michał



 Comments   
Comment by Alex Miller [ 11/Sep/17 1:38 PM ]

Thanks Michał, it looks like you've done a lot of good work here. I think you've just missed the window for looking at new feature stuff for 1.9 but would like to circle back in next release on this.

It seems undesirable for recur to change from special form to macro, so would probably be better to either extend the existing form or add a recur-to special form.

Did you consider other options for specifying a name, like a keyword? Keywords don't carry the expectation of lexical shadowing you have with locals so could side-step those issues maybe? Maybe they are undesirable for other reasons.

Comment by Michał Marczyk [ 17/Sep/17 8:01 PM ]

Cheers, that's good to know.

recur-to as a separate special form

That's fair enough re: changing recur. Here's a version of the patch that makes recur-to a new, separate special form. Note that it still shares the RecurExpr class in the compiler the way loop and let share LetExpr. This patch is meant to be applied on top of the previous one to make it clear what the delta is:

0002-CLJ-2235-implement-recur-recur-to-as-separate-specia.patch

https://github.com/michalmarczyk/clojure/tree/recur-to

I've also prepared a squashed version that takes the current master directly to the same state:

0001-CLJ-2235-implement-named-loop-recur-to-separate-special-form.patch

https://github.com/michalmarczyk/clojure/tree/recur-to-separate-special-form

Incidentally, in implementing this patch I had to revert a change that the original patch makes to clojure.pprint that I guess demonstrates why it's a better idea to go with a new, separate recur-to – I overlooked it when preparing the original posting, otherwise it probably would have tipped the scales for me. (The change is marked by a #_ comment that I forgot to remove in the original patch. The new squashed patch cleans it up automatically by not touching that file.)

Symbols vs keywords as loop names

I used symbols partly because fn expects the optional name argument to be a symbol if present, and so recur-to had to support symbolic names anyway (assuming here that we want to recur-to to use the same form of the recur target name that was used to introduce it, which seems reasonable); and partly just "by default", because static symbolic references generally use symbols. (clojure.core/extend uses keywords, but those aren't really static references.) Not sure if the ability to attach metadata to loop names is likely to be useful, but there's that as well.

The second part about existing usage may or may not be a major consideration, particularly since loop names are somewhat unique in that they can only be referred to by a single special form – recur-to – and are otherwise invisible in the source. This also distinguishes them from fn-introduced named recur targets which of course do double as locals.

So I suppose we could use keywords for loop names and symbols for fn names if we're happy to have metadata-less loop names. We could even allow fn forms to use keyword names if the intention is to establish a named recur target without providing a local reference to the fn instance. (That'd basically be sugar over fn + loop.)

I'll have to think some more about which approach I prefer. I still like the consistency of using symbols for recur targets everywhere (fn / loop / recur-to), but having the local/not-a-local difference be accompanied by a syntactic distinction is tempting.

In the way of some brainstorming-in-the-open, I find it interesting that by using keyword loop names now we'd keep the possibility open of adding support for symbols in the future – perhaps for those VM-TCE-based Scheme-like loop names that would provide local references to closures. Or we could make "symbol labels" a generic feature of "let-like" forms (the ones backed by LetExpr, i.e. let & loop) once TCE lands. Then we'd have consider whether it should be possible for recur-to to target such named lets… And what about plain let? Might be simpler to stick to label+goto looping in loop/recur/recur-to and Scheme-like lets supported through a separate facility (perhaps simply let), with fn something of a point of intersection of the two models (which it already is, since it does introduce recur targets even when unnamed).

As a final note, the one instance where I think the possibility of using keywords for something comparable was brought up was shorthand field access syntax – IIRC (. x :foo) / (.:foo x) was brought up as a possible syntax for what has ultimately become (. x -foo) / (.-foo x). Despite being a road not taken, I think it illustrates how one could plausibly get used to keywords/symbols in effect accessing two namespaces. (Well, in the longhand version; .:foo is technically a symbol. Using keywords for loop names would not involve affording special treatment to any class of "keyword-derived" symbols.)

In any case, I'll see about preparing a version of the patch using keywords for loop names.

Comment by Michał Marczyk [ 17/Sep/17 10:02 PM ]

Here's a first cut at a "keyword loop names" patch:

0002-CLJ-2235-use-keywords-as-loop-names.patch

This is to be applied on top of the squashed "separate special forms" patch:

0001-CLJ-2235-implement-named-loop-recur-to-separate-special-form.patch

Also attaching squashed patch for convenience:

0001-CLJ-2235-recur-to-keyword-loop-names.patch

Here's an example of the new scheme:

(let [m [[[1 2 3] [4 5 6] [7 8 9]]
                 [[10 11 12] [13 14 15] [16 17 18]]
                 [[19 20 21] [22 23 24] [25 26 27]]]]
          ((fn iloop [i ires]
             (if (== i (count m))
               ires
               (loop :jloop [j 0 jres 0]
                 (if (== j (count (get m i)))
                   (recur-to iloop (inc i) (+ ires jres))
                   (loop [k 0 kres 0]
                     (if (== k (count (get-in m [i j])))
                       (recur-to :jloop (inc j) (+ jres kres))
                       (recur (inc k) (+ kres (get-in m [i j k])))))))))
            0
            0))

Note that recur-to still uses symbols where the target is an fn form.

Also note that the approach taken in this patch has the side effect that a loop named :foo won't shadow an fn-introduced recur target named foo. If we wanted eventually to introduce non-fn-introduced recur target using symbols as names (recur-to-targetable Scheme-style let/loop forms as discussed in the previous comment), that would definitely be the way to go. If not, perhaps it's still less arbitrary than declaring that :foo shadows foo in this context?





[CLJ-1906] Clojure should make representing iterated api calls easier Created: 30/Mar/16  Updated: 15/Sep/17

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

Type: Feature Priority: Minor
Reporter: Kevin Downey Assignee: Ghadi Shayban
Resolution: Unresolved Votes: 5
Labels: None

Attachments: Text File 0001-CLJ-1906-add-ingeminate-function.patch     Text File 0001-CLJ-1906-add-unfold-function.patch     Text File 0001-CLJ-1906-transducer-enabled-iterate.patch     File CLJ-1906-seqable-reducible.diff     Text File CLJ-1906-successions.patch    

 Description   

Many apis (elasticsearch, github, s3, etc) have parts of the api
which, in usage, end up being used in an interative way. You make an
api call, and you use the result to make another api call, and so
on. This most often shows up in apis have some concept of pages of
results that you page through, and is very prevalent in http apis.

This appears to be such a common pattern that it would be great if
Clojure had in built support for it.

You may think Clojure already does have support for it, after all,
Clojure has `iterate`. In fact the docstring for `iterate`
specifically says the function you give it must be free of side
effects.

I propose adding a function `unfold` to clojure.core to support this
use case. `unfold` would return an implementation of ReduceInit. The
name `unfold` matches what would be a similar Haskell function
(https://hackage.haskell.org/package/base-4.8.2.0/docs/Data-List.html#v:unfoldr)
and also matches the name for a similar function used in some existing
Clojure libraries
(https://github.com/amalloy/useful/blob/develop/src/flatland/useful/seq.clj#L128-L147).

`unfold` in some ways looks like a combination of `take-while` and
`iterate`, except for the fact that `iterate` requires a pure
function. Another possible solution would be a version of `iterate`
that doesn't require a pure function.

It seems like given the use case I envision for `unfold`, a
non-caching reducible would be perfect. But that would leave those
that prefer seqs high and dry, so maybe at least some consideration
should be given to seqs.

Mailing list discussion is here
(https://groups.google.com/forum/#!topic/clojure-dev/89RNvkLdYc4)

A sort of dummy api you might want to interact with would look something like

(import '(java.util UUID))

(def uuids (repeatedly 1000 #(UUID/randomUUID)))

(def uuid-index
  (loop [uuids uuids
         index  {}]
    (if (seq uuids)
      (recur (rest uuids) (assoc index (first uuids) (rest uuids)))
      index)))

(defn api
  "pages through uuids, 10 at a time. a list-from of :start starts the listing"
  [list-from]
  (let [page (take 10 (if (= :start list-from)
                        uuids
                        (get uuid-index list-from)))]
    {:page page
     :next (last page)}))

given the above api, if you had an implementation of `unfold` that took a predicate that decided when to continue unfolding, a producer which given a value in a sequence produced the next value, and an initial value, you could do something like this:

(= uuids (into [] (mapcat :page) (unfold :next (comp api :next) (api :start))))

and the result would be true.

The equivilant take-while + iterate would be something like:

;; the halting condition is not strictly the same
(= uuids (into [] (mapcat :page) (take-while (comp seq :page) (iterate (comp api :next) (api :start)))))


 Comments   
Comment by Kevin Downey [ 31/Mar/16 4:21 PM ]

I made two patches, one adds unfold as discussed above, one adds ingeminate which is like iterate but without the function purity restrictions, and doesn't return a seq.

Comment by Ghadi Shayban [ 11/Apr/16 10:46 AM ]

Though syntax is less important than the semantics, may I propose the name `progression` for this? Clojure's fold is called reduce, so unfold is too much like Haskell. Other names I was considering include evolve & derivations.

Comment by Alex Miller [ 11/Apr/16 11:23 AM ]

Another option would be `productions` (reminiscent of `reductions`).

Comment by Kevin Downey [ 11/Apr/16 9:32 PM ]

productions has a nice ring to it. emanate could work too, would sort near eduction

Comment by Ghadi Shayban [ 12/Apr/16 10:08 PM ]

Adding a patch with a generator impl that is clojure.lang.{Seqable,IReduceInit}.

Generative tests assert that the seq and reduce halves are equivalent.

Tests assert basic functionality, obeying reduced, and maximal laziness of the seq impl.

Docstring has been wordsmithed and the function named `productions`.

Comment by Kevin Downey [ 18/Apr/16 3:21 PM ]

apparently unfold is part of SRFI 1: List Library in scheme land http://srfi.schemers.org/srfi-1/srfi-1.html#FoldUnfoldMap

it looks like their unfold is take-while + iterate + map

Comment by Ghadi Shayban [ 23/Apr/16 11:06 PM ]

Main differences between Scheme's impl and this proposed one:
Predicate reversed (stop? vs continue?)
Scheme has a "mapping function" to produce a different value from the current seed, Clojure doesn't (but has transducers)
Scheme has an extra optional arg to build the tail of the list

Now I'm partial to the name successions.

Comment by Michał Marczyk [ 10/May/16 11:07 AM ]

I can confirm that I found unfold quite useful in my Scheme days.

In Clojure, this general pattern can be expressed using transducers at a modest cost in keystrokes:

(def numbers (doall (range 1000)))

(defn api [list-from]
  (if list-from
    (let [page (vec
                 (take 10 (if (= :start list-from)
                            numbers
                            (drop list-from numbers))))]
      {:page page
       :next (some-> (last page) inc)})))

(= numbers
   (sequence (comp (take-while some?)
                   (mapcat :page))
             (iterate (comp api :next)
                      (api :start))))
;= true

Maybe this could be simplified with an xform-enabled version of iterate?

(defn iterate*
  ([f seed]
   (iterate f seed))
  ([xform f seed]
   (sequence xform (iterate f seed))))

(= numbers
   (iterate*
     (comp (take-while some?) (mapcat :page))
     (comp api :next)
     (api :start)))
;= true

Admittedly this takes more characters, but is quite generic and a transducer-enabled overload in iterate feels pretty natural to me. Attaching a simple patch implementing this in clojure.core/iterate – I'll look at clojure.lang.Iterate to see if it's worth implementing direct support later, unless of course nobody wants this.

Comment by Michał Marczyk [ 10/May/16 11:08 AM ]

0001-CLJ-1906-transducer-enabled-iterate.patch adds a ternary overload to iterate that delegates to the binary overload and sequence.

Comment by Ghadi Shayban [ 10/May/16 12:56 PM ]

A few unsatisfactory things about overloading {iterate}
1) iterate's docstring says {f must be free of side-effects}
2) There is boilerplate and subtlety around the terminating item. In this case the final API call is made unconditionally, leading to an extra empty/marker item that is filtered by take-while. With the current proposal, the predicate controls iteration from the inside out

Comment by Ghadi Shayban [ 06/Jun/16 8:40 AM ]

updated patch to apply cleanly to core

Comment by Ghadi Shayban [ 18/Sep/16 11:40 PM ]

I'm not sure I'm sold on this anymore, and have suggested a different approach on the mailing list https://groups.google.com/d/msg/clojure-dev/89RNvkLdYc4/PAJh8gfmDAAJ

Comment by Ghadi Shayban [ 17/Apr/17 1:40 PM ]

I have been marinating upon two generator functions that cover use-cases including the one listed above.

One of them is similar to Scheme's unfold but with some deviations more appropriate to Clojure. The other function takes a side-effecting producer and a sentinel value.

Ignore the naming and examine the semantics. https://gist.github.com/ghadishayban/902373e247e920855139902912d237f0





Generated at Fri Sep 22 04:58:06 CDT 2017 using JIRA 4.4#649-r158309.