Fork me on GitHub
#hoplon
<
2018-06-26
>
flyboarder16:06:05

@alandipert can we get your thoughts on something

alandipert16:06:51

sure, what's up?

flyboarder16:06:41

@jjttjj is working on some performance improvements for 7.3

flyboarder16:06:25

currently we are using a transient which seems to add better performance for large collections of elements

flyboarder16:06:15

@jjttjj maybe you should explain it 😛

jjttjj16:06:54

so basically these changes

flyboarder16:06:58

^ here is the relevant changes

jjttjj16:06:17

it seems like for child-vec i can add a transient and it becomes more performant after the size of the vec = 10, otherwise it's faster without, so it depends what you want to optimize for (i'm leaning towards the non-transient)

jjttjj16:06:54

for vflatten i'm getting pretty tossup results with and without transient but either way both are faster than the current vflatten

jjttjj16:06:05

(formatting benchmarks and results in a gist now)

alandipert16:06:19

i have to finish a mtg, but i'll look and get back to you

alandipert16:06:39

my first blush is, can we just use arrays where we use child-vec and get max perf

alandipert16:06:59

since we don't expose the children as an immutable collection anywhere

alandipert16:06:36

(sinci this is an internal state maintenance bit. and transients are helpful when you eventually need to export an immutable thing and don't want to pay the cost of copying from an array to an immutable thing)

alandipert16:06:37

could perhaps be (aset kids i %2) ?

flyboarder16:06:41

@jjttjj looking at ^ I think we can make it non-associative

flyboarder16:06:54

everywhere else its used as a vec

flyboarder16:06:11

so I think we could use a js array in place

flyboarder16:06:32

@alandipert thanks for the insight

alandipert16:06:51

np, fwiw i think transients are a good step, and i'm +1 on the PR

alandipert16:06:01

but i just know that the end of the perf game is writing basically JS

alandipert16:06:10

so if you have the time and interest, maybe just go that route

jjttjj16:06:12

so you think it's worth basically just switching out all the swap!s for asets in core.cljs and work with js objects?

flyboarder16:06:31

yeah basically get as close to js as possible

alandipert16:06:49

immutable -> transient -> mutable/JS -> rethink algorithm

👍 4
jjttjj16:06:29

yeah after a few more runs it does look like the transient version is faster, but pretty close to non-transient

alandipert16:06:27

mutating arrays directly is a lot easier on the GC also

alandipert16:06:36

especially if you eliminate destructuring/seq construction everywhere

alandipert16:06:23

when micha switched javelin to use arrays instead of transients/atoms/vectors there was a dramatic speedup and i didn't think it was much more complicated

jjttjj16:06:39

so i think child-vec can be managed internally with js-objects, but vflatten will always have to work with clojure vectors, otherwise users would have to pass in js arrays for sibling elements

alandipert16:06:30

vflatten could build an array instead of a vector though, right? like its argument could be a vector but its return could be a js array

jjttjj16:06:14

oh yeah true good point

alandipert16:06:26

also in add-children! it looks like vflatten is used just to make a collection to iterate over

alandipert16:06:33

could avoid creating the collection by iterating and flattening at the same time

alandipert16:06:46

i should get back to my work in my other even weirder language, R. but this is really exciting, i wish you guys luck

alandipert16:06:54

lemme know if you want me to look at anything more

flyboarder16:06:00

thanks alan!

jjttjj16:06:07

cool thanks!

alandipert16:06:25

oh, one more thing i wanted to mention

alandipert16:06:48

in that situation, add-children!, where you want to avoid an intermediate collection by walking a structure and doing something in the same loop

alandipert16:06:11

one way to do that efficiently without needing to encode in each instance the walking logic (the vflatten)

alandipert16:06:22

is a custom iteration macro, in the spirit of areduce

alandipert16:06:14

if that doesn't make sense i can elaborate. just a nugget i've picked up along the way that i thought could be helpful

jjttjj17:06:05

gotcha i think i know what you mean

jjttjj17:06:30

actually discovered areduce for the first time when looking at improving child-vec

jjttjj17:06:51

FWIW my benchmark code before had a dumb bug that made the results totally wrong

jjttjj17:06:02

this is updated with an array version

jjttjj17:06:03

also shows how close a call the transient and non-transient version are which was my original point, that i messed explaining due to that benchmark bug:man-facepalming:

flyboarder18:06:42

@jjttjj nice work on the benchmarking

jjttjj22:06:37

After taking an initial crack at using js arrays instead of vector to store the hoplon-kids i have a few more thoughts. I didn't get it quite working yet but just started work on it. I got a sense that this is gonna be a pretty clear tradeoff, getting performance in exchange for giving up a lot of clojure niceties in hoplon.core Is performance an immediate priority for hoplon? It seems like this would be a pretty big change in terms of amount of code in core that is effected. Not to mention there should probably be some benchmarks/performance measurements set up first to actually gauge how much this improves things, and to see where the bottlenecks are in the first place? To be honest, hoplon's performance has never really been an issue for me. With the two changes in the PR I was looking for really low hanging fruit things that could be refactored as my main priority, and get speed gains where possible (these managed to get ~35%+ speedup, and cleaned up the code with no tradeoffs :)) The re-working of the innards to use JS is an interesting project I think it deserves more research and thought but it doesn't seem obvious to me that this would be a great fit for 7.3 in the immediate term? I guess having some sense of the goals of hoplon now would be useful. Any thoughts @alandipert @flyboarder

flyboarder22:06:29

@jjttjj im all for pushing the move to js arrays off to another version, the improvements to the core loops should be enough for now to combat any performance criticism

flyboarder22:06:32

also this already cleans up all the remaining old code from the v6-alpha’s

flyboarder22:06:02

with the exception being the loop-tpl*

flyboarder22:06:21

that could be greatly simplified from it’s current form

jjttjj22:06:45

yeah I looked at that issue you sent me regarding that, the new version there looks pretty good initially, will play around with that too a bit extensively soon

alandipert22:06:09

i think it's a really good idea to hit a conceptual clarity milestone before a major perf/JS excursion

alandipert22:06:29

like javelin tho i have the sense hoplon is a good place to mine for performance, since we're happy with what it does

alandipert22:06:35

and so can safely play around with how it does it

alandipert22:06:07

so, excellent points @jjttjj

jjttjj22:06:33

here's my very initial, not working naive attempt at using js kids https://gist.github.com/jjttjj/64933897fd45985a3642980fda57fac5

jjttjj22:06:53

actually I just realized amap exists which would clean up some of those reduces

jjttjj22:06:41

i could see it being done cleanly eventually just kind of a big overhaul

flyboarder22:06:42

@jjttjj well lets skip the js arrays for now, I’ll open a ticket and set it for 7.5

jjttjj22:06:53

cool so for now i guess the question is back to:

(defn- vflatten
  "Takes a sequential collection and returns a flattened vector of any nested
  sequential collections."
  ([x] (vflatten [] x))
  ([acc x] (if (sequential? x) (reduce vflatten acc x) (conj acc x))))
vs
(defn- vflatten
  "Takes a sequential collection and returns a flattened vector of any nested
  sequential collections."
  ([tree]
   (persistent! (vflatten (transient []) tree)))
  ([acc tree]
   (if (sequential? tree)
     (reduce vflatten acc tree)
     (conj! acc tree))))
The NON transient wins for smaller, flatter values https://gist.github.com/jjttjj/b9ab914009e360f7f765782ef7462bb1

jjttjj22:06:38

i have a similar benchmark for child-vec with vs without transient, but there i think it's even more clear the non-transient wins

jjttjj22:06:50

it all depends what params we want to optimize for though, I have a feeling smaller, flatter children vectors are the vast majority