Fork me on GitHub
#cljs-dev
<
2017-06-18
>
rauh15:06:12

@dnolen Note, I also change first to -first in to-array, minor optimization. Not sure it matters a lot.

dnolen15:06:46

it’s generally not safe to do that and it should pretty much always be avoided

dnolen15:06:58

the only case where I would accept that type of thing is inside a deftype

dnolen15:06:38

and only self calls

rauh15:06:35

Ok, I thought I'd be safe since first, if not natively implemented, calls seq and then -first, which we know we already did. But I'm not 100% sure.

dnolen15:06:45

that’s the problem

rauh15:06:53

The improved apply is a big patch. Just a heads up 🙂

dnolen15:06:07

if you use -foo then everybody has to hunt around to figure out when it is or isn’t OK

dnolen15:06:17

so better to just avoid and adopt a safe convention

rauh15:06:24

Also, can I add an improved spread that avoids calling next twice? Just bind in a let and re-use.

dnolen15:06:12

like I said before, the less patch does the better

dnolen15:06:27

but I don’t see why it’s a problem here

rauh15:06:28

Ok, I figured as much 🙂

dnolen15:06:45

I know the apply code and only spread is used there far as I know

dnolen15:06:48

it’s not for users

rauh15:06:50

Yeah it's very little change, very confident it would change anything.

rauh15:06:13

It's also used in list

rauh15:06:07

Patch attached for 2099.

mfikes15:06:14

Until we have CI running on https://github.com/clojure/clojurescript, I’ll pull upstream changes into https://github.com/mfikes/clojurescript periodically. This will allow us to shake out any issues with running tests this way. You can see the latest building at https://travis-ci.org/mfikes/clojurescript

rauh15:06:56

Hold off 2099, I need to move the let bindings which aren't needed in the "simple apply case"

rauh15:06:29

Alright, now the patch should be optimal.

rauh16:06:03

I'm having trouble getting it to work with bootstrapped CLJS. I'm always getting

Can't take value of macro cljs.core/gen-apply-to-simple
gen-apply-to-simple already refers to: cljs.core/gen-apply-to-simple being replaced by: cljs.core$macros/gen-apply-to-simple at line

rauh16:06:14

What am I missing?

rauh16:06:32

Ahhhh, needed to add the macro to :refer-clojure :exclude in the ns declaration.

dnolen19:06:12

@rauh that’s nice, there’s always been a big perf gap between ClojureScript apply and Clojure apply and this seems like it significantly closes the gap.

dnolen19:06:37

@rauh I’ve left a comment on the ticket, apply-to-simple is yucky and needs to private and have warning docstring

dnolen19:06:04

@rauh wow the performance difference is pretty amazing 🙂

dnolen19:06:23

4-5X for calling apply w/ a set

dnolen19:06:08

still 3-4X slower than Clojure but at least not 10X anymore

rauh19:06:40

So I have this idea of --instead of calling first/next-- just dumping it all into an array first with reduce and then dispatch with a simple case. Though, then the values would need to be plucked out of the array again. Could be faster. But I don't think I feel like doing this implementation right now 🙂

dnolen19:06:32

understandable

rauh19:06:11

Note, I also messed with the bounded-count stuff in the apply when calling variadic functions. Just some basic arithmetic, I hope that's ok.

rauh19:06:27

bounded-count can deal with negative numbers, so it should be safe thing.

dnolen19:06:47

it’s fine - this is all ugly perf stuff

dnolen19:06:02

applied to master

dnolen19:06:52

something for later would be helper macros for this stuff to clean up the implementation and hide the names

dnolen19:06:00

like (invoke f ...)

rauh19:06:36

Agreed, it's super easy to just mess up one number on all those IFn calls.

dnolen19:06:39

(max-fixed-arity f …) etc.

rauh19:06:35

FYI, because of this apply there is now no calls to .call of cljs data structures, this was the last bit of CLJS code that relied on it.

rauh19:06:41

So at some point in the future we could potentially consider removing those.

rauh20:06:36

Ugh, I think my changes around bounded-count might've been bad. Needs some testing/thought. What if mfa was 3, previously the the count stopped right there and called the arity 3. Now I do (+ 4 (bounded-count some-neg-number ...)) which would be (+ 4 0) and we call the 4th arity which might not exist. Going to bed now. Gonna think about this tomorrow.

dnolen20:06:57

@rauh sure, seems simple to test, but also seems simple to backout this micro opt