ClojureScript

Instrumented self-calling multi-arity fn throws maximum call stack exceeded with optimizations advanced

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Blocker Blocker
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Accepted

Description

Multi-arity function cannot be instrumented and then run with optimizations advanced.

The self-call from arity-0 to another one is giving problems.
The self-call from another arity to arity-0 is also giving problems.

Repro:

(ns repro
  (:require [clojure.spec.alpha :as s]
            [clojure.spec.test.alpha :as stest]))

(defn foo
  ([] (foo 0))
  ([a] (foo a 1))
  ([a b] [a b]))

(s/fdef foo
  :args (s/cat :a (s/? number?)
               :b (s/? number?)))

(defn -main [& args]
  (stest/instrument)
  (foo 1)
  (println "(foo 1) worked")
  (foo 1 2)
  (println "(foo 1 2) worked")
  (foo)
  (println "(foo) worked"))

(set! *main-cli-fn* -main)

Compile, run and output with advanced:

clj -Srepro -Sdeps '{:deps {org.clojure/clojurescript {:mvn/version "1.10.439"} org.clojure/test.check {:mvn/version "RELEASE"}}}' -m cljs.main -t nodejs -O advanced -co '{:pseudo-names true :pretty-print true}' -c repro
node out/main.js
(foo 1) worked
(foo 1 2) worked
/private/tmp/repro/out/main.js:2477
function $cljs$core$seq$$($coll$jscomp$37$$) {
                         ^

RangeError: Maximum call stack size exceeded
    at $cljs$core$seq$$ (/private/tmp/repro/out/main.js:2477:26)
    at $cljs$core$apply_to$$ (/private/tmp/repro/out/main.js:4916:26)
    at $cljs$core$apply$cljs$0core$0IFn$0_invoke$0arity$02$$ (/private/tmp/repro/out/main.js:5147:38)
    at Function.$cljs$core$IFn$_invoke$arity$0$ (/private/tmp/repro/out/main.js:19504:16)
    at $cljs$core$apply_to$$ (/private/tmp/repro/out/main.js:4918:78)
    at $cljs$core$apply$cljs$0core$0IFn$0_invoke$0arity$02$$ (/private/tmp/repro/out/main.js:5147:38)
    at Function.$cljs$core$IFn$_invoke$arity$0$ (/private/tmp/repro/out/main.js:19504:16)
    at $cljs$core$apply_to$$ (/private/tmp/repro/out/main.js:4918:78)
    at $cljs$core$apply$cljs$0core$0IFn$0_invoke$0arity$02$$ (/private/tmp/repro/out/main.js:5147:38)
    at Function.$cljs$core$IFn$_invoke$arity$0$ (/private/tmp/repro/out/main.js:19504:16)

Activity

Hide
Michiel Borkent added a comment -

Updated the repro with more general repro.

Show
Michiel Borkent added a comment - Updated the repro with more general repro.
Hide
Michiel Borkent added a comment -

Narrowed it down the the call from 0-arity to 1-arity.

Show
Michiel Borkent added a comment - Narrowed it down the the call from 0-arity to 1-arity.
Hide
Michiel Borkent added a comment -

The self-call from arity-0 to another one is giving problems.
The self-call from another arity to arity-0 is also giving problems.

Show
Michiel Borkent added a comment - The self-call from arity-0 to another one is giving problems. The self-call from another arity to arity-0 is also giving problems.
Hide
Michiel Borkent added a comment - - edited

From Slack:

dnolen [4:42 PM]
I think we should rewind here a bit - after looking at the code - this is what I think
1) `ret` is for when we don't have arity info - just in case we get invoked via `.call` for some reason - it checks then defers to `f`
2) the individual arity methods on `ret` are only for advanced compilation - to provide a matching interface, but currently we call `apply` (slow) and then re-enter `ret` (loop) (edited)
3) instead of doing 2) we could do 1) - do not re-enter `ret` instead just check and invoke `f` (since we have arity info we can skip `apply`)
the only issue I see is code size, but we can fix that after we get it right
i.e. only produce dispatch cases for the ones we actually need, share the checking fn

mfikes [4:48 PM]
I agree. For step 3, it sounds like you are suggesting inlining (or perhaps making a call to shared) checking code in each static dispatch, and then after that, perhaps doing the same static dispatch on `f`. Critically, avoiding any call to `apply` which could lead to infinite loops (and generally difficult-to-reason-about logic, given that `apply` is another function altogether.)

dnolen [4:49 PM]
yep

and making it clear that path is always the same
one check then invoke the original

mfikes [4:52 PM]
Right here is the critical piece that would change. https://github.com/clojure/clojurescript/blob/7d3b94de959cafc3e56be5b869c57977119c51f3/src/main/cljs/cljs/spec/test/alpha.cljc#L297

mfikes [4:57 PM]
If we did this, this might be the only remaining use of `apply`, the 2nd line here. Maybe there is some way to avoid it as well and just use the 1st line unconditionally if we have all of the static dispatch cases covered. (This is not completely clear to me yet, but avoiding that `apply` as well would be cool, if we could pull it off.)
https://github.com/clojure/clojurescript/blob/7d3b94de959cafc3e56be5b869c57977119c51f3/src/main/cljs/cljs/spec/test/alpha.cljs#L113-L114

dnolen [4:58 PM]
@mfikes can't we invoke `applyTo`

mfikes [4:58 PM]
Oh, there is another one there: https://github.com/clojure/clojurescript/blob/7d3b94de959cafc3e56be5b869c57977119c51f3/src/main/cljs/cljs/spec/test/alpha.cljs#L127

Yeah, I suspect we are closer to killing this problematic bit of code :slightly_smiling_face:

The main theme appears to involve: Direct, simple logic, even if it leads to bloat, avoiding any reliance on `apply` if possible.

borkdude [5:01 PM]
this might also solve the (unimportant) issue of being unable to instrument `apply`

dnolen [5:01 PM]
yep

Show
Michiel Borkent added a comment - - edited From Slack: dnolen [4:42 PM] I think we should rewind here a bit - after looking at the code - this is what I think 1) `ret` is for when we don't have arity info - just in case we get invoked via `.call` for some reason - it checks then defers to `f` 2) the individual arity methods on `ret` are only for advanced compilation - to provide a matching interface, but currently we call `apply` (slow) and then re-enter `ret` (loop) (edited) 3) instead of doing 2) we could do 1) - do not re-enter `ret` instead just check and invoke `f` (since we have arity info we can skip `apply`) the only issue I see is code size, but we can fix that after we get it right i.e. only produce dispatch cases for the ones we actually need, share the checking fn mfikes [4:48 PM] I agree. For step 3, it sounds like you are suggesting inlining (or perhaps making a call to shared) checking code in each static dispatch, and then after that, perhaps doing the same static dispatch on `f`. Critically, avoiding any call to `apply` which could lead to infinite loops (and generally difficult-to-reason-about logic, given that `apply` is another function altogether.) dnolen [4:49 PM] yep and making it clear that path is always the same one check then invoke the original mfikes [4:52 PM] Right here is the critical piece that would change. https://github.com/clojure/clojurescript/blob/7d3b94de959cafc3e56be5b869c57977119c51f3/src/main/cljs/cljs/spec/test/alpha.cljc#L297 mfikes [4:57 PM] If we did this, this might be the only remaining use of `apply`, the 2nd line here. Maybe there is some way to avoid it as well and just use the 1st line unconditionally if we have all of the static dispatch cases covered. (This is not completely clear to me yet, but avoiding that `apply` as well would be cool, if we could pull it off.) https://github.com/clojure/clojurescript/blob/7d3b94de959cafc3e56be5b869c57977119c51f3/src/main/cljs/cljs/spec/test/alpha.cljs#L113-L114 dnolen [4:58 PM] @mfikes can't we invoke `applyTo` mfikes [4:58 PM] Oh, there is another one there: https://github.com/clojure/clojurescript/blob/7d3b94de959cafc3e56be5b869c57977119c51f3/src/main/cljs/cljs/spec/test/alpha.cljs#L127 Yeah, I suspect we are closer to killing this problematic bit of code :slightly_smiling_face: The main theme appears to involve: Direct, simple logic, even if it leads to bloat, avoiding any reliance on `apply` if possible. borkdude [5:01 PM] this might also solve the (unimportant) issue of being unable to instrument `apply` dnolen [5:01 PM] yep
Hide
Michiel Borkent added a comment -

Patch attached.

Show
Michiel Borkent added a comment - Patch attached.
Hide
Mike Fikes added a comment -

CLJS-2995.patch passes CI and Canary

Show
Mike Fikes added a comment - CLJS-2995.patch passes CI and Canary
Hide
Mike Fikes added a comment -

CLJS-2995.patch in patch-tender

Show
Mike Fikes added a comment - CLJS-2995.patch in patch-tender

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: