Fork me on GitHub
#cljs-dev
<
2017-05-19
>
rauh06:05:11

@thheller Didn't you have a ticket or patch removing all the declare after def's? I can't find it. (I don't mean the warning ticket, but the changes to core.cljs.)

thheller06:05:51

well actually it doesn't remove the declare anymore, it just fixes that bug so its a proper no-op

rauh06:05:46

Right, but do you have a list of changes for core.cljs? I can see atom fn is being declare'd after def

rauh06:05:50

What others?

thheller06:05:03

yes, one sec I have that written down somewhere

thheller06:05:16

deref reset! atom

thheller06:05:46

-(declare create-inode-seq create-array-node-seq reset! create-node atom deref)
+(declare create-inode-seq create-array-node-seq create-node)

thheller06:05:34

-(declare tv-editable-root tv-editable-tail TransientVector deref
+(declare tv-editable-root tv-editable-tail TransientVector

thheller06:05:18

I can give you the original patch but that includes the bad fix in src/main/clojure/cljs/analyzer.cljc

thheller06:05:31

don't know how to git only the core.cljs changes out of that

rauh06:05:11

No worries, I only wanted those changes.

dnolen13:05:43

@rauh thanks for the extensive writeup, it clarifies things but also calls into question the change even more. The variadic case is actually important and we’re not going to remove that. There’s no way to handle this case in higher order situations without using .call

dnolen13:05:04

re: CLJS-2041

rauh13:05:07

@dnolen My comment was a little confusing since I'm mixed in "removing all dispatching functions", which I is a no-go after I tried it out (`cljs.core/apply` relies on them being there). So for variadic functions it'd still all work. The given example with = would still work.

rauh13:05:17

That was the idea "now I wonder" I had yesterday, but the dispatchers would stay.

dnolen13:05:48

ok right because the fn is the dispatcher

dnolen13:05:12

so yeah I guess when we fixed IFn protocol to implement all the arities dropping .call became possible

rauh13:05:13

Right, only dispatchers for data structures are gone. But IFn can't be variadic anyways... so no issue there.

dnolen13:05:41

@rauh actually that’s not quite true

dnolen13:05:53

we just haven’t implemented it yet and we’re probably not going to close the door

dnolen13:05:06

if there’s greater than 20 args, the 21 arg case is variadic

dnolen13:05:14

same as in Clojure

dnolen13:05:48

the finally arity of the interface is a variadic case and you get all extra arguments in Java array

rauh14:05:01

Right but even that would be properly generated by the compiler then.

dnolen14:05:02

in anycase that needs more thought - and not important for dropping .call

rauh14:05:04

Let's say a crazy data structure implements a variadic 21 arg IFn, then that can just be emitted. (at the call site)

dnolen14:05:05

so yeah we’re back to where we were before which is that as far as ClojureScript is concerned .call can probably be dropped now. But it’s hard to say how many libraries know about this detail and take advantage of it.

rauh14:05:19

I think as an opt-in, people can do the switch and fix the errors. It'd also make them aware of inefficient calls that seem efficient.

dnolen14:05:29

I don’t understand your point about inefficient calls and why that matters for this ticket

dnolen14:05:42

this is just a perf enhancement for fn call overhead

rauh14:05:36

I mean going through the dispatcher for simple map lookups seems wasteful. For instance datascript calls the -attrs-by a TON during transactions/lookups.

dnolen14:05:56

ok sure but not relevant for this ticket - in general we should be a bit more focused in tickets like this - I’m reviewing your patch now

rauh14:05:39

@dnolen Ok sounds good. Note my first comment about the binding being bad: ana/*cljs-static-fns*

rauh14:05:05

I can resubmit after feedback.

dnolen14:05:42

@rauh attached patch review in comment - summary there appears to be a bug - and dropping .call code gen is not important for this ticket.

dnolen14:05:29

We can make a seperate issue IFn .call codegen

dnolen14:05:05

@rauh thanks for the work / research on this thus far - it’ll be a nice perf boost 🙂

dnolen14:05:49

@rauh book keeping thing, I don’t see your name on the CA list have you sent it in - if not please do so now

rauh14:05:51

@dnolen You're welcome. Glad I can contribute. And just a warning, there is 3-4 places in core which need "fixing" for this to work. (all multi methods)

rauh14:05:01

So in detail: (@method-table default-dispatch-val) -> (get @.... ...), and ((:ancestors h) child) -> (get ... .. )

dnolen14:05:44

@rauh no - we shouldn’t have to change code to make this work

dnolen14:05:55

that would be unnacceptable

dnolen14:05:49

I’m pretty sure we have a pass for assigning fns to a local so that inline dispatch can work

dnolen14:05:09

first thing would be to figure why it can’t apply here

rauh14:05:55

I'm not sure where I'd look for that. Right now ((:a m) :b) definitely doesn't invoke the intermediate result (:a m) with a check on IFn but just does a straight (inefficient) .call

dnolen14:05:08

@rauh parse-invoke in analyzer.cljc

dnolen14:05:48

we need an optimization pass that lifts out the fn expression and assigns the result to a local.

dnolen14:05:18

if you look at the last few expression you’ll see how we do this for trivial fn expressions (the symbol case)

dnolen14:05:58

in that case we eval the arguments first so we don’t dupe code in both branches of the invoke dispatch

dnolen14:05:55

in this case what we need is that ((exp ...) args...) gets rewritten to (let [f# exp] (f# args...))

dnolen14:05:16

this may be a tradeoff though

rauh14:05:39

Right, makes sense. But I'm not too versed with the analyzer.

dnolen14:05:45

we should re-run all benchmarks and eyeball the generated code - let can introduce closures

dnolen14:05:09

@rauh, it’s no harder than anything else you’re looking at 🙂 I would start with parse-invoke and if you have questions than I’m happy to answer them

rauh14:05:29

@dnolen Wouldn't the (not (symbol? f)) kick in and not do the else part for ((:a x) :b)?

dnolen14:05:48

@rauh there’s no optimization if (no (symbol? f))

dnolen14:05:12

the optimization is last expression in parse-invoke

dnolen14:05:33

note we analyze something the user didn’t write, but a optimized thing by rewriting their code

rauh17:05:35

@dnolen It looks like https://dev.clojure.org/jira/browse/CLJS-855 is only for the arguments, not for an expression in the call position?

dnolen18:05:56

@rauh yes that’s what I was alluding to earlier

thheller18:05:23

(ns shadow.cljs.npm.cli
  (:require ["path" :as path]
            ["fs" :as fs]
            ["child_process" :as cp]))

thheller18:05:39

I implemented that a few days ago btw, if anyone wants to play around with it

thheller18:05:56

:refer also works

thheller18:05:33

also have a ns parser using spec now, probably with a billion bugs but fun

richiardiandrea18:05:09

(ups sorry for the mention, too excited :))

thheller18:05:24

thats good 🙂

anmonteiro20:05:10

cljs.user=> (doc keyword-identical?)
-------------------------
cljs.core/keyword-identical?
([x y])
  Efficient test to determine that two keywords are identical.
nil
cljs.user=> (def a :foo)
#'cljs.user/a
cljs.user=> (def b :foo)
#'cljs.user/b
cljs.user=> (time (dotimes [_ 10000000] (= a b)))
"Elapsed time: 903.233088 msecs"
nil
cljs.user=> (time (dotimes [_ 10000000] (keyword-identical? a b)))
"Elapsed time: 1933.157710 msecs"
nil
cljs.user=> (def a :bar)
#'cljs.user/a
cljs.user=> (time (dotimes [_ 10000000] (= a b)))
"Elapsed time: 901.383346 msecs"
nil
cljs.user=> (time (dotimes [_ 10000000] (keyword-identical? a b)))
"Elapsed time: 1937.850767 msecs"
nil

anmonteiro20:05:13

efficient, you say

anmonteiro20:05:35

I wonder if keyword-identical?‘s perf could be improved

thheller20:05:03

(time (dotimes [_ 10000000] (keyword-identical? a a))) is that faster at least?

anmonteiro20:05:08

so that was in self-host

anmonteiro20:05:19

in regular CLJS I’m seeing opposite results

anmonteiro20:05:45

Regular JVM CLJS:

cljs.user=> (def a :foo)
#'cljs.user/a
cljs.user=> (def b :foo)
#'cljs.user/b
cljs.user=> (time (dotimes [_ 10000000] (= a b)))
"Elapsed time: 730.534774 msecs"
nil
cljs.user=> (time (dotimes [_ 10000000] (keyword-identical? a b)))
"Elapsed time: 128.760093 msecs"
nil
cljs.user=> (def a :bar)
#'cljs.user/a
cljs.user=> (time (dotimes [_ 10000000] (= a b)))
"Elapsed time: 685.319173 msecs"
nil
cljs.user=> (time (dotimes [_ 10000000] (keyword-identical? a b)))
"Elapsed time: 120.184054 msecs"
nil

anmonteiro20:05:49

I wonder what’s the story here

anmonteiro20:05:25

so turns out it only exhibits that weird behavior in Lumo/Node

anmonteiro20:05:39

In Planck, keyword-identical? is also faster than =

dnolen20:05:18

@anmonteiro seems like your hitting some kind deoptimization? Or perhaps there’s something weird about string interning when you dump a V8 image?

anmonteiro20:05:02

maybe I need to provide a warmup script to V8 when providing the snapshot too

anmonteiro20:05:35

something is definitely up

anmonteiro20:05:46

cljs.user=> (time (dotimes [_ 10000000] (and (not (identical? a b)) (keyword? a) (keyword? b) (identical? (.-fqn a) (.-fqn b)))))
"Elapsed time: 29.568403 msecs"
nil

anmonteiro20:05:05

I wonder if it’s in the function dispatch

mfikes20:05:21

I got it to be faster by re-defining keyword-identical? from its source in Lumo’s REPL

anmonteiro20:05:56

yeah, I wonder what that is telling us

mfikes20:05:57

(You get different JS that way.)

anmonteiro20:05:07

I might try compiling Lumo with optimizations :whitespace instead of :simple

anmonteiro20:05:11

wonder if that would change anything

anmonteiro20:05:05

I would probably sacrifice startup time over runtime perf

anmonteiro20:05:12

if push comes to shove

mfikes21:05:00

FWIW, Planck was slightly faster with :none than with :simple

mfikes21:05:49

But, of course, that might have meant JavaScriptCore was better at handling different JavaScript files than one huge concatenated file, and I think I was obsessed with startup latency at the time.