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

@rauh yes, https://dev.clojure.org/jira/browse/CLJS-1992

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

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

What others?

yes, one sec I have that written down somewhere

deref reset! atom

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

Thanks!

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

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

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

No worries, I only wanted those changes.

@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

re: CLJS-2041

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

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

ok right because the fn is the dispatcher

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

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

@rauh actually that’s not quite true

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

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

same as in Clojure

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

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

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

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

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.

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.

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

this is just a perf enhancement for fn call overhead

globally

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

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

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

I can resubmit after feedback.

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

We can make a seperate issue IFn .call codegen

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

@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

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

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

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

that would be unnacceptable

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

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

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

@rauh parse-invoke in analyzer.cljc

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

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

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

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

this may be a tradeoff though

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

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

@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

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

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

the optimization is last expression in parse-invoke

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

@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?

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

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

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

nice

:refer also works

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

https://github.com/thheller/shadow-cljs/blob/master/src/main/shadow/cljs/ns_form.clj

@thheller cool stuff

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

thats good 🙂

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

efficient, you say

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

hrm

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

so that was in self-host

in regular CLJS I’m seeing opposite results

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

I wonder what’s the story here

hehe nice

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

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

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

could be

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

something is definitely up

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

I wonder if it’s in the function dispatch

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

yeah, I wonder what that is telling us

(You get different JS that way.)

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

wonder if that would change anything

I would probably sacrifice startup time over runtime perf

if push comes to shove

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

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.