Fork me on GitHub
#cljs-dev
<
2017-06-29
>
rauh10:06:29

I can'f find anything in the old tickets about the history of -cljs$lang$maxFixedArity, is there a reason CLJS doesn't just use f.cljs$core$IFn$_invoke$arity$variadic.length - 1 instead?

bronsa10:06:51

(fn ([a]) ([a b c d & rest]))

bronsa10:06:13

would variadic -1 work in that case?

rauh10:06:56

@bronsa Yes, it'll be set to 4 by CLJS right now. And it'd be 4 with using variadic - 1 too.

rauh10:06:12

I'm just brainstorming how to centralize all dispatchers & applyto + .call + .apply. I'm pretty sure we can get away with removing everything on multi arity functions except the actual implementations.

dnolen11:06:24

@rauh before going down that rabbit hole I would spend some time researching performance implications

dnolen11:06:52

JS engines optimize functions whose argument types aren’t changing

rauh11:06:53

@dnolen You mean wrt to centralizing? Or just removing MFA?

dnolen11:06:28

checking the length of the function, that is reflective capability

dnolen11:06:39

do JS engines deopt that?

dnolen11:06:53

these kind of things you want to know before spending a whole lot of time on this

rauh11:06:21

Yeah I'll test.

dnolen11:06:54

I suspect the primary benefit of doing this would be code size - not performance which I’m pretty confident will degrade significantly

dnolen11:06:28

while I won’t say don’t look into this - I would argue that there are performance gains to be had that don’t require so much research or vetting

rauh11:06:50

FF is equally fast. Chrome is ~2x slower.

dnolen11:06:03

so already a bad sign

dnolen11:06:13

and you still need to assess JSC

rauh11:06:06

Hard to measure, V8 could just inline the heck out of the current MFA for all I know

dnolen11:06:37

all the engines are very finicky

dnolen11:06:44

the only answer is to keep everything super simple

rauh11:06:19

Ok, keeping it simple then 🙂

dnolen11:06:00

but again the single dispatcher idea is fundamentally not a good one

dnolen11:06:19

that will break any kind of fn optimization

rauh11:06:34

@dnolen So you mentioned refactoring IFn invokes a few weeks ago. Is https://dev.clojure.org/jira/browse/CLJS-2127 too many changes? Should I just introduce new fn's and use them only in --let's say-- apply for now? I kinda wanna get those in before starting the other work.

dnolen11:06:03

I won’t have time to look at this week

rauh11:06:09

@dnolen I disagree, the single dispatcher is never used really. It just does't run under "normal" code in advanced

dnolen11:06:24

it doesn’t matter

dnolen11:06:46

you don’t want the gap between advanced and dev usage to be all that different

dnolen11:06:21

You can do meaningful perf tests with :none and the relevant compiler options

dnolen11:06:42

and sometimes it’s faster than advanced because Closure inlining is a joke

dnolen11:06:49

compared to VM inlining

dnolen11:06:25

in the past Closure would also dedupe code

dnolen11:06:38

and that of course just makes VMs generate less optimal stuff

dnolen12:06:29

@rauh but yes first glance at CLJS-2127, it’s just too much stuff to review

dnolen12:06:10

basically the first thing I want to see is (invoke* ...) and to just use that consistently everywhere

rauh12:06:00

@dnolen That's what I did, no?

rauh12:06:23

invoke-ifn and I also generated set-ifn!

dnolen12:06:45

no you did more

dnolen12:06:02

just call it (invoke* ...) and don’t make any other higher level helpers

dnolen12:06:47

maybe-invoke-ifn and set-ifn none of that stuff should be in first patch

rauh12:06:55

Ok, will do.

dnolen12:06:14

no invoke-variadic

dnolen12:06:28

I updated the ticket to reflect this and assigned it back to you

rauh12:06:34

Thanks! I'll provide an updated patch at some point.

dnolen12:06:14

cool thanks

tmulvaney12:06:30

list seems to fail on some collections because they don't implement INext.

tmulvaney12:06:53

This fails for example (apply list 1 2 (hash-map 1 2 3 4))

tmulvaney12:06:06

where as (apply list 1 2 (array-map 1 2 3 4)) works

tmulvaney12:06:58

It looks like the code assumes that all seqs implement INext.

dnolen12:06:37

need to reconcile this with Clojure

dnolen12:06:52

meaning - check how it works there

tmulvaney13:06:54

Under Clojure, both those cases seem to work.

dnolen13:06:35

well yeah behavior is the first thing to check, but I meant more implementation details abou the seq types

tmulvaney13:06:36

Ok, so digging into the clojure implementation it also seems to call next on the args. The two sequences involved with PersistentHashMap NodeSeq and ArraySeq also seem to have a next method

dnolen13:06:00

ok so then that’s the bug, not implementing INext consistently everywhere

tmulvaney13:06:50

Should all implementors of ISeq implement INext? it looks like in Clojure next is really part of ISeq

dpsutton13:06:41

for(ISeq s = RT.seq(args); s != null; s = s.next()) seems to confirm that is an assumption

dnolen13:06:22

@tmulvaney next appeared early on in Clojure so wasn’t a big deal, pre deftype

dnolen13:06:46

unfortunately early ClojureScript did not have next, and protocols were first class

dnolen13:06:12

so when I added them for perf I didn’t fold into an existing protocol for backward-compatibility

dnolen13:06:33

anyways that’s the backstory

dnolen13:06:01

and nothing we can do about that - except make sure INext is on all the existing ISeq iplementations

tmulvaney13:06:58

ok cool. Well it looks like its just those ArrayNodeSeq and NodeSeq, as well as PersistentQueue seq which are missing it

tmulvaney13:06:10

happy to open a patch

dnolen13:06:15

yes please, thanks

tmulvaney14:06:49

Thanks for the back story. It's always good to know the how and why. I've attached a patch at https://dev.clojure.org/jira/browse/CLJS-2137

tmulvaney14:06:25

PersistentTreeMapSeq also ended up needing INext.

dnolen15:06:12

@tmulvaney patch looks ok but why did you change the test code?

tmulvaney15:06:03

I mentioned in the comment that there was an undeclared variable assoc-n @favila pointed it out to me yesterday

tmulvaney15:06:27

that path isn't called.

dnolen15:06:05

so why not just remove it?

tmulvaney15:06:37

I needed to implement IVector, even though the method is not called

tmulvaney15:06:22

actually maybe it is not required. let me test that quickly

tmulvaney15:06:53

Ok so it is needed because subvec checks that whatever is passed in is an IVector.

dnolen15:06:31

@tmulvaney can you update the patch to leave a comment there please

dnolen15:06:34

it’s very confusing

tmulvaney15:06:28

as in a comment in the test itself or in the commit message?

dnolen15:06:56

just a comment on the test please

dnolen15:06:12

stating you’re satisfying IVector trivially for subvec

tmulvaney15:06:59

Ok updated patch attached.

dnolen15:06:50

@tmulvaney looks like your patch needs a rebase?

tmulvaney15:06:47

weird. it applies cleanly for me.

tmulvaney15:06:08

i'm definitely up to date with master

dnolen15:06:00

@tmulvaney you’re right something weird locally

dnolen15:06:55

@rauh that’s interesting but not really all that useful since we don’t prioritize V8 at all

dnolen15:06:00

and JSC leaves it in the dust these days

dnolen15:06:32

we also need constantly counterbalance w/ SpiderMonkey which continues to lag on the perf front

rauh15:06:20

Personally I don't care about desktop/laptop performance. Everything will be super fast anyways. My only problem is mobile. And in that case Apple products are pretty darn fast b/c of their processors. And given Android is much more spread on third world countries which is a HUGE market and often have slower phones. Hence why IMO Android Chrome/v8 needs most attention.

dnolen16:06:26

just not going to happen sorry

rauh16:06:27

Also, the hidden class thing is the same on almost all browser engines. So I'll see if setting IFn protocols up to 10 args simply to nil in the centralized dispatcher could lead to all multiy arity function getting the same hidden class.

dnolen16:06:40

we don’t care about V8 in this way and will never spend any effort optimizing specifically for it

dnolen16:06:58

(but if you’re doing your own thing cool! :)

rauh16:06:17

Yeah, I'm not saying I want to specifically target any browser. It's just the easiest to develop with. The v8 command gives some easy and quick insights

rauh16:06:31

Just wanted to share, in case somebody wants to play around. I'll come up with real benchmarks once I implement the central dispatchers idea.

dnolen16:06:46

I’m skeptical, but happy to have someone gather more hard evidence 🙂

tmulvaney16:06:42

@dnolen I noticed a bit of dead code whilst writing the last patch, which I've addressed in CLJS-2138

rauh18:06:04

(counted? #js[1 2]) -> false. That hits unnecessary inefficient bounded-count in apply.

dnolen18:06:28

it’s supposed to be false

dnolen18:06:37

same as Clojure

rauh19:06:49

@dnolen Re: Refactoring: So you mean add helper fn (not macro)? I kinda really need a macro to invoke the $variadic$ or it'll get ugly. Can I add an invoke-variadic* macro?

dnolen19:06:30

I’m asking for a helper fn

dnolen19:06:38

just for generating the name - that’s it

rauh19:06:36

Right, but I'm realizing that I'd need a helper macro for invoking the variadic property of a fn. Or I'd have to repeat (.cljs$core$IFn$_invoke$arity$variadic f (seq args)) many times.

dnolen19:06:07

I’m OK with invoke-variadic* and it should use the helper fn I’m suggesting

dnolen22:06:45

won’t have time to look at this today or tomorrow but this is a pretty bad unresolved warning regression - https://dev.clojure.org/jira/browse/CLJS-2139

dnolen22:06:23

my hunch is that it’s due to the analyzed warning change - we definitely want to provide an analyzer test for whatever the fix is

darwin22:06:53

dean in the snippet should be defn?

dnolen22:06:11

yeah fixed

dnolen22:06:26

and confirmed it’s not what I thought - unrelated to analyzed stuff

dnolen22:06:36

probably need a bisect to sort this one out

mfikes23:06:54

I added a git bisect result to CLJS-2139