Initial transducers implementation

[kevinbeaty]

• Squashed from buzzedecafe:transduce
• Rebased to master
• Discussed in ramduce
• Credits to @buzzdecafe and @kedashoe

Squashed Commits:

• we have a map transducer
• Apply all arguments to transducer as function.
• Use switch case dispatch for transducer function
• port @kedashoe and @kevinbeaty changes
• Fix build and tests.
• _appendXf as single expression and fix require order for bulid
• foldl uses _xwrap for use with functions
• _dispatchable accepts function for use in _groupBy
• _isIterable uses _symIterator
• Fix filter/map to use _dispatchable appropriately
• Curry _xany, _xfilter, _xmap for use with _dispatchable
• Fix idx lint warning in _arrayReduce
• Remove no longer supported transformer dispatch test
• Transducers for take, takeWhile, drop, dropWhile
• get tests working again
• transduce tests
• improve map test coverage
• Add into
• all and any working
• Add find, findLast, findIndex and findLastIndex transducers
• Step default value in any/all
• stepLast no longer requires initial value since transducers always step single value
• Allow null/undefined accumulators when checking reduced
• groupBy transducer

[kevinbeaty]

Alternative to #865

[kevinbeaty]

Note, the mocha tests pass, but it is failing on creation of index.js ... I still need to look into this. If anybody could offer any pointers, it would be appreciated.

[buzzdecafe]

thanks for doing this so fast @kevinbeaty .

it is failing on creation of index.js

have you tried make clean and then try again?

[kevinbeaty]

it is failing on creation of index.js

All is well now.

[buzzdecafe]

Man, this is a lot of work! I dropped the ball on this because i got very discouraged by the perf numbers. However, there is much that is good about this impl (if I say so myself) and there may be opportunities to optimize later.

I am thinking more and more about using properties on functions to store metadata about them, as recommended long ago by @megawac . I wonder if we could include an iteration strategy on a function, s.t. when called e.g. lazily(map(f)) it would employ a lazy iteration strategy on map; or more germane to this case, transduce(map(f), filter(g)) would use the transducer strategy over the composition. I don't know that this is possible,or how much work it would be, or how invasive. But if there is a way to thread the needle, it could be a way to get the best of several worlds.

That would address one of my primary concerns about this approach as it stands today: It is very invasive, essentially routing all iteration through reduce. It costs a lot in terms of typical-use performance, and you will not see the benefits until you get up to very large list sizes.

[svozza]

Wow. This is amazing.

[kevinbeaty]

I wonder if we could include an iteration strategy on a function

I think we could change the iteration strategy internally to the _transduceDispatch so I don't think it would be too invasive. My initial thought would be to include a Ramda meta data property using a Symbol on the function with fallbacks similar to how we are handling iterator and transformer symbols.

Something like:

var symRamdaMeta = typeof Symbol !== 'undefined' ? Symbol('ramdaMeta') : '@@ramdaMeta'
fn[symRamdaMeta] = {}

// in _transduceDispatch or transduce... whatever makes sense
fn[symRamdaMeta].iterationStrategy = 'transduce'

[buzzdecafe]

My initial thought would be to include a Ramda meta data property using a Symbol on the function with fallbacks similar to how we are handling iterator and transformer symbols.

👍

fn[symRamdaMeta].iterationStrategy = 'transduce'

I was imagining a reference to a function, maybe something like:

fn[symRamdaMeta].strategy = {
    lazy: lzIter,
    xf: xfIter,
    //... etc.
};

then the implementation of lazily could query the function and grab the correct iteration strategy. We can fight over design details later.

[kevinbeaty]

I was imagining a reference to a function

Ah. Gotcha. That could work well.

We can fight over design details later.

Do you want me to propose something and update the PR, or are you saying this is a separate issue?

[buzzdecafe]

Do you want me to propose something and update the PR, or are you saying this is a separate issue?

I would like to build/see a smaller scale proof of concept first. So I think a separate issue/PR makes sense

[kevinbeaty]

I updated the PR with a few minor changes to _dispatchable to help reduce the intrusion/performance concerns. _dispatchable now resembles _checkForMethod with the exception of dispatching on a transformer given in list position. Like _checkForMethod, the function to call as a fallback is passed in to allow an optimized version in replace of using transducers and reduce for the common case. I changed map to use the internal version. Note I did not need to do anything with function meta data. We might be able to make of use of that later, but it was not necessary with this change.

Using old version (map through reduce in common case on my laptop):

Running suite map [lib/bench/map.bench.js]...
_.map x 2,915,839 ops/sec ±4.59% (85 runs sampled)
map(sq, nums) x 339,775 ops/sec ±4.09% (84 runs sampled)
map(sq)(nums) x 307,052 ops/sec ±4.41% (80 runs sampled)
mapSq(nums) x 321,318 ops/sec ±4.33% (81 runs sampled)
Fastest test is _.map at 8.6x faster than map(sq, nums)

Using new version (map using internal _map in common case):

Running suite map [lib/bench/map.bench.js]...
_.map x 3,464,050 ops/sec ±2.51% (91 runs sampled)
map(sq, nums) x 2,057,462 ops/sec ±0.89% (100 runs sampled)
map(sq)(nums) x 1,715,320 ops/sec ±0.95% (100 runs sampled)
mapSq(nums) x 1,977,764 ops/sec ±0.96% (98 runs sampled)
Fastest test is _.map at 1.68x faster than map(sq, nums)

Does this make you feel better about things? I can update the other functions to fallback to the internal versions if you like this approach.

[buzzdecafe]

wow @kevinbeaty that is quite an improvement

[kevinbeaty]

Latest update, after a few more tweaks to _dispatchable

Running suite map [lib/bench/map.bench.js]...
_.map x 3,526,052 ops/sec ±0.92% (100 runs sampled)
map(sq, nums) x 3,165,482 ops/sec ±0.93% (100 runs sampled)
map(sq)(nums) x 2,337,636 ops/sec ±3.02% (93 runs sampled)
mapSq(nums) x 3,297,122 ops/sec ±1.16% (94 runs sampled)
_.map at 1.07x faster than mapSq(nums)

[buzzdecafe]

🎆

[kevinbeaty]

Yeah. So TIL that typeof checks can make a big difference in micro benchmarks.

So is that a 👍 to convert the rest of the _dispatchable functions in this PR to this new approach? Does it alleviate your concerns?

[buzzdecafe]

i'm pretty gobsmacked actually. well done! and yes, let's proceed. Anyone else have $0.02 to contribute?

[kevinbeaty]

Ok. Converted the other functions and removed no longer needed helper functions (including _transduceDispatch). The diff is cleaner now. Probably also a good time for interested parties to review.

A few notes:

• Some functions now have "functor method dispatch" that didn't before all any drop dropWhile find findIndex findLast findLastIndex groupBy ... If this is controversial, I can remove this by passing null as methodname for these functions and handle appropriately in _dispatchable. Say the word and I'll make this change.
• reduce (and by extension groupBy) still has a performance hit due to addition of iterable dispatching and __reduced__ checks. Maybe this is OK. I haven't looked at it much so there may be opportunity for improvement.

[buzzdecafe]

all any drop dropWhile find findIndex findLast findLastIndex groupBy

do any other libs have these object methods that we want to enable point-free composition for? most of these seem to be pretty array specific.

[buzzdecafe]

took a quick look:

• highland has find; that would be nice support;
• lazy.js: every (all); some (any); this would mean 👽 which is ok by me, but may be a fight; also drop and dropWhile; also find
• etc.

so for the most part i guess we do want to dispatch on these; in some cases we may have to alias 😱 if we want interoperability

[kevinbeaty]

Incorporated latest tweaks from @AutoSponge and @davidchambers

[davidchambers]

Terrific description!

[CrossEye]

👍

[megawac]

👍 looks pretty much good to me. I would say making a X base class would be something worth doing in the future to remove some of the redundancy tho

[davidchambers]

This isn't quite right. Perhaps something like this:

a -> (b -> b) -> [c] -> a

I sense there's some relationship between a, b, and c, but this might only be apparent in b's methods.

[kevinbeaty]

Will change here and similarly in transduce.

[CrossEye]

This is fantastic. I've finally gotten through a once-over of everything and managed to run some performance tests (no significant changes for current benchmarks; presumably there would be speed-ups in combination ones we never wrote.)

I'm very, very impressed!

I was barely listening as the initial passes at this stuff was going on. I knew what you were talking about but not much about how you were going about it. This looks really well-done.

I wouldn't delay merging the code for this, as we can address it later. But I'd like to either have a discussion or to learn about reasons already discussed for choosing function constructors over Object.create (even if we shimmed it) or a possibility like this, for _xmap:

var _curry2 = require('./_curry2');


module.exports = _curry2(function _xmap(f, xf) {
    return {
        xf: xf,
        f: f,
        init: function() {
            return this.xf.init();
        },
        result: function(result) {
            return this.xf.result(result);
        },
        step: function(result, input) {
            return this.xf.step(result, this.f(input));
        }
    }; 
});

It's not at all that I think one of these other options is necessarily better, but I think we should consider them... unless this was already hashed out when I was not paying attention.

Really nice work!

[kevinbeaty]

• @sig tweaks per @davidchambers
• R.add in test per @CrossEye
• Rebased to master

[kevinbeaty]

@megawac

I would say making a X base class would be something worth doing in the future to remove some of the redundancy tho

@CrossEye

But I'd like to either have a discussion or to learn about reasons already discussed for choosing function constructors over Object.create

All options worth exploring.

I wouldn't delay merging the code for this, as we can address it later.

👍 Please :)

[CrossEye]

Everyone ok with merging this one? This is one of our most sweeping changes to date, so I'd really like to hear from @buzzdecafe, @davidchambers, @kedashoe, and (I suppose 😉) @kevinbeaty if there any objections. Of course anyone is free to chime in. (Especially you @megawac. 😉)

[buzzdecafe]

let's do it -- big thanks to @kevinbeaty and @kedashoe for their great work -- particularly reviving this when i had dropped the ball.

One caveat -- this now means we have clojure-style iterators when transducing. SO they support array-like, string, iterator-protocol, and object; this means weird unspecified behavior when iterating objects because of indeterminate iteration order.

[AutoSponge]

👍

Great work. One question, would this constitute a major or minor version?

[buzzdecafe]

would this constitute a major or minor version?

since we're pre-1.0 I think we can get away with a minor bump

[kevinbeaty]

One caveat -- this now means we have clojure-style iterators when transducing. SO they support array-like, string, iterator-protocol, and object; this means weird unspecified behavior when iterating objects because of indeterminate iteration order.

Actually, there is no implicit object iteration in this PR for this very reason (we could certainly add it in the future). It currently supports merging objects on the accumulator side but iteration of objects must be explicit (using ES6 style Symbol.iterator or @@iterator)

[buzzdecafe]

i stand corrected! thanks @kevinbeaty

[davidchambers]

🌳

[buzzdecafe]

🎉

[megawac]

🎊 👯 🎈

[svozza]

Fantastic work!

[kevinbeaty]

Awesome.

I have to admit, hitting that "Delete Branch" button felt pretty good ;)

Thanks all.