Clojurians Log v2

@alandipert can we get your thoughts on something

sure, what's up?

@jjttjj is working on some performance improvements for 7.3

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

@jjttjj maybe you should explain it 😛

https://github.com/hoplon/hoplon/compare/merge-kids...jjttjj:merge-kids

so basically these changes

^ here is the relevant changes

https://github.com/hoplon/hoplon/compare/merge-kids...jjttjj:merge-kids

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)

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

(formatting benchmarks and results in a gist now)

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

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

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

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

needs to be associative due to the stuff like this: https://github.com/hoplon/hoplon/blob/master/src/hoplon/core.cljs#L287

could perhaps be (aset kids i %2) ?

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

everywhere else its used as a vec

so I think we could use a js array in place

@alandipert thanks for the insight

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

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

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

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

yeah basically get as close to js as possible

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

https://gist.github.com/jjttjj/b9ab914009e360f7f765782ef7462bb1

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

mutating arrays directly is a lot easier on the GC also

especially if you eliminate destructuring/seq construction everywhere

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

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

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

oh yeah true good point

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

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

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

lemme know if you want me to look at anything more

thanks alan!

cool thanks!

oh, one more thing i wanted to mention

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

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

is a custom iteration macro, in the spirit of areduce

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

gotcha i think i know what you mean

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

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

https://gist.github.com/jjttjj/b9ab914009e360f7f765782ef7462bb1

this is updated with an array version

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:

@jjttjj nice work on the benchmarking

https://twitter.com/degree9io/status/1011675229683236864

hi there @smnplk

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

@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

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

with the exception being the loop-tpl*

that could be greatly simplified from it’s current form

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

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

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

and so can safely play around with how it does it

so, excellent points @jjttjj

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

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

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

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

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

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

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

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

child-vec: https://gist.github.com/jjttjj/686b3b84537e24a542e4ccfb53f8d73f