Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instrumenting hash-map gives StackOverflow on JVM #264

Open
borkdude opened this issue Jan 26, 2019 · 0 comments
Open

Instrumenting hash-map gives StackOverflow on JVM #264

borkdude opened this issue Jan 26, 2019 · 0 comments

Comments

@borkdude
Copy link
Owner

borkdude commented Jan 26, 2019

The cause is probably that spec-checking-fn calls with-instrument-disabled, which calls binding which expands in a call to hash-map, so there's a loop.
See https://github.com/clojure/clojure/blob/ee3553362de9bc3bfd18d4b0b3381e3483c2a34c/src/clj/clojure/core.clj#L1947.

Repro:

$ clj -A:test -m cljs.main -re node

ClojureScript 1.10.439
cljs.user=> (require '[speculative.core])
nil
cljs.user=> (require '[clojure.spec.test.alpha :as stest])
nil
cljs.user=> (stest/instrument `hash-map)
[cljs.core/hash-map]
cljs.user=> (apply hash-map [1])
repl:13
throw e__6573__auto__;
^

Error: Call to #'cljs.core/hash-map did not conform to spec.

$ clj -A:test

Clojure 1.10.0
user=> (require '[speculative.core])
nil
user=> (require '[clojure.spec.test.alpha :as stest])
nil
user=> (stest/instrument `hash-map)
[clojure.core/hash-map]
user=> (apply hash-map [1])
Execution error (StackOverflowError) at (REPL:1).
null

A possible solution would be to replace in the binding macro:
(push-thread-bindings (hash-map ~@(var-ize bindings)))
with
(push-thread-bindings (clojure.lang.PersistentHashMap/create ~(var-ize bindings))),
i.e. inline the hash-map call.

An additional improvement would be to make the call to hash-map at macro-expansion time:

(defmacro binding
  "binding => var-symbol init-expr

  Creates new bindings for the (already-existing) vars, with the
  supplied initial values, executes the exprs in an implicit do, then
  re-establishes the bindings that existed before.  The new bindings
  are made in parallel (unlike let); all init-exprs are evaluated
  before the vars are bound to their new values."
  {:added "1.0"}
  [bindings & body]
  (assert-args
    (vector? bindings) "a vector for its binding"
    (even? (count bindings)) "an even number of forms in binding vector")
  (let [var-ize (fn [var-vals]
                  (loop [ret [] vvs (seq var-vals)]
                    (if vvs
                      (recur  (conj (conj ret `(var ~(first vvs))) (second vvs))
                             (next (next vvs)))
                      (apply hash-map (seq ret)))))]
    `(let []
       (push-thread-bindings ~(var-ize bindings))
       (try
         ~@body
         (finally
           (pop-thread-bindings))))))

Benchmark:

user=> (time (dotimes [_ 10000000] (binding [*print-length* 10])))
"Elapsed time: 3754.649969 msecs"
nil
user=> (time (dotimes [_ 10000000] (binding* [*print-length* 10])))
"Elapsed time: 1356.306343 msecs"

With this fix (hash-map 1 1) works, but (hash-map 1) doesn't yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant