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

Bad performance of _Reader created by 'then' method #14

Closed
jakublabenski opened this issue Apr 6, 2021 · 3 comments
Closed

Bad performance of _Reader created by 'then' method #14

jakublabenski opened this issue Apr 6, 2021 · 3 comments

Comments

@jakublabenski
Copy link

I do not know if this is a bug or correct behaviour. This is what I observed:

from pymonad.reader import Compose

def inc(x: int) -> int:
    print("inc:", x)
    return x + 1

def dec(x: int) -> int:
    print("dec:", x)
    return x - 1

iid = Compose(inc).then(inc).then(dec)
print(iid(0)) 

The result of execution is:

inc: 0
inc: 1
inc: 0
inc: 1
dec: 2
inc: 0
inc: 1
inc: 0
inc: 1
dec: 2
1 

I expected to see this:

inc: 0
inc: 1
dec: 2
1 

Implementation of _bind_or_map is responsible for this:

@pymonad.tools.curry(3)
def _bind_or_map(function_f, function_g, read_only):
    try:
        return _bind(function_f, function_g, read_only)
    except TypeError:
        return _map(function_f, function_g, read_only)

First _bind is used to compute the result. Bind do mapping and fails on actual binding.
Then everything is computed again by _map. This has exponential complexity.

I believe fix could do 'map' then try to 'bind':

@pymonad.tools.curry(3)
def _bind_or_map(function_f, function_g, read_only):
    ret = _map(function_f, function_g, read_only)
    try:
        return ret(read_only)
    except TypeError:
        return ret
@jasondelaat
Copy link
Owner

Hey, thanks for bringing this to my attention. Especially since it's something I should have noticed myself ages ago. Safe to say that exponential complexity is definitely not the intended behaviour.

So I've had a quick look at this and, unfortunately, won't have time to really dig into it until at least the weekend. But the good news is your solution appears to work for Reader. The bad news is other monad have similar constructions and therefore suffer the same complexity problem. And a similar solution doesn't appear to work for those. State in particular has even bigger problems which I'll need to figure out. Hopefully I can find a general solution to fix the problem across the board.

I'll keep you posted. In the meantime, using map and bind directly instead of deferring to then should avoid performance problems in most cases.

@jasondelaat
Copy link
Owner

So, I've fixed the exponential complexity problem in every monad where it was a problem and also fixed Promise which, it turns out, was really broken.
I haven't pushed the code yet because I need to test it thoroughly and clean it up some first which at this point will happen some time tomorrow afternoon my time (EST). New version should be on PyPi around the same time.

Thanks again for bringing this to my attention.

@jasondelaat
Copy link
Owner

Alright, all fixed. New version is 2.3.5 available here and on PyPi.

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

No branches or pull requests

2 participants