Fork me on GitHub
#cljs-dev
<
2018-11-29
>
borkdude12:11:01

@dnolen would the priority of https://dev.clojure.org/jira/browse/CLJS-2995 change if it wasn’t about spec’ing range but about fns in general?

borkdude12:11:08

I updated the repro now. Discovered that it’s a more general problem

mfikes12:11:58

Spec is alpha 🙂 ¯\(ツ)

borkdude12:11:12

🙂 In general, features that work in none should probably keep working under advanced. Also, we want to keep parity with supported features in clojure spec?

thheller12:11:04

some features just can't work in :advanced. I'm honestly surprised spec instrument works at all 😉

john17:12:39

Yeah, I'm trying to figure that out right now, for other purposes.

mfikes12:11:06

Right. One recent example is the inability to get the name of a spec'd function under advanced. But that's for a good reason.

dnolen13:11:32

@borkdude not going to change the priority for now - apply appears in that stack trace

dnolen13:11:04

the issue just needs more information

borkdude13:11:51

@dnolen not sure why apply is important here?

dnolen13:11:32

@borkdude the ticket just needs more information

dnolen13:11:01

I suspect it's related to the fact that apply can't be instrumented itself, and there's some kind of loop here

dnolen13:11:17

but this is because we have to use apply internally

borkdude13:11:21

apply isn’t intrumented in this repro. only repro.foo is

dnolen13:11:33

it doesn't matter 🙂

dnolen13:11:42

it's showing up in the stack trace

dnolen13:11:52

I guarantee you this isn't happening w/o instrumenting

borkdude13:11:11

it’s also not happening without optimizations advanced 🙂 but noted…

dnolen13:11:31

everything around instrumenting is only about advanced

dnolen13:11:38

getting it to work otherwise is trivial

borkdude13:11:34

just to be clear: one of the goals of cljs is to make instrument work under advanced? if not, I won’t spend more time doing more research

dnolen13:11:53

I think that was clear in what I just said above

dnolen13:11:04

everything around instrumenting is only about advanced

borkdude13:11:23

I’m alluding to what @thheller said: > some features just can’t work in :advanced. I’m honestly surprised spec instrument works at all just making sure 🙂

dnolen13:11:29

not expecting you to do more research

dnolen13:11:12

and the report is welcome

borkdude13:11:47

I’d be happy to, but only if I’m sure this is supposed to work, and I think you confirmed this.

dnolen13:11:51

given the consistent train of issues - it may turn out that instrument is simply not practical under advanced and it has to be testing time thing only

dnolen13:11:30

but issues like this just need more info for me to spend time on it myself

borkdude13:11:56

ok. thanks. and have a nice conj!

dnolen13:11:07

@borkdude quick thing to check to narrow that ticket to the actually problem, as I was walking to get coffee - I was thinking this probably about the self-call case

dnolen13:11:19

not really only multi-arity

borkdude13:11:27

that’s correct, I already tested this

borkdude13:11:45

oh, you mean self-call in general

dnolen13:11:51

but somehow not in the issue description? 🙂

dnolen13:11:57

nor even in the issue title?

dnolen13:11:03

stuff like this is a huge time saver

dnolen13:11:13

please put all information and the most narrow title

borkdude13:11:26

well, I didn’t figure it was about self-call in general yet. I have to walk for a coffee too probably 😉

borkdude13:11:03

let me make it narrower

dnolen13:11:10

if there are additional observations, it always worth putting it in the issue

dnolen13:11:14

"first impression" etc.

mfikes13:11:23

Oh, snap. Yeah, self-call is going to be hard to intervene and instrument.

dnolen13:11:41

I think the issue is that the self-call is instrumented

dnolen13:11:52

thus the infinite loop

borkdude13:11:03

so why do we see this only with advanced?

dnolen13:11:04

instrumented -> real -> instrumented

dnolen13:11:38

because in advanced you have direct invokes and we have some annoying machinery to make this "just work"

dnolen14:11:47

probably that advanced machinery is slightly wrong

borkdude14:11:40

it turns out it only happens with a self-calling multi-arity fn. I’ll update the issue

dnolen14:11:36

@mfikes in your maybe-setup-static-dispatch I note that you modify ret and then apply ret

dnolen14:11:44

this seems like it would trigger an infinite loop in advanced

dnolen14:11:01

invoke arity 2, apply, invoke arity 2, etc.

dnolen14:11:17

perhaps you meant to apply f?

borkdude14:11:19

so: only the arity-0 -> arity-1 call throws, not the arity-1 -> arity-2 call

borkdude14:11:00

also arity-0 to arity-2 throws. so the problems seems to be with arity-0 to another one. arity-n -> arity-0 is also problematic.

mfikes14:11:10

I’ll take a look at the apply f stuff in a bit. (In a meeting.)

borkdude14:11:20

thanks for looking into it

mfikes15:11:08

@dnolen Here is what I was thinking when I wrote maybe-setup-static-dispatch: I wanted to fundamentally endow ret with all of the same static dispatch properties that f had (so that existing callsites still work). (I think our previous approach with using goog to extend things had the same goal in mind.) But, I wanted to do it “by hand” to gain more control over the dispatch logic, and in particular to make the static dispatch properties on ret match f to high fidelity. Now having said that, whenever callsites invoke on ret, I wanted any call to be routed to the variadic arity that exists on ret, because that’s where the spec-checking occurs. If a static dispatch on ret were implemented to apply directly on f, my first reaction is to wonder if spec checking might be skipped. Thinking about the possibility of infinite recursion: I was thinking that if you invoke arity 2 on ret, that implementation would do an apply on ret, and my hope was that this would then land control right in the spec-checking code. But… (I haven’t looked), perhaps apply itself then looks for the invoke arity 2 on ret and then instead of going to the variadic place where I wanted it, you end up back in the invoke arity 2 on ret and you are then in an infinite loop. This could very well be happening (I haven’t looked yet). I also still think that doing an apply on f might fix it if that’s the case, but it might also lead to skipping the intrumentation check.

dnolen15:11:09

I think we should rewind here a bit - after looking at the code - this is what I think

dnolen15:11:32

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

dnolen15:11:23

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)

dnolen15:11:35

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)

dnolen15:11:25

the only issue I see is code size, but we can fix that after we get it right

dnolen15:11:47

i.e. only produce dispatch cases for the ones we actually need, share the checking fn

mfikes15:11:54

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.)

dnolen15:11:28

and making it clear that path is always the same

dnolen15:11:34

one check then invoke the original

mfikes15:11:44

Yeah, it avoids the bits that I claim I am uncertain about above. 🙂

borkdude15:11:21

one check per (recursive) call or one only for the initial call?

borkdude15:11:58

(the first seems more correct to me)

dnolen15:11:42

what I've suggested is the first one

mfikes15:11:41

Instead of (apply ~ret ~argv) conceptually it would first do the spec check, and then delegeate directly to f

mfikes15:11:36

Implement it @borkdude 🙂

dnolen15:11:41

might also be a good time to write at least the basic test cases around fn specs - we're definitely lacking that right now

mfikes15:11:44

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

dnolen15:11:17

@mfikes can't we invoke applyTo

mfikes15:11:04

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

mfikes16:11:49

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

borkdude16:11:17

this might also solve the (unimportant) issue of being unable to instrument apply 😉

borkdude16:11:10

@mfikes I’d be happy to implement, but I would need some time to get familiar with this code. Some instructions in the issue would help.

mfikes16:11:30

Yeah, it might not hurt, given your involvement with Speculative. You’d be in a better position to see some of the subtleties of whether a new candidate implementation holds water.

borkdude21:11:48

I have a branch here: https://github.com/clojure/clojurescript/compare/master...borkdude:CLJS-2995 - [x] The CLJS tests pass. - [x] The test for CLJS-2995 issue passes - [x] The self-parity tests pass Feedback welcome! I’d like to get rid of the apply calls in spec-checking-fn, but that’s a nice bonus. Not sure how to do that yet.