Fork me on GitHub
#cljs-dev
<
2017-05-18
>
dnolen01:05:16

@mfikes not sure if I fully understand your point - you’re saying funky can expand w/o refering?

mfikes01:05:44

In Clojure, you need to refer both funky and add. In ClojureScript, doing the same doesn't work.

dnolen01:05:22

oh you’re just saying that eval won’t work?

dnolen01:05:33

even if you refer both macros in ClojureScript

mfikes01:05:32

A theory is that in ClojureScript (require-macros '[foo.core :refer [funky add]]), the add symbol doesn't propagate to the point where the eval is done, which makes some sense if the refer add really only affects the ClojureScript compiler environment, and doesn't really affect the Clojure macroexpansion environment.

mfikes01:05:20

The specific error you get is an inability to resolve add.

mfikes01:05:14

cljs.user=> (require-macros '[foo.core :refer [funky add]])
nil
cljs.user=> (funky (add 2 3))
clojure.lang.ExceptionInfo: java.lang.RuntimeException: Unable to resolve symbol: add in this context, compiling:(NO_SOURCE_PATH:1:8) at line 1 <cljs repl> {:file "<cljs repl>", :line 1, :column 1, :root-source-info {:source-type :fragment, :source-form (funky (add 2 3))}, :tag :cljs/analysis-error}

mfikes01:05:15

eliding a lot of the stack, this is at the bottom

Caused by: java.lang.RuntimeException: Unable to resolve symbol: add in this context
	at clojure.lang.Util.runtimeException(Util.java:221)
	at clojure.lang.Compiler.resolveIn(Compiler.java:7214)
	at clojure.lang.Compiler.resolve(Compiler.java:7158)
	at clojure.lang.Compiler.analyzeSymbol(Compiler.java:7119)
	at clojure.lang.Compiler.analyze(Compiler.java:6680)
... 160 more

dnolen01:05:50

ah hrm would need to see the full trace to understand

dnolen01:05:38

but yes it looks like Clojure wants to resolve the symbols after the result of macroexpansion

dnolen01:05:17

oh not even

mfikes01:05:05

My theory: When funky is being macroexpanded, the eval is executed (during expansion), with an environment that has no clue what add is.

dnolen01:05:22

yes I mean the stacktrace shows that

mfikes01:05:52

I just found it interesting because Clojure somehow succeeds. It made me think that it might be a corner case not handled by ClojureScript. (Either way to difficult to handle, or not even worth handling.)

dnolen01:05:46

I can’t think of an obvious way to make that work, since yes the eval happens in Clojure

dnolen01:05:53

and Clojure can’t know about ClojureScript requires

dnolen01:05:34

so probably just a limitation around how macros work in ClojureScript

dnolen01:05:35

eval seems strange in this case anyhow - it would work if you called the analyzer macroexpand I think?

dnolen01:05:29

ok, looking at the SO question I understand a bit more - but yeah that doesn’t seem like something you should try in ClojureScript

mfikes01:05:25

Odd aside: It "works" in self-host (if I replace the eval with Planck's imitation of Clojure's eval) Right, it is pushing the boundaries of what is possible. The only eval I've cared about during macroexpansion are the three in cljs.spec.test.alpha (here's one https://github.com/clojure/clojurescript/blob/master/src/main/cljs/cljs/spec/test/alpha.cljc#L111)

dnolen01:05:08

@mfikes right to me it makes sense it works in self-host

dnolen01:05:30

since the compilation env and runtime environment are in fact the same

mfikes01:05:09

Yeah. To make that work in JVM-ClojureScript would involve some mind-bending conveyance of refers from ClojureScript into Clojure.

mfikes03:05:10

Interestingly, even if the ClojureScript compiler doesn't "convey" the refers, you can manually use refer in Clojure macro definition for the SO use case and then the symbols resolve. (http://stackoverflow.com/a/44031185/4284484)

rauh07:05:30

Is https://dev.clojure.org/jira/browse/CLJS-1495 still and issue? Seems to work fine for me

rauh08:05:07

An extremely simple but kind of ugly way to speed up mapv: Add a second (unused) arity in the reducing function, thus avoiding the f.call(null...) call within the reducer:

(-> (reduce (fn
                 ([x] x)
                 ([v o] (conj! v (f o)))) (transient []) coll)
       persistent!)

rauh08:05:32

Speedup about 5x in FF and 4x in Chrome (production builds). Thoughts?

rauh08:05:14

Other candidates: filterv, replace, frequencies, into-array, find-and-cache-best-method, merge, merge-with, max/min-key, preserving-reduced (used in cat), run! (also probably fn->comparator)

thheller09:05:11

@rauh better fix the f.call issue?

rauh09:05:38

@thheller How? I'm assuming the .call(null, ..) has a purpose?

thheller09:05:46

ah wait ... :static-fns doesn't do the f.call right?

rauh09:05:30

Yes they do if there is only one arity for the function.

thheller09:05:29

sorry, was in a different context. but if adding a unused second arity fixes anything thats a bug IMHO

thheller09:05:51

there should never be work arounds like that

thheller09:05:01

do you have a full example?

rauh09:05:38

Well this is embarrassing, I messed up a number in the benchmark. It's only like 30% faster. Nowhere near as significant. 😕

dnolen13:05:08

@rauh yeah the speed diff isn’t huge, I did a lot of testing around this

rauh13:05:59

Could a compiler option be of interest that switches off emitting those .call calls? I'm pretty sure most CLJS code would "just work", no?

dnolen14:05:23

.call is how you can use non JS-fns as functions

rauh14:05:10

I thought the .call(null, ...) is just so that this is set to the global object, no?

dnolen14:05:18

@rauh I don’t think you understand what I’m saying

dnolen14:05:34

vectors, set, maps etc. are all functions … but not JS functions

dnolen14:05:01

@rauh we do implement specific invokes for IFn, but I think this would need careful testing to see that this won’t break anything - so low priority

rauh15:05:38

@dnolen Right, I see what you mean. I'm not sure the .call is ever necessary since it'll never be invoked that way (from CLJS only projects). At least the chrome coverage tool shows it's never invoked.

rauh15:05:17

I also don't understand why the IFn protocols are again added to the .call function:

(.. :a -call -cljs$core$IFn$_invoke$arity$2)

dnolen15:05:18

yes now that I think about it

dnolen15:05:43

I believe we optimized fn invokes before data structure invokes, so we needed .call

dnolen15:05:17

@rauh that just seems like a bug

dnolen15:05:38

@rauh if you want to open an ticket with details from Chrome coverage tool for a typical project that would be cool

dnolen15:05:54

it would be nice to see that this works with something non-trivial, like Re-frame or om.next

rauh15:05:28

I'll play around and write down my findings.

rauh15:05:44

Alright, killed off both .call and .apply prototype set!'ed by extend-protocol in core.cljc. Only change needed was one line in datascript:

((.-rschema db) prop)
Which emitted a .call on the reverse schema hash map. Other than that it all works fine. 15k code less, 2kb gzipped less (minified production).

rauh15:05:28

Changed it to

(let [rschema (.-rschema db)]
  (rschema property))
And it "found" the IFn protocol.

rauh15:05:49

The "weird" (.. :a -call -cljs$core$IFn$_invoke$arity$2) was just because of the multi arity fn that get's set! on the prototype.call.

rauh15:05:07

That was on a large cljs project, ~17k LOC. Rum + datascript

richiardiandrea15:05:59

@rauh did you completely remove the .call emission and still works or only for fns

rauh15:05:09

I didn't change the compiler emitting the .call. I only changed the extend-type macro emitting prototype.call + apply for anything that implements IFn's

rauh15:05:36

But those .call would fail if they were ever used on IFn, as it did for datascript.

rauh16:05:22

So my hypothesis is that this could be changed so these obj.call aren't emitted any longer.

richiardiandrea16:05:46

Oh right I read better ;)

dnolen16:05:10

@rauh right but your investigation is kind of a deal breaker

dnolen16:05:23

needing to fix DataScript means such a change would be off the table

rauh16:05:29

No, don't say that 😞

rauh16:05:44

I'll fix datascript myself 🙂

dnolen16:05:44

too much downstream damage sorry

dnolen16:05:53

you can’t test the universe

dnolen16:05:10

anyways that’s what I was afraid of above as I said

dnolen16:05:15

and you immediately ran into it

rauh16:05:16

Is a compiler option a chance?

dnolen16:05:29

but not a default

rauh16:05:41

I mean all my other code worked without any issues. And I write some really weird code 🙂

rauh16:05:02

I'm totally happy with just an option and non-default.

dnolen16:05:17

@rauh, for extra credit, warning on setting call apply would be a good transition thing

dnolen16:05:30

then we can fix this further down the road

rauh16:05:55

Now I wonder, should this option also avoid emitting the multi arity dispatcher on multi arity functions? Probably yes

rauh17:05:12

Patch attached. Not yet dealt the possible optimization of avoid emitting the multi arity dispatch fn.