Fork me on GitHub
#cljs-dev
<
2018-01-15
>
rauh10:01:49

I have no skin in this game (so I won't create a ticket for it) since I always pass JS arrays to React but: React will use ES6 iterators to go through collections. Currently this uses first/next for all CLJS data structures which obviously isn't optimal. So there is a quite a few ideas that could make this faster (and produce much less garbage). 1. For iterables use -iterable and adapt it to ES6. 2. Add an iterator for chunked sequences which efficiently walks over the array chunks.

dnolen14:01:34

@rauh, don’t our collections return ES6 iterator? we’re not changing how our own interators work

dnolen14:01:01

but if there’s some suboptimal about the ES6 iterator we return we can do something about that

rauh15:01:15

@dnolen Yes, they all do and it works. The (es6-iterable TheCljsDataType) will add the Symbol.iterator to the prototype which always returns the naive iterator:

(deftype ES6Iterator [^:mutable s]
  Object
  (next [_]
    (if-not (nil? s)
      (let [x (first s)]
        (set! s (next s))
        #js {:value x :done false})
      #js {:value nil :done true})))

rauh15:01:04

Our IIterable are often smarter. They're effcient. But they use Java'y .hasNext / .next style API which isn't a standard

rauh15:01:34

The ES6 are a standard and are the ones used:

rauh15:01:06

It'd be an easy change: Check for IIterable in the es6-iterator function and adapt it. Or --much more of an overhaul-- make the ES6 iterator implementation the default for the data structures and make the adapter the other way around (thus saving on the extra indirection for ES6 iterators).

dnolen15:01:54

Or just write faster ES6 iterators

dnolen15:01:02

adapting and sharing stuff just sounds like more bugs

rauh15:01:36

Yeah but then we'd bring it a lot more non-DCE'able code and for each datasctructure add 2 iterators: ES6-style and Java-style (which already exist for many).

rauh15:01:46

Easiest could be: Add a param for most iterators: es6? that check on the call to .next() if it's supposed to return an ES6 style entry or just the Java-style entry. The extra check on that field should be neglible in performance.

dnolen15:01:19

I’m just not concerned about the code size from this change sorry

rauh15:01:41

Yeah it wouldn't add much anyway, but it might really not be worth it. Here would be a pretty good start that avoids a lot of the overhead:

(defn es6-iterator
  [coll]
  (if (iterable? coll)
    (let [it (-iterator coll)]
      #js{:next (fn []
                  (if ^boolean (.hasNext it)
                    #js{:value (.next it), :done false}
                    #js{:value nil, :done true}))})
    ;; Fallback to naive first/next iterator:
    (ES6Iterator. (seq coll))))

rauh15:01:51

That would avoid writing countless iterators. Since we still need the Java style iterators which are used a bunch around core.

rauh15:01:30

Optim: Should use implements? instead of iterable? since we always deal with non-natives here.

dnolen16:01:21

@rauh will that snippet work? you need to adapt at each step

dnolen16:01:35

but ok, write a thing that wraps the iterator and does the right thing and let’s see some benchmarks

rauh16:01:12

@dnolen Yeah it should work. It's very straight forward. You can eval the other way around with (es6-iterator-seq (es6-iterator some-cljs-seq))

rauh16:01:54

The state is within the CLJS iterator, so we can even leave it inline like this, no reason for a deftype.

dnolen16:01:41

hrm @rauh value will be wrong

dnolen16:01:59

the underlying next won’t do the right thing

dnolen16:01:30

but we don’t need to do code review now, come up with something and I will look at it and request adjustments

rauh16:01:18

@dnolen I don't understand. Why would value be wrong?

dnolen16:01:25

if there’s missing tests for the all the things that can go wrong we should add those

dnolen16:01:32

I have to do other stuff now, sorry

rauh16:01:24

OK. I'll just create a ticket for now.