Fork me on GitHub

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.


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


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


@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]
  (next [_]
    (if-not (nil? s)
      (let [x (first s)]
        (set! s (next s))
        #js {:value x :done false})
      #js {:value nil :done true})))


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


The ES6 are a standard and are the ones used:


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


Or just write faster ES6 iterators


adapting and sharing stuff just sounds like more bugs


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


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.


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


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


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


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


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


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


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


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


hrm @rauh value will be wrong


the underlying next won’t do the right thing


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


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


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


I have to do other stuff now, sorry


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