Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update path.js #1997

Closed
wants to merge 3 commits into from
Closed

Update path.js #1997

wants to merge 3 commits into from

Conversation

FrancoisPala
Copy link

Hi, I've tried to do this function recursively and found out that the performances were better (unless for deep cases, like 5 or 6-deep keys in a object) using benchmark.js (https://github.com/bestiejs/benchmark.js/), is there any reason you guys did it iteratively instead?
Also I know it adds a new 'i' parameter which makes it less practical but I was just wondering...
Thanks!

Hi, I've tried to do this function recursively and found out that the performances were better (unless for very deep cases, like 5 or 6-deep keys in a object) using benchmark.js (https://github.com/bestiejs/benchmark.js/), is there any reason you guys did it iteratively instead?
Thanks!
while (idx < paths.length) {
if (val == null) {
return;
module.exports = _curry2(function path(paths, obj, i) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this "secret" third parameter a bit confusing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah definitely, one way to remove it would be to nest the function inside an other one calling for this one: you'd have the path(paths, obj) function calling inside it the recursiveFunc(paths, obj, 0).

This way the I parameter disappears
indentation
@CrossEye
Copy link
Member

CrossEye commented Dec 6, 2016

Ramda mostly avoids deep recursion for its functions. It's predecessor, Eweda, was built with beautiful, elegant, recursive code, and it started falling down on the job as lists grew longer. Ramda is an attempt to make things much more practical.

While I'd love to see your benchmarks, the notion of moving to deeply recursive implementations before tail-call optimizations are ubiquitous scares me.

Also note that your version seems to break a number of tests. I haven't had time to dig into why.

@FrancoisPala
Copy link
Author

FrancoisPala commented Dec 6, 2016

Hi, thanks for the input! So about the tests, some broke because the return values are 'null' instead of 'undefined' in case of empty objects or errors and they were waiting for the latter. Some other tests broke on other functions like lensPath which I suppose use path but I haven't had time to look into it yet.
Now for the benchmarks, I used the benchmark module and compared the result, also changed the path variable for different tests:
`

var obj = {
    a: "a",
    b: {
        z: "z",
        c: {
            e: "e",
            d: {
                r: "r",
                d: {
                    r: "r",
                    d: {
                        r: "r",
                        d: {
                            r: "r",
                            d: {
                                r: "r",
                            }
                        }
                    }
                }
            },
        },
   },
};
`

const path = ['b', 'c', 'd', 'r'];

b.add('with ramda', function() {
    R.path(path, obj);
})

.add('with recursive', function() {
    path(path, obj);
})

.on('cycle', function(event) {
    console.log(String(event.target));
})

.on('complete', function() {
    console.log('Fastest is ' + this.filter('fastest').map('name'));
})

.run({ 'async': true });

Sorry for the bad formatting, I'm having trouble with the editor (fixed for you -- CrossEye)

@CrossEye
Copy link
Member

CrossEye commented Dec 6, 2016

I really meant I wanted to see the results of your tests. 😄.

I didn't get that running in my 3-minute attempt. Perhaps I'll try later.

But breaking tests, especially when it involves other functions is a very bright red light. Do you think you could fix your function so that it doesn't break them and still has these performance gains?

@FrancoisPala
Copy link
Author

Hey, thanks for fixing the formatting :D I'll try to make it return approriate values when I get a moment

@CrossEye CrossEye closed this Jan 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants